On Wed, Nov 3, 2021 at 6:01 PM Paul Moore [off-list ref] wrote:

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

quoted

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.

 SECSID_WILD is used to avoid client's new socket's sid overwritten by
old socket's.

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.

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);
  }

Let's say, this socket has 3 associations now, how can we ensure
the new socket's sid is set to the right sid? I don't think we can use
"sksec->secid" in this place, this is not TCP.

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.

The association's label/secid will never change once set in this patchset.
it's just a temporary record, and later it will be used to set socket's
label/secid. I think that's the idea at the beginning.

quoted

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.

Hi, Paul,

The original issue this patchset fixes is a crucial one (it could cause
peeloff sockets on client side to not work) which I think
can already be fixed now. If you think SECSID_WILD is tricky but
no better way yet, my suggestion is to leave it for now until we have
a better solution to follow up. As I couldn't find a better way to work
it out. Also, we may want to hear Richard's opinion on how it should
work and how this should be fixed.

Thanks.

--
paul moore
www.paul-moore.com