On Wed, Mar 17, 2021 at 11:44 AM David Brazdil <dbrazdil@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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.
--
paul moore
www.paul-moore.com
