On Wed, Nov 3, 2021 at 1:36 PM Xin Long [off-list ref] wrote:

On Wed, Nov 3, 2021 at 1:33 PM Xin Long [off-list ref] wrote:

quoted

On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek [off-list ref] wrote:

quoted

On Tue, Nov 2, 2021 at 1:03 PM Xin Long [off-list ref] wrote:

quoted

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.

quoted

quoted

quoted

Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
Reported-by: Prashanth Prahlad <redacted>
Reviewed-by: Richard Haines <redacted>
Tested-by: Richard Haines <redacted>

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