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

[Ondrej Mosnacek <omosnace@redhat.com>]

On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@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.)
> >
> > >
> > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  security/selinux/hooks.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index f025fc00421b..793fdcbc68bd 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
> > >         selinux_netlbl_sctp_sk_clone(sk, newsk);
> > >  }
> > >
> > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc,
> > > +                                          struct sk_buff *skb)
> > > +{
> > > +       struct sk_security_struct *sksec = asoc->base.sk->sk_security;
> > > +       u16 family = asoc->base.sk->sk_family;
> > > +
> > > +       /* handle mapped IPv4 packets arriving via IPv6 sockets */
> > > +       if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP))
> > > +               family = PF_INET;
> > > +
> > > +       selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);
> >
> > You could replace the above with
> > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code
> > duplication.
> Hi Ondrej,
>
> will do, thanks!
>
> >
> > > +       asoc->secid = sksec->sid;
> > > +       asoc->peer_secid = sksec->peer_sid;
> > > +}
> > > +
> Now I'm thinking: 'peer_sid' should be correct here.
>
> BUT 'sid' is copied from its parent socket. Later when doing peel-off,
> asoc->secid will be set back to the peel-off socket's sksec->sid.

Hi,

I'm not sure I follow... When doing peel-off, security_sctp_sk_clone()
should be called, which sets the peel-off socket's sksec->sid to
asoc->secid, not the other way around. (Are we hitting the language
barrier here? :)

> Do you think this is okay? or should the peel-off socket have its own
> sksec->sid, which might be different from the parent socket's?
> (Note the socket's sid initially was set in selinux_socket_post_create())

I *think* in case of a client socket it is expected for the
peeloff-style child socket to just inherit the same sksec->sid. But
frankly I haven't been with SELinux and kernel development long enough
to understand the intricacies of SELinux's network connection handling
very well... Hopefully Paul/Richard/Stephen can give a more confident
answer/review here.

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.