mlsxfrm: Various fixes

PATCH In review Venkat Yekkirala 3 participants 7 messages Nov 7, 2006 – Nov 8, 2006

Subsystems (5)

Networking Network drivers SELinux security module Security (LSM) +1 more subsystems

lore

Share & tools

lore

Venkat Yekkirala

author

2006-11-07 17:17 UTC #0 lore raw

PATCH 3/3 mlsxfrm: Various fixes

Fix the selection of an SA for an outgoing packet to be at the same context as the originating socket/flow. This eliminates the SELinux policy's ability to use/sendto SAs with contexts other than the socket's.

With this patch applied, the SELinux policy will require one or more of the following for a socket to be able to communicate with/without SAs:

1. To enable a socket to communicate without using labeled-IPSec SAs:

allow socket_t unlabeled_t:association { sendto recvfrom }

2. To enable a socket to communicate with labeled-IPSec SAs:

allow socket_t self:association { sendto }; allow socket_t peer_sa_t:association { recvfrom };

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

---
 include/linux/security.h            |   19 -----
 net/xfrm/xfrm_policy.c              |    3 
 security/dummy.c                    |    7 -
 security/selinux/hooks.c            |   26 ++++--
 security/selinux/include/security.h |    2 
 security/selinux/include/xfrm.h     |    7 -
 security/selinux/ss/services.c      |   44 +++++++++++
 security/selinux/xfrm.c             |   97 ++++++++++++--------------
 8 files changed, 112 insertions(+), 93 deletions(-)

include/linux/security.h +0 ~~-19~~

@@ -886,11 +886,6 @@ struct request_sock;
  *	@xp contains the policy to check for a match.
  *	@fl contains the flow to check for a match.
  *	Return 1 if there is a match.
- * @xfrm_flow_state_match:
- *	@fl contains the flow key to match.
- *	@xfrm points to the xfrm_state to match.
- *	@xp points to the xfrm_policy to match.
- *	Return 1 if there is a match.
  * @xfrm_decode_session:
  *	@skb points to skb to decode.
  *	@secid points to the flow key secid to set.

@@ -1388,8 +1383,6 @@ struct security_operations {
 	int (*xfrm_policy_lookup)(struct xfrm_policy *xp, u32 fl_secid, u8 dir);
 	int (*xfrm_state_pol_flow_match)(struct xfrm_state *x,
 			struct xfrm_policy *xp, struct flowi *fl);
-	int (*xfrm_flow_state_match)(struct flowi *fl, struct xfrm_state *xfrm,
-			struct xfrm_policy *xp);
 	int (*xfrm_decode_session)(struct sk_buff *skb, u32 *secid, int ckall);
 #endif	/* CONFIG_SECURITY_NETWORK_XFRM */

@@ -3186,12 +3179,6 @@ static inline int security_xfrm_state_po
 	return security_ops->xfrm_state_pol_flow_match(x, xp, fl);
 }
 
-static inline int security_xfrm_flow_state_match(struct flowi *fl,
-			struct xfrm_state *xfrm, struct xfrm_policy *xp)
-{
-	return security_ops->xfrm_flow_state_match(fl, xfrm, xp);
-}
-
 static inline int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid)
 {
 	return security_ops->xfrm_decode_session(skb, secid, 1);

@@ -3255,12 +3242,6 @@ static inline int security_xfrm_state_po
 	return 1;
 }
 
-static inline int security_xfrm_flow_state_match(struct flowi *fl,
-			struct xfrm_state *xfrm, struct xfrm_policy *xp)
-{
-	return 1;
-}
-
 static inline int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid)
 {
 	return 0;

net/xfrm/xfrm_policy.c +2 -1

@@ -1894,7 +1894,8 @@ int xfrm_bundle_ok(struct xfrm_policy *p
 
 		if (fl && !xfrm_selector_match(&dst->xfrm->sel, fl, family))
 			return 0;
-		if (fl && !security_xfrm_flow_state_match(fl, dst->xfrm, pol))
+		if (fl && pol &&
+		    !security_xfrm_state_pol_flow_match(dst->xfrm, pol, fl))
 			return 0;
 		if (dst->xfrm->km.state != XFRM_STATE_VALID)
 			return 0;

security/dummy.c +0 -7

@@ -886,12 +886,6 @@ static int dummy_xfrm_state_pol_flow_mat
 	return 1;
 }
 
-static int dummy_xfrm_flow_state_match(struct flowi *fl, struct xfrm_state *xfrm,
-				struct xfrm_policy *xp)
-{
-	return 1;
-}
-
 static int dummy_xfrm_decode_session(struct sk_buff *skb, u32 *fl, int ckall)
 {
 	return 0;

@@ -1126,7 +1120,6 @@ void security_fixup_ops (struct security
 	set_to_dummy_if_null(ops, xfrm_state_delete_security);
 	set_to_dummy_if_null(ops, xfrm_policy_lookup);
 	set_to_dummy_if_null(ops, xfrm_state_pol_flow_match);
-	set_to_dummy_if_null(ops, xfrm_flow_state_match);
 	set_to_dummy_if_null(ops, xfrm_decode_session);
 #endif	/* CONFIG_SECURITY_NETWORK_XFRM */
 #ifdef CONFIG_KEYS

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

@@ -19,9 +19,6 @@ int selinux_xfrm_state_delete(struct xfr
 int selinux_xfrm_policy_lookup(struct xfrm_policy *xp, u32 fl_secid, u8 dir);
 int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x,
 			struct xfrm_policy *xp, struct flowi *fl);
-int selinux_xfrm_flow_state_match(struct flowi *fl, struct xfrm_state *xfrm,
-			struct xfrm_policy *xp);
-
 
 /*
  * Extract the security blob from the sock (it's actually on the socket)

@@ -38,7 +35,7 @@ static inline struct inode_security_stru
 int selinux_xfrm_sock_rcv_skb(u32 sid, struct sk_buff *skb,
 			struct avc_audit_data *ad);
 int selinux_xfrm_postroute_last(u32 isec_sid, struct sk_buff *skb,
-			struct avc_audit_data *ad);
+			struct avc_audit_data *ad, u8 proto);
 u32 selinux_socket_getpeer_dgram(struct sk_buff *skb);
 int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall);
 #else

@@ -49,7 +46,7 @@ static inline int selinux_xfrm_sock_rcv_
 }
 
 static inline int selinux_xfrm_postroute_last(u32 isec_sid, struct sk_buff *skb,
-			struct avc_audit_data *ad)
+			struct avc_audit_data *ad, u8 proto)
 {
 	return 0;
 }

security/selinux/include/security.h +2 -0

@@ -85,6 +85,8 @@ int security_validate_transition(u32 old
 
 int security_sid_mls_copy(u32 sid, u32 mls_sid, u32 *new_sid);
 
+int security_sid_compare(u32 sid1, u32 sid2);
+
 #define SECURITY_FS_USE_XATTR		1 /* use xattr */
 #define SECURITY_FS_USE_TRANS		2 /* use transition SIDs, e.g. devpts/tmpfs */
 #define SECURITY_FS_USE_TASK		3 /* use task SIDs, e.g. pipefs/sockfs */

security/selinux/hooks.c +17 -9

@@ -2888,7 +2888,8 @@ static void selinux_task_to_inode(struct
 }
 
 /* Returns error only if unable to parse addresses */
-static int selinux_parse_skb_ipv4(struct sk_buff *skb, struct avc_audit_data *ad)
+static int selinux_parse_skb_ipv4(struct sk_buff *skb,
+			struct avc_audit_data *ad, u8 *proto)
 {
 	int offset, ihlen, ret = -EINVAL;
 	struct iphdr _iph, *ih;

@@ -2906,6 +2907,9 @@ static int selinux_parse_skb_ipv4(struct
 	ad->u.net.v4info.daddr = ih->daddr;
 	ret = 0;
 
+	if (proto)
+		*proto = ih->protocol;
+
 	switch (ih->protocol) {
         case IPPROTO_TCP: {
         	struct tcphdr _tcph, *th;

@@ -2949,7 +2953,8 @@ out:
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 
 /* Returns error only if unable to parse addresses */
-static int selinux_parse_skb_ipv6(struct sk_buff *skb, struct avc_audit_data *ad)
+static int selinux_parse_skb_ipv6(struct sk_buff *skb,
+			struct avc_audit_data *ad, u8 *proto)
 {
 	u8 nexthdr;
 	int ret = -EINVAL, offset;

@@ -2970,6 +2975,9 @@ static int selinux_parse_skb_ipv6(struct
 	if (offset < 0)
 		goto out;
 
+	if (proto)
+		*proto = nexthdr;
+
 	switch (nexthdr) {
 	case IPPROTO_TCP: {
         	struct tcphdr _tcph, *th;

@@ -3006,13 +3014,13 @@ out:
 #endif /* IPV6 */
 
 static int selinux_parse_skb(struct sk_buff *skb, struct avc_audit_data *ad,
-			     char **addrp, int *len, int src)
+			     char **addrp, int *len, int src, u8 *proto)
 {
 	int ret = 0;
 
 	switch (ad->u.net.family) {
 	case PF_INET:
-		ret = selinux_parse_skb_ipv4(skb, ad);
+		ret = selinux_parse_skb_ipv4(skb, ad, proto);
 		if (ret || !addrp)
 			break;
 		*len = 4;

@@ -3022,7 +3030,7 @@ static int selinux_parse_skb(struct sk_b
 
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case PF_INET6:
-		ret = selinux_parse_skb_ipv6(skb, ad);
+		ret = selinux_parse_skb_ipv6(skb, ad, proto);
 		if (ret || !addrp)
 			break;
 		*len = 16;

@@ -3487,7 +3495,7 @@ static int selinux_socket_sock_rcv_skb(s
 	ad.u.net.netif = skb->dev ? skb->dev->name : "[unknown]";
 	ad.u.net.family = family;
 
-	err = selinux_parse_skb(skb, &ad, &addrp, &len, 1);
+	err = selinux_parse_skb(skb, &ad, &addrp, &len, 1, NULL);
 	if (err)
 		goto out;

@@ -3813,6 +3821,7 @@ static unsigned int selinux_ip_postroute
 	struct avc_audit_data ad;
 	struct net_device *dev = (struct net_device *)out;
 	struct sk_security_struct *sksec;
+	u8 proto;
 
 	sk = skb->sk;
 	if (!sk)

@@ -3824,7 +3833,7 @@ static unsigned int selinux_ip_postroute
 	ad.u.net.netif = dev->name;
 	ad.u.net.family = family;
 
-	err = selinux_parse_skb(skb, &ad, &addrp, &len, 0);
+	err = selinux_parse_skb(skb, &ad, &addrp, &len, 0, &proto);
 	if (err)
 		goto out;

@@ -3838,7 +3847,7 @@ static unsigned int selinux_ip_postroute
 	if (err)
 		goto out;
 
-	err = selinux_xfrm_postroute_last(sksec->sid, skb, &ad);
+	err = selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto);
 out:
 	return err ? NF_DROP : NF_ACCEPT;
 }

@@ -4757,7 +4766,6 @@ static struct security_operations selinu
 	.xfrm_state_delete_security =	selinux_xfrm_state_delete,
 	.xfrm_policy_lookup = 		selinux_xfrm_policy_lookup,
 	.xfrm_state_pol_flow_match =	selinux_xfrm_state_pol_flow_match,
-	.xfrm_flow_state_match =	selinux_xfrm_flow_state_match,
 	.xfrm_decode_session =		selinux_xfrm_decode_session,
 #endif

security/selinux/xfrm.c +45 ~~-52~~

138 lines not on this page · raw message

security/selinux/ss/services.c +44 -0

50 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-11-07 20:29 UTC #1 lore raw

Quoted 33 lines · Venkat Yekkirala

security/selinux/ss/services.c +33 -0

+/*
+ * security_sid_compare() - compares two given sid contexts.
+ * Returns 1 if they are equal, 0 otherwise.
+ */
+int security_sid_compare(u32 sid1, u32 sid2)
+{
+	struct context *context1;
+	struct context *context2;
+	int rc;
+
+	if (!ss_initialized)
+		return 1;
+
+	if (sid1 == sid2)
+		return 1;
+	else if (sid1 > SECINITSID_NUM && sid2 > SECINITSID_NUM)
+		return 0;
+
+	/* explicit comparison in order */
+
+	POLICY_RDLOCK;
+	context1 = sidtab_search(&sidtab, sid1);
+	if (!context1) {
+		printk(KERN_ERR "security_sid_compare:  unrecognized SID "
+		       "%u\n", sid1);
+		rc = 0;
+		goto out_unlock;
+	}
+
+	context2 = sidtab_search(&sidtab, sid2);
+	if (!context2) {
+		printk(KERN_ERR "security_sid_compare:  unrecognized SID "
+		       "%u\n", sid2);

security/selinux/ss/services.c out: · line 1942

Quoted 10 lines

security/selinux/ss/services.c +10 -0

+		rc = 0;
+		goto out_unlock;
+	}
+
+	rc = context_cmp(context1, context2);
+
+out_unlock:
+	POLICY_RDUNLOCK;
+	return rc;
+}

I understand wanting a generic LSM interface to do secid token comparisons, but in the SELinux implementation of this function I think we can get away with only a simple "sid1 == sid2" since the security server shouldn't be creating duplicate SID/secid values for identical contexts, I think. Did you run into something in testing that would indicate otherwise?

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-11-07 20:48 UTC #2 lore raw

Quoted 42 lines · Paul Moore

Quoted 41 lines · Venkat Yekkirala

security/selinux/ss/services.c +41 -0

+/*
+ * security_sid_compare() - compares two given sid contexts.
+ * Returns 1 if they are equal, 0 otherwise.
+ */
+int security_sid_compare(u32 sid1, u32 sid2)
+{
+	struct context *context1;
+	struct context *context2;
+	int rc;
+
+	if (!ss_initialized)
+		return 1;
+
+	if (sid1 == sid2)
+		return 1;
+	else if (sid1 > SECINITSID_NUM && sid2 > SECINITSID_NUM)
+		return 0;
+
+	/* explicit comparison in order */
+
+	POLICY_RDLOCK;
+	context1 = sidtab_search(&sidtab, sid1);
+	if (!context1) {
+		printk(KERN_ERR "security_sid_compare:  unrecognized SID "
+		       "%u\n", sid1);
+		rc = 0;
+		goto out_unlock;
+	}
+
+	context2 = sidtab_search(&sidtab, sid2);
+	if (!context2) {
+		printk(KERN_ERR "security_sid_compare:  unrecognized SID "
+		       "%u\n", sid2);
+		rc = 0;
+		goto out_unlock;
+	}
+
+	rc = context_cmp(context1, context2);
+
+out_unlock:
+	POLICY_RDUNLOCK;

Quoted 8 lines

security/selinux/ss/services.c out: · line 1942
Quoted 2 lines
security/selinux/ss/services.c +2 -0
+	return rc;
+}
I understand wanting a generic LSM interface to do secid token comparisons, but in the SELinux implementation of this function I think we can get away with only a simple "sid1 == sid2" since the security server shouldn't be creating duplicate SID/secid values for identical contexts, I think. Did you run into something in testing that would indicate otherwise?

Such duplication can occur among the initial SIDs. Not sure though when that would apply here, and it would only apply if both SIDs were initial SIDs.

Signature · 8 lines

Stephen Smalley
National Security Agency


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

Venkat Yekkirala

author

2006-11-08 14:34 UTC #3 lore raw

Quoted 1 line

Such duplication can occur among the initial SIDs.

For some reason I thought that could happen between an initial SID and a non-initial SID.

Quoted 3 lines

Not sure
though when
that would apply here,

It could apply to xfrms if they happen to be using the context represented by any of the initial SIDs.

Quoted 3 lines

and it would only apply if both SIDs
were initial
SIDs.

OK. Will narrow the full context comparison to just this case.

Stephen Smalley

2006-11-08 17:34 UTC #4 lore raw

Quoted 4 lines · Venkat Yekkirala

Quoted 1 line
Such duplication can occur among the initial SIDs.
For some reason I thought that could happen between an initial SID and a non-initial SID.

I think only in the case where a non-initial SID has been invalidated by policy reload that renders its context illegal and is thus remapped to the unlabeled SID.

Quoted 6 lines

Quoted 3 lines
Not sure
though when
that would apply here,
It could apply to xfrms if they happen to be using the context represented by any of the initial SIDs.

Which would happen when?

Quoted 5 lines

Quoted 3 lines
and it would only apply if both SIDs
were initial
SIDs.
OK. Will narrow the full context comparison to just this case.

What's the harm from just using the SID comparison and allowing for the possibility that there might be a few duplicates in rare circumstances? Does it break any assumptions in the rest of the logic?

Signature · 8 lines

Stephen Smalley
National Security Agency


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

Venkat Yekkirala

author

2006-11-08 19:17 UTC #5 lore raw

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Not sure
though when
that would apply here,
It could apply to xfrms if they happen to be using the context represented by any of the initial SIDs.
Which would happen when?

If one were attempting to use a context pertaining to the unlabeled init sid in the SPD and/or the SAD. But would I be correct in assuming that the same sid (unlabeled init sid in all likelyhood) would end up being returned when the context is turned into a sid, resulting in the SPD and the SAD using
the same init sid, thus making a full-context compare unnecessary?

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
and it would only apply if both SIDs
were initial
SIDs.

Quoted 7 lines

Quoted 1 line
OK. Will narrow the full context comparison to just this case.
What's the harm from just using the SID comparison and allowing for the
possibility that there might be a few duplicates in rare circumstances?
Does it break any assumptions in the rest of the logic?

The best I can think of is if the SA's sid doesn't match the socket's SID, IKE would come into play, if it's configured.

I also wanted to conversely ask what harm exists if we did a full-context compare in the event the sids didn't match?

Are we just trying to generally avoid extra code?

Stephen Smalley

2006-11-08 21:57 UTC #6 lore raw

Quoted 7 lines · Venkat Yekkirala

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
Not sure
though when
that would apply here,
It could apply to xfrms if they happen to be using the context represented by any of the initial SIDs.

Quoted 8 lines

Quoted 1 line
Which would happen when?
If one were attempting to use a context pertaining to the unlabeled init sid in the SPD and/or the SAD. But would I be correct in assuming that the same sid (unlabeled init sid in all likelyhood) would end up being returned when the context is turned into a sid, resulting in the SPD and the SAD using
the same init sid, thus making a full-context compare unnecessary?

Yes.

Quoted 6 lines

Quoted 5 lines
What's the harm from just using the SID comparison and allowing for the
possibility that there might be a few duplicates in rare circumstances?
Does it break any assumptions in the rest of the logic?

Quoted 7 lines

The best I can think of is if the SA's sid doesn't match the socket's SID, IKE would come into play, if it's configured.
I also wanted to conversely ask what harm exists if we did a full-context compare in the event the sids didn't match?
Are we just trying to generally avoid extra code?

More complexity and overhead for no real gain.

Signature · 8 lines

Stephen Smalley
National Security Agency


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