[PATCHv2 net 0/4] security: fixups for the security hooks in sctp

[PATCHv2 net 0/4] security: fixups for the security hooks in sctp

[Xin Long]

There are a couple of problems in the currect security hooks in sctp:

1. The hooks incorrectly treat sctp_endpoint in SCTP as request_sock in
   TCP, while it's in fact no more than an extension of the sock, and
   represents the local host. It is created when sock is created, not
   when a conn request comes. sctp_association is actually the correct
   one to represent the connection, and created when a conn request
   arrives.

2. security_sctp_assoc_request() hook should also be called in processing
   COOKIE ECHO, as that's the place where the real assoc is created and
   used in the future.

The problems above may cause accept sk, peeloff sk or client sk having
the incorrect security labels.

So this patchset is to change some hooks and pass asoc into them and save
these secids into asoc, as well as add the missing sctp_assoc_request
hook into the COOKIE ECHO processing.

v1->v2:
  - See each patch, and thanks the help from Ondrej, Paul and Richard.

Xin Long (4):
  security: pass asoc to sctp_assoc_request and sctp_sk_clone
  security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce
  security: add sctp_assoc_established hook
  security: implement sctp_assoc_established hook in selinux

 Documentation/security/SCTP.rst     | 65 +++++++++++++++--------------
 include/linux/lsm_hook_defs.h       |  6 ++-
 include/linux/lsm_hooks.h           | 13 ++++--
 include/linux/security.h            | 17 +++++---
 include/net/sctp/structs.h          | 20 ++++-----
 net/sctp/sm_statefuns.c             | 31 ++++++++------
 net/sctp/socket.c                   |  5 +--
 security/security.c                 | 15 +++++--
 security/selinux/hooks.c            | 34 ++++++++++-----
 security/selinux/include/netlabel.h |  4 +-
 security/selinux/netlabel.c         | 18 ++++----
 11 files changed, 133 insertions(+), 95 deletions(-)

-- 
2.27.0

[PATCHv2 net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone

[Xin Long]

This patch is to move secid and peer_secid from endpoint to association,
and pass asoc to sctp_assoc_request and sctp_sk_clone instead of ep. As
ep is the local endpoint and asoc represents a connection, and in SCTP
one sk/ep could have multiple asoc/connection, saving secid/peer_secid
for new asoc will overwrite the old asoc's.

Note that since asoc can be passed as NULL, security_sctp_assoc_request()
is moved to the place right after the new_asoc is created in
sctp_sf_do_5_1B_init() and sctp_sf_do_unexpected_init().

v1->v2:
  - fix the description of selinux_netlbl_skbuff_setsid(), as Jakub noticed.
  - fix the annotation in selinux_sctp_assoc_request(), as Richard Noticed.

Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
Tested-by: Richard Haines <richard_c_haines@btinternet.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 Documentation/security/SCTP.rst     | 28 ++++++++++++++--------------
 include/linux/lsm_hook_defs.h       |  4 ++--
 include/linux/lsm_hooks.h           |  8 ++++----
 include/linux/security.h            | 10 +++++-----
 include/net/sctp/structs.h          | 20 ++++++++++----------
 net/sctp/sm_statefuns.c             | 26 +++++++++++++-------------
 net/sctp/socket.c                   |  5 ++---
 security/security.c                 |  8 ++++----
 security/selinux/hooks.c            | 22 +++++++++++-----------
 security/selinux/include/netlabel.h |  4 ++--
 security/selinux/netlabel.c         | 18 +++++++++---------
 11 files changed, 76 insertions(+), 77 deletions(-)

diff --git a/Documentation/security/SCTP.rst b/Documentation/security/SCTP.rst
index 0bcf6c1245ee..415b548d9ce0 100644
--- a/Documentation/security/SCTP.rst
+++ b/Documentation/security/SCTP.rst
@@ -26,11 +26,11 @@ described in the `SCTP SELinux Support`_ chapter.
 
 security_sctp_assoc_request()
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Passes the ``@ep`` and ``@chunk->skb`` of the association INIT packet to the
+Passes the ``@asoc`` and ``@chunk->skb`` of the association INIT packet to the
 security module. Returns 0 on success, error on failure.
 ::
 
-    @ep - pointer to sctp endpoint structure.
+    @asoc - pointer to sctp association structure.
     @skb - pointer to skbuff of association packet.
 
 
@@ -117,9 +117,9 @@ Called whenever a new socket is created by **accept**\(2)
 calls **sctp_peeloff**\(3).
 ::
 
-    @ep - pointer to current sctp endpoint structure.
+    @asoc - pointer to current sctp association structure.
     @sk - pointer to current sock structure.
-    @sk - pointer to new sock structure.
+    @newsk - pointer to new sock structure.
 
 
 security_inet_conn_established()
@@ -200,22 +200,22 @@ hooks with the SELinux specifics expanded below::
 
 security_sctp_assoc_request()
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Passes the ``@ep`` and ``@chunk->skb`` of the association INIT packet to the
+Passes the ``@asoc`` and ``@chunk->skb`` of the association INIT packet to the
 security module. Returns 0 on success, error on failure.
 ::
 
-    @ep - pointer to sctp endpoint structure.
+    @asoc - pointer to sctp association structure.
     @skb - pointer to skbuff of association packet.
 
 The security module performs the following operations:
-     IF this is the first association on ``@ep->base.sk``, then set the peer
+     IF this is the first association on ``@asoc->base.sk``, then set the peer
      sid to that in ``@skb``. This will ensure there is only one peer sid
-     assigned to ``@ep->base.sk`` that may support multiple associations.
+     assigned to ``@asoc->base.sk`` that may support multiple associations.
 
-     ELSE validate the ``@ep->base.sk peer_sid`` against the ``@skb peer sid``
+     ELSE validate the ``@asoc->base.sk peer_sid`` against the ``@skb peer sid``
      to determine whether the association should be allowed or denied.
 
-     Set the sctp ``@ep sid`` to socket's sid (from ``ep->base.sk``) with
+     Set the sctp ``@asoc sid`` to socket's sid (from ``asoc->base.sk``) with
      MLS portion taken from ``@skb peer sid``. This will be used by SCTP
      TCP style sockets and peeled off connections as they cause a new socket
      to be generated.
@@ -259,13 +259,13 @@ security_sctp_sk_clone()
 Called whenever a new socket is created by **accept**\(2) (i.e. a TCP style
 socket) or when a socket is 'peeled off' e.g userspace calls
 **sctp_peeloff**\(3). ``security_sctp_sk_clone()`` will set the new
-sockets sid and peer sid to that contained in the ``@ep sid`` and
-``@ep peer sid`` respectively.
+sockets sid and peer sid to that contained in the ``@asoc sid`` and
+``@asoc peer sid`` respectively.
 ::
 
-    @ep - pointer to current sctp endpoint structure.
+    @asoc - pointer to current sctp association structure.
     @sk - pointer to current sock structure.
-    @sk - pointer to new sock structure.
+    @newsk - pointer to new sock structure.
 
 
 security_inet_conn_established()
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2adeea44c0d5..0024273a7382 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -328,11 +328,11 @@ LSM_HOOK(int, 0, tun_dev_create, void)
 LSM_HOOK(int, 0, tun_dev_attach_queue, void *security)
 LSM_HOOK(int, 0, tun_dev_attach, struct sock *sk, void *security)
 LSM_HOOK(int, 0, tun_dev_open, void *security)
-LSM_HOOK(int, 0, sctp_assoc_request, struct sctp_endpoint *ep,
+LSM_HOOK(int, 0, sctp_assoc_request, struct sctp_association *asoc,
 	 struct sk_buff *skb)
 LSM_HOOK(int, 0, sctp_bind_connect, struct sock *sk, int optname,
 	 struct sockaddr *address, int addrlen)
-LSM_HOOK(void, LSM_RET_VOID, sctp_sk_clone, struct sctp_endpoint *ep,
+LSM_HOOK(void, LSM_RET_VOID, sctp_sk_clone, struct sctp_association *asoc,
 	 struct sock *sk, struct sock *newsk)
 #endif /* CONFIG_SECURITY_NETWORK */
 
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5c4c5c0602cb..240b92d89852 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1024,9 +1024,9 @@
  * Security hooks for SCTP
  *
  * @sctp_assoc_request:
- *	Passes the @ep and @chunk->skb of the association INIT packet to
+ *	Passes the @asoc and @chunk->skb of the association INIT packet to
  *	the security module.
- *	@ep pointer to sctp endpoint structure.
+ *	@asoc pointer to sctp association structure.
  *	@skb pointer to skbuff of association packet.
  *	Return 0 on success, error on failure.
  * @sctp_bind_connect:
@@ -1044,9 +1044,9 @@
  *	Called whenever a new socket is created by accept(2) (i.e. a TCP
  *	style socket) or when a socket is 'peeled off' e.g userspace
  *	calls sctp_peeloff(3).
- *	@ep pointer to current sctp endpoint structure.
+ *	@asoc pointer to current sctp association structure.
  *	@sk pointer to current sock structure.
- *	@sk pointer to new sock structure.
+ *	@newsk pointer to new sock structure.
  *
  * Security hooks for Infiniband
  *
diff --git a/include/linux/security.h b/include/linux/security.h
index 5b7288521300..a16407444871 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -179,7 +179,7 @@ struct xfrm_policy;
 struct xfrm_state;
 struct xfrm_user_sec_ctx;
 struct seq_file;
-struct sctp_endpoint;
+struct sctp_association;
 
 #ifdef CONFIG_MMU
 extern unsigned long mmap_min_addr;
@@ -1418,10 +1418,10 @@ int security_tun_dev_create(void);
 int security_tun_dev_attach_queue(void *security);
 int security_tun_dev_attach(struct sock *sk, void *security);
 int security_tun_dev_open(void *security);
-int security_sctp_assoc_request(struct sctp_endpoint *ep, struct sk_buff *skb);
+int security_sctp_assoc_request(struct sctp_association *asoc, struct sk_buff *skb);
 int security_sctp_bind_connect(struct sock *sk, int optname,
 			       struct sockaddr *address, int addrlen);
-void security_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
+void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk,
 			    struct sock *newsk);
 
 #else	/* CONFIG_SECURITY_NETWORK */
@@ -1624,7 +1624,7 @@ static inline int security_tun_dev_open(void *security)
 	return 0;
 }
 
-static inline int security_sctp_assoc_request(struct sctp_endpoint *ep,
+static inline int security_sctp_assoc_request(struct sctp_association *asoc,
 					      struct sk_buff *skb)
 {
 	return 0;
@@ -1637,7 +1637,7 @@ static inline int security_sctp_bind_connect(struct sock *sk, int optname,
 	return 0;
 }
 
-static inline void security_sctp_sk_clone(struct sctp_endpoint *ep,
+static inline void security_sctp_sk_clone(struct sctp_association *asoc,
 					  struct sock *sk,
 					  struct sock *newsk)
 {
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 651bba654d77..899c29c326ba 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1355,16 +1355,6 @@ struct sctp_endpoint {
 	      reconf_enable:1;
 
 	__u8  strreset_enable;
-
-	/* Security identifiers from incoming (INIT). These are set by
-	 * security_sctp_assoc_request(). These will only be used by
-	 * SCTP TCP type sockets and peeled off connections as they
-	 * cause a new socket to be generated. security_sctp_sk_clone()
-	 * will then plug these into the new socket.
-	 */
-
-	u32 secid;
-	u32 peer_secid;
 };
 
 /* Recover the outter endpoint structure. */
@@ -2104,6 +2094,16 @@ struct sctp_association {
 	__u64 abandoned_unsent[SCTP_PR_INDEX(MAX) + 1];
 	__u64 abandoned_sent[SCTP_PR_INDEX(MAX) + 1];
 
+	/* Security identifiers from incoming (INIT). These are set by
+	 * security_sctp_assoc_request(). These will only be used by
+	 * SCTP TCP type sockets and peeled off connections as they
+	 * cause a new socket to be generated. security_sctp_sk_clone()
+	 * will then plug these into the new socket.
+	 */
+
+	u32 secid;
+	u32 peer_secid;
+
 	struct rcu_head rcu;
 };
 
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index fb3da4d8f4a3..3206374209bc 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -326,11 +326,6 @@ enum sctp_disposition sctp_sf_do_5_1B_init(struct net *net,
 	struct sctp_packet *packet;
 	int len;
 
-	/* Update socket peer label if first association. */
-	if (security_sctp_assoc_request((struct sctp_endpoint *)ep,
-					chunk->skb))
-		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
-
 	/* 6.10 Bundling
 	 * An endpoint MUST NOT bundle INIT, INIT ACK or
 	 * SHUTDOWN COMPLETE with any other chunks.
@@ -415,6 +410,12 @@ enum sctp_disposition sctp_sf_do_5_1B_init(struct net *net,
 	if (!new_asoc)
 		goto nomem;
 
+	/* Update socket peer label if first association. */
+	if (security_sctp_assoc_request(new_asoc, chunk->skb)) {
+		sctp_association_free(new_asoc);
+		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
+	}
+
 	if (sctp_assoc_set_bind_addr_from_ep(new_asoc,
 					     sctp_scope(sctp_source(chunk)),
 					     GFP_ATOMIC) < 0)
@@ -780,7 +781,6 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
 		}
 	}
 
-
 	/* Delay state machine commands until later.
 	 *
 	 * Re-build the bind address for the association is done in
@@ -1517,11 +1517,6 @@ static enum sctp_disposition sctp_sf_do_unexpected_init(
 	struct sctp_packet *packet;
 	int len;
 
-	/* Update socket peer label if first association. */
-	if (security_sctp_assoc_request((struct sctp_endpoint *)ep,
-					chunk->skb))
-		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
-
 	/* 6.10 Bundling
 	 * An endpoint MUST NOT bundle INIT, INIT ACK or
 	 * SHUTDOWN COMPLETE with any other chunks.
@@ -1594,6 +1589,12 @@ static enum sctp_disposition sctp_sf_do_unexpected_init(
 	if (!new_asoc)
 		goto nomem;
 
+	/* Update socket peer label if first association. */
+	if (security_sctp_assoc_request(new_asoc, chunk->skb)) {
+		sctp_association_free(new_asoc);
+		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
+	}
+
 	if (sctp_assoc_set_bind_addr_from_ep(new_asoc,
 				sctp_scope(sctp_source(chunk)), GFP_ATOMIC) < 0)
 		goto nomem;
@@ -2255,8 +2256,7 @@ enum sctp_disposition sctp_sf_do_5_2_4_dupcook(
 	}
 
 	/* Update socket peer label if first association. */
-	if (security_sctp_assoc_request((struct sctp_endpoint *)ep,
-					chunk->skb)) {
+	if (security_sctp_assoc_request(new_asoc, chunk->skb)) {
 		sctp_association_free(new_asoc);
 		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
 	}
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 6b937bfd4751..33391254fa82 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -9412,7 +9412,6 @@ void sctp_copy_sock(struct sock *newsk, struct sock *sk,
 	struct inet_sock *inet = inet_sk(sk);
 	struct inet_sock *newinet;
 	struct sctp_sock *sp = sctp_sk(sk);
-	struct sctp_endpoint *ep = sp->ep;
 
 	newsk->sk_type = sk->sk_type;
 	newsk->sk_bound_dev_if = sk->sk_bound_dev_if;
@@ -9457,9 +9456,9 @@ void sctp_copy_sock(struct sock *newsk, struct sock *sk,
 		net_enable_timestamp();
 
 	/* Set newsk security attributes from original sk and connection
-	 * security attribute from ep.
+	 * security attribute from asoc.
 	 */
-	security_sctp_sk_clone(ep, sk, newsk);
+	security_sctp_sk_clone(asoc, sk, newsk);
 }
 
 static inline void sctp_copy_descendant(struct sock *sk_to,
diff --git a/security/security.c b/security/security.c
index 9ffa9e9c5c55..b0f1c007aa3b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2366,9 +2366,9 @@ int security_tun_dev_open(void *security)
 }
 EXPORT_SYMBOL(security_tun_dev_open);
 
-int security_sctp_assoc_request(struct sctp_endpoint *ep, struct sk_buff *skb)
+int security_sctp_assoc_request(struct sctp_association *asoc, struct sk_buff *skb)
 {
-	return call_int_hook(sctp_assoc_request, 0, ep, skb);
+	return call_int_hook(sctp_assoc_request, 0, asoc, skb);
 }
 EXPORT_SYMBOL(security_sctp_assoc_request);
 
@@ -2380,10 +2380,10 @@ int security_sctp_bind_connect(struct sock *sk, int optname,
 }
 EXPORT_SYMBOL(security_sctp_bind_connect);
 
-void security_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
+void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk,
 			    struct sock *newsk)
 {
-	call_void_hook(sctp_sk_clone, ep, sk, newsk);
+	call_void_hook(sctp_sk_clone, asoc, sk, newsk);
 }
 EXPORT_SYMBOL(security_sctp_sk_clone);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e7ebd45ca345..a9977a2ae8ac 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5356,10 +5356,10 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)
  * connect(2), sctp_connectx(3) or sctp_sendmsg(3) (with no association
  * already present).
  */
-static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
+static int selinux_sctp_assoc_request(struct sctp_association *asoc,
 				      struct sk_buff *skb)
 {
-	struct sk_security_struct *sksec = ep->base.sk->sk_security;
+	struct sk_security_struct *sksec = asoc->base.sk->sk_security;
 	struct common_audit_data ad;
 	struct lsm_network_audit net = {0,};
 	u8 peerlbl_active;
@@ -5376,7 +5376,7 @@ static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
 		/* This will return peer_sid = SECSID_NULL if there are
 		 * no peer labels, see security_net_peersid_resolve().
 		 */
-		err = selinux_skb_peerlbl_sid(skb, ep->base.sk->sk_family,
+		err = selinux_skb_peerlbl_sid(skb, asoc->base.sk->sk_family,
 					      &peer_sid);
 		if (err)
 			return err;
@@ -5400,7 +5400,7 @@ static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
 		 */
 		ad.type = LSM_AUDIT_DATA_NET;
 		ad.u.net = &net;
-		ad.u.net->sk = ep->base.sk;
+		ad.u.net->sk = asoc->base.sk;
 		err = avc_has_perm(&selinux_state,
 				   sksec->peer_sid, peer_sid, sksec->sclass,
 				   SCTP_SOCKET__ASSOCIATION, &ad);
@@ -5409,7 +5409,7 @@ static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
 	}
 
 	/* Compute the MLS component for the connection and store
-	 * the information in ep. This will be used by SCTP TCP type
+	 * the information in asoc. This will be used by SCTP TCP type
 	 * sockets and peeled off connections as they cause a new
 	 * socket to be generated. selinux_sctp_sk_clone() will then
 	 * plug this into the new socket.
@@ -5418,11 +5418,11 @@ static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
 	if (err)
 		return err;
 
-	ep->secid = conn_sid;
-	ep->peer_secid = peer_sid;
+	asoc->secid = conn_sid;
+	asoc->peer_secid = peer_sid;
 
 	/* Set any NetLabel labels including CIPSO/CALIPSO options. */
-	return selinux_netlbl_sctp_assoc_request(ep, skb);
+	return selinux_netlbl_sctp_assoc_request(asoc, skb);
 }
 
 /* Check if sctp IPv4/IPv6 addresses are valid for binding or connecting
@@ -5507,7 +5507,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
 }
 
 /* Called whenever a new socket is created by accept(2) or sctp_peeloff(3). */
-static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
+static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk,
 				  struct sock *newsk)
 {
 	struct sk_security_struct *sksec = sk->sk_security;
@@ -5519,8 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
 	if (!selinux_policycap_extsockclass())
 		return selinux_sk_clone_security(sk, newsk);
 
-	newsksec->sid = ep->secid;
-	newsksec->peer_sid = ep->peer_secid;
+	newsksec->sid = asoc->secid;
+	newsksec->peer_sid = asoc->peer_secid;
 	newsksec->sclass = sksec->sclass;
 	selinux_netlbl_sctp_sk_clone(sk, newsk);
 }
diff --git a/security/selinux/include/netlabel.h b/security/selinux/include/netlabel.h
index 0c58f62dc6ab..4d0456d3d459 100644
--- a/security/selinux/include/netlabel.h
+++ b/security/selinux/include/netlabel.h
@@ -39,7 +39,7 @@ int selinux_netlbl_skbuff_getsid(struct sk_buff *skb,
 int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
 				 u16 family,
 				 u32 sid);
-int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
+int selinux_netlbl_sctp_assoc_request(struct sctp_association *asoc,
 				     struct sk_buff *skb);
 int selinux_netlbl_inet_conn_request(struct request_sock *req, u16 family);
 void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family);
@@ -98,7 +98,7 @@ static inline int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
 	return 0;
 }
 
-static inline int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
+static inline int selinux_netlbl_sctp_assoc_request(struct sctp_association *asoc,
 						    struct sk_buff *skb)
 {
 	return 0;
diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
index abaab7683840..1e0b63848751 100644
--- a/security/selinux/netlabel.c
+++ b/security/selinux/netlabel.c
@@ -260,30 +260,30 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
 
 /**
  * selinux_netlbl_sctp_assoc_request - Label an incoming sctp association.
- * @ep: incoming association endpoint.
+ * @asoc: incoming association.
  * @skb: the packet.
  *
  * Description:
- * A new incoming connection is represented by @ep, ......
+ * A new incoming connection is represented by @asoc, ......
  * Returns zero on success, negative values on failure.
  *
  */
-int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
+int selinux_netlbl_sctp_assoc_request(struct sctp_association *asoc,
 				     struct sk_buff *skb)
 {
 	int rc;
 	struct netlbl_lsm_secattr secattr;
-	struct sk_security_struct *sksec = ep->base.sk->sk_security;
+	struct sk_security_struct *sksec = asoc->base.sk->sk_security;
 	struct sockaddr_in addr4;
 	struct sockaddr_in6 addr6;
 
-	if (ep->base.sk->sk_family != PF_INET &&
-				ep->base.sk->sk_family != PF_INET6)
+	if (asoc->base.sk->sk_family != PF_INET &&
+	    asoc->base.sk->sk_family != PF_INET6)
 		return 0;
 
 	netlbl_secattr_init(&secattr);
 	rc = security_netlbl_sid_to_secattr(&selinux_state,
-					    ep->secid, &secattr);
+					    asoc->secid, &secattr);
 	if (rc != 0)
 		goto assoc_request_return;
 
@@ -293,11 +293,11 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
 	if (ip_hdr(skb)->version == 4) {
 		addr4.sin_family = AF_INET;
 		addr4.sin_addr.s_addr = ip_hdr(skb)->saddr;
-		rc = netlbl_conn_setattr(ep->base.sk, (void *)&addr4, &secattr);
+		rc = netlbl_conn_setattr(asoc->base.sk, (void *)&addr4, &secattr);
 	} else if (IS_ENABLED(CONFIG_IPV6) && ip_hdr(skb)->version == 6) {
 		addr6.sin6_family = AF_INET6;
 		addr6.sin6_addr = ipv6_hdr(skb)->saddr;
-		rc = netlbl_conn_setattr(ep->base.sk, (void *)&addr6, &secattr);
+		rc = netlbl_conn_setattr(asoc->base.sk, (void *)&addr6, &secattr);
 	} else {
 		rc = -EAFNOSUPPORT;
 	}
-- 
2.27.0

[PATCHv2 net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce

[Xin Long]

The asoc created when receives the INIT chunk is a temporary one, it
will be deleted after INIT_ACK chunk is replied. So for the real asoc
created in sctp_sf_do_5_1D_ce() when the COOKIE_ECHO chunk is received,
security_sctp_assoc_request() should also be called.

v1->v2:
  - fix some typo and grammar errors, noticed by Ondrej.

Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
Tested-by: Richard Haines <richard_c_haines@btinternet.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 Documentation/security/SCTP.rst | 15 +++++++++------
 net/sctp/sm_statefuns.c         |  5 +++++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Documentation/security/SCTP.rst b/Documentation/security/SCTP.rst
index 415b548d9ce0..d5fd6ccc3dcb 100644
--- a/Documentation/security/SCTP.rst
+++ b/Documentation/security/SCTP.rst
@@ -151,9 +151,9 @@ establishing an association.
          INIT --------------------------------------------->
                                                    sctp_sf_do_5_1B_init()
                                                  Respond to an INIT chunk.
-                                             SCTP peer endpoint "A" is
-                                             asking for an association. Call
-                                             security_sctp_assoc_request()
+                                             SCTP peer endpoint "A" is asking
+                                             for a temporary association.
+                                             Call security_sctp_assoc_request()
                                              to set the peer label if first
                                              association.
                                              If not first association, check
@@ -163,9 +163,12 @@ establishing an association.
           |                                       discard the packet.
           |
     COOKIE ECHO ------------------------------------------>
-                                                          |
-                                                          |
-                                                          |
+                                                  sctp_sf_do_5_1D_ce()
+                                             Respond to an COOKIE ECHO chunk.
+                                             Confirm the cookie and create a
+                                             permanent association.
+                                             Call security_sctp_assoc_request() to
+                                             do the same as for INIT chunk Response.
           <------------------------------------------- COOKIE ACK
           |                                               |
     sctp_sf_do_5_1E_ca                                    |
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 3206374209bc..b818532c3fc2 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -781,6 +781,11 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net,
 		}
 	}
 
+	if (security_sctp_assoc_request(new_asoc, chunk->skb)) {
+		sctp_association_free(new_asoc);
+		return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands);
+	}
+
 	/* Delay state machine commands until later.
 	 *
 	 * Re-build the bind address for the association is done in
-- 
2.27.0

[PATCHv2 net 3/4] security: add sctp_assoc_established hook

[Xin Long]

security_sctp_assoc_established() is added to replace
security_inet_conn_established() called in
sctp_sf_do_5_1E_ca(), so that asoc can be accessed in security
subsystem and save the peer secid to asoc->peer_secid.

v1->v2:
  - fix the return value of security_sctp_assoc_established() in
    security.h, found by kernel test robot and Ondrej.

Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
Tested-by: Richard Haines <richard_c_haines@btinternet.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 Documentation/security/SCTP.rst | 22 ++++++++++------------
 include/linux/lsm_hook_defs.h   |  2 ++
 include/linux/lsm_hooks.h       |  5 +++++
 include/linux/security.h        |  7 +++++++
 net/sctp/sm_statefuns.c         |  2 +-
 security/security.c             |  7 +++++++
 6 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/Documentation/security/SCTP.rst b/Documentation/security/SCTP.rst
index d5fd6ccc3dcb..406cc68b8808 100644
--- a/Documentation/security/SCTP.rst
+++ b/Documentation/security/SCTP.rst
@@ -15,10 +15,7 @@ For security module support, three SCTP specific hooks have been implemented::
     security_sctp_assoc_request()
     security_sctp_bind_connect()
     security_sctp_sk_clone()
-
-Also the following security hook has been utilised::
-
-    security_inet_conn_established()
+    security_sctp_assoc_established()
 
 The usage of these hooks are described below with the SELinux implementation
 described in the `SCTP SELinux Support`_ chapter.
@@ -122,11 +119,12 @@ calls **sctp_peeloff**\(3).
     @newsk - pointer to new sock structure.
 
 
-security_inet_conn_established()
+security_sctp_assoc_established()
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Called when a COOKIE ACK is received::
+Called when a COOKIE ACK is received, and the peer secid will be
+saved into ``@asoc->peer_secid`` for client::
 
-    @sk  - pointer to sock structure.
+    @asoc - pointer to sctp association structure.
     @skb - pointer to skbuff of the COOKIE ACK packet.
 
 
@@ -134,7 +132,7 @@ Security Hooks used for Association Establishment
 -------------------------------------------------
 
 The following diagram shows the use of ``security_sctp_bind_connect()``,
-``security_sctp_assoc_request()``, ``security_inet_conn_established()`` when
+``security_sctp_assoc_request()``, ``security_sctp_assoc_established()`` when
 establishing an association.
 ::
 
@@ -172,7 +170,7 @@ establishing an association.
           <------------------------------------------- COOKIE ACK
           |                                               |
     sctp_sf_do_5_1E_ca                                    |
- Call security_inet_conn_established()                    |
+ Call security_sctp_assoc_established()                   |
  to set the peer label.                                   |
           |                                               |
           |                               If SCTP_SOCKET_TCP or peeled off
@@ -198,7 +196,7 @@ hooks with the SELinux specifics expanded below::
     security_sctp_assoc_request()
     security_sctp_bind_connect()
     security_sctp_sk_clone()
-    security_inet_conn_established()
+    security_sctp_assoc_established()
 
 
 security_sctp_assoc_request()
@@ -271,12 +269,12 @@ sockets sid and peer sid to that contained in the ``@asoc sid`` and
     @newsk - pointer to new sock structure.
 
 
-security_inet_conn_established()
+security_sctp_assoc_established()
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 Called when a COOKIE ACK is received where it sets the connection's peer sid
 to that in ``@skb``::
 
-    @sk  - pointer to sock structure.
+    @asoc - pointer to sctp association structure.
     @skb - pointer to skbuff of the COOKIE ACK packet.
 
 
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 0024273a7382..e9870118cc67 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -334,6 +334,8 @@ LSM_HOOK(int, 0, sctp_bind_connect, struct sock *sk, int optname,
 	 struct sockaddr *address, int addrlen)
 LSM_HOOK(void, LSM_RET_VOID, sctp_sk_clone, struct sctp_association *asoc,
 	 struct sock *sk, struct sock *newsk)
+LSM_HOOK(void, LSM_RET_VOID, sctp_assoc_established, struct sctp_association *asoc,
+	 struct sk_buff *skb)
 #endif /* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 240b92d89852..ba42c22204e2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1047,6 +1047,11 @@
  *	@asoc pointer to current sctp association structure.
  *	@sk pointer to current sock structure.
  *	@newsk pointer to new sock structure.
+ * @sctp_assoc_established:
+ *	Passes the @asoc and @chunk->skb of the association COOKIE_ACK packet
+ *	to the security module.
+ *	@asoc pointer to sctp association structure.
+ *	@skb pointer to skbuff of association packet.
  *
  * Security hooks for Infiniband
  *
diff --git a/include/linux/security.h b/include/linux/security.h
index a16407444871..c2ac6b15e50b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1423,6 +1423,8 @@ int security_sctp_bind_connect(struct sock *sk, int optname,
 			       struct sockaddr *address, int addrlen);
 void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk,
 			    struct sock *newsk);
+void security_sctp_assoc_established(struct sctp_association *asoc,
+				     struct sk_buff *skb);
 
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct sock *sock,
@@ -1642,6 +1644,11 @@ static inline void security_sctp_sk_clone(struct sctp_association *asoc,
 					  struct sock *newsk)
 {
 }
+
+static inline void security_sctp_assoc_established(struct sctp_association *asoc,
+						   struct sk_buff *skb)
+{
+}
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index b818532c3fc2..5fabaa54b77d 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -946,7 +946,7 @@ enum sctp_disposition sctp_sf_do_5_1E_ca(struct net *net,
 	sctp_add_cmd_sf(commands, SCTP_CMD_INIT_COUNTER_RESET, SCTP_NULL());
 
 	/* Set peer label for connection. */
-	security_inet_conn_established(ep->base.sk, chunk->skb);
+	security_sctp_assoc_established((struct sctp_association *)asoc, chunk->skb);
 
 	/* RFC 2960 5.1 Normal Establishment of an Association
 	 *
diff --git a/security/security.c b/security/security.c
index b0f1c007aa3b..4b2b4b5beb27 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2387,6 +2387,13 @@ void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk,
 }
 EXPORT_SYMBOL(security_sctp_sk_clone);
 
+void security_sctp_assoc_established(struct sctp_association *asoc,
+				     struct sk_buff *skb)
+{
+	call_void_hook(sctp_assoc_established, asoc, skb);
+}
+EXPORT_SYMBOL(security_sctp_assoc_established);
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND
-- 
2.27.0

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

[Xin Long]

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.

Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
Tested-by: Richard Haines <richard_c_haines@btinternet.com>
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

Re: [PATCHv2 net 0/4] security: fixups for the security hooks in sctp

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Tue,  2 Nov 2021 08:02:46 -0400 you wrote:
> There are a couple of problems in the currect security hooks in sctp:
> 
> 1. The hooks incorrectly treat sctp_endpoint in SCTP as request_sock in
>    TCP, while it's in fact no more than an extension of the sock, and
>    represents the local host. It is created when sock is created, not
>    when a conn request comes. sctp_association is actually the correct
>    one to represent the connection, and created when a conn request
>    arrives.
> 
> [...]

Here is the summary with links:
  - [PATCHv2,net,1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone
    https://git.kernel.org/netdev/net/c/c081d53f97a1
  - [PATCHv2,net,2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce
    https://git.kernel.org/netdev/net/c/e215dab1c490
  - [PATCHv2,net,3/4] security: add sctp_assoc_established hook
    https://git.kernel.org/netdev/net/c/7c2ef0240e6a
  - [PATCHv2,net,4/4] security: implement sctp_assoc_established hook in selinux
    https://git.kernel.org/netdev/net/c/e7310c94024c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Ondrej Mosnacek]

Hi Xin,

On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> 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()).

>
> Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
> Tested-by: Richard Haines <richard_c_haines@btinternet.com>

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.

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

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

[Xin Long]

On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Hi Xin,
>
> On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
> >
> > 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?

>
> >
> > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
> > Tested-by: Richard Haines <richard_c_haines@btinternet.com>
>
> 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.

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

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

[Xin Long]

On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Hi Xin,
> >
> > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
> > >
> > > 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().
For client side, secid should NOT be copied from its parent socket directly, but
gets it from socket_sockcreate_sid().
>
> and you?
>
> >
> > >
> > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
> > > Tested-by: Richard Haines <richard_c_haines@btinternet.com>
> >
> > 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.
>
> >
> > > 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.
> >

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

[Paul Moore]

On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote:
> On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > >
> > > > 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.

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

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.

> > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
> > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com>
> > >
> > > 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.

-- 
paul moore
www.paul-moore.com

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

[Xin Long]

On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > >
> > > > > 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.

>
> > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
> > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com>
> > > >
> > > > 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

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

[Paul Moore]

On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote:
> On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > >
> > > > > > 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.

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

> > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
> > > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com>
> > > > >
> > > > > 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.

While I understand you did not intend to mislead DaveM and the netdev
folks with the v2 patchset, your failure to properly manage the
patchset's metadata *did* mislead them and as a result a patchset with
serious concerns from the SELinux side was merged.  You need to revert
this patchset while we continue to discuss, develop, and verify a
proper fix that we can all agree on.  If you decide not to revert this
patchset I will work with DaveM to do it for you, and that is not
something any of us wants.

-- 
paul moore
www.paul-moore.com

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

[Richard Haines]

On Wed, 2021-11-03 at 23:17 -0400, Paul Moore wrote:
> On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com>
> > wrote:
> > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com>
> > > wrote:
> > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com>
> > > > wrote:
> > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek
> > > > > <omosnace@redhat.com> wrote:
> > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long
> > > > > > <lucien.xin@gmail.com> wrote:
> > > > > > > 
> > > > > > > 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.
> 
> > 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;
>   }

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

[Ondrej Mosnacek]

On Thu, Nov 4, 2021 at 4:17 AM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > > >
> > > > > > > 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.
>
> > 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;
>     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;
>   }

This code would be functionally equivalent to the v1 patchset for the
client side, but on server side you want to set newsksec->secid to
asoc->secid, as this contains the "connection secid" computed by
selinux_conn_sid() in selinux_sctp_assoc_request(). This is supposed
to mirror what selinux_inet_conn_request() -> selinux_inet_csk_clone()
does for non-SCTP sockets. So I think we should rather go back to the
v1 patchset variant, where the parent socket's sid is stashed in
asoc->secid to be picked up by selinux_sctp_sk_clone().

As for the sctp_do_peeloff-calls-sock_create problem - I was oblivious
about the difference between the sock vs. socket structs, so this
would be a bit more difficult to fix than replacing one function call.
But if we end up just overwriting the label assigned in
selinux_socket_post_create() as it is now, then the only difference is
an unexpected SCTP_SOCKET__CREATE permission check and a pointless
computation of socket_sockcreate_sid(), so it can be addressed
separately. I'll try to suggest a patch and then we can discuss
whether it makes sense or not.

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

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

[Xin Long]

On Wed, Nov 3, 2021 at 11:17 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > > >
> > > > > > > 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 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].

[2] I'm guessing the client associations might also want to follow the
setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more
info.
"""
What I got is to take it's label from the parent process, which means
we get it from socket_sockcreate_sid(), not directly copy from parent
socket. It seems I misunderstood that, Sorry, maybe we should just
use the v1 patchset.


> (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.
Right, that's what it currently does in this patchset, no *socket* sid
is changed, and only *socket*'s peer label.

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

>
> > 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;
>     newsksec->peer_sid = asoc->peer_secid;
>     newsksec->sclass = sksec->sclass;
>     selinux_netlbl_sctp_sk_clone(sk, newsk);
>   }
here, SCTP is one-to-many socket, and it means one socket can have
multiple associations or connections, so for sksec->sid in one socket
it can only save the latest cid, if we peel off an old one, it will get the
wrong cid on server side.

>
>   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;
>   }
>
> > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > > > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com>
> > > > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com>
> > > > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com>
> > > > > >
> > > > > > 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.
>
> While I understand you did not intend to mislead DaveM and the netdev
> folks with the v2 patchset, your failure to properly manage the
> patchset's metadata *did* mislead them and as a result a patchset with
> serious concerns from the SELinux side was merged.  You need to revert
> this patchset while we continue to discuss, develop, and verify a
> proper fix that we can all agree on.  If you decide not to revert this
> patchset I will work with DaveM to do it for you, and that is not
> something any of us wants.
>
> --
> paul moore
> www.paul-moore.com

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

[David Miller]

From: Paul Moore <paul@paul-moore.com>
Date: Wed, 3 Nov 2021 23:17:00 -0400

> 
> While I understand you did not intend to mislead DaveM and the netdev
> folks with the v2 patchset, your failure to properly manage the
> patchset's metadata *did* mislead them and as a result a patchset with
> serious concerns from the SELinux side was merged.  You need to revert
> this patchset while we continue to discuss, develop, and verify a
> proper fix that we can all agree on.  If you decide not to revert this
> patchset I will work with DaveM to do it for you, and that is not
> something any of us wants.

I would prefer a follow-up rathewr than a revert at this point.

Please work with Xin to come up with a fix that works for both of you.

Thanks.

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

[Paul Moore]

On Thu, Nov 4, 2021 at 7:02 AM David Miller <davem@davemloft.net> wrote:
> From: Paul Moore <paul@paul-moore.com>
> Date: Wed, 3 Nov 2021 23:17:00 -0400
> >
> > While I understand you did not intend to mislead DaveM and the netdev
> > folks with the v2 patchset, your failure to properly manage the
> > patchset's metadata *did* mislead them and as a result a patchset with
> > serious concerns from the SELinux side was merged.  You need to revert
> > this patchset while we continue to discuss, develop, and verify a
> > proper fix that we can all agree on.  If you decide not to revert this
> > patchset I will work with DaveM to do it for you, and that is not
> > something any of us wants.
>
> I would prefer a follow-up rathewr than a revert at this point.
>
> Please work with Xin to come up with a fix that works for both of you.

We are working with Xin (see this thread), but you'll notice there is
still not a clear consensus on the best path forward.  The only thing
I am clear on at this point is that the current code in linux-next is
*not* something we want from a SELinux perspective.  I don't like
leaving known bad code like this in linux-next for more than a day or
two so please revert it, now.  If your policy is to merge substantive
non-network subsystem changes into the network tree without the proper
ACKs from the other subsystem maintainers, it would seem reasonable to
also be willing to revert those patches when the affected subsystems
request it.

I understand that if a patchset is being ignored you might feel the
need to act without an explicit ACK, but this particular patchset
wasn't even a day old before you merged into the netdev tree.  Not to
mention that the patchset was posted during the second day of the
merge window, a time when many maintainers are busy testing code,
sending pull requests to Linus, and generally managing merge window
fallout.

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Thu, Nov 4, 2021 at 6:40 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Nov 4, 2021 at 4:17 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > > > >
> > > > > > > > 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.
> >
> > > 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;
> >     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;
> >   }
>
> This code would be functionally equivalent to the v1 patchset for the
> client side, but on server side you want to set newsksec->secid to
> asoc->secid, as this contains the "connection secid" computed by
> selinux_conn_sid() in selinux_sctp_assoc_request(). This is supposed
> to mirror what selinux_inet_conn_request() -> selinux_inet_csk_clone()
> does for non-SCTP sockets. So I think we should rather go back to the
> v1 patchset variant, where the parent socket's sid is stashed in
> asoc->secid to be picked up by selinux_sctp_sk_clone().
>
> As for the sctp_do_peeloff-calls-sock_create problem - I was oblivious
> about the difference between the sock vs. socket structs, so this
> would be a bit more difficult to fix than replacing one function call.
> But if we end up just overwriting the label assigned in
> selinux_socket_post_create() as it is now, then the only difference is
> an unexpected SCTP_SOCKET__CREATE permission check and a pointless
> computation of socket_sockcreate_sid(), so it can be addressed
> separately. I'll try to suggest a patch and then we can discuss
> whether it makes sense or not.

Okay, I'll wait on that patchset before commenting further.

-- 
paul moore
www.paul-moore.com

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

[Xin Long]

On Thu, Nov 4, 2021 at 3:10 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Nov 4, 2021 at 7:02 AM David Miller <davem@davemloft.net> wrote:
> > From: Paul Moore <paul@paul-moore.com>
> > Date: Wed, 3 Nov 2021 23:17:00 -0400
> > >
> > > While I understand you did not intend to mislead DaveM and the netdev
> > > folks with the v2 patchset, your failure to properly manage the
> > > patchset's metadata *did* mislead them and as a result a patchset with
> > > serious concerns from the SELinux side was merged.  You need to revert
> > > this patchset while we continue to discuss, develop, and verify a
> > > proper fix that we can all agree on.  If you decide not to revert this
> > > patchset I will work with DaveM to do it for you, and that is not
> > > something any of us wants.
> >
> > I would prefer a follow-up rathewr than a revert at this point.
> >
> > Please work with Xin to come up with a fix that works for both of you.
>
> We are working with Xin (see this thread), but you'll notice there is
> still not a clear consensus on the best path forward.  The only thing
> I am clear on at this point is that the current code in linux-next is
> *not* something we want from a SELinux perspective.  I don't like
> leaving known bad code like this in linux-next for more than a day or
> two so please revert it, now.  If your policy is to merge substantive
> non-network subsystem changes into the network tree without the proper
> ACKs from the other subsystem maintainers, it would seem reasonable to
> also be willing to revert those patches when the affected subsystems
> request it.
>
> I understand that if a patchset is being ignored you might feel the
> need to act without an explicit ACK, but this particular patchset
> wasn't even a day old before you merged into the netdev tree.  Not to
> mention that the patchset was posted during the second day of the
> merge window, a time when many maintainers are busy testing code,
> sending pull requests to Linus, and generally managing merge window
> fallout.
>
> --
> paul moore
> www.paul-moore.com
Hi Paul,

It's applied on net tree, I think mostly because I posted this on net.git tree.
Also, it's well related to the network part and affects SCTP protocol
quite a lot.

I wanted to post it on selinux tree: pcmoore/selinux.git, but I noticed the
commit on top is written in 2019:

commit 6e6934bae891681bc23b2536fff20e0898683f2c (HEAD -> main,
origin/main, origin/HEAD)
Author: Paul Moore <paul@paul-moore.com>
Date:   Tue Sep 17 15:02:56 2019 -0400

    selinux: add a SELinux specific README.md

    DO NOT SUBMIT UPSTREAM

Then I thought this tree was no longer active, sorry about that.

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

[Paul Moore]

On Thu, Nov 4, 2021 at 3:49 PM Xin Long <lucien.xin@gmail.com> wrote:
> On Thu, Nov 4, 2021 at 3:10 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Nov 4, 2021 at 7:02 AM David Miller <davem@davemloft.net> wrote:
> > > From: Paul Moore <paul@paul-moore.com>
> > > Date: Wed, 3 Nov 2021 23:17:00 -0400
> > > >
> > > > While I understand you did not intend to mislead DaveM and the netdev
> > > > folks with the v2 patchset, your failure to properly manage the
> > > > patchset's metadata *did* mislead them and as a result a patchset with
> > > > serious concerns from the SELinux side was merged.  You need to revert
> > > > this patchset while we continue to discuss, develop, and verify a
> > > > proper fix that we can all agree on.  If you decide not to revert this
> > > > patchset I will work with DaveM to do it for you, and that is not
> > > > something any of us wants.
> > >
> > > I would prefer a follow-up rathewr than a revert at this point.
> > >
> > > Please work with Xin to come up with a fix that works for both of you.
> >
> > We are working with Xin (see this thread), but you'll notice there is
> > still not a clear consensus on the best path forward.  The only thing
> > I am clear on at this point is that the current code in linux-next is
> > *not* something we want from a SELinux perspective.  I don't like
> > leaving known bad code like this in linux-next for more than a day or
> > two so please revert it, now.  If your policy is to merge substantive
> > non-network subsystem changes into the network tree without the proper
> > ACKs from the other subsystem maintainers, it would seem reasonable to
> > also be willing to revert those patches when the affected subsystems
> > request it.
> >
> > I understand that if a patchset is being ignored you might feel the
> > need to act without an explicit ACK, but this particular patchset
> > wasn't even a day old before you merged into the netdev tree.  Not to
> > mention that the patchset was posted during the second day of the
> > merge window, a time when many maintainers are busy testing code,
> > sending pull requests to Linus, and generally managing merge window
> > fallout.
>
> Hi Paul,
>
> It's applied on net tree, I think mostly because I posted this on net.git tree.
> Also, it's well related to the network part and affects SCTP protocol
> quite a lot.

Yes, I know it is in the net tree, that is how it made its way into
linux-next.  I wouldn't have merged it yet, and if not me who else
would have merged it beside the netdev folks?

Am I misunderstanding your comment?

> I wanted to post it on selinux tree: pcmoore/selinux.git, but I noticed the
> commit on top is written in 2019:
>
> commit 6e6934bae891681bc23b2536fff20e0898683f2c (HEAD -> main,
> origin/main, origin/HEAD)
> Author: Paul Moore <paul@paul-moore.com>
> Date:   Tue Sep 17 15:02:56 2019 -0400
>
>     selinux: add a SELinux specific README.md
>
>     DO NOT SUBMIT UPSTREAM
>
> Then I thought this tree was no longer active, sorry about that.

Like many kernel trees the default/main branch for the SELinux tree
doesn't contain anything useful, for the SELinux tree (and audit for
that matter) it is basically just the most recent major/minor tag from
Linus tree with a single tree specific README.md file patch so that
the GitHub mirror has a pretty landing page and a canonical reference
for how the tree is maintained.

* https://github.com/SELinuxProject/selinux-kernel

The general approach to the SELinux tree, as documented in the
README.md, is to do all of the linux-next work in the selinux/next
branch with the stable work happening in the selinux/stable-X.Y
branches.

FWIW, once we've resolved things I would be happy to have the patchset
live in the SELinux tree as opposed to the netdev tree.

-- 
paul moore
www.paul-moore.com