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@xxxxxxxxxx>
>> Signed-off-by: Paolo Abeni <pabeni@xxxxxxxxxx>
>
> The MPTCP content looks good to me:
>
> Acked-by: Mat Martineau <mathew.j.martineau@xxxxxxxxxxxxxxx>
>
>
> 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
