Re: [PATCH] selinux: vsock: Set SID for socket returned by accept()
flat view
From: Paul Moore <paul@paul-moore.com>
Date: 2021-03-17 20:17:44
Also in:
lkml, netdev, selinux
On Wed, Mar 17, 2021 at 11:44 AM David Brazdil [off-list ref] 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 <redacted> --- 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.
quoted hunk ↗ jump to hunk
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 :)
quoted hunk ↗ jump to hunk
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. -- paul moore www.paul-moore.com