[PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

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

Signed-off-by: Venkat Yekkirala <vyekkirala@TrustedCS.com>
---
 security/selinux/hooks.c        |  129 +++++++++++++++++++++++-------
 security/selinux/include/xfrm.h |    5 +
 security/selinux/ss/mls.c       |    2 
 security/selinux/ss/services.c  |    2 
 security/selinux/xfrm.c         |   28 ++++++
 5 files changed, 139 insertions(+), 27 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5a66c4c..143b4b8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -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;
 }
@@ -3626,13 +3626,16 @@ 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;
+			return 0;
+		}
+	} else
+		peersid = skb->secmark;
 
 	err = security_sid_mls_copy(sksec->sid, peersid, &newsid);
 	if (err)
@@ -3662,6 +3665,69 @@ 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, trans_sid;
+	int err;
+
+	if (selinux_compat_net)
+		return 1;
+
+	/* xfrm/cipso inapplicable for loopback traffic */
+	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) {
+		err = security_transition_sid(xfrm_sid, skb->secmark,
+						SECCLASS_PACKET, &trans_sid);
+		if (err)
+			goto out;
+
+		skb->secmark = trans_sid;
+	}
+
+	/* See if CIPSO 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)
+{
+	u32 trans_sid;
+	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);
+
+out:
+	return err ? 0 : 1;
+}
+
 static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
 {
 	int err = 0;
@@ -3700,7 +3766,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 +3777,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 +3838,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 +3856,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 +3869,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 +4794,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,
diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h
index 81eb598..ed07f92 100644
--- a/security/selinux/include/xfrm.h
+++ b/security/selinux/include/xfrm.h
@@ -41,6 +41,7 @@ int selinux_xfrm_postroute_last(u32 isec
 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)
@@ -68,6 +69,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_ */
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 119bd60..c551def 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -548,6 +548,8 @@ int mls_compute_sid(struct context *scon
 				}
 			}
 		}
+		else if (tclass == SECCLASS_PACKET)
+			return mls_copy_context(newcontext, scontext);
 		/* Fallthrough */
 	case AVTAB_CHANGE:
 		if (tclass == SECCLASS_PROCESS)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 22ed17c..26176c2 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -832,6 +832,7 @@ static int security_compute_sid(u32 ssid
 
 	if (!ss_initialized) {
 		switch (tclass) {
+		case SECCLASS_PACKET:
 		case SECCLASS_PROCESS:
 			*out_sid = ssid;
 			break;
@@ -876,6 +877,7 @@ static int security_compute_sid(u32 ssid
 
 	/* Set the role and type to default values. */
 	switch (tclass) {
+	case SECCLASS_PACKET:
 	case SECCLASS_PROCESS:
 		/* Use the current role and type of process. */
 		newcontext.role = scontext->role;
diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
index 3e742b8..3a68723 100644
--- a/security/selinux/xfrm.c
+++ b/security/selinux/xfrm.c
@@ -155,7 +155,7 @@ int selinux_xfrm_flow_state_match(struct
 }
 
 /*
- * LSM hook implementation that determines the sid for the session.
+ * LSM hook implementation that checks/returns the xfrm sid for the incoming packet.
  */
 
 int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall)
@@ -193,6 +193,32 @@ int selinux_xfrm_decode_session(struct s
 }
 
 /*
+ * LSM hook implementation that returns the xfrm sid for the outgoing packet.
+ */
+
+void selinux_skb_xfrm_sid(struct sk_buff *skb, u32 *sid)
+{
+	struct dst_entry *dst;
+
+	*sid = SECSID_NULL;
+
+	dst = skb->dst;
+	if (dst) {
+		struct dst_entry *dst_test;
+		for (dst_test = dst; dst_test != 0;
+			dst_test = dst_test->child) {
+			struct xfrm_state *x = dst_test->xfrm;
+
+			if (x && selinux_authorizable_xfrm(x)) {
+				struct xfrm_sec_ctx *ctx = x->security;
+				*sid = ctx->ctx_sid;
+				break;
+			}
+		}
+	}
+}
+
+/*
  * Security blob allocation for xfrm_policy and xfrm_state
  * CTX does not have a meaningful value on input
  */

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Joshua Brindle]

Venkat Yekkirala wrote:
> <snip>
> +
> +	err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
> +					PACKET__FLOW_IN, NULL);
> +	if (err)
> +		goto out;
> +
> +	if (xfrm_sid) {
> +		err = security_transition_sid(xfrm_sid, skb->secmark,
> +						SECCLASS_PACKET, &trans_sid);
> +		if (err)
> +			goto out;
> +
>   
I thought we weren't doing transitions to label packets anymore per the 
conference call?

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Stephen Smalley]

On Thu, 2006-09-28 at 23:52 -0400, Joshua Brindle wrote:
> Venkat Yekkirala wrote:
> > <snip>
> > +
> > +	err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
> > +					PACKET__FLOW_IN, NULL);
> > +	if (err)
> > +		goto out;
> > +
> > +	if (xfrm_sid) {
> > +		err = security_transition_sid(xfrm_sid, skb->secmark,
> > +						SECCLASS_PACKET, &trans_sid);
> > +		if (err)
> > +			goto out;
> > +
> >   
> I thought we weren't doing transitions to label packets anymore per the 
> conference call?

No, transitions are still part of the reconciliation process.  By
default, this just means that we end up with the xfrm_sid (which is what
you want).  But it allows us the freedom to define transitions on the
secmark label if desired, and those transitions can still yield subject
labels.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Joshua Brindle]

On Fri, 2006-09-29 at 08:59 -0400, Stephen Smalley wrote:
> On Thu, 2006-09-28 at 23:52 -0400, Joshua Brindle wrote:
> > Venkat Yekkirala wrote:
> > > <snip>
> > > +
> > > +	err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
> > > +					PACKET__FLOW_IN, NULL);
> > > +	if (err)
> > > +		goto out;
> > > +
> > > +	if (xfrm_sid) {
> > > +		err = security_transition_sid(xfrm_sid, skb->secmark,
> > > +						SECCLASS_PACKET, &trans_sid);
> > > +		if (err)
> > > +			goto out;
> > > +
> > >   
> > I thought we weren't doing transitions to label packets anymore per the 
> > conference call?
> 
> No, transitions are still part of the reconciliation process.  By
> default, this just means that we end up with the xfrm_sid (which is what
> you want).  But it allows us the freedom to define transitions on the
> secmark label if desired, and those transitions can still yield subject
> labels.
> 

This is not consistent with my perception of the decision made in the
conference call. I thought that the secid was either going to be 1) the
secmark label if no external labeling is present or 2) the external
label if it is present. The flow_in permission would be checked between
the external label and the secmark label in either case (unlabeled in
the case of #1)

How is this different from the implementation before the call?

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Stephen Smalley]

On Fri, 2006-09-29 at 10:00 -0400, Joshua Brindle wrote:
> On Fri, 2006-09-29 at 08:59 -0400, Stephen Smalley wrote:
> > On Thu, 2006-09-28 at 23:52 -0400, Joshua Brindle wrote:
> > > Venkat Yekkirala wrote:
> > > > <snip>
> > > > +
> > > > +	err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
> > > > +					PACKET__FLOW_IN, NULL);
> > > > +	if (err)
> > > > +		goto out;
> > > > +
> > > > +	if (xfrm_sid) {
> > > > +		err = security_transition_sid(xfrm_sid, skb->secmark,
> > > > +						SECCLASS_PACKET, &trans_sid);
> > > > +		if (err)
> > > > +			goto out;
> > > > +
> > > >   
> > > I thought we weren't doing transitions to label packets anymore per the 
> > > conference call?
> > 
> > No, transitions are still part of the reconciliation process.  By
> > default, this just means that we end up with the xfrm_sid (which is what
> > you want).  But it allows us the freedom to define transitions on the
> > secmark label if desired, and those transitions can still yield subject
> > labels.
> > 
> 
> This is not consistent with my perception of the decision made in the
> conference call. I thought that the secid was either going to be 1) the
> secmark label if no external labeling is present or 2) the external
> label if it is present. The flow_in permission would be checked between
> the external label and the secmark label in either case (unlabeled in
> the case of #1)

The behavior you describe is precisely what will happen in the absence
of any type_transition rules on packet class in the policy, given the
way in which he has defined security_transition_sid on packet class.
The only question is whether there is any value in allowing a transition
to be configured in policy (and if such a transition is configured,
whether the resulting SID requires its own separate permission check,
which is what is usually done - the transition doesn't implicitly
authorize anything).  I don't recall a particular decision on the
transition issue during the call nor do I see it in the posted notes.
However, since the transition was removed in the flow_out case, it would
be logical to remove it from the flow_in case as well, and that would
have the side benefit of less overhead.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[James Morris]

On Fri, 29 Sep 2006, Stephen Smalley wrote:

> However, since the transition was removed in the flow_out case, it would
> be logical to remove it from the flow_in case as well, and that would
> have the side benefit of less overhead.

How about adding secmark transitions later, if needed, perhaps with an 
/selinux config control ?

It does keep things simpler for now, in terms of getting this code merged, 
deployed into distros and likely certified.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Stephen Smalley]

On Fri, 2006-09-29 at 10:33 -0400, James Morris wrote:
> On Fri, 29 Sep 2006, Stephen Smalley wrote:
> 
> > However, since the transition was removed in the flow_out case, it would
> > be logical to remove it from the flow_in case as well, and that would
> > have the side benefit of less overhead.
> 
> How about adding secmark transitions later, if needed, perhaps with an 
> /selinux config control ?
> 
> It does keep things simpler for now, in terms of getting this code merged, 
> deployed into distros and likely certified.

Fine with me, unless Venkat has an immediate use case for such
transitions in the flow_in case (but I think this is mostly my fault for
suggesting transitions a while ago).

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

Stephen Smalley wrote:
> On Fri, 2006-09-29 at 10:33 -0400, James Morris wrote:
> 
>>On Fri, 29 Sep 2006, Stephen Smalley wrote:
>>
>>
>>>However, since the transition was removed in the flow_out case, it would
>>>be logical to remove it from the flow_in case as well, and that would
>>>have the side benefit of less overhead.
>>
>>How about adding secmark transitions later, if needed, perhaps with an 
>>/selinux config control ?
>>
>>It does keep things simpler for now, in terms of getting this code merged, 
>>deployed into distros and likely certified.
> 
> 
> Fine with me, unless Venkat has an immediate use case for such
> transitions in the flow_in case (but I think this is mostly my fault for
> suggesting transitions a while ago).

Unless I'm confusing something, there still may be a need for transitions
if we want to support both IPsec and NetLabel labeling on the same
connection.
If we don't support transitions and allow both labeling methods on the
same connection we'll need to decide how to handle resolving the two -
maybe use a transition is this one case?

-- 
paul moore
linux security @ hp

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[James Morris]

On Fri, 29 Sep 2006, Paul Moore wrote:

> Unless I'm confusing something, there still may be a need for transitions
> if we want to support both IPsec and NetLabel labeling on the same
> connection.

I'd prefer not to support this, as it's too complicated, and CIPSO is a 
legacy protocol.

Normal IPsec protection applied to CIPSO: yes, but not IPsec labeling and 
CIPSO labeling on the same connection.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

James Morris wrote:
> On Fri, 29 Sep 2006, Paul Moore wrote:
> 
> 
>>Unless I'm confusing something, there still may be a need for transitions
>>if we want to support both IPsec and NetLabel labeling on the same
>>connection.
> 
> I'd prefer not to support this, as it's too complicated, and CIPSO is a 
> legacy protocol.
> 
> Normal IPsec protection applied to CIPSO: yes, but not IPsec labeling and 
> CIPSO labeling on the same connection.

I tend to agree, I just can't see it being all that useful in the real
world.  However, each time it comes up (including the conference call
earlier this week) it seems that people would prefer to use both at the
same time.

The good news is that it sounds like there is a reasonable solution (see
the last email exchance between Venkat and myself).

-- 
paul moore
linux security @ hp

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

Venkat Yekkirala wrote:
> This defines SELinux enforcement of the 2 new LSM hooks as well
> as related changes elsewhere in the SELinux code.

As soon as I hit send on this mail I'll start working on the related
patch to provide the missing NetLabel support.

Additional comments are below.

> Signed-off-by: Venkat Yekkirala <vyekkirala@TrustedCS.com>
> ---
>  security/selinux/hooks.c        |  129 +++++++++++++++++++++++-------
>  security/selinux/include/xfrm.h |    5 +
>  security/selinux/ss/mls.c       |    2 
>  security/selinux/ss/services.c  |    2 
>  security/selinux/xfrm.c         |   28 ++++++
>  5 files changed, 139 insertions(+), 27 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5a66c4c..143b4b8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -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;
>  }
> @@ -3626,13 +3626,16 @@ 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;
> +			return 0;
> +		}
> +	} else
> +		peersid = skb->secmark;
>  
>  	err = security_sid_mls_copy(sksec->sid, peersid, &newsid);
>  	if (err)
> @@ -3662,6 +3665,69 @@ 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, trans_sid;
> +	int err;
> +
> +	if (selinux_compat_net)
> +		return 1;
> +
> +	/* xfrm/cipso inapplicable for loopback traffic */
> +	if (skb->dev == &loopback_dev)
> +		return 1;

Just to clarify (I believe this is the case from your remarks as well as
the code, but better safe than sorry) the secmark for loopback traffic
is set by the originating socket, yes?

Beware: a bit of a nit follows :)

While I understand that NetLabel currently only supports CIPSO, it is a
framework that can support multiple labeling procotols (i.e. RIPSO
support is planned).  Please change the comment from cipso/CIPSO to
netlabel/NetLabel so it is consistent with the rest of the code which
makes use of NetLabel.

> +	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) {
> +		err = security_transition_sid(xfrm_sid, skb->secmark,
> +						SECCLASS_PACKET, &trans_sid);
> +		if (err)
> +			goto out;
> +
> +		skb->secmark = trans_sid;
> +	}
> +	/* See if CIPSO can flow in thru the current secmark here */

Same nit applies about the use of "CIPSO" vs "NetLabel".

> +out:
> +	return err ? 0 : 1;
> +};
> +
> +static int selinux_skb_flow_out(struct sk_buff *skb, u32 nf_secid)
> +{
> +	u32 trans_sid;
> +	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);

While I don't see any explicit mention of it in the documentation or
your comments, I assume we would want a flow_out check for NetLabel here
as well?

> +out:
> +	return err ? 0 : 1;
> +}
> +
>  static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
>  {
>  	int err = 0;
> @@ -3700,7 +3766,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 +3777,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 +3838,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 +3856,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 +3869,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);
> +	}

This looks nearly identical to selinux_skb_flow_out() as implemented
above, the only real differences being two of the args to the
avc_has_perm() call.  Any reason you don't abstract out the common parts
to a separate function?

Also, the same comment/question about NetLabel support in
selinux_skb_flow_out() applies here as well I suspect.

>  out:
>  	return err ? NF_DROP : NF_ACCEPT;
>  }
> @@ -4719,6 +4794,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,
> diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h
> index 81eb598..ed07f92 100644
> --- a/security/selinux/include/xfrm.h
> +++ b/security/selinux/include/xfrm.h
> @@ -41,6 +41,7 @@ int selinux_xfrm_postroute_last(u32 isec
>  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)
> @@ -68,6 +69,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_ */
> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> index 119bd60..c551def 100644
> --- a/security/selinux/ss/mls.c
> +++ b/security/selinux/ss/mls.c
> @@ -548,6 +548,8 @@ int mls_compute_sid(struct context *scon
>  				}
>  			}
>  		}
> +		else if (tclass == SECCLASS_PACKET)
> +			return mls_copy_context(newcontext, scontext);
>  		/* Fallthrough */
>  	case AVTAB_CHANGE:
>  		if (tclass == SECCLASS_PROCESS)
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 22ed17c..26176c2 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -832,6 +832,7 @@ static int security_compute_sid(u32 ssid
>  
>  	if (!ss_initialized) {
>  		switch (tclass) {
> +		case SECCLASS_PACKET:
>  		case SECCLASS_PROCESS:
>  			*out_sid = ssid;
>  			break;
> @@ -876,6 +877,7 @@ static int security_compute_sid(u32 ssid
>  
>  	/* Set the role and type to default values. */
>  	switch (tclass) {
> +	case SECCLASS_PACKET:
>  	case SECCLASS_PROCESS:
>  		/* Use the current role and type of process. */
>  		newcontext.role = scontext->role;
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index 3e742b8..3a68723 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -155,7 +155,7 @@ int selinux_xfrm_flow_state_match(struct
>  }
>  
>  /*
> - * LSM hook implementation that determines the sid for the session.
> + * LSM hook implementation that checks/returns the xfrm sid for the incoming packet.
>   */
>  
>  int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall)
> @@ -193,6 +193,32 @@ int selinux_xfrm_decode_session(struct s
>  }
>  
>  /*
> + * LSM hook implementation that returns the xfrm sid for the outgoing packet.
> + */
> +
> +void selinux_skb_xfrm_sid(struct sk_buff *skb, u32 *sid)
> +{
> +	struct dst_entry *dst;
> +
> +	*sid = SECSID_NULL;
> +
> +	dst = skb->dst;
> +	if (dst) {
> +		struct dst_entry *dst_test;
> +		for (dst_test = dst; dst_test != 0;
> +			dst_test = dst_test->child) {
> +			struct xfrm_state *x = dst_test->xfrm;
> +
> +			if (x && selinux_authorizable_xfrm(x)) {
> +				struct xfrm_sec_ctx *ctx = x->security;
> +				*sid = ctx->ctx_sid;
> +				break;
> +			}
> +		}
> +	}
> +}
> +
> +/*
>   * Security blob allocation for xfrm_policy and xfrm_state
>   * CTX does not have a meaningful value on input
>   */
> 
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.


-- 
paul moore
linux security @ hp

RE: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

> > Fine with me, unless Venkat has an immediate use case for such
> > transitions in the flow_in case (but I think this is mostly 
> my fault for
> > suggesting transitions a while ago).

I don't have a use case currently.

> 
> Unless I'm confusing something, there still may be a need for 
> transitions
> if we want to support both IPsec and NetLabel labeling on the same
> connection.
> If we don't support transitions and allow both labeling methods on the
> same connection we'll need to decide how to handle resolving the two -
> maybe use a transition is this one case?

Since CIPSO doesn't do full contexts currently, it would be just a
matter of an additional flow_in check. The base sid used here would
be the current secmark at that point (which will be the xfrm sid
if xfrm was used). So, no transitions needed here currently.

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

Venkat Yekkirala wrote:
>>>Fine with me, unless Venkat has an immediate use case for such
>>>transitions in the flow_in case (but I think this is mostly 
>>
>>my fault for
>>
>>>suggesting transitions a while ago).
> 
> I don't have a use case currently.
> 
>>Unless I'm confusing something, there still may be a need for 
>>transitions
>>if we want to support both IPsec and NetLabel labeling on the same
>>connection.
>>If we don't support transitions and allow both labeling methods on the
>>same connection we'll need to decide how to handle resolving the two -
>>maybe use a transition is this one case?
> 
> 
> Since CIPSO doesn't do full contexts currently, it would be just a
> matter of an additional flow_in check. The base sid used here would
> be the current secmark at that point (which will be the xfrm sid
> if xfrm was used). So, no transitions needed here currently.

That's fine by me, I just wanted to make sure something like that would
be acceptable.  So, in summary, we would do the normal flow_in checks
for both IPsec and NetLabel and then set the secmark using the IPsec
label as the "base sid" for the NetLabel's generated SID?

-- 
paul moore
linux security @ hp

RE: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

> > Unless I'm confusing something, there still may be a need 
> for transitions
> > if we want to support both IPsec and NetLabel labeling on the same
> > connection.
> 
> I'd prefer not to support this, as it's too complicated,

Actually, from my vantage point, it actually seems "natural".

> and 
> CIPSO is a 
> legacy protocol.

Sure.

> 
> Normal IPsec protection applied to CIPSO: yes, but not IPsec 
> labeling and 
> CIPSO labeling on the same connection.

One use case example can be one SA for Secret in combination with
any/all/none of the compartments. And another SA for Top Secret ...

RE: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

> That's fine by me, I just wanted to make sure something like 
> that would
> be acceptable.  So, in summary, we would do the normal flow_in checks
> for both IPsec and NetLabel and then set the secmark using the IPsec
> label as the "base sid" for the NetLabel's generated SID?

That's correct (in short you won't care if IPSec was in use or not, you
would just use the secmark at that point as the base sid in coming up
with the NetLabel sid, and if it flow controls fine vis a vis the secmark
you would replace secmark with the NetLabel sid. The logic flow is quite
natural and intuitive for the users as well).

RE: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

> I tend to agree, I just can't see it being all that useful in the real
> world.  However, each time it comes up (including the conference call
> earlier this week) it seems that people would prefer to use 
> both at the
> same time.

A matter of providing options to users. It seems more of a pain to actually
prevent their use at the same time and/or explain strange/unnatural
behavior.

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

Venkat Yekkirala wrote:
>>I tend to agree, I just can't see it being all that useful in the real
>>world.  However, each time it comes up (including the conference call
>>earlier this week) it seems that people would prefer to use 
>>both at the
>>same time.
>  
> A matter of providing options to users.

As long as those options receive the blessings of the maintainers ;)

> It seems more of a pain to actually
> prevent their use at the same time and/or explain strange/unnatural
> behavior.

Agreed, the solution that we agreed upon is much easier to implement and
explain than a lot of the alternatives.

-- 
paul moore
linux security @ hp

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[James Morris]

On Fri, 29 Sep 2006, Paul Moore wrote:

> > It seems more of a pain to actually
> > prevent their use at the same time and/or explain strange/unnatural
> > behavior.
> 
> Agreed, the solution that we agreed upon is much easier to implement and
> explain than a lot of the alternatives.

Ok, can you please explain it further?

i.e. show me what the policy looks like, exactly what the user is trying 
to achieve, and explain what happens to each packet exactly in terms of 
labeling on the input and output paths.




-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[James Morris]

On Fri, 29 Sep 2006, James Morris wrote:

> On Fri, 29 Sep 2006, Paul Moore wrote:
> 
> > > It seems more of a pain to actually
> > > prevent their use at the same time and/or explain strange/unnatural
> > > behavior.
> > 
> > Agreed, the solution that we agreed upon is much easier to implement and
> > explain than a lot of the alternatives.
> 
> Ok, can you please explain it further?
> 
> i.e. show me what the policy looks like, exactly what the user is trying 
> to achieve, and explain what happens to each packet exactly in terms of 
> labeling on the input and output paths.

Also, why can't this be done just with xfrm labeling?

CIPSO is not there to provide a mechanism for separating the label of the 
connection from the label of the data, it's only there to provide interop 
with legacy systems.

If you need to have two labels for a packet (the object and the domain), 
then this needs to be supported directly by xfrm labeling.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

James Morris wrote:
> On Fri, 29 Sep 2006, James Morris wrote:
> 
> 
>>On Fri, 29 Sep 2006, Paul Moore wrote:
>>
>>
>>>>It seems more of a pain to actually
>>>>prevent their use at the same time and/or explain strange/unnatural
>>>>behavior.
>>>
>>>Agreed, the solution that we agreed upon is much easier to implement and
>>>explain than a lot of the alternatives.
>>
>>Ok, can you please explain it further?
>>
>>i.e. show me what the policy looks like, exactly what the user is trying 
>>to achieve, and explain what happens to each packet exactly in terms of 
>>labeling on the input and output paths.
>  
> Also, why can't this be done just with xfrm labeling?

I believe the issue Venkat and I were discussing was how to handle the
case of multiple external labeling protocols, i.e. what to do if we get
a packet through labeled SA which has a CIPSO option.  As I've said
before, I don't believe this is something we will see much in practice
but I think we need to decide what to do: handle it somehow or just punt
on the problem and drop it.  Several people with experience with
external labeling have commented on how supporting both external
labeling protocols would be a good idea so Venkat and I are trying to
come up with a solution that works.

Please see my reponse with the pseudo code/policy examples as this might
help clear things up.

-- 
paul moore
linux security @ hp

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

James Morris wrote:
> On Fri, 29 Sep 2006, Paul Moore wrote:
> 
>>>It seems more of a pain to actually
>>>prevent their use at the same time and/or explain strange/unnatural
>>>behavior.
>>
>>Agreed, the solution that we agreed upon is much easier to implement and
>>explain than a lot of the alternatives.
> 
> Ok, can you please explain it further?
> 
> i.e. show me what the policy looks like, exactly what the user is trying 
> to achieve, and explain what happens to each packet exactly in terms of 
> labeling on the input and output paths.

All right, here is my take on it, perhaps Venkat can chime in too.

 * packet labeling, input

Code below is taken from the secid patchset, minus the SID transition
code as it sounds like that is going to be dropped.  Please note that
this is not a patch just something to help explain.

+static int selinux_skb_flow_in(struct sk_buff *skb, unsigned short family)
+{
+	u32 xfrm_sid, trans_sid;
        u32 nlbl_sid;
+	int err;
+
+	if (selinux_compat_net)
+		return 1;
+
+	/* xfrm/cipso inapplicable for loopback traffic */
+	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;

        err = selinux_netlbl_skbuff_getsid(skb,
                                           skb->secmark,
                                           &nlbl_sid);
        if (err)
                goto out;

        if (nlbl_sid != SECINITSID_UNLABELED) {
                /* not sure if we want this avc check above this if
                 * block? */
                err = avc_has_perm(nlbl_sid,
                                   skb->secmark,
                                   SECCLASS_PACKET,
                                   PACKET__FLOW_IN,
                                   NULL);
                if (err)
                        goto out;
                skb->secmark = nlbl_sid;
        }
+
+out:
+	return err ? 0 : 1;
+};

 * packet labeling, output

This is currently being discussed, so take this with a few grains of salt.

+static int selinux_skb_flow_out(struct sk_buff *skb, u32 nf_secid)
+{
+	u32 trans_sid;
        u32 nlbl_sid;
+	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 = selinux_netlbl_skbuff_getsid(skb,
                                                   skb->secmark,
                                                   &nlbl_sid);
                if (err)
                        goto out;
                if (nlbl_sid != SECINITSID_UNLABELED) {
                        err = avc_has_perm(nlbl_sid,
                                           skb->secmark,
                                           SECCLASS_PACKET,
                                           PACKET__FLOW_OUT,
                                           NULL);
                        if (err)
                                goto out;
                        skb->secmark = nlbl_sid;
                }
+	}
+
+	err = avc_has_perm(skb->secmark, nf_secid, SECCLASS_PACKET,
+				PACKET__FLOW_OUT, NULL);
+
+out:
+	return err ? 0 : 1;
+}

 * pseudo policy

Since NetLabel presently only provides a MLS label I don't believe there
would be any additional SELinux allow rules beyond those needed for
IPsec labeling (maybe in the flow_out, not sure).  The MLS/MCS
constraints would be the only thing affected I believe, and even then
the existing constraints should suffice.

-- 
paul moore
linux security @ hp

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[James Morris]

On Fri, 29 Sep 2006, Paul Moore wrote:

> James Morris wrote:
> > Ok, can you please explain it further?
> > 
> > i.e. show me what the policy looks like, exactly what the user is trying 
> > to achieve, and explain what happens to each packet exactly in terms of 
> > labeling on the input and output paths.
> 
> All right, here is my take on it, perhaps Venkat can chime in too.

Thanks, that cleared up many things, but how does this interact with 
CONNSECMARK?

Please provide some example iptables rules, SELinux policy statements, 
racoon config and netlabel config.  I need to understand exactly what 
happens to each packet in, say, an FTP session and how you envisage the 
configuration.

Here's a sample scenario for the above (let me know if this is not how 
you expect this to be used):

Say that the SA is labeled "secret" and you have two FTP clients 
connecting to a server via xinetd on this SA.  Each client additionally 
labels their packets via CIPSO as secret:c1 and secret:c2 respectively.  
xinetd launches an FTP server for each at the correct level.



- James 
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

James Morris wrote:
> On Fri, 29 Sep 2006, Paul Moore wrote:
> 
>>James Morris wrote:
>>
>>>Ok, can you please explain it further?
>>>
>>>i.e. show me what the policy looks like, exactly what the user is trying 
>>>to achieve, and explain what happens to each packet exactly in terms of 
>>>labeling on the input and output paths.
>>
>>All right, here is my take on it, perhaps Venkat can chime in too.
> 
> Thanks, that cleared up many things, but how does this interact with 
> CONNSECMARK?
> 
> Please provide some example iptables rules, SELinux policy statements, 
> racoon config and netlabel config.  I need to understand exactly what 
> happens to each packet in, say, an FTP session and how you envisage the 
> configuration.

Hopefully Venkat can talk to the iptables rule, policy statements, and
racoon config.  He has the best understanding of how this works with the
secid patches.  There really is no specific NetLabel config as the
NetLabel config only specifies how to create the explicit packet label
(CIPSO IPv4 option) using the socket's SID.  NetLabel, like SECMARK, is
just a packet labeling mechanism.

I think the key thing to remember is that the only change brought about
by the pseudo-code I posted earlier is that the secmark's MLS label
would be adjusted to match the value of the NetLabel (CIPSO option)
assuming it passes the avc flow_in checks.

> Here's a sample scenario for the above (let me know if this is not how 
> you expect this to be used):
> 
> Say that the SA is labeled "secret" and you have two FTP clients 
> connecting to a server via xinetd on this SA.  Each client additionally 
> labels their packets via CIPSO as secret:c1 and secret:c2 respectively.  
> xinetd launches an FTP server for each at the correct level.

I believe Venkat can address this.

-- 
paul moore
linux security @ hp

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[James Morris]

On Fri, 29 Sep 2006, Paul Moore wrote:

> > Say that the SA is labeled "secret" and you have two FTP clients 
> > connecting to a server via xinetd on this SA.  Each client additionally 
> > labels their packets via CIPSO as secret:c1 and secret:c2 respectively.  
> > xinetd launches an FTP server for each at the correct level.
> 
> I believe Venkat can address this.

Ok, I'd still really like to see a worked example of just Netlabel + 
secmark/connseckmark, to see what happens to the connection marks.  It 
seems that the connection mark should always be correct, and restored to 
the packet.  In which case, what happens when a CIPSO label on an 
established or related packet doesn't match, or you get no CIPSO label 
(e.g. ICMP from intermediate router) ?  Or, is would you be always 
overwriting secmark/connsecmark labeling, and if so, how/why are you using 
them?

Venkat,

With xfrm labeling, the external packets are always going to be protocol 
ESP or AH, and we can't connection track the inner protocols.  So, 
external labeling when using xfrm labeling seems somewhat superfluous, 
except for the case of setting a label based on the interface the packets 
arrived on.  Correct?  If so, all you can realistically do with the flow 
permissions is bind the ESP/AH packets to types of interfaces (which does 
seem useful for some folk).


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

James Morris wrote:
> On Fri, 29 Sep 2006, Paul Moore wrote:
> 
> 
>>>Say that the SA is labeled "secret" and you have two FTP clients 
>>>connecting to a server via xinetd on this SA.  Each client additionally 
>>>labels their packets via CIPSO as secret:c1 and secret:c2 respectively.  
>>>xinetd launches an FTP server for each at the correct level.
>>
>>I believe Venkat can address this.
> 
> Ok, I'd still really like to see a worked example of just Netlabel + 
> secmark/connseckmark, to see what happens to the connection marks.

Fair enough.

> It 
> seems that the connection mark should always be correct, and restored to 
> the packet.  In which case, what happens when a CIPSO label on an 
> established {connection} ...

The following would happen, in order, in selinux_skb_flow_in() (I'm
ommitting the IPsec relevant portions of this function for clarity):

1. A NetLabel SID would be generated using the secmark as the
"base_sid"; this means that the NetLabel would be the concatenation of
the secmark's TE context and the NetLabel's MLS label.  The secmark is
not yet modified.

2. The NetLabel SID would be avc checked against the secmark:
    avc_has_perm(nlbl_sid,
                 skb->secmark,
                 SECCLASS_PACKET,
		 PACKET__FLOW_IN,
                 NULL)
   Note that in practice this is basically just a MLS label check.

3. If the NetLabel SID != 0 the secmark would be replaced with the
NetLabel SID.

I am trying to make NetLabel behave in a similar fashion as to how
labeled IPsec works in the secid patches; I believe the above steps
acomplish this.  If not please let me know and I'll make the necessary
changes.

> ... or related packet doesn't match ...

Not sure what you mean by "related packet", but if the check in step #2
fails then the packet would be dropped.  The only case where I can see
this happening is that if the MLS/MCS constraints did not allow the
flow_in permission.  This allows administrators to set specific MLS
labels for any iptables "object".

> ... or you get no CIPSO label (e.g. ICMP from intermediate router) ...

If there is no packet label that NetLabel recognizes and NetLabel is
configured to allow unlabeled traffic then the NetLabel SID generated in
step #1 above would be 0.

> Or, is would you be always 
> overwriting secmark/connsecmark labeling, and if so, how/why are you using 
> them?

I think I addressed that above.

FYI: in between emails I'm working on a patch against Venkat's secid
patches which implements this, hopefully Venkat can roll this patch into
his secid patchset so this is all committed/rejected at once.

-- 
paul moore
linux security @ hp

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[James Morris]

On Fri, 29 Sep 2006, Paul Moore wrote:

> > ... or related packet doesn't match ...
> 
> Not sure what you mean by "related packet",

A related packet with conntrack would be an FTP data packet related to a 
specific FTP control session (where the conntrack is initialized, and has 
a secmark saved to it).

> but if the check in step #2
> fails then the packet would be dropped. 

Ok.

> The only case where I can see
> this happening is that if the MLS/MCS constraints did not allow the
> flow_in permission.  This allows administrators to set specific MLS
> labels for any iptables "object".

Yep, one thing to watch for here is packets arriving on a different 
interface for valid reasons (e.g. routing changes), but that's a policy 
issue.

> > ... or you get no CIPSO label (e.g. ICMP from intermediate router) ...
> 
> If there is no packet label that NetLabel recognizes and NetLabel is
> configured to allow unlabeled traffic then the NetLabel SID generated in
> step #1 above would be 0.

Well, conntrack will say that this packet is related to the connection 
and CONNSECMARK will restore the secmark label to it (i.e. it'll have the 
same secmark as the initial syn packet).  But, no CIPSO label.  I guess 
this needs to be considered in any case, secmark or not.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

James Morris wrote:
> On Fri, 29 Sep 2006, Paul Moore wrote:
>>>... or you get no CIPSO label (e.g. ICMP from intermediate router) ...
>>
>>If there is no packet label that NetLabel recognizes and NetLabel is
>>configured to allow unlabeled traffic then the NetLabel SID generated in
>>step #1 above would be 0.
> 
> 
> Well, conntrack will say that this packet is related to the connection 
> and CONNSECMARK will restore the secmark label to it (i.e. it'll have the 
> same secmark as the initial syn packet).  But, no CIPSO label.  I guess 
> this needs to be considered in any case, secmark or not.

Yep, I would categorize this case as 'external label not present,
internal label present'.  I believe the code as described would do the
right thing in allowing admins to control this, it's just up to how you
configure the system and what your policy dictates.

-- 
paul moore
linux security @ hp

RE: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

> > +static int selinux_skb_flow_in(struct sk_buff *skb, 
> unsigned short family)
> > +{
> > +	u32 xfrm_sid, trans_sid;
> > +	int err;
> > +
> > +	if (selinux_compat_net)
> > +		return 1;
> > +
> > +	/* xfrm/cipso inapplicable for loopback traffic */
> > +	if (skb->dev == &loopback_dev)
> > +		return 1;
> 
> Just to clarify (I believe this is the case from your remarks 
> as well as
> the code, but better safe than sorry) the secmark for loopback traffic
> is set by the originating socket, yes?

That's correct.

> 
> Beware: a bit of a nit follows :)
> 
> While I understand that NetLabel currently only supports 
> CIPSO, it is a
> framework that can support multiple labeling procotols (i.e. RIPSO
> support is planned).  Please change the comment from cipso/CIPSO to
> netlabel/NetLabel so it is consistent with the rest of the code which
> makes use of NetLabel.

Sure.

> 
> > +	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) {
> > +		err = security_transition_sid(xfrm_sid, skb->secmark,
> > +						
> SECCLASS_PACKET, &trans_sid);
> > +		if (err)
> > +			goto out;
> > +
> > +		skb->secmark = trans_sid;
> > +	}
> > +	/* See if CIPSO can flow in thru the current secmark here */
> 
> Same nit applies about the use of "CIPSO" vs "NetLabel".

Sure.

> 
> > +out:
> > +	return err ? 0 : 1;
> > +};
> > +
> > +static int selinux_skb_flow_out(struct sk_buff *skb, u32 nf_secid)
> > +{
> > +	u32 trans_sid;
> > +	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);
> 
> While I don't see any explicit mention of it in the documentation or
> your comments, I assume we would want a flow_out check for 
> NetLabel here
> as well?

I don't believe we do. By this time, the packet is or should already be
carrying the CIPSO/NetLabel option which should already be the right one
(derived from the socket or flow as appropriate), but you would want to
audit the code to make sure. IOW, the label option in the IP header should
already be reflecting the secmark on the skb. But again, you may want to
audit the code to make sure.

> 
> > +out:
> > +	return err ? 0 : 1;
> > +}
> > +
> >  static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
> >  {
> >  	int err = 0;
> > @@ -3700,7 +3766,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 +3777,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 +3838,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 +3856,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 +3869,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);
> > +	}
> 
> This looks nearly identical to selinux_skb_flow_out() as implemented
> above, the only real differences being two of the args to the
> avc_has_perm() call.  Any reason you don't abstract out the 
> common parts
> to a separate function?

May be in the future.

> 
> Also, the same comment/question about NetLabel support in
> selinux_skb_flow_out() applies here as well I suspect.

My comments earlier should apply here as well. 

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

Venkat Yekkirala wrote:
>>>+static int selinux_skb_flow_out(struct sk_buff *skb, u32 nf_secid)
>>>+{
>>>+	u32 trans_sid;
>>>+	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);
>>
>>While I don't see any explicit mention of it in the documentation or
>>your comments, I assume we would want a flow_out check for 
>>NetLabel here
>>as well?
>  
> I don't believe we do. By this time, the packet is or should already be
> carrying the CIPSO/NetLabel option which should already be the right one
> (derived from the socket or flow as appropriate), but you would want to
> audit the code to make sure. IOW, the label option in the IP header should
> already be reflecting the secmark on the skb. But again, you may want to
> audit the code to make sure.

In the case above I am concerned about the situation where the
skb->secmark == 0 and there is a IPv4 option (i.e. it is NetLabel
labeled) on the packet.  It would seem that the right/consistent thing
to do would be to adjust the secmark accordingly, similar to what we
would do on the flow_in case, yes?

>>>+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 +3777,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 +3838,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 +3856,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 +3869,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);
>>
>>>+	}
>>
>>This looks nearly identical to selinux_skb_flow_out() as implemented
>>above, the only real differences being two of the args to the
>>avc_has_perm() call.  Any reason you don't abstract out the 
>>common parts
>>to a separate function?
> 
> May be in the future.
> 
>>Also, the same comment/question about NetLabel support in
>>selinux_skb_flow_out() applies here as well I suspect.
> 
> My comments earlier should apply here as well. 

Mine too :)

-- 
paul moore
linux security @ hp

RE: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

> >>While I don't see any explicit mention of it in the documentation or
> >>your comments, I assume we would want a flow_out check for 
> >>NetLabel here
> >>as well?
> >  
> > I don't believe we do. By this time, the packet is or 
> should already be
> > carrying the CIPSO/NetLabel option which should already be 
> the right one
> > (derived from the socket or flow as appropriate), but you 
> would want to
> > audit the code to make sure. IOW, the label option in the 
> IP header should
> > already be reflecting the secmark on the skb. But again, 
> you may want to
> > audit the code to make sure.
> 
> In the case above I am concerned about the situation where the
> skb->secmark == 0 and there is a IPv4 option (i.e. it is NetLabel
> labeled) on the packet.

Where we initialize the secmark should be immaterial from the NetLabel
point of view. The kernel mechanisms should assure that the IP option
reflects the MLS portion (or a label in the SA range) elsewhere. In any
case, a flow_out check doesn't make sense since the IP option and the
secmark are (should be) mirroring each other and there's in actuality
no "flow out" happening; they are just 2 representation of the SAME label.

Your suggestion as to adjusting the secmark per the IP option might be
fraught with danger since, in certain cases, I believe, you just return
the incoming options in the outgoing packet (timewait, openreq, etc.?),
and there's no assurance that that's a valid enough option that you can
retrieve a sid with it, correct?

Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Paul Moore]

Venkat Yekkirala wrote:
>>>>While I don't see any explicit mention of it in the documentation or
>>>>your comments, I assume we would want a flow_out check for 
>>>>NetLabel here
>>>>as well?
>>>
>>> 
>>>I don't believe we do. By this time, the packet is or 
>>
>>should already be
>>
>>>carrying the CIPSO/NetLabel option which should already be 
>>
>>the right one
>>
>>>(derived from the socket or flow as appropriate), but you 
>>
>>would want to
>>
>>>audit the code to make sure. IOW, the label option in the 
>>
>>IP header should
>>
>>>already be reflecting the secmark on the skb. But again, 
>>
>>you may want to
>>
>>>audit the code to make sure.
>>
>>In the case above I am concerned about the situation where the
>>skb->secmark == 0 and there is a IPv4 option (i.e. it is NetLabel
>>labeled) on the packet.

It's unfortunate that you cut out the code in your reply.

> Where we initialize the secmark should be immaterial from the NetLabel
> point of view. The kernel mechanisms should assure that the IP option
> reflects the MLS portion (or a label in the SA range) elsewhere.

Yep, I agree.

> In any
> case, a flow_out check doesn't make sense since the IP option and the
> secmark are (should be) mirroring each other and there's in actuality
> no "flow out" happening; they are just 2 representation of the SAME label.

Well, reading the code I see that if the secmark is zero upon entering
the function you query the XFRM subsystem to query the SA label and set
the secmark accordingly, yes?  All I am suggesting is that we do the
same thing for NetLabel.

Please see the mail I sent earlier with the code in it to address James'
concerns, this has a proposed solution for the flow_out case.

> Your suggestion as to adjusting the secmark per the IP option might be
> fraught with danger since, in certain cases, I believe, you just return
> the incoming options in the outgoing packet (timewait, openreq, etc.?),
> and there's no assurance that that's a valid enough option that you can
> retrieve a sid with it, correct?

As implemented in the code snippets I sent earlier the generated
NetLabel SID would reflect the secmark as determined by the IPsec label
and the IP options on the packet.  I believe this is what we want as the
resulting secmark value would directly represent the security attributes
of the packet.

-- 
paul moore
linux security @ hp

RE: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

> Venkat,
> 
> With xfrm labeling, the external packets are always going to 
> be protocol 
> ESP or AH, and we can't connection track the inner protocols.  So, 

Are you sure? This doesn't compare to what my limited testing seems
to have turned up (normal netfiltering of inner protos followed by xfrms,
interspersed with their own netfiltering).

> external labeling when using xfrm labeling seems somewhat 
> superfluous, 
> except for the case of setting a label based on the interface 
> the packets 
> arrived on.  Correct?  If so, all you can realistically do 
> with the flow 
> permissions is bind the ESP/AH packets to types of interfaces 
> (which does 
> seem useful for some folk).

RE: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

> >>>>It seems more of a pain to actually
> >>>>prevent their use at the same time and/or explain 
> strange/unnatural
> >>>>behavior.
> >>>
> >>>Agreed, the solution that we agreed upon is much easier to 
> implement and
> >>>explain than a lot of the alternatives.
> >>
> >>Ok, can you please explain it further?
> >>
> >>i.e. show me what the policy looks like, exactly what the 
> user is trying 
> >>to achieve, and explain what happens to each packet exactly 
> in terms of 
> >>labeling on the input and output paths.
> >  
> > Also, why can't this be done just with xfrm labeling?
> 
> I believe the issue Venkat and I were discussing was how to handle the
> case of multiple external labeling protocols, i.e. what to do 
> if we get
> a packet through labeled SA which has a CIPSO option.  As I've said
> before, I don't believe this is something we will see much in practice
> but I think we need to decide what to do: handle it somehow 
> or just punt
> on the problem and drop it.  Several people with experience with
> external labeling have commented on how supporting both external
> labeling protocols would be a good idea so Venkat and I are trying to
> come up with a solution that works.

Here's the scoop on using xfrm and cipso at the same time:

JUST REDUNDANT

I was only paying attention to the inbound side when I was
saying a ranged SA could be used in conjunction with fine-grained
cipso labeling. In actuality, this isn't possible because of the
change we are instituting for SA selection to be based on the context
of the sock (specifically, the sock's MLS range must "equal" the range
on the SA, and cipso at this point would be representing this same range).

So, if someone were to configure CIPSO along with labeled ipsec, BOTH
mechanisms will be used, with BOTH labels carrying the same MLS range,
and with CIPSO always overriding the SA (after a
flow_in check) on the inbound. Should just be additional overhead, that's
all.

RE: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux

[Venkat Yekkirala]

> >>In the case above I am concerned about the situation where the
> >>skb->secmark == 0 and there is a IPv4 option (i.e. it is NetLabel
> >>labeled) on the packet.
> 
> It's unfortunate that you cut out the code in your reply.

It's even more unfortunate that you should say this. The proper
thing to do is to simply repaste portions that you feel like you
need to.

Remember, we aren't opponents standing for election this season. :)