Hello,
On Thu, 2022-12-08 at 18:40 -0500, Paul Moore wrote:
> On Wed, Dec 7, 2022 at 9:19 PM Mat Martineau
> <mathew.j.martineau@xxxxxxxxxxxxxxx> wrote:
> > On Wed, 7 Dec 2022, Paolo Abeni wrote:
> >
> > > MPTCP sockets created via accept() inherit their LSM label
> > > from the initial request socket, which in turn get it from the
> > > listener socket's first subflow. The latter is a kernel socket,
> > > and get the relevant labeling at creation time.
> > >
> > > Due to all the above even the accepted MPTCP socket get a kernel
> > > label, causing unexpected behaviour and failure on later LSM tests.
> > >
> > > Address the issue factoring out a socket creation helper that does
> > > not include the post-creation LSM checks. Use such helper to create
> > > mptcp subflow as in-kernel sockets and doing explicitly LSM validation:
> > > vs the current user for the first subflow, as a kernel socket otherwise.
> > >
> > > Fixes: 0c14846032f2 ("mptcp: fix security context on server socket")
> > > Reported-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > > Signed-off-by: Paolo Abeni <pabeni@xxxxxxxxxx>
> >
> > The MPTCP content looks good to me:
> >
> > Acked-by: Mat Martineau <mathew.j.martineau@xxxxxxxxxxxxxxx>
> >
> > I didn't see issues with the socket.c changes but I'd like to get some
> > security community feedback before upstreaming - Paul or other security
> > reviewers, what do you think?
>
> Sorry, I was distracted by other things the past few days ...
>
> One thing that jumps out is the potential for misuse of
> __sock_create_nosec(); I can see people accidentally using this
> function by accident in other areas of the stack and causing a new set
> of problems.
>
> We discussed this in the other thread, but there is an issue with
> subflows being labeled based on the mptcp_subflow_create_socket()
> caller and not the main MPTCP socket.
>
> I know there is a desire to get a small (in size) patch to fix this,
> but I think creating a new LSM hook may be the only way to solve this
> in a sane manner. My original thought was a new LSM hook call inside
> mptcp_subflow_create_socket() right after the sock_create_kern() call.
> The only gotcha is that it would occur after
> security_socket_post_create(), but that should be easy enough to
> handle inside the LSMs.
If the preferrede solution is via a new LSM hook I have no objections
at all. I'm sorry to put another hook mainteinance on you.
How do you propose to preceed? After quickly digging into the relevant
LSM code, the only module I think I could handle in a reasonable
timeframe is selinux. Hopefully the new hook implementation should be
quite straight-forward for the relevant SME.
I guess the patch[es] should target the LSM tree, as the change in the
mptcp code should be a one-liner. On the flip side, that would likely
lead to some merge conflict, as the mptcp protocol impl. is quite a
moving target.
Cheers,
Paolo
