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

Hi Xin,

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()).

please check Paul's comment:

"""
 The initial SCTP client association would
need to take it's label from the parent process so perhaps that is the
right answer for all SCTP client associations[2].

[1] I would expect server side associations to follow the more
complicated selinux_conn_sid() labeling, just as we do for TCP/stream
connections today.

[2] I'm guessing the client associations might also want to follow the
setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more
info.
"""

That's what I got from it:
For client side, secid should be copied from its parent socket directly, but
get it from socket_sockcreate_sid().

and you?

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?

Thanks.

quoted

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 security/selinux/hooks.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a9977a2ae8ac..341cd5dccbf5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c

@@ -5519,7 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
        if (!selinux_policycap_extsockclass())
                return selinux_sk_clone_security(sk, newsk);

-       newsksec->sid = asoc->secid;
+       if (asoc->secid != SECSID_WILD)
+               newsksec->sid = asoc->secid;
        newsksec->peer_sid = asoc->peer_secid;
        newsksec->sclass = sksec->sclass;
        selinux_netlbl_sctp_sk_clone(sk, newsk);

@@ -5575,6 +5576,16 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb)
        selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);
 }

+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;
+       asoc->secid = SECSID_WILD;
+}
+
 static int selinux_secmark_relabel_packet(u32 sid)
 {
        const struct task_security_struct *__tsec;

@@ -7290,6 +7301,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
        LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request),
        LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone),
        LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect),
+       LSM_HOOK_INIT(sctp_assoc_established, selinux_sctp_assoc_established),
        LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request),
        LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone),
        LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established),
--

2.27.0

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.