[PATCH v2] selinux: use socket SID for SCTP ASCONF permission checks

[Tristan Madani <tristmd@gmail.com>]

From: Tristan Madani <tristan@talencesecurity.com>

__selinux_socket_bind() and selinux_socket_connect_helper() call
sock_has_perm() which uses current_sid() as the AVC subject.  When
selinux_sctp_bind_connect() is invoked from the ASCONF processing
path (sctp_process_asconf), current refers to whichever task was
interrupted, so the permission check evaluates an unrelated subject.

Fix by extracting the sock_has_perm() call out of
__selinux_socket_bind() and selinux_socket_connect_helper() into
their callers.  The process-context wrappers (selinux_socket_bind,
selinux_socket_connect) call sock_has_perm() directly.

For selinux_sctp_bind_connect(), determine the caller SID based on
the optname: SCTP_PARAM_SET_PRIMARY and SCTP_PARAM_ADD_IP are ASCONF
chunk parameter types only passed from the softirq processing path,
so use the socket own SID (sksec->sid), consistent with other
softirq-context hooks such as selinux_socket_sock_rcv_skb() and
selinux_sctp_assoc_request().  All other optnames originate from
process-context syscalls and use current_sid().

Factor out __sock_has_perm() with an explicit subject SID parameter
so that sock_has_perm() remains unchanged for all other callers.

Fixes: d452930fd3b9 ("selinux: Add SCTP support")
Cc: stable@vger.kernel.org
Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
Changes in v2:
  - Do not use sksec->sid unconditionally; determine the caller SID
    based on the optname so that process-context callers (bind, connect,
    connectx, sendmsg) still use current_sid()  [Paul Moore, Sashiko]
  - Factor out __sock_has_perm() with an explicit SID parameter and keep
    sock_has_perm() as a thin wrapper for all other callers
  - Move the sock_has_perm() call out of __selinux_socket_bind() and
    selinux_socket_connect_helper() into their respective callers
    (selinux_socket_bind, selinux_socket_connect, selinux_sctp_bind_connect)
 security/selinux/hooks.c | 47 ++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 035aaf113d1da..ab9653ae06fc8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4918,7 +4918,7 @@ static bool sock_skip_has_perm(u32 sid)
 }
 
 
-static int sock_has_perm(struct sock *sk, u32 perms)
+static int __sock_has_perm(struct sock *sk, u32 sid, u32 perms)
 {
 	struct sk_security_struct *sksec = selinux_sock(sk);
 	struct common_audit_data ad;
@@ -4929,10 +4929,15 @@ static int sock_has_perm(struct sock *sk, u32 perms)
 
 	ad_net_init_from_sk(&ad, &net, sk);
 
-	return avc_has_perm(current_sid(), sksec->sid, sksec->sclass, perms,
+	return avc_has_perm(sid, sksec->sid, sksec->sclass, perms,
 			    &ad);
 }
 
+static int sock_has_perm(struct sock *sk, u32 perms)
+{
+	return __sock_has_perm(sk, current_sid(), perms);
+}
+
 static int selinux_socket_create(int family, int type,
 				 int protocol, int kern)
 {
@@ -5006,11 +5011,7 @@ static int __selinux_socket_bind(struct sock *sk, struct sockaddr *address, int
 {
 	struct sk_security_struct *sksec = selinux_sock(sk);
 	u16 family;
-	int err;
-
-	err = sock_has_perm(sk, SOCKET__BIND);
-	if (err)
-		goto out;
+	int err = 0;
 
 	/* If PF_INET or PF_INET6, check name_bind permission for the port. */
 	family = sk->sk_family;
@@ -5135,6 +5136,11 @@ static int __selinux_socket_bind(struct sock *sk, struct sockaddr *address, int
 
 static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen)
 {
+	int err;
+
+	err = sock_has_perm(sock->sk, SOCKET__BIND);
+	if (err)
+		return err;
 	return __selinux_socket_bind(sock->sk, address, addrlen);
 }
 
@@ -5145,11 +5151,8 @@ static int selinux_socket_connect_helper(struct sock *sk,
 					 struct sockaddr *address, int addrlen)
 {
 	struct sk_security_struct *sksec = selinux_sock(sk);
-	int err;
+	int err = 0;
 
-	err = sock_has_perm(sk, SOCKET__CONNECT);
-	if (err)
-		return err;
 	if (addrlen < offsetofend(struct sockaddr, sa_family))
 		return -EINVAL;
 
@@ -5232,6 +5235,9 @@ static int selinux_socket_connect(struct socket *sock,
 	int err;
 	struct sock *sk = sock->sk;
 
+	err = sock_has_perm(sk, SOCKET__CONNECT);
+	if (err)
+		return err;
 	err = selinux_socket_connect_helper(sk, address, addrlen);
 	if (err)
 		return err;
@@ -5731,13 +5737,25 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
 				     struct sockaddr *address,
 				     int addrlen)
 {
+	struct sk_security_struct *sksec = selinux_sock(sk);
 	int len, err = 0, walk_size = 0;
 	void *addr_buf;
 	struct sockaddr *addr;
+	u32 caller_sid;
 
 	if (!selinux_policycap_extsockclass())
 		return 0;
 
+	/* SCTP_PARAM_SET_PRIMARY and SCTP_PARAM_ADD_IP are ASCONF chunk
+	 * parameter types passed from sctp_process_asconf() in softirq
+	 * context where current is not meaningful. Use the socket own
+	 * SID for permission checks in that case; all other optnames
+	 * originate from process-context syscalls.
+	 */
+	caller_sid = (optname == SCTP_PARAM_SET_PRIMARY ||
+		      optname == SCTP_PARAM_ADD_IP) ?
+		     sksec->sid : current_sid();
+
 	/* Process one or more addresses that may be IPv4 or IPv6 */
 	addr_buf = address;
 
@@ -5767,13 +5785,18 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
 		case SCTP_PRIMARY_ADDR:
 		case SCTP_SET_PEER_PRIMARY_ADDR:
 		case SCTP_SOCKOPT_BINDX_ADD:
-			err = __selinux_socket_bind(sk, addr, len);
+			err = __sock_has_perm(sk, caller_sid, SOCKET__BIND);
+			if (!err)
+				err = __selinux_socket_bind(sk, addr, len);
 			break;
 		/* Connect checks */
 		case SCTP_SOCKOPT_CONNECTX:
 		case SCTP_PARAM_SET_PRIMARY:
 		case SCTP_PARAM_ADD_IP:
 		case SCTP_SENDMSG_CONNECT:
+			err = __sock_has_perm(sk, caller_sid, SOCKET__CONNECT);
+			if (err)
+				return err;
 			err = selinux_socket_connect_helper(sk, addr, len);
 			if (err)
 				return err;
-- 
2.47.3