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

[Paul Moore]

On Tue, Dec 6, 2022 at 9:43 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> On Mon, Dec 5, 2022 at 9:58 PM Paolo Abeni <pabeni at redhat.com> wrote:
> >
> > On Fri, 2022-12-02 at 15:16 -0500, Paul Moore wrote:
> [...]
> > > What if we added a new LSM call in mptcp_subflow_create_socket(), just
> > > after the sock_create_kern() call?
> >
> > That should work, I think. I would like to propose a (last) attempt
> > that will not need an additional selinux hook - to try to minimize the
> > required changes and avoid unnecessary addional work for current and
> > future LSM mainteniance and creation.
> >
> > I tested the following patch and passes the reproducer (and mptcp self-
> > tests). Basically it introduces and uses a sock_create_nosec variant,
> > to allow mptcp_subflow_create_socket() calling
> > security_socket_post_create() with the corrct arguments. WDYT?

I'm still working my way through the recent patch posted by Paolo
(sorry, I lost some time due to build failures and other discussions),
but I wanted to comment on a few things in Ondrej's email ...

> This seems like a step in the right direction, but I wonder if we
> shouldn't solve the current overloading of the "kern" flag more
> explicitly - i.e. split it into two flags: one to indicate that the
> socket will only be used internally by the kernel ("internal") and
> another one to indicate if it should be labeled according to the
> current task or as a kernel-created socket ("kern"?).

The problem here is that labeling behavior isn't always going to be
the same across LSMs, or rather I don't want to force a specific
labeling behavior at the LSM layer.  We pass the @kern flag to
indicate a kernel socket that is not visible to userspace, but we
leave it up to the individual LSMs to handle that as they see fit
(including if they should label sockets based on @current, an
associated socket, or something else).

> Another concern I have about this approach is whether it is possible
> (in some more advanced scenario) for mptcp_subflow_create_socket() to
> be called in the context of a different task than the one
> creating/handling the main socket. Because then a potential socket
> accepted from the new subflow socket would end up with an unexpected
> (and probably semantically wrong) label. Glancing over the call tree,
> it seems it can be called via some netlink commands - presumably
> intended to be used by mptcpd?

Yes, this is something we need to be careful about, if we want to
treat all of these subflows just as we would treat the main socket,
the subflows need to be labeled the same as the main socket,
regardless of the process which creates them.

As a FYI to the non-SELinux folks, a socket's label can be important
even when it is not visible to userspace as the label is used both to
set the on-the-wire label (CIPSO, CALIPSO, labeled IPsec) and in the
per-packet access controls (see security_sock_rcv_skb()).

-- 
paul-moore.com