On Wed, 2021-11-03 at 23:17 -0400, Paul Moore wrote:

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

quoted

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

quoted

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.

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.

quoted

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