[PATCH nf] netfilter: bridge: don't leak skb in error paths

[PATCH nf] netfilter: bridge: don't leak skb in error paths

[Florian Westphal]

br_nf_dev_queue_xmit must free skb in its error path.
NF_DROP is misleading -- its an okfn, not a netfilter hook.

Fixes: 462fb2af9788a ("bridge : Sanitize skb before it enters the IP stack")
Fixes: efb6de9b4ba00 ("netfilter: bridge: forward IPv6 fragmented packets")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Not sure the br_validate* calls are needed. When we reach br_nf_dev_queue_xmit
 skbs have already been through all brnf hooks where we also have these checks.

diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index d89f4fa..1a6fa67 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c
@@ -742,7 +742,7 @@ static int br_nf_dev_queue_xmit(struct sock *sk, struct sk_buff *skb)
 		struct brnf_frag_data *data;
 
 		if (br_validate_ipv4(skb))
-			return NF_DROP;
+			goto drop;
 
 		IPCB(skb)->frag_max_size = nf_bridge->frag_max_size;
 
@@ -767,7 +767,7 @@ static int br_nf_dev_queue_xmit(struct sock *sk, struct sk_buff *skb)
 		struct brnf_frag_data *data;
 
 		if (br_validate_ipv6(skb))
-			return NF_DROP;
+			goto drop;
 
 		IP6CB(skb)->frag_max_size = nf_bridge->frag_max_size;
 
@@ -782,12 +782,16 @@ static int br_nf_dev_queue_xmit(struct sock *sk, struct sk_buff *skb)
 
 		if (v6ops)
 			return v6ops->fragment(sk, skb, br_nf_push_frag_xmit);
-		else
-			return -EMSGSIZE;
+
+		kfree_skb(skb);
+		return -EMSGSIZE;
 	}
 #endif
 	nf_bridge_info_free(skb);
 	return br_dev_queue_push_xmit(sk, skb);
+ drop:
+	kfree_skb(skb);
+	return 0;
 }
 
 /* PF_BRIDGE/POST_ROUTING ********************************************/
-- 
2.0.5

Re: [PATCH nf] netfilter: bridge: don't leak skb in error paths

[Pablo Neira Ayuso]

On Tue, Jun 30, 2015 at 10:27:51PM +0200, Florian Westphal wrote:
> br_nf_dev_queue_xmit must free skb in its error path.
> NF_DROP is misleading -- its an okfn, not a netfilter hook.

Good catch.

> Fixes: 462fb2af9788a ("bridge : Sanitize skb before it enters the IP stack")
> Fixes: efb6de9b4ba00 ("netfilter: bridge: forward IPv6 fragmented packets")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Not sure the br_validate* calls are needed. When we reach br_nf_dev_queue_xmit
>  skbs have already been through all brnf hooks where we also have these checks.

Locally originated traffic should be sane at this point, and forwarded
traffic was already validated.

> diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> index d89f4fa..1a6fa67 100644
> --- a/net/bridge/br_netfilter_hooks.c
> +++ b/net/bridge/br_netfilter_hooks.c
> @@ -742,7 +742,7 @@ static int br_nf_dev_queue_xmit(struct sock *sk, struct sk_buff *skb)
>  		struct brnf_frag_data *data;
>  
>  		if (br_validate_ipv4(skb))
> -			return NF_DROP;
> +			goto drop;
>  
>  		IPCB(skb)->frag_max_size = nf_bridge->frag_max_size;
>  
> @@ -767,7 +767,7 @@ static int br_nf_dev_queue_xmit(struct sock *sk, struct sk_buff *skb)
>  		struct brnf_frag_data *data;
>  
>  		if (br_validate_ipv6(skb))
> -			return NF_DROP;
> +			goto drop;
>  
>  		IP6CB(skb)->frag_max_size = nf_bridge->frag_max_size;
>  
> @@ -782,12 +782,16 @@ static int br_nf_dev_queue_xmit(struct sock *sk, struct sk_buff *skb)
>  
>  		if (v6ops)
>  			return v6ops->fragment(sk, skb, br_nf_push_frag_xmit);
> -		else
> -			return -EMSGSIZE;
> +
> +		kfree_skb(skb);
> +		return -EMSGSIZE;
>  	}
>  #endif
>  	nf_bridge_info_free(skb);
>  	return br_dev_queue_push_xmit(sk, skb);
> + drop:
> +	kfree_skb(skb);
> +	return 0;
>  }
>  
>  /* PF_BRIDGE/POST_ROUTING ********************************************/
> -- 
> 2.0.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf] netfilter: bridge: don't leak skb in error paths

[Pablo Neira Ayuso]

On Thu, Jul 02, 2015 at 02:59:44PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jun 30, 2015 at 10:27:51PM +0200, Florian Westphal wrote:
> > br_nf_dev_queue_xmit must free skb in its error path.
> > NF_DROP is misleading -- its an okfn, not a netfilter hook.
> 
> Good catch.

Florian, would you send me a backport of this patch for -stable?

git describe says this problem is there since v2.6.37.

Let me know, thanks.