secid reconciliation-v04: Enforcement for SELinux

PATCH In review Venkat Yekkirala 4 participants 5 messages Oct 1, 2006 – Oct 2, 2006

Subsystems (3)

Networking Network drivers SELinux security module

lore

Share & tools

lore

Venkat Yekkirala

author

2006-10-01 21:27 UTC #0 lore raw

PATCH 7/9 secid reconciliation-v04: Enforcement for SELinux

This defines SELinux enforcement of the 2 new LSM hooks as well as related changes elsewhere in the SELinux code.

This also now keeps track of the peersid thru the establishment of a connection on the server (tracking peersid on the client is covered later in this patch set).

Signed-off-by Venkat Yekkirala <[email protected]>

---
 security/selinux/hooks.c        |  148 +++++++++++++++++++++++-------
 security/selinux/include/xfrm.h |   11 +-
 security/selinux/xfrm.c         |   66 +++++--------
 3 files changed, 150 insertions(+), 75 deletions(-)

security/selinux/hooks.c +116 ~~-32~~

index 5a66c4c..fc65cc7 100644
@@ -3449,8 +3449,12 @@ static int selinux_sock_rcv_skb_compat(s
 
 		err = avc_has_perm(sock_sid, port_sid,
 				   sock_class, recv_perm, ad);
+		if (err)
+			goto out;
 	}
 
+	err = selinux_xfrm_sock_rcv_skb(sock_sid, skb, ad);
+
 out:
 	return err;
 }

@@ -3489,10 +3493,6 @@ static int selinux_socket_sock_rcv_skb(s
 		goto out;
 
 	err = selinux_netlbl_sock_rcv_skb(sksec, skb, &ad);
-	if (err)
-		goto out;
-
-	err = selinux_xfrm_sock_rcv_skb(sksec->sid, skb, &ad);
 out:	
 	return err;
 }

@@ -3505,7 +3505,7 @@ static int selinux_socket_getpeersec_str
 	u32 scontext_len;
 	struct sk_security_struct *ssec;
 	struct inode_security_struct *isec;
-	u32 peer_sid = 0;
+	u32 peer_sid;
 
 	isec = SOCK_INODE(sock)->i_security;

@@ -3516,8 +3516,10 @@ static int selinux_socket_getpeersec_str
 	}
 	else if (isec->sclass == SECCLASS_TCP_SOCKET) {
 		peer_sid = selinux_netlbl_socket_getpeersec_stream(sock);
-		if (peer_sid == SECSID_NULL)
-			peer_sid = selinux_socket_getpeer_stream(sock->sk);
+		if (peer_sid == SECSID_NULL) {
+			ssec = sock->sk->sk_security;
+			peer_sid = ssec->peer_sid;
+		}
 		if (peer_sid == SECSID_NULL) {
 			err = -ENOPROTOOPT;
 			goto out;

@@ -3550,7 +3552,8 @@ out:	
 	return err;
 }
 
-static int selinux_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
+static int selinux_socket_getpeersec_dgram(struct socket *sock,
+				struct sk_buff *skb, u32 *secid)
 {
 	u32 peer_secid = SECSID_NULL;
 	int err = 0;

@@ -3559,8 +3562,12 @@ static int selinux_socket_getpeersec_dgr
 		selinux_get_inode_sid(SOCK_INODE(sock), &peer_secid);
 	else if (skb) {
 		peer_secid = selinux_netlbl_socket_getpeersec_dgram(skb);
-		if (peer_secid == SECSID_NULL)
-			peer_secid = selinux_socket_getpeer_dgram(skb);
+		if (peer_secid == SECSID_NULL) {
+			if (selinux_compat_net)
+				peer_secid = selinux_socket_getpeer_dgram(skb);
+			else
+				peer_secid = skb->secmark;
+		}
 	}
 
 	if (peer_secid == SECSID_NULL)

@@ -3626,19 +3633,24 @@ static int selinux_inet_conn_request(str
 		return 0;
 	}
 
-	err = selinux_xfrm_decode_session(skb, &peersid, 0);
-	BUG_ON(err);
+	if (selinux_compat_net) {
+		err = selinux_xfrm_decode_session(skb, &peersid, 0);
+		BUG_ON(err);
 
-	if (peersid == SECSID_NULL) {
-		req->secid = sksec->sid;
-		return 0;
-	}
+		if (peersid == SECSID_NULL) {
+			req->secid = sksec->sid;
+			req->peer_secid = 0;
+			return 0;
+		}
+	} else
+		peersid = skb->secmark;
 
 	err = security_sid_mls_copy(sksec->sid, peersid, &newsid);
 	if (err)
 		return err;
 
 	req->secid = newsid;
+	req->peer_secid = peersid;
 	return 0;
 }

@@ -3648,6 +3660,7 @@ static void selinux_inet_csk_clone(struc
 	struct sk_security_struct *newsksec = newsk->sk_security;
 
 	newsksec->sid = req->secid;
+	newsksec->peer_sid = req->peer_secid;
 	/* NOTE: Ideally, we should also get the isec->sid for the
 	   new socket in sync, but we don't have the isec available yet.
 	   So we will wait until sock_graft to do it, by which

@@ -3662,6 +3675,66 @@ static void selinux_req_classify_flow(co
 	fl->secid = req->secid;
 }
 
+static int selinux_skb_flow_in(struct sk_buff *skb, unsigned short family)
+{
+	u32 xfrm_sid;
+	int err;
+
+	if (selinux_compat_net)
+		return 1;
+
+	/*
+	 * loopback traffic already labeled and
+	 * flow-controlled on outbound. We may
+	 * need to flow-control on the inbound
+	 * as well if there's ever a use-case for it.
+	 */
+	if (skb->dev == &loopback_dev)
+		return 1;
+
+	err = selinux_xfrm_decode_session(skb, &xfrm_sid, 0);
+	BUG_ON(err);
+
+	err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
+					PACKET__FLOW_IN, NULL);
+	if (err)
+		goto out;
+
+	if (xfrm_sid)
+		skb->secmark = xfrm_sid;
+
+	/* See if NetLabel can flow in thru the current secmark here */
+
+out:
+	return err ? 0 : 1;
+};
+
+static int selinux_skb_flow_out(struct sk_buff *skb, u32 nf_secid)
+{
+	int err;
+
+	if (selinux_compat_net)
+		return 1;
+
+	if (!skb->secmark) {
+		u32 xfrm_sid;
+
+		selinux_skb_xfrm_sid(skb, &xfrm_sid);
+
+		if (xfrm_sid)
+			skb->secmark = xfrm_sid;
+		else if (skb->sk) {
+			struct sk_security_struct *sksec = skb->sk->sk_security;
+			skb->secmark = sksec->sid;
+		}
+	}
+
+	err = avc_has_perm(skb->secmark, nf_secid, SECCLASS_PACKET,
+				PACKET__FLOW_OUT, NULL);
+
+	return err ? 0 : 1;
+}
+
 static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
 {
 	int err = 0;

@@ -3700,7 +3773,8 @@ out:
 
 #ifdef CONFIG_NETFILTER
 
-static int selinux_ip_postroute_last_compat(struct sock *sk, struct net_device *dev,
+static int selinux_ip_postroute_last_compat(struct sock *sk, struct sk_buff *skb,
+					    struct net_device *dev,
 					    struct avc_audit_data *ad,
 					    u16 family, char *addrp, int len)
 {

@@ -3710,6 +3784,9 @@ static int selinux_ip_postroute_last_com
 	struct inode *inode;
 	struct inode_security_struct *isec;
 
+	if (!sk)
+		goto out;
+
 	sock = sk->sk_socket;
 	if (!sock)
 		goto out;

@@ -3768,7 +3845,11 @@ static int selinux_ip_postroute_last_com
 
 		err = avc_has_perm(isec->sid, port_sid, isec->sclass,
 				   send_perm, ad);
+		if (err)
+			goto out;
 	}
+
+	err = selinux_xfrm_postroute_last(isec->sid, skb, ad);
 out:
 	return err;
 }

@@ -3782,17 +3863,9 @@ static unsigned int selinux_ip_postroute
 {
 	char *addrp;
 	int len, err = 0;
-	struct sock *sk;
 	struct sk_buff *skb = *pskb;
 	struct avc_audit_data ad;
 	struct net_device *dev = (struct net_device *)out;
-	struct sk_security_struct *sksec;
-
-	sk = skb->sk;
-	if (!sk)
-		goto out;
-
-	sksec = sk->sk_security;
 
 	AVC_AUDIT_DATA_INIT(&ad, NET);
 	ad.u.net.netif = dev->name;

@@ -3803,16 +3876,25 @@ static unsigned int selinux_ip_postroute
 		goto out;
 
 	if (selinux_compat_net)
-		err = selinux_ip_postroute_last_compat(sk, dev, &ad,
+		err = selinux_ip_postroute_last_compat(skb->sk, skb, dev, &ad,
 						       family, addrp, len);
-	else
-		err = avc_has_perm(sksec->sid, skb->secmark, SECCLASS_PACKET,
-				   PACKET__SEND, &ad);
+	else {
+		if (!skb->secmark) {
+			u32 xfrm_sid;
 
-	if (err)
-		goto out;
+			selinux_skb_xfrm_sid(skb, &xfrm_sid);
 
-	err = selinux_xfrm_postroute_last(sksec->sid, skb, &ad);
+			if (xfrm_sid)
+				skb->secmark = xfrm_sid;
+			else if (skb->sk) {
+				struct sk_security_struct *sksec =
+						skb->sk->sk_security;
+				skb->secmark = sksec->sid;
+			}
+		}
+		err = avc_has_perm(skb->secmark, SECINITSID_UNLABELED,
+				   SECCLASS_PACKET, PACKET__FLOW_OUT, &ad);
+	}
 out:
 	return err ? NF_DROP : NF_ACCEPT;
 }

@@ -4719,6 +4801,8 @@ static struct security_operations selinu
 	.inet_conn_request =		selinux_inet_conn_request,
 	.inet_csk_clone =		selinux_inet_csk_clone,
 	.req_classify_flow =		selinux_req_classify_flow,
+	.skb_flow_in =			selinux_skb_flow_in,
+	.skb_flow_out =			selinux_skb_flow_out,
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	.xfrm_policy_alloc_security =	selinux_xfrm_policy_alloc,

security/selinux/include/xfrm.h +5 -6

index 81eb598..b1e8425 100644
@@ -38,9 +38,9 @@ int selinux_xfrm_sock_rcv_skb(u32 sid, s
 			struct avc_audit_data *ad);
 int selinux_xfrm_postroute_last(u32 isec_sid, struct sk_buff *skb,
 			struct avc_audit_data *ad);
-u32 selinux_socket_getpeer_stream(struct sock *sk);
 u32 selinux_socket_getpeer_dgram(struct sk_buff *skb);
 int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall);
+void selinux_skb_xfrm_sid(struct sk_buff *skb, u32 *sid);
 #else
 static inline int selinux_xfrm_sock_rcv_skb(u32 isec_sid, struct sk_buff *skb,
 			struct avc_audit_data *ad)

@@ -54,11 +54,6 @@ static inline int selinux_xfrm_postroute
 	return 0;
 }
 
-static inline int selinux_socket_getpeer_stream(struct sock *sk)
-{
-	return SECSID_NULL;
-}
-
 static inline int selinux_socket_getpeer_dgram(struct sk_buff *skb)
 {
 	return SECSID_NULL;

@@ -68,6 +63,10 @@ static inline int selinux_xfrm_decode_se
 	*sid = SECSID_NULL;
 	return 0;
 }
+static inline void selinux_skb_xfrm_sid(struct sk_buff *skb, u32 *sid)
+{
+	*sid = SECSID_NULL;
+}
 #endif
 
 #endif /* _SELINUX_XFRM_H_ */

security/selinux/xfrm.c +29 ~~-37~~

85 lines not on this page · raw message

--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

Paul Moore

2006-10-02 16:12 UTC #1 lore raw

Quoted 20 lines · Venkat Yekkirala

This defines SELinux enforcement of the 2 new LSM hooks as well as related changes elsewhere in the SELinux code.
This also now keeps track of the peersid thru the establishment of a connection on the server (tracking peersid on the client is covered later in this patch set).
Signed-off-by Venkat Yekkirala <[email protected]>
{snip}
security/selinux/hooks.c +9 -0
+static int selinux_skb_flow_in(struct sk_buff *skb, unsigned short family)
+{
+	u32 xfrm_sid;
+	int err;
+
+	if (selinux_compat_net)
+		return 1;
+
+	/*

security/selinux/hooks.c static void selinux_req_classify_flow(co · line 3696

Quoted 10 lines

security/selinux/hooks.c +10 -0

+	 * loopback traffic already labeled and
+	 * flow-controlled on outbound. We may
+	 * need to flow-control on the inbound
+	 * as well if there's ever a use-case for it.
+	 */
+	if (skb->dev == &loopback_dev)
+		return 1;
+
+	err = selinux_xfrm_decode_session(skb, &xfrm_sid, 0);
+	BUG_ON(err);

Just a quick question that has been nagging me for awhile - any particular reason why this is a BUG_ON() and not an "if (err) goto out;"?

Quoted 13 lines

security/selinux/hooks.c +13 -0

+	err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
+					PACKET__FLOW_IN, NULL);
+	if (err)
+		goto out;
+
+	if (xfrm_sid)
+		skb->secmark = xfrm_sid;
+
+	/* See if NetLabel can flow in thru the current secmark here */
+
+out:
+	return err ? 0 : 1;
+};

Signature · 7 lines

paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Stephen Smalley

2006-10-02 16:34 UTC #2 lore raw

Quoted 26 lines · Paul Moore

Quoted 25 lines · Venkat Yekkirala
This defines SELinux enforcement of the 2 new LSM hooks as well as related changes elsewhere in the SELinux code.
This also now keeps track of the peersid thru the establishment of a connection on the server (tracking peersid on the client is covered later in this patch set).
Signed-off-by Venkat Yekkirala <[email protected]>
{snip}
security/selinux/hooks.c +14 -0
+static int selinux_skb_flow_in(struct sk_buff *skb, unsigned short family)
+{
+	u32 xfrm_sid;
+	int err;
+
+	if (selinux_compat_net)
+		return 1;
+
+	/*
+	 * loopback traffic already labeled and
+	 * flow-controlled on outbound. We may
+	 * need to flow-control on the inbound
+	 * as well if there's ever a use-case for it.
+	 */

Quoted 8 lines

security/selinux/hooks.c static void selinux_req_classify_flow(co · line 3696
Quoted 5 lines
security/selinux/hooks.c +5 -0
+	if (skb->dev == &loopback_dev)
+		return 1;
+
+	err = selinux_xfrm_decode_session(skb, &xfrm_sid, 0);
+	BUG_ON(err);
Just a quick question that has been nagging me for awhile - any particular reason why this is a BUG_ON() and not an "if (err) goto out;"?

It appears that selinux_xfrm_decode_session() can only legitimately return an error if the last argument (ckall) is non-zero. security_skb_classify_flow() was doing the same thing prior to this patch series. It would be clearer if there were two separate interfaces that internally use the same helper, with one of the functions returning void.

Quoted 14 lines

Quoted 13 lines

security/selinux/hooks.c +13 -0

+	err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
+					PACKET__FLOW_IN, NULL);
+	if (err)
+		goto out;
+
+	if (xfrm_sid)
+		skb->secmark = xfrm_sid;
+
+	/* See if NetLabel can flow in thru the current secmark here */
+
+out:
+	return err ? 0 : 1;
+};

Signature · 2 lines

Stephen Smalley
National Security Agency

James Morris

2006-10-02 16:43 UTC #3 lore raw

Quoted 6 lines · Stephen Smalley

It appears that selinux_xfrm_decode_session() can only legitimately return an error if the last argument (ckall) is non-zero. security_skb_classify_flow() was doing the same thing prior to this patch series. It would be clearer if there were two separate interfaces that internally use the same helper, with one of the functions returning void.

Ok, this can be a followup patch request (and not block merging).

- James

Signature · 2 lines

James Morris
<[email protected]>

Paul Moore

2006-10-02 16:58 UTC #4 lore raw

In reply to Stephen Smalley

Quoted 38 lines · Stephen Smalley

Quoted 27 lines · Paul Moore
Quoted 25 lines · Venkat Yekkirala
This defines SELinux enforcement of the 2 new LSM hooks as well as related changes elsewhere in the SELinux code.
This also now keeps track of the peersid thru the establishment of a connection on the server (tracking peersid on the client is covered later in this patch set).
Signed-off-by Venkat Yekkirala <[email protected]>
{snip}
security/selinux/hooks.c +14 -0
+static int selinux_skb_flow_in(struct sk_buff *skb, unsigned short family)
+{
+	u32 xfrm_sid;
+	int err;
+
+	if (selinux_compat_net)
+		return 1;
+
+	/*
+	 * loopback traffic already labeled and
+	 * flow-controlled on outbound. We may
+	 * need to flow-control on the inbound
+	 * as well if there's ever a use-case for it.
+	 */
Quoted 8 lines
security/selinux/hooks.c static void selinux_req_classify_flow(co · line 3696
Quoted 5 lines
security/selinux/hooks.c +5 -0
+	if (skb->dev == &loopback_dev)
+		return 1;
+
+	err = selinux_xfrm_decode_session(skb, &xfrm_sid, 0);
+	BUG_ON(err);
Just a quick question that has been nagging me for awhile - any particular reason why this is a BUG_ON() and not an "if (err) goto out;"?

Quoted 6 lines

It appears that selinux_xfrm_decode_session() can only legitimately return an error if the last argument (ckall) is non-zero. security_skb_classify_flow() was doing the same thing prior to this patch series. It would be clearer if there were two separate interfaces that internally use the same helper, with one of the functions returning void.

My immediate concern is not really what selinux_xfrm_decode_session() returns, but how to handle it, or rather errors in general, in selinux_skb_flow_in(). I'm in the process of creating a patch to add the missing NetLabel support to the secid patches and I am wondering if I should BUG_ON() for an error condition or simply jump to "out". Jumping seems a bit cleaner to me, although perhaps harder to debug, so I was just wondering what the reasoning was behind the use of BUG_ON().

I honestly don't care at this point, it's a rather minor detail, I'd just like to "do the right thing" with the NetLabel patch.

Signature · 7 lines

paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.