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

[Paul Moore <paul@paul-moore.com>]

On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote:
> On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Tue, Nov 2, 2021 at 1:03 PM 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.
> > > >
> > > > 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.

In my mind, selinux_sctp_sk_clone() should end up looking like this.

  void selinux_sctp_sk_clone(asoc, sk, newsk)
  {
    struct sk_security_struct sksec = sk->sk_security;
    struct sk_security_struct newsksec = newsk->sk_security;

    if (!selinux_policycap_extsockclass())
        return selinux_sk_clone_security(sk, newsk);

    newsksec->sid = sksec->secid;
    newsksec->peer_sid = asoc->peer_secid;
    newsksec->sclass = sksec->sclass;
    selinux_netlbl_sctp_sk_clone(sk, newsk);
  }

Also, to be clear, the "assoc->secid = SECSID_WILD;" line should be
removed from selinux_sctp_assoc_established().  If we are treating
SCTP associations similarly to TCP connections, the association's
label/secid should be set once and not changed during the life of the
association.

> > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
> > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com>
> > >
> > > You made non-trivial changes since the last revision in this patch, so
> > > you should have also dropped the Reviewed-by and Tested-by here. Now
> > > David has merged the patches probably under the impression that they
> > > have been reviewed/approved from the SELinux side, which isn't
> > > completely true.
> >
> > Oh, that's a mistake, I thought I didn't add it.
> > Will he be able to test this new patchset?

While I tend to try to avoid reverts as much as possible, I think the
right thing to do is to get these patches reverted out of DaveM's tree
while we continue to sort this out and do all of the necessary testing
and verification.

Xin Long, please work with the netdev folks to get your patchset
reverted and then respin this patchset using the feedback provided.

-- 
paul moore
www.paul-moore.com