Message-ID: <CAHC9VhRV4j9i7YuKFJkNe9RYnKvCMLHHOi0LrRvwaFWbGJTbHA@mail.gmail.com>
Date: Tue, 20 Aug 2024 15:51:07 -0400
From: Paul Moore <paul@...l-moore.com>
To: Stephen Smalley <stephen.smalley.work@...il.com>
Cc: Ondrej Mosnacek <omosnace@...hat.com>, Jeongjun Park <aha310510@...il.com>, selinux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: selinux: support IPPROTO_SMC in socket_type_to_security_class()

On Tue, Aug 20, 2024 at 2:24 PM Stephen Smalley
<stephen.smalley.work@...il.com> wrote:
> On Mon, Aug 19, 2024 at 5:46 AM Ondrej Mosnacek <omosnace@...hat.com> wrote:
> > On Thu, Aug 15, 2024 at 10:32 AM Jeongjun Park <aha310510@...il.com> wrote:
> > >
> > > IPPROTO_SMC feature has been added to net/smc. It is now possible to
> > > create smc sockets in the following way:
> > >
> > >   /* create v4 smc sock */
> > >   v4 = socket(AF_INET, SOCK_STREAM, IPPROTO_SMC);
> > >
> > >   /* create v6 smc sock */
> > >   v6 = socket(AF_INET6, SOCK_STREAM, IPPROTO_SMC);
> > >
> > > Therefore, we need to add code to support IPPROTO_SMC in
> > > socket_type_to_security_class().
> > >
> > > Signed-off-by: Jeongjun Park <aha310510@...il.com>
> > > ---
> > >  security/selinux/hooks.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index bfa61e005aac..36f951f0c574 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -1176,6 +1176,8 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
> > >                                 return SECCLASS_TCP_SOCKET;
> > >                         else if (extsockclass && protocol == IPPROTO_SCTP)
> > >                                 return SECCLASS_SCTP_SOCKET;
> > > +                       else if (extsockclass && protocol == IPPROTO_SMC)
> > > +                               return SECCLASS_SMC_SOCKET;
> > >                         else
> > >                                 return SECCLASS_RAWIP_SOCKET;
> > >                 case SOCK_DGRAM:
> > > --
> > >
> >
> > I'm not sure if this is the solution we want to go with... Consider
> > the following from af_smc(7):
> >
> > >   Usage modes
> > >      Two usage modes are possible:
> > >
> > >      AF_SMC native usage
> > >             uses the socket domain AF_SMC instead of AF_INET and AF_INET6.  Specify SMCPROTO_SMC for AF_INET compatible socket semantics, and SMC_PROTO_SMC6 for AF_INET6 respectively.
> > >
> > >      Usage of AF_INET socket applications with SMC preload library
> > >             converts AF_INET and AF_INET6 sockets to AF_SMC sockets.  The SMC preload library is part of the SMC tools package.
> > >
> > >      SMC socket capabilities are negotiated at connection setup. If one peer is not SMC capable, further socket processing falls back to TCP usage automatically.
> >
> > This means that the SMC sockets are intended to be used (also) as a
> > drop-in compatible replacement for normal TCP sockets in applications
> > and they even fall back to TCP when the endpoints fail to negotiate
> > communication via SMC. That's a situation similar to MPTCP, where we
> > just mapped MPTCP sockets to the tcp_socket SELinux class, so that
> > MPTCP can be swapped in place of TCP transparently without having to
> > do extensive policy changes. We may want to consider the same/similar
> > approach here.
> >
> > I briefly played with this idea a couple of months ago, when I was
> > asked by someone at Red Hat about SMC sockets and their integration
> > with SELinux. IIRC, when I tried to implement the MPTCP approach and
> > adjusted the selinux-testsuite to test SMC similarly as TCP and MPTCP,
> > I saw that the netlabel-related tests (may have been more, I don't
> > remember) weren't passing out of the box like with MPTCP. However, the
> > person then didn't follow up on my questions, so I didn't look into it
> > further...
> >
> > I'm attaching the WIP patches I worked with, in case someone would
> > like to continue the experiments.
>
> I am not in favor of your approach, for the following reasons:
> 1. It would be backward-incompatible with any current code using
> AF_SMC (although this could be addressed by making it conditional on a
> new policy capability, so this is not too difficult to overcome),
> 2. It would not allow any distinction to ever be made in policy
> between SMC sockets and TCP sockets, so we could never allow one
> without the other.
>
> Hence, I am still in favor of Jeongjun's patch to consistently treat
> AF_SMC and (AF_INET, SOCK_STREAM, IPPROTO_SMC) sockets, and then if
> someone wants to extend that support to also provide more complete
> access controls and/or networking labeling, defer that to a future
> patch.

Without passing any judgement on the patches Ondrej submitted (I tend
to ignore patches as attachments for various reasons), I do share
Ondrej's concerns that this may not be as simple as suggested in the
original patch in this thread.  I saw the same thing as Ondrej
regarding the TCP fallback and that immediately raised a number of
questions that I don't believe have been properly addressed yet.

Someone needs to dig into how the standard SMC protocol works first to
ensure we have the necessary access controls for the current code; my
guess is that we are probably okay since the socket-level controls are
fairly generic, but I'm not sure we've actually seen proper
confirmation that everything is good from a conceptual standpoint.
Once that is done, we need to examine how the TCP fallback works,
specifically how are connections managed and are the existing TCP
hooks sufficient for SMC (the early connection state stuff can be
tricky) and how to distinguish between normal-TCP and SMC-TCP.

Basically I'm looking for some basic design concepts and not simply a
passing test without any understanding of why/how it passed.

-- 
paul-moore.com

Powered by blists - more mailing lists