[PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paul Moore]

On Wed, Dec 21, 2022 at 2:24 PM Paolo Abeni <pabeni at redhat.com> wrote:
> On Tue, 2022-12-20 at 17:07 -0500, Paul Moore wrote:
> > On Mon, Dec 19, 2022 at 12:34 PM Paolo Abeni <pabeni at redhat.com> wrote:
> > >
> > > 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 <pabeni at redhat.com>
> > > ---
> > > 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(-)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 3c5be76a9199..f785600b666a 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -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);
> > > +}
> >
> > 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 :(

It's never easy, is it? ;)

> The first subflow creations happens inside af_inet->create, via the sk-
> >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.

> 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.

> - 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).

Let's see if we can avoid having to do this too.

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

That's a non-starter from a SELinux perspective as the label is
incorrect.  It would be similar to saying that for each fork() call
the resulting child process would always run as root since we couldn't
figure out how to assign the credentials properly :)

> 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?

--
paul-moore.com