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

From: Paolo Abeni <pabeni@redhat.com>
To: Paul Moore <paul@paul-moore.com>
Cc: linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	 mptcp@lists.linux.dev
Subject: Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook
Date: Wed, 21 Dec 2022 20:23:58 +0100	[thread overview]
Message-ID: <944c4ab043713f75ad3bb512fc146e48de7b3e25.camel@redhat.com> (raw)
In-Reply-To: <CAHC9VhRYr9=qKUeF0EuY46koCnkeZ5d-=umV5TxbiUZ7qNXJ6w@mail.gmail.com>

On Tue, 2022-12-20 at 17:07 -0500, Paul Moore wrote:
> On Mon, Dec 19, 2022 at 12:34 PM Paolo Abeni <pabeni@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@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 :(

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

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?

Thanks,

Paolo