[PATCH mptcp-net] mptcp: fix LSM labeling for passive msk

[Casey Schaufler]

On 12/7/2022 6:19 PM, Mat Martineau wrote:
> On Wed, 7 Dec 2022, Paolo Abeni wrote:
>
>> MPTCP sockets created via accept() inherit their LSM label
>> from the initial request socket, which in turn get it from the
>> listener socket's first subflow. The latter is a kernel socket,
>> and get the relevant labeling at creation time.
>>
>> Due to all the above even the accepted MPTCP socket get a kernel
>> label, causing unexpected behaviour and failure on later LSM tests.
>>
>> Address the issue factoring out a socket creation helper that does
>> not include the post-creation LSM checks. Use such helper to create
>> mptcp subflow as in-kernel sockets and doing explicitly LSM validation:
>> vs the current user for the first subflow, as a kernel socket otherwise.
>>
>> Fixes: 0c14846032f2 ("mptcp: fix security context on server socket")
>> Reported-by: Ondrej Mosnacek <omosnace at redhat.com>
>> Signed-off-by: Paolo Abeni <pabeni at redhat.com>
>
> The MPTCP content looks good to me:
>
> Acked-by: Mat Martineau <mathew.j.martineau at linux.intel.com>
>
>
> I didn't see issues with the socket.c changes but I'd like to get some
> security community feedback before upstreaming - Paul or other
> security reviewers, what do you think?

I haven't had the opportunity to work out what impact, if any, this will
have on Smack. I haven't seen a reproducer for the problem, is one available?
Sorry to chime in late.

>
>
> Thanks,
>
> Mat
>
>
>> ---
>> include/linux/net.h |  2 ++
>> net/mptcp/subflow.c | 19 ++++++++++++--
>> net/socket.c        | 60 ++++++++++++++++++++++++++++++---------------
>> 3 files changed, 59 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/linux/net.h b/include/linux/net.h
>> index b73ad8e3c212..91713012504d 100644
>> --- a/include/linux/net.h
>> +++ b/include/linux/net.h
>> @@ -251,6 +251,8 @@ int sock_wake_async(struct socket_wq *sk_wq, int
>> how, int band);
>> int sock_register(const struct net_proto_family *fam);
>> void sock_unregister(int family);
>> bool sock_is_registered(int family);
>> +int __sock_create_nosec(struct net *net, int family, int type, int
>> proto,
>> +            struct socket **res, int kern);
>> int __sock_create(struct net *net, int family, int type, int proto,
>>           struct socket **res, int kern);
>> int sock_create(int family, int type, int proto, struct socket **res);
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index d5ff502c88d7..e7e6f17df7ef 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -1646,11 +1646,26 @@ int mptcp_subflow_create_socket(struct sock
>> *sk, struct socket **new_sock)
>>     if (unlikely(!sk->sk_socket))
>>         return -EINVAL;
>>
>> -    err = sock_create_kern(net, sk->sk_family, SOCK_STREAM,
>> IPPROTO_TCP,
>> -                   &sf);
>> +    /* the subflow is created by the kernel, and we need kernel
>> annotation
>> +     * for lockdep's sake...
>> +     */
>> +    err = __sock_create_nosec(net, sk->sk_family, SOCK_STREAM,
>> IPPROTO_TCP,
>> +                  &sf, 1);
>>     if (err)
>>         return err;
>>
>> +    /* ... but the MPC subflow will be indirectly exposed to the
>> +     * user-space via accept(). Let's attach the current user security
>> +     * label to the first subflow, that is when msk->first is not yet
>> +     * initialized.
>> +     */
>> +    err = security_socket_post_create(sf, sk->sk_family, SOCK_STREAM,
>> +                      IPPROTO_TCP, !!mptcp_sk(sk)->first);
>> +    if (err) {
>> +        sock_release(sf);
>> +        return err;
>> +    }
>> +
>>     lock_sock(sf->sk);
>>
>>     /* the newly created socket has to be in the same cgroup as its
>> parent */
>> diff --git a/net/socket.c b/net/socket.c
>> index 55c5d536e5f6..d5d51e4e26ae 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -1426,23 +1426,11 @@ int sock_wake_async(struct socket_wq *wq, int
>> how, int band)
>> }
>> EXPORT_SYMBOL(sock_wake_async);
>>
>> -/**
>> - *    __sock_create - creates a socket
>> - *    @net: net namespace
>> - *    @family: protocol family (AF_INET, ...)
>> - *    @type: communication type (SOCK_STREAM, ...)
>> - *    @protocol: protocol (0, ...)
>> - *    @res: new socket
>> - *    @kern: boolean for kernel space sockets
>> - *
>> - *    Creates a new socket and assigns it to @res, passing through LSM.
>> - *    Returns 0 or an error. On failure @res is set to %NULL. @kern
>> must
>> - *    be set to true if the socket resides in kernel space.
>> - *    This function internally uses GFP_KERNEL.
>> - */
>>
>> -int __sock_create(struct net *net, int family, int type, int protocol,
>> -             struct socket **res, int kern)
>> +
>> +/*creates a socket leaving LSM post-creation checks to the caller */
>> +int __sock_create_nosec(struct net *net, int family, int type, int
>> protocol,
>> +            struct socket **res, int kern)
>> {
>>     int err;
>>     struct socket *sock;
>> @@ -1528,11 +1516,8 @@ int __sock_create(struct net *net, int family,
>> int type, int protocol,
>>      * module can have its refcnt decremented
>>      */
>>     module_put(pf->owner);
>> -    err = security_socket_post_create(sock, family, type, protocol,
>> kern);
>> -    if (err)
>> -        goto out_sock_release;
>> -    *res = sock;
>>
>> +    *res = sock;
>>     return 0;
>>
>> out_module_busy:
>> @@ -1548,6 +1533,41 @@ int __sock_create(struct net *net, int family,
>> int type, int protocol,
>>     rcu_read_unlock();
>>     goto out_sock_release;
>> }
>> +
>> +/**
>> + *    __sock_create - creates a socket
>> + *    @net: net namespace
>> + *    @family: protocol family (AF_INET, ...)
>> + *    @type: communication type (SOCK_STREAM, ...)
>> + *    @protocol: protocol (0, ...)
>> + *    @res: new socket
>> + *    @kern: boolean for kernel space sockets
>> + *
>> + *    Creates a new socket and assigns it to @res, passing through LSM.
>> + *    Returns 0 or an error. On failure @res is set to %NULL. @kern
>> must
>> + *    be set to true if the socket resides in kernel space.
>> + *    This function internally uses GFP_KERNEL.
>> + */
>> +
>> +int __sock_create(struct net *net, int family, int type, int protocol,
>> +          struct socket **res, int kern)
>> +{
>> +    struct socket *sock;
>> +    int err;
>> +
>> +    err = __sock_create_nosec(net, family, type, protocol, &sock,
>> kern);
>> +    if (err)
>> +        return err;
>> +
>> +    err = security_socket_post_create(sock, family, type, protocol,
>> kern);
>> +    if (err) {
>> +        sock_release(sock);
>> +        return err;
>> +    }
>> +
>> +    *res = sock;
>> +    return 0;
>> +}
>> EXPORT_SYMBOL(__sock_create);
>>
>> /**
>> -- 
>> 2.38.1
>>
>>
>>
>
> -- 
> Mat Martineau
> Intel