Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210317154448.1034471-1-dbrazdil@google.com>
In-Reply-To: <20210317154448.1034471-1-dbrazdil@google.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Wed, 17 Mar 2021 16:16:27 -0400
Message-ID: <CAHC9VhT_+i9V9N7NAdCCUgO5xBZpffvVPeh=jK8weZr3WzZ4Bw@mail.gmail.com>
Subject: Re: [PATCH] selinux: vsock: Set SID for socket returned by accept()
To:     David Brazdil <dbrazdil@google.com>
Cc:     selinux@vger.kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, linux-security-module@vger.kernel.org,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        James Morris <jmorris@namei.org>,
        "Serge E . Hallyn" <serge@hallyn.com>,
        Stephen Smalley <stephen.smalley.work@gmail.com>,
        Eric Paris <eparis@parisplace.org>,
        Kees Cook <keescook@chromium.org>,
        Jeff Vander Stoep <jeffv@google.com>,
        Alistair Delva <adelva@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

-- 
paul moore
www.paul-moore.com