[PATCH net 4/4] security: implement sctp_assoc_established hook in selinux

[Paul Moore]

On Tue, Oct 26, 2021 at 12:47 AM Xin Long <lucien.xin at gmail.com> wrote:
> On Tue, Oct 26, 2021 at 5:51 AM Paul Moore <paul at paul-moore.com> wrote:
> > On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin at gmail.com> wrote:
> > > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> > >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin at gmail.com> wrote:
> > >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> > >> > > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin at gmail.com> wrote:
> > >> > > > Different from selinux_inet_conn_established(), it also gives the
> > >> > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(),
> > >> > > > as one UDP-type socket may have more than one asocs.
> > >> > > >
> > >> > > > Note that peer_secid in asoc will save the peer secid for this
> > >> > > > asoc connection, and peer_sid in sksec will just keep the peer
> > >> > > > secid for the latest connection. So the right use should be do
> > >> > > > peeloff for UDP-type socket if there will be multiple asocs in
> > >> > > > one socket, so that the peeloff socket has the right label for
> > >> > > > its asoc.
> > >> > >
> > >> > > Hm... this sounds like something we should also try to fix (if
> > >> > > possible). In access control we can't trust userspace to do the right
> > >> > > thing - receiving from multiple peers on one SOCK_SEQPACKET socket
> > >> > > shouldn't cause checking against the wrong peer_sid. But that can be
> > >> > > addressed separately. (And maybe it's even already accounted for
> > >> > > somehow - I didn't yet look at the code closely.)
> >
> > There are a couple of things we need to worry about here: the
> > per-packet access controls (e.g. can this packet be received by this
> > socket?) and the userspace peer label queries (e.g. SO_GETPEERSEC and
> > IP_CMSG_PASSSEC).
> >
> > The per-packet access controls work by checking the individual
> > packet's security label against the corresponding sock label on the
> > system (sk->sk_security->sid).  Because of this it is important that
> > the sock's label is correct.  For unconnected sockets this is fairly
> > straightforward as it follows the usual inherit-from-parent[1]
> > behavior we see in other areas of SELinux.  For connected stream
> > sockets this can be a bit more complicated.  However, since we are
> > only discussing the client side things aren't too bad with the
> > behavior essentially the same, inherit-from-parent, with the only
> > interesting piece worth noting being the sksec->peer_sid
> > (sk->sk_security->peer_sid) that we record from the packet passed to
> > the LSM/SELinux hook (using selinux_skb_peerlbl_sid()).  The
> > sksec->peer_sid is recorded primarily so that the kernel can correctly
> > respond to SO_GETPEERSEC requests from userspace; it shouldn't be used
> > in any access control decisions.
>
> Hi, Paul
>
> Understand now, the issue reported seems caused by when
> doing peel-off the peel-off socket gets the uninitialised sid
> from 'ep' on the client, though it should be "asoc".

Hi Xin Long,

Yes, that is my understanding.  I got the impression from the thread
that there was some confusion about the different labels and what they
were used for in SELinux, I was trying to provide some background in
the text above.  If you are already familiar with how things should
work you can disregard it :)

> > In the case of SCTP, I would expect things to behave similarly: the
> > sksec->peer_sid should match the packet label of the traffic which
> > acknowledged/accepted the new connection, e.g. the other end of the
> > connected socket.  You will have to forgive me some of the details,
> > it's been a while since I last looked at the SCTP bits, but I would
> > expect that if a client created a new connection and/or spun-off a new
> > socket the new socket's sksec->peer_sid would have the same property,
> > it would represent the security label of the other end of the
> > connection/association.
>
> In SCTP, a socket doesn't represent a peer connection, it's more an
> object binding some addresses and receiving incoming connecting
> request, then creates 'asoc' to represent the connection, so asoc->
> peer_secid represents the security label of the other end of the
> connection/association.

As mentioned previously the asoc->peer_secid *should* be the security
label of the remote end, so I think we are okay here.  My concern
remains the asoc->secid label as I don't believe it is being set to
the correct value (more on that below).

> After doing peel-off, it makes one asoc 'bind' to one new socket,
> and this socket is used for userspace to control this asoc (conection),
> so naturally we set sksec->peer_sid to asoc->secid for access control
> in socket.

The sksec->peer_sid represents the security label of the remote end so
it should be set to the asoc->peer_secid and *not* the asoc->secid
value.  Yes, they are presently the same value in your patches, but I
believe that is a mistake; I believe the asoc->secid value should be
set to that of the parent (see the prior inherit-from-parent
discussion) which in this case would likely be either the parent
association or the client process, I'm not entirely clear on which is
correct in the SCTP case.  The initial SCTP client association would
need to take it's label from the parent process so perhaps that is the
right answer for all SCTP client associations[2].

[1] I would expect server side associations to follow the more
complicated selinux_conn_sid() labeling, just as we do for TCP/stream
connections today.

[2] I'm guessing the client associations might also want to follow the
setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more
info.

-- 
paul moore
www.paul-moore.com