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

On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek [off-list ref] wrote:

On Fri, Oct 22, 2021 at 8:36 AM 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.

Hm... this sounds like something we should also try to fix (if
possible). In access control we can't trust userspace to do the right
thing - receiving from multiple peers on one SOCK_SEQPACKET socket
shouldn't cause checking against the wrong peer_sid. But that can be
addressed separately. (And maybe it's even already accounted for
somehow - I didn't yet look at the code closely.)

quoted

Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
Reported-by: Prashanth Prahlad <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 security/selinux/hooks.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f025fc00421b..793fdcbc68bd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c

@@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
        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;
+       u16 family = asoc->base.sk->sk_family;
+
+       /* handle mapped IPv4 packets arriving via IPv6 sockets */
+       if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP))
+               family = PF_INET;
+
+       selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);

You could replace the above with
`selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code
duplication.

Hi Ondrej,

will do, thanks!

quoted

+       asoc->secid = sksec->sid;
+       asoc->peer_secid = sksec->peer_sid;
+}
+

Now I'm thinking: 'peer_sid' should be correct here.

BUT 'sid' is copied from its parent socket. Later when doing peel-off,
asoc->secid will be set back to the peel-off socket's sksec->sid.

Do you think this is okay? or should the peel-off socket have its own
sksec->sid, which might be different from the parent socket's?
(Note the socket's sid initially was set in selinux_socket_post_create())

quoted

 static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
                                     struct request_sock *req)
 {

@@ -7290,6 +7305,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.