netdev - Re: [PATCH] selinux: vsock: Set SID for socket returned by accept()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhT_+i9V9N7NAdCCUgO5xBZpffvVPeh=jK8weZr3WzZ4Bw@mail.gmail.com>
Date:   Wed, 17 Mar 2021 16:16:27 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     David Brazdil <dbrazdil@...gle.com>
Cc:     selinux@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, linux-security-module@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>,
        Kees Cook <keescook@...omium.org>,
        Jeff Vander Stoep <jeffv@...gle.com>,
        Alistair Delva <adelva@...gle.com>
Subject: Re: [PATCH] selinux: vsock: Set SID for socket returned by accept()

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

-- 
paul moore
www.paul-moore.com