lsm: introduce and use security_mptcp_add_subflow()

MPTCP can create subflows in kernel context, and later indirectly expose them to user-space, via the owning mptcp socket.

As discussed in the reported link, the above causes unexpected failures for server, MPTCP-enabled applications.

Let's introduce a new LSM hook to allow the security module to relabel the subflow according to the owing process.

Note that the new hook requires both the mptcp socket and the new subflow. This could allow future extensions, e.g. explicitly validating the mptcp <-> subflow linkage.

Link https://lore.kernel.org/mptcp/CAHC9VhTNh-YwiyTds=P1e3rixEDqbRTFj22bpya=+qJqfcaMfg@mail.gmail.com/ open here

Signed-off-by Paolo Abeni <[email protected]>

---
v1 -> v2:
- fix build issue with !CONFIG_SECURITY_NETWORK
---

include/linux/lsm_hook_defs.h | 1 +
include/linux/lsm_hooks.h     | 9 +++++++++
include/linux/security.h      | 6 ++++++
net/mptcp/subflow.c           | 6 ++++++
security/security.c           | 5 +++++
5 files changed, 27 insertions(+)

net/mptcp/subflow.c +4 -0

index bd387d4b5a38..43b90784d914 100644
@@ -1680,6 +1680,10 @@ int mptcp_subflow_create_socket(struct sock *sk, struct socket **new_sock)
 
 	lock_sock(sf->sk);
 
+	err = security_mptcp_add_subflow(sk, sf->sk);
+	if (err)
+		goto release_ssk;
+
 	/* the newly created socket has to be in the same cgroup as its parent */
 	mptcp_attach_cgroup(sk, sf->sk);
 

Newly added subflows should inherit the associated label from the current process context, regarless of the sk_kern_sock flag value.

This patch implements the above resetting the subflow sid, deleting the existing subflow label, if any, and then re-creating a new one.

The new helper reuses the selinux_netlbl_sk_security_free() function, and it can end-up being called multiple times with the same argument; we additionally need to make it idempotent.

Signed-off-by Paolo Abeni <[email protected]>

---
v1 -> v2:
- fix build issue with !CONFIG_NETLABEL

---
 security/selinux/hooks.c    | 27 +++++++++++++++++++++++++++
 security/selinux/netlabel.c |  4 +++-
 2 files changed, 30 insertions(+), 1 deletion(-)

security/selinux/hooks.c +15 -0

index 3c5be76a9199..f785600b666a 100644
@@ -5476,6 +5476,32 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
        selinux_netlbl_sctp_sk_clone(sk, newsk);
 }
 
+static int selinux_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
+{
+       const struct task_security_struct *tsec = selinux_cred(current_cred());
+       struct sk_security_struct *ssksec = ssk->sk_security;
+       u16 sclass;
+       u32 sid;
+       int err;
+
+       /* create the sid using the current cred, regardless of the ssk kern
+        * flag
+        */
+       sclass = socket_type_to_security_class(ssk->sk_family, ssk->sk_type,
+                                              ssk->sk_protocol);
+       err = socket_sockcreate_sid(tsec, sclass, &sid);
+       if (err)

security/selinux/netlabel.c +0 -0

index 1321f15799e2..8e0080b8a8ef 100644

Quoted 53 lines · Paolo Abeni

Newly added subflows should inherit the associated label from the current process context, regarless of the sk_kern_sock flag value.

This patch implements the above resetting the subflow sid, deleting the existing subflow label, if any, and then re-creating a new one.

The new helper reuses the selinux_netlbl_sk_security_free() function, and it can end-up being called multiple times with the same argument; we additionally need to make it idempotent.

Signed-off-by Paolo Abeni <[email protected]>

---
v1 -> v2:
- fix build issue with !CONFIG_NETLABEL

---
 security/selinux/hooks.c    | 27 +++++++++++++++++++++++++++
 security/selinux/netlabel.c |  4 +++-
 2 files changed, 30 insertions(+), 1 deletion(-)

security/selinux/hooks.c +24 -0

index 3c5be76a9199..f785600b666a 100644
@@ -5476,6 +5476,32 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
        selinux_netlbl_sctp_sk_clone(sk, newsk);
 }
 
+static int selinux_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
+{
+       const struct task_security_struct *tsec = selinux_cred(current_cred());
+       struct sk_security_struct *ssksec = ssk->sk_security;
+       u16 sclass;
+       u32 sid;
+       int err;
+
+       /* create the sid using the current cred, regardless of the ssk kern
+        * flag
+        */
+       sclass = socket_type_to_security_class(ssk->sk_family, ssk->sk_type,
+                                              ssk->sk_protocol);
+       err = socket_sockcreate_sid(tsec, sclass, &sid);
+       if (err)
+               return err;
+
+       ssksec->sid = sid;
+
+       /* replace the existing subflow label deleting the existing one
+        * and re-recrating a new label using the current context
+        */
+       selinux_netlbl_sk_security_free(ssksec);
+       return selinux_netlbl_socket_post_create(ssk, ssk->sk_family);

security/selinux/hooks.c static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk · line 5503

Quoted 1 line

security/selinux/hooks.c +1 -0

+}

I thought the idea was to ensure that new subflows of an existing MPTCP connection would be created with the same label as the main MPTCP connection socket? The code above labels the new subflow based on the current process, not the main MPTCP connection; it matches the commit description, but not what we had previously discussed - or I am horribly mis-remembering something? :)

sk_prot->init() hook. The security_socket_post_create() call on the

Quoted 54 lines · Paul Moore

Quoted 53 lines · Paolo Abeni

Newly added subflows should inherit the associated label from the current process context, regarless of the sk_kern_sock flag value.

This patch implements the above resetting the subflow sid, deleting the existing subflow label, if any, and then re-creating a new one.

The new helper reuses the selinux_netlbl_sk_security_free() function, and it can end-up being called multiple times with the same argument; we additionally need to make it idempotent.

Signed-off-by Paolo Abeni <[email protected]>

---
v1 -> v2:
- fix build issue with !CONFIG_NETLABEL

---
 security/selinux/hooks.c    | 27 +++++++++++++++++++++++++++
 security/selinux/netlabel.c |  4 +++-
 2 files changed, 30 insertions(+), 1 deletion(-)

security/selinux/hooks.c +24 -0

index 3c5be76a9199..f785600b666a 100644
@@ -5476,6 +5476,32 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
        selinux_netlbl_sctp_sk_clone(sk, newsk);
 }
 
+static int selinux_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
+{
+       const struct task_security_struct *tsec = selinux_cred(current_cred());
+       struct sk_security_struct *ssksec = ssk->sk_security;
+       u16 sclass;
+       u32 sid;
+       int err;
+
+       /* create the sid using the current cred, regardless of the ssk kern
+        * flag
+        */
+       sclass = socket_type_to_security_class(ssk->sk_family, ssk->sk_type,
+                                              ssk->sk_protocol);
+       err = socket_sockcreate_sid(tsec, sclass, &sid);
+       if (err)
+               return err;
+
+       ssksec->sid = sid;
+
+       /* replace the existing subflow label deleting the existing one
+        * and re-recrating a new label using the current context
+        */
+       selinux_netlbl_sk_security_free(ssksec);
+       return selinux_netlbl_socket_post_create(ssk, ssk->sk_family);

Quoted 8 lines

security/selinux/hooks.c static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk · line 5503

Quoted 1 line

security/selinux/hooks.c +1 -0

+}

I thought the idea was to ensure that new subflows of an existing MPTCP connection would be created with the same label as the main MPTCP connection socket? The code above labels the new subflow based on the current process, not the main MPTCP connection; it matches the commit description, but not what we had previously discussed - or I am horribly mis-remembering something? :)

You are right, I picked a wrong turn.

I just tested the other option and there is another problem :(

The first subflow creations happens inside af_inet->create, via the sk-

Quoted 1 line

sk_prot->init() hook. The security_socket_post_create() call on the

owning MPTCP sockets happens after that point. So we copy data from a not yet initialized security context (and the test fail badly).

There are a few options to cope with that:
- [ugly hack] call security_socket_post_create() on the mptcp code before creating the subflow. I experimented this just to double the problem and a possible solution.

- refactor the mptcp code to create the first subflow on later syscalls, as needed. This will require quite a bit of refactoring in the MPTCP protocol as we will need also to update the shutdown/disconnect accordingly (currently we keep the first subflow around, instead we will need to close it).

- use the code proposed in these patches as-is ;)

WDYT?

Quoted 1 line · Paolo Abeni

I just tested the other option and there is another problem :(

It's never easy, is it? ;)

Quoted 4 lines

The first subflow creations happens inside af_inet->create, via the sk-

Quoted 1 line

sk_prot->init() hook. The security_socket_post_create() call on the

owning MPTCP sockets happens after that point. So we copy data from a not yet initialized security context (and the test fail badly).

Hmmm. Let's come back to this later on down this email.

Quoted 4 lines

There are a few options to cope with that:
- [ugly hack] call security_socket_post_create() on the mptcp code before creating the subflow. I experimented this just to double the problem and a possible solution.

I'm guessing "[ugly hack]" is probably a bit of an understatement. Let's see if we can do better before we explore this option too much further.

Quoted 1 line

WDYT?

Let's go back to the the inet_create() case for a little bit. I'm thinking we might be able to do something by leveraging the sk_alloc()->sk_prot_alloc()->security_sk_alloc() code path. As inet_create() is going to be called from task context here, it seems like we could do the sock's sid/sclass determination here, cached in separate fields in the sk_security_struct if necessary, and use those in a new MPTCP subflow hook. We could also update selinux_socket_post_create() to take advantage of this as well. We could also possibly pass the proto struct into security_sk_alloc() if we needed to identify IPPROTO_MPTCP there as well.

I'll admit to not chasing down all the details, but I suspect this may be the cleanest option - thoughts?

Quoted 12 lines · Paul Moore

Quoted 1 line · Paolo Abeni

I just tested the other option and there is another problem :(

It's never easy, is it? ;)

Quoted 4 lines

The first subflow creations happens inside af_inet->create, via the sk-

Quoted 1 line

sk_prot->init() hook. The security_socket_post_create() call on the

owning MPTCP sockets happens after that point. So we copy data from a not yet initialized security context (and the test fail badly).

Hmmm. Let's come back to this later on down this email.

Quoted 8 lines

Quoted 4 lines

There are a few options to cope with that:
- [ugly hack] call security_socket_post_create() on the mptcp code before creating the subflow. I experimented this just to double the problem and a possible solution.

I'm guessing "[ugly hack]" is probably a bit of an understatement. Let's see if we can do better before we explore this option too much further.

Yup, I compiled the list in "brainstom-mode", trying to include whatever would be possible even if clearly not suitable.

[...]

Quoted 13 lines

Quoted 1 line

WDYT?

Let's go back to the the inet_create() case for a little bit. I'm thinking we might be able to do something by leveraging the sk_alloc()->sk_prot_alloc()->security_sk_alloc() code path. As inet_create() is going to be called from task context here, it seems like we could do the sock's sid/sclass determination here, cached in separate fields in the sk_security_struct if necessary, and use those in a new MPTCP subflow hook. We could also update selinux_socket_post_create() to take advantage of this as well. We could also possibly pass the proto struct into security_sk_alloc() if we needed to identify IPPROTO_MPTCP there as well.

Quoted 2 lines

I'll admit to not chasing down all the details, but I suspect this may be the cleanest option - thoughts?

Thanks, I did not consider such possibility!

I think we should be careful to avoid increasing sk_security_struct size. Currently it is 16 bytes, nicely matching a kmalloc slab, any increase will move it on kmalloc-32 bytes slab possibly causing performance and memory regressions).

More importantly, I think there is a problem with the sk_clone_lock() -> sk_prot_alloc() -> security_sk_alloc() code path.

sk_clone_lock() happens in BH context, if security_transition_sid() needs process context that would be a problem - quickly skimming the code it does not look so, I need to double check.

Perhaps the cleanest option could be the one involving the mptcp refactoring, moving subflow creation at a later stage. It could have some minor side benefit for MPTCP, too - solving:

https://github.com/multipath-tcp/mptcp_net-next/issues/290

but I'm not fond of that option because it will require quite a bit of
time: we need first to have the mptcp refactor in place and then cook the lsm patches. I guess such process will require at least 2 release cycles, due to the needed mptcp(netdev)/lsm trees synchronization.

If that would prove to be the most reasonable option, could we consider to transiently merge first something alike:

https://lore.kernel.org/mptcp/CAHC9VhSQnhH3UL4gqzu+YiA1Q3YyLLCv88gLJOvw-0+uw5Lvkw@mail.gmail.com/T/#m06c612f84f6b6fe759e670573b2c8092df71607b

to have a workable short-term solution, and later revert it when the final solution would be in place?

Quoted 44 lines · Paolo Abeni

Quoted 12 lines · Paul Moore

Quoted 1 line · Paolo Abeni

I just tested the other option and there is another problem :(

It's never easy, is it? ;)

Quoted 4 lines

The first subflow creations happens inside af_inet->create, via the sk-

Quoted 1 line

sk_prot->init() hook. The security_socket_post_create() call on the

owning MPTCP sockets happens after that point. So we copy data from a not yet initialized security context (and the test fail badly).

Hmmm. Let's come back to this later on down this email.

Quoted 8 lines

Quoted 4 lines

There are a few options to cope with that:
- [ugly hack] call security_socket_post_create() on the mptcp code before creating the subflow. I experimented this just to double the problem and a possible solution.

I'm guessing "[ugly hack]" is probably a bit of an understatement. Let's see if we can do better before we explore this option too much further.

Yup, I compiled the list in "brainstom-mode", trying to include whatever would be possible even if clearly not suitable.

[...]

Quoted 13 lines

Quoted 1 line

WDYT?

Let's go back to the the inet_create() case for a little bit. I'm thinking we might be able to do something by leveraging the sk_alloc()->sk_prot_alloc()->security_sk_alloc() code path. As inet_create() is going to be called from task context here, it seems like we could do the sock's sid/sclass determination here, cached in separate fields in the sk_security_struct if necessary, and use those in a new MPTCP subflow hook. We could also update selinux_socket_post_create() to take advantage of this as well. We could also possibly pass the proto struct into security_sk_alloc() if we needed to identify IPPROTO_MPTCP there as well.

Quoted 2 lines

I'll admit to not chasing down all the details, but I suspect this may be the cleanest option - thoughts?

Quoted 6 lines

Thanks, I did not consider such possibility!

I think we should be careful to avoid increasing sk_security_struct size. Currently it is 16 bytes, nicely matching a kmalloc slab, any increase will move it on kmalloc-32 bytes slab possibly causing performance and memory regressions).

FWIW, it is likely that this will end up growing in the future to support stacking LSMs. It's unfortunate, messy, and generally ugly, but inevitable.

See the selinux_inode() function as a similar example using inode/inode_security_struct.

Quoted 7 lines

More importantly, I think there is a problem with the sk_clone_lock() -> sk_prot_alloc() -> security_sk_alloc() code path.

sk_clone_lock() happens in BH context, if security_transition_sid() needs process context that would be a problem - quickly skimming the code it does not look so, I need to double check.

The problem is that in both selinux_socket_create() and selinux_socket_post_create() the credentials from @current are needed to determine the sock/sk_security_stuct label. In selinux_socket_create() @current's credentials are also used to determine if the socket can be created.

It's looking like doing labeling determinations in the security_sk_alloc() struct is not going to work. While sk_clone_lock() will end up calling into security_sk_clone()/selinux_sk_clone_security() via sock_copy(), if I understand you correctly that won't help as the main MPTCP socket is not yet setup (e.g. selinux_socket_post_create() has not yet been run on the main socket).

Quoted 10 lines

Perhaps the cleanest option could be the one involving the mptcp refactoring, moving subflow creation at a later stage. It could have some minor side benefit for MPTCP, too - solving:

https://github.com/multipath-tcp/mptcp_net-next/issues/290

but I'm not fond of that option because it will require quite a bit of
time: we need first to have the mptcp refactor in place and then cook the lsm patches. I guess such process will require at least 2 release cycles, due to the needed mptcp(netdev)/lsm trees synchronization.

I generally take the long term view when it comes to Linux Kernel development; given the nature of the kernel, and all the constraints that come with it, I would much rather pursue solutions that we believe have the longest staying power.

I'm also happy to work on, and/or review, LSM patches in conjunction with a MPTCP refactor. If the only reason to split the work over two kernel releases is to soften the blow during the merge window, I think we can work that out in a single release ... at least I say that now :)

Basically when it comes down to it, I want to make sure that any fix we come up with *works*. In my mind that means doing the LSM fix in conjunction with the rework; I would hate to see all of the rework happen only to find out later that it still didn't solve the LSM problem.

Does that make sense?

Quoted 5 lines

If that would prove to be the most reasonable option, could we consider to transiently merge first something alike:

https://lore.kernel.org/mptcp/CAHC9VhSQnhH3UL4gqzu+YiA1Q3YyLLCv88gLJOvw-0+uw5Lvkw@mail.gmail.com/T/#m06c612f84f6b6fe759e670573b2c8092df71607b

Quoted 2 lines

to have a workable short-term solution, and later revert it when the final solution would be in place?

I would need to go back through that to make sure that it makes sense, and ensure that the hook is idempotent for SELinux, AppArmor, and Smack (current hook implementations), *aaaand* if we promise that this is just a *temporary* hack I think I would be okay with that.

Hello,

I'm sorry for the long delay: I was on PTO with limited internet access.

Quoted 3 lines · Paolo Abeni

Quoted 2 lines · Paul Moore

Quoted 1 line · Paolo Abeni

Quoted 10 lines

I generally take the long term view when it comes to Linux Kernel development; given the nature of the kernel, and all the constraints that come with it, I would much rather pursue solutions that we believe have the longest staying power.

I'm also happy to work on, and/or review, LSM patches in conjunction with a MPTCP refactor. If the only reason to split the work over two kernel releases is to soften the blow during the merge window, I think we can work that out in a single release ... at least I say that now :)

I thought about doing the MPTCP and selinux patches sequentially to both avoid the possibly untrivial conflicts resultion issues and to ensure that the mptcp patches are in place when the selinux ones are applied, as there is a fuctional dependency.

Quoted 7 lines

Basically when it comes down to it, I want to make sure that any fix we come up with *works*. In my mind that means doing the LSM fix in conjunction with the rework; I would hate to see all of the rework happen only to find out later that it still didn't solve the LSM problem.

Does that make sense?

Indeed it makes sense to me.

I think we can address that concern in a quite consolidated way. We usually include in the MPTCP tree a (very limited) number of patches that will not be submitted to the netdev because belong to other trees and/or are handled/owned by others devel.

We use the above e.g. to fix build and/or functional issues in our self-tests caused by other subsystems without the need to wait for the proper fix to land into vanilla. When such event happen, we simply drop the local copy of the fixup patch.

We could use a similar schema in this scenario. We can include the the LSM patches to the mptcp in our tree while the rework is in progress to ensure that overall the effort really addresses the LSM issue.

We can rebase the LSM patches as needed to address conflicts as needed/if/when they pops up.

Once that the mptcp patches will land into the LSM tree, we will submit formally the LSM ones to you. During the process I'll check and ensure that the LSM issue is really/still fixed. Would that work for you?

Quoted 5 lines

Quoted 5 lines

If that would prove to be the most reasonable option, could we consider to transiently merge first something alike:

https://lore.kernel.org/mptcp/CAHC9VhSQnhH3UL4gqzu+YiA1Q3YyLLCv88gLJOvw-0+uw5Lvkw@mail.gmail.com/T/#m06c612f84f6b6fe759e670573b2c8092df71607b

Quoted 7 lines

Quoted 2 lines

to have a workable short-term solution, and later revert it when the final solution would be in place?

I would need to go back through that to make sure that it makes sense, and ensure that the hook is idempotent for SELinux, AppArmor, and Smack (current hook implementations), *aaaand* if we promise that this is just a *temporary* hack I think I would be okay with that.

I would appreciate that addtional extra mile a lot, as it will allow a (temporary!) fix to be delivered quite earlier than what the above process will provide.

Of course the deal is that I'll take ownership of pursuing the complete fix till resolution.