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

[Richard Haines]

On Wed, 2021-11-03 at 23:17 -0400, Paul Moore wrote:
> On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin at gmail.com> wrote:
> > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul at paul-moore.com>
> > wrote:
> > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin at gmail.com>
> > > wrote:
> > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin at gmail.com>
> > > > wrote:
> > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek
> > > > > <omosnace at redhat.com> wrote:
> > > > > > On Tue, Nov 2, 2021 at 1:03 PM 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.
> > > > > > > 
> > > > > > > v1->v2:
> > > > > > >   - call selinux_inet_conn_established() to reduce some
> > > > > > > code
> > > > > > >     duplication in selinux_sctp_assoc_established(), as
> > > > > > > Ondrej
> > > > > > >     suggested.
> > > > > > >   - when doing peeloff, it calls sock_create() where it
> > > > > > > actually
> > > > > > >     gets secid for socket from socket_sockcreate_sid(). So
> > > > > > > reuse
> > > > > > >     SECSID_WILD to ensure the peeloff socket keeps using
> > > > > > > that
> > > > > > >     secid after calling selinux_sctp_sk_clone() for client
> > > > > > > side.
> > > > > > 
> > > > > > Interesting... I find strange that SCTP creates the peeloff
> > > > > > socket
> > > > > > using sock_create() rather than allocating it directly via
> > > > > > sock_alloc() like the other callers of sctp_copy_sock()
> > > > > > (which calls
> > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to
> > > > > > avoid the
> > > > > > sock_create() call and just rely on the
> > > > > > security_sctp_sk_clone()
> > > > > > semantic to set up the labels? Would anything break if
> > > > > > sctp_do_peeloff() switched to plain sock_alloc()?
> > > > > > 
> > > > > > I'd rather we avoid this SECSID_WILD hack to support the
> > > > > > weird
> > > > > > created-but-also-cloned socket hybrid and just make the
> > > > > > peeloff socket
> > > > > > behave the same as an accept()-ed socket (i.e. no
> > > > > > security_socket_[post_]create() hook calls, just
> > > > > > security_sctp_sk_clone()).
> > > 
> > > I believe the important part is that sctp_do_peeloff() eventually
> > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). 
> > > Assuming
> > > we have security_sctp_sk_clone() working properly I would expect
> > > that
> > > the new socket would be setup properly when sctp_do_peeloff()
> > > returns
> > > on success.
> > > 
> > > ... and yes, that SECSID_WILD approach is *not* something we want
> > > to do.
> > 
> > SECSID_WILD is used to avoid client's new socket's sid overwritten by
> > old socket's.
> 
> In the case of security_sctp_sk_clone() the new client socket (the
> cloned socket) should inherit the label/sid from the original socket
> (the "parent" in the inherit-from-parent label inheritance behavior
> discussed earlier).  The selinux_sctp_assoc_established() function
> should not change the socket's label/sid at all, only the peer label.
> 
> > If I understand correctly, new socket's should keep using its
> > original
> > sid, namely,
> > the one set from security_socket_[post_]create() on client side. I
> > AGREE with that.
> > Now I want to *confirm* this with you, as it's different from the
> > last version's
> > 'inherit from parent socket' that Richard and Ondrej reviewed.
> 
> Unfortunately I think we are struggling to communicate because you are
> not familiar with SELinux concepts and I'm not as well versed in SCTP
> as you are.  As things currently stand, I am getting a disconnect
> between your explanations and the code you have submitted; they simply
> aren't consistent from my perspective.
> 
> In an effort to help provide something that is hopefully a bit more
> clear, here are the selinux_sctp_sk_clone() and
> selinux_sctp_assoc_established() functions which I believe we need.
> If you feel these are incorrect, please explain and/or provide edits:
> 
>   static void selinux_sctp_sk_clone(struct sctp_association *asoc,
>                                     struct sock *sk, struct sock
> *newsk)
>   {
>     struct sk_security_struct *sksec = sk->sk_security;
>     struct sk_security_struct *newsksec = newsk->sk_security;
> 
>     /* If policy does not support SECCLASS_SCTP_SOCKET then call
>      * the non-sctp clone version.
>      */
>     if (!selinux_policycap_extsockclass())
>       return selinux_sk_clone_security(sk, newsk);
> 
>     newsksec->secid = sksec->secid;
This should be:
    newsksec->sid = sksec->sid;


>     newsksec->peer_sid = asoc->peer_secid;
>     newsksec->sclass = sksec->sclass;
>     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;
> 
>     selinux_inet_conn_established(asoc->base.sk, skb);
>     asoc->peer_secid = sksec->peer_sid;
>   }