diff mbox series

selinux: vsock: Set SID for socket returned by accept()

Message ID 20210317154448.1034471-1-dbrazdil@google.com (mailing list archive)
State New
Headers show
Series selinux: vsock: Set SID for socket returned by accept() | expand

Commit Message

David Brazdil March 17, 2021, 3:44 p.m. UTC
For AF_VSOCK, accept() currently returns sockets that are unlabelled.
Other socket families derive the child's SID from the SID of the parent
and the SID of the incoming packet. This is typically done as the
connected socket is placed in the queue that accept() removes from.

Implement an LSM hook 'vsock_sk_clone' that takes the parent (server)
and child (connection) struct socks, and assigns the parent SID to the
child. There is no packet SID in this case.

Signed-off-by: David Brazdil <dbrazdil@google.com>
---
This is my first patch in this part of the kernel so please comment if I
missed anything, specifically whether there is a packet SID that should
be mixed into the child SID.

Tested on Android.

 include/linux/lsm_hook_defs.h |  1 +
 include/linux/lsm_hooks.h     |  7 +++++++
 include/linux/security.h      |  5 +++++
 net/vmw_vsock/af_vsock.c      |  1 +
 security/security.c           |  5 +++++
 security/selinux/hooks.c      | 10 ++++++++++
 6 files changed, 29 insertions(+)

Comments

Paul Moore March 17, 2021, 8:16 p.m. UTC | #1
On Wed, Mar 17, 2021 at 11:44 AM David Brazdil <dbrazdil@google.com> wrote:
>
> For AF_VSOCK, accept() currently returns sockets that are unlabelled.
> Other socket families derive the child's SID from the SID of the parent
> and the SID of the incoming packet. This is typically done as the
> connected socket is placed in the queue that accept() removes from.
>
> Implement an LSM hook 'vsock_sk_clone' that takes the parent (server)
> and child (connection) struct socks, and assigns the parent SID to the
> child. There is no packet SID in this case.
>
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> ---
> This is my first patch in this part of the kernel so please comment if I
> missed anything, specifically whether there is a packet SID that should
> be mixed into the child SID.
>
> Tested on Android.
>
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/lsm_hooks.h     |  7 +++++++
>  include/linux/security.h      |  5 +++++
>  net/vmw_vsock/af_vsock.c      |  1 +
>  security/security.c           |  5 +++++
>  security/selinux/hooks.c      | 10 ++++++++++
>  6 files changed, 29 insertions(+)

Additional comments below, but I think it would be a good idea for you
to test your patches on a more traditional Linux distribution as well
as Android.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5546710d8ac1..a9bf3b90cb2f 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -755,6 +755,7 @@ static struct sock *__vsock_create(struct net *net,
>                 vsk->buffer_size = psk->buffer_size;
>                 vsk->buffer_min_size = psk->buffer_min_size;
>                 vsk->buffer_max_size = psk->buffer_max_size;
> +               security_vsock_sk_clone(parent, sk);

Did you try calling the existing security_sk_clone() hook here?  I
would be curious to hear why it doesn't work in this case.

Feel free to educate me on AF_VSOCK, it's entirely possible I'm
misunderstanding something here :)

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ddd097790d47..7b92d6f2e0fd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5616,6 +5616,15 @@ static int selinux_tun_dev_open(void *security)
>         return 0;
>  }
>
> +static void selinux_socket_vsock_sk_clone(struct sock *sock, struct sock *newsk)
> +{
> +       struct sk_security_struct *sksec_sock = sock->sk_security;
> +       struct sk_security_struct *sksec_new = newsk->sk_security;
> +
> +       /* Always returns 0 when packet SID is SECSID_NULL. */
> +       WARN_ON_ONCE(selinux_conn_sid(sksec_sock->sid, SECSID_NULL, &sksec_new->sid));
> +}

If you are using selinux_conn_sid() with the second argument always
SECSID_NULL it probably isn't the best choice; it ends up doing a
simple "sksec_new->sid = sksec_sock->sid" ... which gets us back to
this function looking like a reimplementation of
selinux_sk_clone_security(), minus the peer_sid and sclass
initializations (which should be important things to have).

I strongly suggest you try making use of the existing
security_sk_clone() hook in the vsock code, it seems like a better way
to solve this problem.
David Brazdil March 19, 2021, 12:57 p.m. UTC | #2
Hi Paul,

I'll post a v2 shortly but will address your comments here.

> >  include/linux/lsm_hooks.h     |  7 +++++++
> >  include/linux/security.h      |  5 +++++
> >  net/vmw_vsock/af_vsock.c      |  1 +
> >  security/security.c           |  5 +++++
> >  security/selinux/hooks.c      | 10 ++++++++++
> >  6 files changed, 29 insertions(+)
> 
> Additional comments below, but I think it would be a good idea for you
> to test your patches on a more traditional Linux distribution as well
> as Android.
> 

No problem, I was going to add a test case into selinux-testsuite
anyway. Done now (link in v2) and tested on Fedora 33 with v5.12-rc3.

> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index 5546710d8ac1..a9bf3b90cb2f 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -755,6 +755,7 @@ static struct sock *__vsock_create(struct net *net,
> >                 vsk->buffer_size = psk->buffer_size;
> >                 vsk->buffer_min_size = psk->buffer_min_size;
> >                 vsk->buffer_max_size = psk->buffer_max_size;
> > +               security_vsock_sk_clone(parent, sk);
> 
> Did you try calling the existing security_sk_clone() hook here?  I
> would be curious to hear why it doesn't work in this case.
> 
> Feel free to educate me on AF_VSOCK, it's entirely possible I'm
> misunderstanding something here :)
> 

No, you're completely right. security_sk_clone does what's needed here.
Adding a new hook was me trying to mimic other socket families going via
selinux_conn_sid. Happy to reuse the existing hook - makes this a nice
oneliner. :)

Please note that I'm marking v2 with 'Fixes' for backporting. This does
feel to me like a bug, an integration that was never considered. Please
shout if you disagree.

-David
diff mbox series

Patch

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 477a597db013..f35e422b2b5c 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -329,6 +329,7 @@  LSM_HOOK(int, 0, sctp_bind_connect, struct sock *sk, int optname,
 	 struct sockaddr *address, int addrlen)
 LSM_HOOK(void, LSM_RET_VOID, sctp_sk_clone, struct sctp_endpoint *ep,
 	 struct sock *sk, struct sock *newsk)
+LSM_HOOK(void, LSM_RET_VOID, vsock_sk_clone, struct sock *sock, struct sock *newsk)
 #endif /* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index fb7f3193753d..1b4e92990401 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1033,6 +1033,13 @@ 
  *	@sk pointer to current sock structure.
  *	@sk pointer to new sock structure.
  *
+ * Security hooks for vSockets
+ *
+ * @vsock_sk_clone:
+ *	Clone SID from the server socket to a newly connected child socket.
+ *	@sock contains the sock structure.
+ *	@newsk contains the new sock structure.
+ *
  * Security hooks for Infiniband
  *
  * @ib_pkey_access:
diff --git a/include/linux/security.h b/include/linux/security.h
index 8aeebd6646dc..ffac67058355 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1404,6 +1404,7 @@  int security_sctp_bind_connect(struct sock *sk, int optname,
 			       struct sockaddr *address, int addrlen);
 void security_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
 			    struct sock *newsk);
+void security_vsock_sk_clone(struct sock *sock, struct sock *newsk);
 
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct sock *sock,
@@ -1623,6 +1624,10 @@  static inline void security_sctp_sk_clone(struct sctp_endpoint *ep,
 					  struct sock *newsk)
 {
 }
+
+static inline void security_vsock_sk_clone(struct sock *sock, struct sock *newsk)
+{
+}
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 5546710d8ac1..a9bf3b90cb2f 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -755,6 +755,7 @@  static struct sock *__vsock_create(struct net *net,
 		vsk->buffer_size = psk->buffer_size;
 		vsk->buffer_min_size = psk->buffer_min_size;
 		vsk->buffer_max_size = psk->buffer_max_size;
+		security_vsock_sk_clone(parent, sk);
 	} else {
 		vsk->trusted = ns_capable_noaudit(&init_user_ns, CAP_NET_ADMIN);
 		vsk->owner = get_current_cred();
diff --git a/security/security.c b/security/security.c
index 5ac96b16f8fa..050b653405e0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2335,6 +2335,11 @@  void security_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
 }
 EXPORT_SYMBOL(security_sctp_sk_clone);
 
+void security_vsock_sk_clone(struct sock *sock, struct sock *newsk)
+{
+	call_void_hook(vsock_sk_clone, sock, newsk);
+}
+EXPORT_SYMBOL(security_vsock_sk_clone);
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ddd097790d47..7b92d6f2e0fd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5616,6 +5616,15 @@  static int selinux_tun_dev_open(void *security)
 	return 0;
 }
 
+static void selinux_socket_vsock_sk_clone(struct sock *sock, struct sock *newsk)
+{
+	struct sk_security_struct *sksec_sock = sock->sk_security;
+	struct sk_security_struct *sksec_new = newsk->sk_security;
+
+	/* Always returns 0 when packet SID is SECSID_NULL. */
+	WARN_ON_ONCE(selinux_conn_sid(sksec_sock->sid, SECSID_NULL, &sksec_new->sid));
+}
+
 #ifdef CONFIG_NETFILTER
 
 static unsigned int selinux_ip_forward(struct sk_buff *skb,
@@ -7228,6 +7237,7 @@  static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(tun_dev_attach_queue, selinux_tun_dev_attach_queue),
 	LSM_HOOK_INIT(tun_dev_attach, selinux_tun_dev_attach),
 	LSM_HOOK_INIT(tun_dev_open, selinux_tun_dev_open),
+	LSM_HOOK_INIT(vsock_sk_clone, selinux_socket_vsock_sk_clone),
 #ifdef CONFIG_SECURITY_INFINIBAND
 	LSM_HOOK_INIT(ib_pkey_access, selinux_ib_pkey_access),
 	LSM_HOOK_INIT(ib_endport_manage_subnet,