| Message ID | 20210317154448.1034471-1-dbrazdil@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selinux: vsock: Set SID for socket returned by accept() | expand |
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.
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 --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,
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(+)