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

[Mat Martineau <mathew.j.martineau@xxxxxxxxxxxxxxx>]

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?


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