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

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

[Paolo Abeni]

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@redhat.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 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

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

[Mat Martineau]

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@redhat.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

The MPTCP content looks good to me:

Acked-by: Mat Martineau <mathew.j.martineau@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?


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

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

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

[Mat Martineau]

On Thu, 8 Dec 2022, Casey Schaufler wrote:

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

Hi Casey -

There's more context in the original thread started by Ondrej, including 
reproducer information:

https://lore.kernel.org/all/CAFqZXNs2LF-OoQBUiiSEyranJUXkPLcCfBkMkwFeM6qEwMKCTw@mail.gmail.com/

For impact on Smack, also check Paul's recent reply to this specific patch 
(proposing a new LSM hook):

https://lore.kernel.org/all/CAHC9VhQzJAhNtpMnU7STEfq6QpaJo-xiie8HoHH2w3io3aXBHw@mail.gmail.com/

--
Mat Martineau
Intel

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

[Paul Moore]

On Wed, Dec 7, 2022 at 9:19 PM Mat Martineau
<mathew.j.martineau@linux.intel.com> 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@redhat.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>
> The MPTCP content looks good to me:
>
> Acked-by: Mat Martineau <mathew.j.martineau@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?

Sorry, I was distracted by other things the past few days ...

One thing that jumps out is the potential for misuse of
__sock_create_nosec(); I can see people accidentally using this
function by accident in other areas of the stack and causing a new set
of problems.

We discussed this in the other thread, but there is an issue with
subflows being labeled based on the mptcp_subflow_create_socket()
caller and not the main MPTCP socket.

I know there is a desire to get a small (in size) patch to fix this,
but I think creating a new LSM hook may be the only way to solve this
in a sane manner.  My original thought was a new LSM hook call inside
mptcp_subflow_create_socket() right after the sock_create_kern() call.
The only gotcha is that it would occur after
security_socket_post_create(), but that should be easy enough to
handle inside the LSMs.

-- 
paul-moore.com

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

[Paolo Abeni]

Hello,

On Thu, 2022-12-08 at 18:40 -0500, Paul Moore wrote:
> On Wed, Dec 7, 2022 at 9:19 PM Mat Martineau
> <mathew.j.martineau@linux.intel.com> 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@redhat.com>
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > 
> > The MPTCP content looks good to me:
> > 
> > Acked-by: Mat Martineau <mathew.j.martineau@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?
> 
> Sorry, I was distracted by other things the past few days ...
> 
> One thing that jumps out is the potential for misuse of
> __sock_create_nosec(); I can see people accidentally using this
> function by accident in other areas of the stack and causing a new set
> of problems.
> 
> We discussed this in the other thread, but there is an issue with
> subflows being labeled based on the mptcp_subflow_create_socket()
> caller and not the main MPTCP socket.
> 
> I know there is a desire to get a small (in size) patch to fix this,
> but I think creating a new LSM hook may be the only way to solve this
> in a sane manner.  My original thought was a new LSM hook call inside
> mptcp_subflow_create_socket() right after the sock_create_kern() call.
> The only gotcha is that it would occur after
> security_socket_post_create(), but that should be easy enough to
> handle inside the LSMs.

If the preferrede solution is via a new LSM hook I have no objections
at all. I'm sorry to put another hook mainteinance on you.

How do you propose to preceed? After quickly digging into the relevant
LSM code, the only module I think I could handle in a reasonable
timeframe is selinux. Hopefully the new hook implementation should be
quite straight-forward for the relevant SME.

I guess the patch[es] should target the LSM tree, as the change in the
mptcp code should be a one-liner. On the flip side, that would likely
lead to some merge conflict, as the mptcp protocol impl. is quite a
moving target.

Cheers,

Paolo

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

[Paul Moore]

On Mon, Dec 12, 2022 at 10:36 AM Paolo Abeni <pabeni@redhat.com> wrote:
> If the preferrede solution is via a new LSM hook I have no objections
> at all. I'm sorry to put another hook mainteinance on you.

Don't worry about it, sure there is a bit more code, but I think it's
a better approach than all of the alternatives we've attempted.  I'd
rather have "better" than a hack ;)

> How do you propose to preceed? After quickly digging into the relevant
> LSM code, the only module I think I could handle in a reasonable
> timeframe is selinux. Hopefully the new hook implementation should be
> quite straight-forward for the relevant SME.

That's sounds reasonable to me.  Our policy in LSM land is that you
should provide at least one LSM implementation when adding a new hook
(the BPF LSM doesn't count due to it's rather unique approach), so if
you can provide a SELinux implementation that should meet that
requirement.  I'm also not sure that any of the other LSMs currently
claim support for MPTCP.

> I guess the patch[es] should target the LSM tree, as the change in the
> mptcp code should be a one-liner. On the flip side, that would likely
> lead to some merge conflict, as the mptcp protocol impl. is quite a
> moving target.

The LSM has also seen a lot of renewed activity lately.  However, I
think the deciding factor is where the bulk of the code will live, and
with the vast majority of the code expected under security/, I think
it makes sense to merge this via the LSM tree.  My general policy for
the LSM tree is to either base your patches of Linus' tree or the
lsm/next branch and I'll handle whatever merge conflicts arise at the
time of merging.

For reference, the current LSM tree can be found here:

* https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git

-- 
paul-moore.com