[PATCH 1/3] netfilter: allow hooks to pass error code back up the stack

[PATCH 1/3] netfilter: allow hooks to pass error code back up the stack

[Eric Paris]

SELinux would like to pass certain fatal errors back up the stack.  This patch
implements the generic netfilter support for this functionality.

Based-on-patch-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: Eric Paris <eparis@redhat.com>
---

 include/linux/netfilter.h |    2 ++
 net/netfilter/core.c      |    6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 03317c8..1893837 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -33,6 +33,8 @@
 
 #define NF_QUEUE_NR(x) ((((x) << NF_VERDICT_BITS) & NF_VERDICT_QMASK) | NF_QUEUE)
 
+#define NF_DROP_ERR(x) (((-x) << NF_VERDICT_BITS) | NF_DROP)
+
 /* only for userspace compatibility */
 #ifndef __KERNEL__
 /* Generic cache responses from hook functions.
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 85dabb8..32fcbe2 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -173,9 +173,11 @@ next_hook:
 			     outdev, &elem, okfn, hook_thresh);
 	if (verdict == NF_ACCEPT || verdict == NF_STOP) {
 		ret = 1;
-	} else if (verdict == NF_DROP) {
+	} else if ((verdict & NF_VERDICT_MASK) == NF_DROP) {
 		kfree_skb(skb);
-		ret = -EPERM;
+		ret = -(verdict >> NF_VERDICT_BITS);
+		if (ret == 0)
+			ret = -EPERM;
 	} else if ((verdict & NF_VERDICT_MASK) == NF_QUEUE) {
 		if (!nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
 			      verdict >> NF_VERDICT_BITS))

[PATCH 2/3] network: tcp_connect should return certain errors up the stack

[Eric Paris]

The current tcp_connect code completely ignores errors from sending an skb.
This makes sense in many situations (like -ENOBUFFS) but I want to be able to
immediately fail connections if they are denied by the SELinux netfilter hook.
Netfilter does not normally return ECONNREFUSED when it drops a packet so we
respect that error code as a final and fatal error that can not be recovered.

Based-on-patch-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: Eric Paris <eparis@redhat.com>
---

 net/ipv4/tcp_output.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e961522..15dcd7b 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2592,6 +2592,7 @@ int tcp_connect(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct sk_buff *buff;
+	int err;
 
 	tcp_connect_init(sk);
 
@@ -2614,7 +2615,9 @@ int tcp_connect(struct sock *sk)
 	sk->sk_wmem_queued += buff->truesize;
 	sk_mem_charge(sk, buff->truesize);
 	tp->packets_out += tcp_skb_pcount(buff);
-	tcp_transmit_skb(sk, buff, 1, sk->sk_allocation);
+	err = tcp_transmit_skb(sk, buff, 1, sk->sk_allocation);
+	if (err == -ECONNREFUSED)
+		return err;
 
 	/* We change tp->snd_nxt after the tcp_transmit_skb() call
 	 * in order to make this packet get counted in tcpOutSegs.

Re: [PATCH 2/3] network: tcp_connect should return certain errors up the stack

[David Miller]

From: Eric Paris <eparis@redhat.com>
Date: Tue, 16 Nov 2010 16:52:49 -0500

> The current tcp_connect code completely ignores errors from sending an skb.
> This makes sense in many situations (like -ENOBUFFS) but I want to be able to
> immediately fail connections if they are denied by the SELinux netfilter hook.
> Netfilter does not normally return ECONNREFUSED when it drops a packet so we
> respect that error code as a final and fatal error that can not be recovered.
> 
> Based-on-patch-by: Patrick McHardy <kaber@trash.net>
> Signed-off-by: Eric Paris <eparis@redhat.com>

Applied.

[PATCH 3/3] SELinux: return -ECONNREFUSED from ip_postroute to signal fatal error

[Eric Paris]

The SELinux netfilter hooks just return NF_DROP if they drop a packet.  We
want to signal that a drop in this hook is a permanant fatal error and is not
transient.  If we do this the error will be passed back up the stack in some
places and applications will get a faster interaction that something went
wrong.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 security/selinux/hooks.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8ba5001..b1104f9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4594,11 +4594,11 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
 				secmark_perm = PACKET__SEND;
 			break;
 		default:
-			return NF_DROP;
+			return NF_DROP_ERR(-ECONNREFUSED);
 		}
 		if (secmark_perm == PACKET__FORWARD_OUT) {
 			if (selinux_skb_peerlbl_sid(skb, family, &peer_sid))
-				return NF_DROP;
+				return NF_DROP_ERR(-ECONNREFUSED);
 		} else
 			peer_sid = SECINITSID_KERNEL;
 	} else {
@@ -4611,28 +4611,28 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
 	ad.u.net.netif = ifindex;
 	ad.u.net.family = family;
 	if (selinux_parse_skb(skb, &ad, &addrp, 0, NULL))
-		return NF_DROP;
+		return NF_DROP_ERR(-ECONNREFUSED);
 
 	if (secmark_active)
 		if (avc_has_perm(peer_sid, skb->secmark,
 				 SECCLASS_PACKET, secmark_perm, &ad))
-			return NF_DROP;
+			return NF_DROP_ERR(-ECONNREFUSED);
 
 	if (peerlbl_active) {
 		u32 if_sid;
 		u32 node_sid;
 
 		if (sel_netif_sid(ifindex, &if_sid))
-			return NF_DROP;
+			return NF_DROP_ERR(-ECONNREFUSED);
 		if (avc_has_perm(peer_sid, if_sid,
 				 SECCLASS_NETIF, NETIF__EGRESS, &ad))
-			return NF_DROP;
+			return NF_DROP_ERR(-ECONNREFUSED);
 
 		if (sel_netnode_sid(addrp, family, &node_sid))
-			return NF_DROP;
+			return NF_DROP_ERR(-ECONNREFUSED);
 		if (avc_has_perm(peer_sid, node_sid,
 				 SECCLASS_NODE, NODE__SENDTO, &ad))
-			return NF_DROP;
+			return NF_DROP_ERR(-ECONNREFUSED);
 	}
 
 	return NF_ACCEPT;

Re: [PATCH 3/3] SELinux: return -ECONNREFUSED from ip_postroute to signal fatal error

[Patrick McHardy]

On 16.11.2010 22:52, Eric Paris wrote:
> The SELinux netfilter hooks just return NF_DROP if they drop a packet.  We
> want to signal that a drop in this hook is a permanant fatal error and is not
> transient.  If we do this the error will be passed back up the stack in some
> places and applications will get a faster interaction that something went
> wrong.

Looks good to me. I'd suggest to have these patches go through Dave's
tree since I want to make use of the netfilter error propagation
mechanism to return proper errno codes for netfilter re-routing
failures.

Re: [PATCH 3/3] SELinux: return -ECONNREFUSED from ip_postroute to signal fatal error

[Eric Paris]

On Wed, 2010-11-17 at 12:43 +0100, Patrick McHardy wrote:
> On 16.11.2010 22:52, Eric Paris wrote:
> > The SELinux netfilter hooks just return NF_DROP if they drop a packet.  We
> > want to signal that a drop in this hook is a permanant fatal error and is not
> > transient.  If we do this the error will be passed back up the stack in some
> > places and applications will get a faster interaction that something went
> > wrong.
> 
> Looks good to me. I'd suggest to have these patches go through Dave's
> tree since I want to make use of the netfilter error propagation
> mechanism to return proper errno codes for netfilter re-routing
> failures.


I'd be happy if Dave pulled patches 1 and 2.  I can resend patch #3 once
I can cajole another of the SELinux maintainers to look at it (I believe
he most likely one is on vacation this week)

-Eric

Re: [PATCH 3/3] SELinux: return -ECONNREFUSED from ip_postroute to signal fatal error

[David Miller]

From: Eric Paris <eparis@redhat.com>
Date: Wed, 17 Nov 2010 09:38:59 -0500

> On Wed, 2010-11-17 at 12:43 +0100, Patrick McHardy wrote:
>> On 16.11.2010 22:52, Eric Paris wrote:
>> > The SELinux netfilter hooks just return NF_DROP if they drop a packet.  We
>> > want to signal that a drop in this hook is a permanant fatal error and is not
>> > transient.  If we do this the error will be passed back up the stack in some
>> > places and applications will get a faster interaction that something went
>> > wrong.
>> 
>> Looks good to me. I'd suggest to have these patches go through Dave's
>> tree since I want to make use of the netfilter error propagation
>> mechanism to return proper errno codes for netfilter re-routing
>> failures.
> 
> 
> I'd be happy if Dave pulled patches 1 and 2.  I can resend patch #3 once
> I can cajole another of the SELinux maintainers to look at it (I believe
> he most likely one is on vacation this week)

I think it's best to pull this all into net-next-2.6 now, so that's what
I'm doing right now.

If there are problems we can apply changes on top.

Thanks.

Re: [PATCH 3/3] SELinux: return -ECONNREFUSED from ip_postroute to signal fatal error

[David Miller]

From: Eric Paris <eparis@redhat.com>
Date: Tue, 16 Nov 2010 16:52:57 -0500

> The SELinux netfilter hooks just return NF_DROP if they drop a packet.  We
> want to signal that a drop in this hook is a permanant fatal error and is not
> transient.  If we do this the error will be passed back up the stack in some
> places and applications will get a faster interaction that something went
> wrong.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

Applied.

Re: [PATCH 3/3] SELinux: return -ECONNREFUSED from ip_postroute to signal fatal error

[Paul Moore]

On Tue, 2010-11-16 at 16:52 -0500, Eric Paris wrote:
> The SELinux netfilter hooks just return NF_DROP if they drop a packet.  We
> want to signal that a drop in this hook is a permanant fatal error and is not
> transient.  If we do this the error will be passed back up the stack in some
> places and applications will get a faster interaction that something went
> wrong.

Sorry for the delay, but I wasn't able to respond until today ...

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8ba5001..b1104f9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4594,11 +4594,11 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
>  				secmark_perm = PACKET__SEND;
>  			break;
>  		default:
> -			return NF_DROP;
> +			return NF_DROP_ERR(-ECONNREFUSED);
>  		}
>  		if (secmark_perm == PACKET__FORWARD_OUT) {
>  			if (selinux_skb_peerlbl_sid(skb, family, &peer_sid))
> -				return NF_DROP;
> +				return NF_DROP_ERR(-ECONNREFUSED);

The error condition here isn't due to a policy decision, but some
runtime error that happened when trying to determine the peer label of
an individual packet.  I think leaving this as just NF_DROP is the right
thing to do as an error here can be temporary.

>  		} else
>  			peer_sid = SECINITSID_KERNEL;
>  	} else {
> @@ -4611,28 +4611,28 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
>  	ad.u.net.netif = ifindex;
>  	ad.u.net.family = family;
>  	if (selinux_parse_skb(skb, &ad, &addrp, 0, NULL))
> -		return NF_DROP;
> +		return NF_DROP_ERR(-ECONNREFUSED);

Same thing, this is a transient error and not due to a policy decision.
We should keep this as NF_DROP.

>  	if (secmark_active)
>  		if (avc_has_perm(peer_sid, skb->secmark,
>  				 SECCLASS_PACKET, secmark_perm, &ad))
> -			return NF_DROP;
> +			return NF_DROP_ERR(-ECONNREFUSED);

Yep, this is a good place as the error is the result of a policy
decision, i.e. an avc_has_perm() call.

>  	if (peerlbl_active) {
>  		u32 if_sid;
>  		u32 node_sid;
>  
>  		if (sel_netif_sid(ifindex, &if_sid))
> -			return NF_DROP;
> +			return NF_DROP_ERR(-ECONNREFUSED);

Another transient error case, should be NF_DROP.

>  		if (avc_has_perm(peer_sid, if_sid,
>  				 SECCLASS_NETIF, NETIF__EGRESS, &ad))
> -			return NF_DROP;
> +			return NF_DROP_ERR(-ECONNREFUSED);

Good.
 
>  		if (sel_netnode_sid(addrp, family, &node_sid))
> -			return NF_DROP;
> +			return NF_DROP_ERR(-ECONNREFUSED);

Bad.

>  		if (avc_has_perm(peer_sid, node_sid,
>  				 SECCLASS_NODE, NODE__SENDTO, &ad))
> -			return NF_DROP;
> +			return NF_DROP_ERR(-ECONNREFUSED);

Good.  I think you get the idea now.

Also, while I think we can ignore the forwarding and output hooks, we do
need to change the NF_DROPs in selinux_ip_postroute_compat() ... I'd
suggest the following (the last change catches more than just policy
decisions but considering the "compat" nature I think a little wiggle
room is okay here):

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 65fa8bf..de1b79e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4520,11 +4520,11 @@ static unsigned int selinux_ip_postroute_compat(struct sk_buff *skb,
        if (selinux_secmark_enabled())
                if (avc_has_perm(sksec->sid, skb->secmark,
                                 SECCLASS_PACKET, PACKET__SEND, &ad))
-                       return NF_DROP;
+                       return NF_DROP_ERR(-ECONNREFUSED);
 
        if (selinux_policycap_netpeer)
                if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
-                       return NF_DROP;
+                       return NF_DROP_ERR(-ECONNREFUSED);
 
        return NF_ACCEPT;
 }

-- 
paul moore
linux @ hp

Re: [PATCH 1/3] netfilter: allow hooks to pass error code back up the stack

[David Miller]

From: Eric Paris <eparis@redhat.com>
Date: Tue, 16 Nov 2010 16:52:38 -0500

> SELinux would like to pass certain fatal errors back up the stack.  This patch
> implements the generic netfilter support for this functionality.
> 
> Based-on-patch-by: Patrick McHardy <kaber@trash.net>
> Signed-off-by: Eric Paris <eparis@redhat.com>

Applied.