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