[PATCH 0/2] Labeled networking core stack changes for 2.6.25

[PATCH 0/2] Labeled networking core stack changes for 2.6.25

[Paul Moore]

Two small patches for 2.6.25 which enable some new labeled networking features
and fixes.  One patch introduces a new outbound packet LSM hook and one adds
the skb 'iif' field to the list of fields copied during a clone operation.

Both of these patches are part of a much larger patchset that has been under
review on the SELinux and LSM mailing lists for the past few months (some bits
have been under review since this past spring).  I'll save everyone the spam
and not post the entire patchset here, but if you want to take a look you can
find the latest bits here:

 * git://git.infradead.org/users/pcmoore/lblnet-2.6_testing

I'm posting these patches here for review and hopefully an 'Acked-by', not
inclusion into net-2.6.25.  If these patches are acceptable then they will
pushed upstream with the rest of the changes when 2.6.25 is ready.

Thanks.

-- 
paul moore
linux security @ hp

[PATCH 1/2] LSM: Add inet_sys_snd_skb() LSM hook

[Paul Moore]

Add an inet_sys_snd_skb() LSM hook to allow the LSM to provide packet level
access control for all outbound packets.  Using the existing postroute_last
netfilter hook turns out to be problematic as it is can be invoked multiple
times for a single packet, e.g. individual IPsec transforms, adding unwanted
overhead and complicating the security policy.

Signed-off-by: Paul Moore <paul.moore@hp.com>
---

 include/linux/security.h |   11 +++++++++++
 net/ipv4/ip_output.c     |    7 +++++++
 net/ipv6/ip6_output.c    |    5 +++++
 security/dummy.c         |    8 +++++++-
 security/security.c      |    6 ++++++
 5 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index db19c92..1b8d332 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -876,6 +876,10 @@ struct request_sock;
  *     Sets the connection's peersid to the secmark on skb.
  * @req_classify_flow:
  *	Sets the flow's sid to the openreq sid.
+ * @inet_sys_snd_skb:
+ *	Check permissions on outgoing network packets.
+ *	@skb is the packet to check
+ *	@family is the packet's address family
  *
  * Security hooks for XFRM operations.
  *
@@ -1416,6 +1420,7 @@ struct security_operations {
 	void (*inet_csk_clone)(struct sock *newsk, const struct request_sock *req);
 	void (*inet_conn_established)(struct sock *sk, struct sk_buff *skb);
 	void (*req_classify_flow)(const struct request_sock *req, struct flowi *fl);
+	int (*inet_sys_snd_skb)(struct sk_buff *skb, int family);
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -2328,6 +2333,7 @@ void security_sk_free(struct sock *sk);
 void security_sk_clone(const struct sock *sk, struct sock *newsk);
 void security_sk_classify_flow(struct sock *sk, struct flowi *fl);
 void security_req_classify_flow(const struct request_sock *req, struct flowi *fl);
+int security_inet_sys_snd_skb(struct sk_buff *skb, int family);
 void security_sock_graft(struct sock*sk, struct socket *parent);
 int security_inet_conn_request(struct sock *sk,
 			struct sk_buff *skb, struct request_sock *req);
@@ -2471,6 +2477,11 @@ static inline void security_req_classify_flow(const struct request_sock *req, st
 {
 }
 
+static inline int security_inet_sys_snd_skb(struct sk_buff *skb, int family)
+{
+	return 0;
+}
+
 static inline void security_sock_graft(struct sock* sk, struct socket *parent)
 {
 }
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index fd99fbd..82a7297 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -204,6 +204,8 @@ static inline int ip_skb_dst_mtu(struct sk_buff *skb)
 
 static int ip_finish_output(struct sk_buff *skb)
 {
+	int err;
+
 #if defined(CONFIG_NETFILTER) && defined(CONFIG_XFRM)
 	/* Policy lookup after SNAT yielded a new policy */
 	if (skb->dst->xfrm != NULL) {
@@ -211,6 +213,11 @@ static int ip_finish_output(struct sk_buff *skb)
 		return dst_output(skb);
 	}
 #endif
+
+	err = security_inet_sys_snd_skb(skb, AF_INET);
+	if (err)
+		return err;
+
 	if (skb->len > ip_skb_dst_mtu(skb) && !skb_is_gso(skb))
 		return ip_fragment(skb, ip_finish_output2);
 	else
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 6338a9c..44ddf32 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -72,8 +72,13 @@ static __inline__ void ipv6_select_ident(struct sk_buff *skb, struct frag_hdr *f
 
 static int ip6_output_finish(struct sk_buff *skb)
 {
+	int err;
 	struct dst_entry *dst = skb->dst;
 
+	err = security_inet_sys_snd_skb(skb, AF_INET6);
+	if (err)
+		return err;
+
 	if (dst->hh)
 		return neigh_hh_output(dst->hh, skb);
 	else if (dst->neighbour)
diff --git a/security/dummy.c b/security/dummy.c
index 0b62f95..384979a 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -848,6 +848,11 @@ static inline void dummy_req_classify_flow(const struct request_sock *req,
 			struct flowi *fl)
 {
 }
+
+static inline int dummy_inet_sys_snd_skb(struct sk_buff *skb, int family)
+{
+	return 0;
+}
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -1122,7 +1127,8 @@ void security_fixup_ops (struct security_operations *ops)
 	set_to_dummy_if_null(ops, inet_csk_clone);
 	set_to_dummy_if_null(ops, inet_conn_established);
 	set_to_dummy_if_null(ops, req_classify_flow);
- #endif	/* CONFIG_SECURITY_NETWORK */
+	set_to_dummy_if_null(ops, inet_sys_snd_skb);
+#endif	/* CONFIG_SECURITY_NETWORK */
 #ifdef  CONFIG_SECURITY_NETWORK_XFRM
 	set_to_dummy_if_null(ops, xfrm_policy_alloc_security);
 	set_to_dummy_if_null(ops, xfrm_policy_clone_security);
diff --git a/security/security.c b/security/security.c
index 3bdcada..7f55459 100644
--- a/security/security.c
+++ b/security/security.c
@@ -961,6 +961,12 @@ void security_req_classify_flow(const struct request_sock *req, struct flowi *fl
 }
 EXPORT_SYMBOL(security_req_classify_flow);
 
+int security_inet_sys_snd_skb(struct sk_buff *skb, int family)
+{
+	return security_ops->inet_sys_snd_skb(skb, family);
+}
+EXPORT_SYMBOL(security_inet_sys_snd_skb);
+
 void security_sock_graft(struct sock *sk, struct socket *parent)
 {
 	security_ops->sock_graft(sk, parent);

[PATCH 2/2] NET: Clone the sk_buff 'iif' field in __skb_clone()

[Paul Moore]

Both NetLabel and SELinux (other LSMs may grow to use it as well) rely on the
'iif' field to determine the receiving network interface of inbound packets.
Unfortunately, at present this field is not preserved across a skb clone
operation which can lead to garbage values if the cloned skb is sent back
through the network stack.  This patch corrects this problem by properly
copying the 'iif' field in __skb_clone() and removing the 'iif' field
assignment from skb_act_clone() since it is no longer needed.

Also, while we are here, get rid of that silly C() macro.

Signed-off-by: Paul Moore <paul.moore@hp.com>
---

 include/net/sch_generic.h |    1 -
 net/core/skbuff.c         |   20 +++++++++-----------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c926551..4c3b351 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -325,7 +325,6 @@ static inline struct sk_buff *skb_act_clone(struct sk_buff *skb, gfp_t gfp_mask)
 		n->tc_verd = SET_TC_VERD(n->tc_verd, 0);
 		n->tc_verd = CLR_TC_OK2MUNGE(n->tc_verd);
 		n->tc_verd = CLR_TC_MUNGED(n->tc_verd);
-		n->iif = skb->iif;
 	}
 	return n;
 }
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5b4ce9b..c726cd4 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -407,31 +407,29 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 
 static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
 {
-#define C(x) n->x = skb->x
-
 	n->next = n->prev = NULL;
 	n->sk = NULL;
 	__copy_skb_header(n, skb);
 
-	C(len);
-	C(data_len);
-	C(mac_len);
+	n->iif = skb->iif;
+	n->len = skb->len;
+	n->data_len = skb->data_len;
+	n->mac_len = skb->mac_len;
 	n->cloned = 1;
 	n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len;
 	n->nohdr = 0;
 	n->destructor = NULL;
-	C(truesize);
+	n->truesize = skb->truesize;
 	atomic_set(&n->users, 1);
-	C(head);
-	C(data);
-	C(tail);
-	C(end);
+	n->head = skb->head;
+	n->data = skb->data;
+	n->tail = skb->tail;
+	n->end = skb->end;
 
 	atomic_inc(&(skb_shinfo(skb)->dataref));
 	skb->cloned = 1;
 
 	return n;
-#undef C
 }
 
 /**

Re: [PATCH 2/2] NET: Clone the sk_buff 'iif' field in __skb_clone()

[Joe Perches]

On Thu, 2008-01-03 at 12:25 -0500, Paul Moore wrote:
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 5b4ce9b..c726cd4 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -407,31 +407,29 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
>  
>  static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
>  {
> -#define C(x) n->x = skb->x
> -
>  	n->next = n->prev = NULL;
>  	n->sk = NULL;
>  	__copy_skb_header(n, skb);
>  
> -	C(len);
> -	C(data_len);
> -	C(mac_len);
> +	n->iif = skb->iif;
> +	n->len = skb->len;
> +	n->data_len = skb->data_len;
> +	n->mac_len = skb->mac_len;
>  	n->cloned = 1;
>  	n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len;
>  	n->nohdr = 0;

To reduce possible cacheline bounces, shouldn't the order of
operation on the elements be in struct order?

iif should be after destructor.

nohdr then hdr_len

>  	n->destructor = NULL;
> -	C(truesize);
> +	n->truesize = skb->truesize;
>  	atomic_set(&n->users, 1);
> -	C(head);
> -	C(data);
> -	C(tail);
> -	C(end);
> +	n->head = skb->head;
> +	n->data = skb->data;
> +	n->tail = skb->tail;
> +	n->end = skb->end;

and perhaps tail,end,head,data,truesize,users?

Re: [PATCH 2/2] NET: Clone the sk_buff 'iif' field in __skb_clone()

[Paul Moore]

On Thursday 03 January 2008 1:33:05 pm Joe Perches wrote:
> On Thu, 2008-01-03 at 12:25 -0500, Paul Moore wrote:
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 5b4ce9b..c726cd4 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -407,31 +407,29 @@ static void __copy_skb_header(struct sk_buff
> > *new, const struct sk_buff *old)
> >
> >  static struct sk_buff *__skb_clone(struct sk_buff *n, struct
> > sk_buff *skb) {
> > -#define C(x) n->x = skb->x
> > -
> >  	n->next = n->prev = NULL;
> >  	n->sk = NULL;
> >  	__copy_skb_header(n, skb);
> >
> > -	C(len);
> > -	C(data_len);
> > -	C(mac_len);
> > +	n->iif = skb->iif;
> > +	n->len = skb->len;
> > +	n->data_len = skb->data_len;
> > +	n->mac_len = skb->mac_len;
> >  	n->cloned = 1;
> >  	n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len;
> >  	n->nohdr = 0;
>
> To reduce possible cacheline bounces, shouldn't the order of
> operation on the elements be in struct order?

Sounds reasonable to me, I'll adjust the function to match the field 
offsets.

-- 
paul moore
linux security @ hp

Re: [PATCH 1/2] LSM: Add inet_sys_snd_skb() LSM hook

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Thu, 03 Jan 2008 12:25:39 -0500

> Add an inet_sys_snd_skb() LSM hook to allow the LSM to provide packet level
> access control for all outbound packets.  Using the existing postroute_last
> netfilter hook turns out to be problematic as it is can be invoked multiple
> times for a single packet, e.g. individual IPsec transforms, adding unwanted
> overhead and complicating the security policy.
> 
> Signed-off-by: Paul Moore <paul.moore@hp.com>

I disagree with this change.

The packet is different each time you see it in the postrouting hook,
and also the new hook is thus redundant.

If it's a performance issue and you can classify the security early,
mark the SKB as "seen" and then on subsequent hooks you can just
return immediately if that flag is set.

Re: [PATCH 1/2] LSM: Add inet_sys_snd_skb() LSM hook

[Paul Moore]

On Thursday 03 January 2008 11:45:49 pm David Miller wrote:
> From: Paul Moore <paul.moore@hp.com>
> Date: Thu, 03 Jan 2008 12:25:39 -0500
>
> > Add an inet_sys_snd_skb() LSM hook to allow the LSM to provide
> > packet level access control for all outbound packets.  Using the
> > existing postroute_last netfilter hook turns out to be problematic
> > as it is can be invoked multiple times for a single packet, e.g.
> > individual IPsec transforms, adding unwanted overhead and
> > complicating the security policy.
> >
> > Signed-off-by: Paul Moore <paul.moore@hp.com>
>
> I disagree with this change.
>
> The packet is different each time you see it in the postrouting hook,
> and also the new hook is thus redundant.

Well, thanks for taking a look.

> If it's a performance issue and you can classify the security early,
> mark the SKB as "seen" and then on subsequent hooks you can just
> return immediately if that flag is set.

Unfortunately, it's not quite that easy at present.  The only field we 
have in the skb where we could possibly set a flag is the secmark field 
which is already taken.  Granted, there is the possibility of 
segmenting the secmark field to some degree but that brings about a new 
set of problems involving the number of unique labels, backwards 
compatibility, etc.

Regardless, back to the drawing board.  I'll have to think a bit harder 
about a way to make the netfilter hooks work ...

-- 
paul moore
linux security @ hp

Re: [PATCH 1/2] LSM: Add inet_sys_snd_skb() LSM hook

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Fri, 4 Jan 2008 09:38:27 -0500

> Unfortunately, it's not quite that easy at present.  The only field we 
> have in the skb where we could possibly set a flag is the secmark field 
> which is already taken.

Herbert Xu added a "peeked" field in net-2.6.25 that is only used on
input while processing socket receive queues.  You could use it on
output.

Re: [PATCH 1/2] LSM: Add inet_sys_snd_skb() LSM hook

[Paul Moore]

On Friday 04 January 2008 4:09:02 pm David Miller wrote:
> From: Paul Moore <paul.moore@hp.com>
> Date: Fri, 4 Jan 2008 09:38:27 -0500
>
> > Unfortunately, it's not quite that easy at present.  The only field
> > we have in the skb where we could possibly set a flag is the
> > secmark field which is already taken.
>
> Herbert Xu added a "peeked" field in net-2.6.25 that is only used on
> input while processing socket receive queues.  You could use it on
> output.

Actually, I went back to the drawing board and I think I found a 
solution that _should_ work using the existing postroute hook.  It 
isn't as general but it is relatively simple.

Historically the problem has been with labeled IPsec and the fact that 
the postroute hook can get hit multiple times when it is in use.  While 
yes, the packet is different each time through the hook but the packet's 
security label never changes (the packet's security label is determined 
by the original sender).  From a security point of view we really only 
want to check the packet once on the way out and we want that check to 
happen at the very end, not while packet transforms are in progress.  
This was the motivation for the new LSM hook.

After the new hook was rejected I took a step back and thought about the 
problem a bit more.  The multi-hit postroute hook problem was really 
only an issue for IPsec; the other labeling protocols don't have this 
problem because they don't do any transformation of the packet.  If we 
could find a quick way to determine when all of the IPsec processing 
was finished would could use the existing postroute hook approach and 
simply fall through if the hook was hit when IPsec processing was still 
needed.

I still need to test this to make sure it does everything we need, but 
I'm pretty certain that using the we can key off the skb->dst->xfrm 
value as a way to determine if a packet is done with it's IPsec 
transformation, if any.  Basically we rewrite our postroute hook to 
look something like this:

 int hook(...)
 {
	/* stuff to do every time */

	if (skb->dst->xfrm != NULL)
		return NF_ACCEPT;

	/* stuff to do only on the last time we are called */

 }

If it doesn't end up meeting our needs I'll look into the 'peeked' 
field, thanks for the suggestion.

-- 
paul moore
linux security @ hp