Broken SELinux/LSM labeling with MPTCP and accept(2)

[Paul Moore]

On Thu, Dec 1, 2022 at 1:26 PM Paolo Abeni <pabeni at redhat.com> wrote:
> Hello,

Hello all.

> On Thu, 2022-12-01 at 14:42 +0100, Ondrej Mosnacek wrote:
> > As discovered by our QE, there is a problem with how the
> > (userspace-facing) sockets returned by accept(2) are labeled when
> > using MPTCP. Currently they always end up with the label representing
> > the kernel (typically system_u:system_r:kernel_t:s0), white they
> > should inherit the context from the parent socket (the one that is
> > passed to accept(2)) ...

Ooof, that's unfortunate :/  Thanks for reporting it though ...

> > So now the big question is: How to fix this? I can think of several
> > possible solutions, but neither of them seems to be the obvious
> > correct one:
> > 1. Wrap the socket cloned from the subflow socket in another socket
> > (similar to how the main socket + subflow(s) are handled), which would
> > be cloned from the non-kern outer socket that has the right label.
> > This could have the disadvantage of adding unnecessary overhead, but
> > would probably be simpler to do.
>
> I would avoid that option: we already have a suboptimal amount of
> indirection.

Agreed, I'm not sure I want to push for this as a fix.

> > 2. Somehow ensure that the cloned socket gets the label from the main
> > socket instead of the subflow socket. This would probably require
> > adding a new LSM hook and I'm not sure at all what would be the best
> > way to implement this.

2a. (?)  Another option would simply be ensuring that the subflow
socket has the proper LSM/SELinux label from the start.  This is
arguably The Right Thing To Do regardless, and in cases where explicit
packet labeling is used, e.g. CALIPSO, it may be necessary to do this
to ensure the traffic is labeled correctly from the start (I would
need to spend more time looking at the MPTCP code to say for certain
here).

> > 3. Somehow communicate the subflow socket <-> main socket relationship
> > to the LSM layer so that it can switch to use the label of the main
> > socket when handling an operation on a subflow socket (thus copying
> > the label correctly on accept(2)). Not a great solution, as it
> > requires each LSM that labels sockets to duplicate the indirection
> > logic.
> > 4. Do not create the subflow sockets as "kern". (Not sure if that
> > would be desirable.)
>
> No, we need subflow being kernel sockets. Lockdep will bail otherwise,
> and the would be the tip of the iceberg.
>
> > 5. Stop labeling kern sockets with the kernel's label on the SELinux
> > side and just label them based on the current task as usual. (This
> > would probably cause other issues, but maybe not...)

I'm not sure we want to open that can of worms right now.

> > Any ideas, suggestions, or patches welcome!
>
> I think something alike the following could work - not even tested,
> with comments on bold assumptions.
>
> I'll try to do some testing later.
>
> ---
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 99f5e51d5ca4..6cad50c6fd24 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -102,6 +102,20 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
>         if (err)
>                 return err;
>
> +       /* The first subflow can later be indirectly exposed to security
> +        * relevant syscall alike accept() and bind(), and at this point
> +        * carries a 'kern' related security context.
> +        * Reset the security context to the relevant user-space one.
> +        * Note that the following assumes security_socket_post_create()
> +        * being idempotent
> +        */
> +       err = security_socket_post_create(ssock, sk->sk_family, SOCK_STREAM,
> +                                         IPPROTO_TCP, 0);
> +       if (err) {
> +               sock_release(ssock);
> +               return err;
> +       }

I'm not sure we want to start calling security_socket_post_create()
twice on a given socket, *maybe* it works okay now but that seems like
an awkward constraint to put on future LSMs (or changes to existing
ones).  If we decide that the best approach is to add a LSM hook call
here, we should create a new hook instead of reusing an existing one;
I think this falls under Ondrej's possible solution #2.

However, I think this simplest solution might be what I mentioned
above as #2a, simply labeling the subflow socket properly from the
beginning.  In the case of SELinux I think we could do that by simply
clearing the @kern flag in the case of IPPROTO_MPTCP; completely
untested (and likely whitespace mangled) hack shown below:

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f553c370397e..de6aa80b2319 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4562,6 +4562,7 @@ static int selinux_socket_create(int family, int type,
       u16 secclass;
       int rc;

+       kern = (protocol == IPPROTO_MPTCP ? 0 : kern);
       if (kern)
               return 0;

@@ -4584,6 +4585,7 @@ static int selinux_socket_post_create(struct
socket *sock, int family,
       u32 sid = SECINITSID_KERNEL;
       int err = 0;

+       kern = (protocol == IPPROTO_MPTCP ? 0 : kern);
       if (!kern) {
               err = socket_sockcreate_sid(tsec, sclass, &sid);
               if (err)

... of course the downside is that this is not a general cross-LSM
solution, but those are very hard, if not impossible, to create as the
individual LSMs can vary tremendously.  There is also the downside of
having to have a protocol specific hack in the LSM socket creation
hooks, which is a bit of a bummer, but then again we already have to
do so much worse elsewhere in the LSM networking hooks that this is
far from the worst thing we've had to do.

All this of course assumes that the quick little hack even works, I
may be missing something very obvious ... ;)

-- 
paul-moore.com