Re: [PATCH nf] netfilter: nat: remove incorrect debug assert

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Feb 08, 2017 at 11:14:29PM +0100, Florian Westphal wrote:
> The comment is incorrect, this function does see fragments when
> IP_NODEFRAG is used.  Remove the wrong assertion.
> 
> As conntrack doesn't track fragments skb->nfct will be null
> and no nat is performed.

With IP_NODEFRAG, ipv4_conntrack_defrag() will just accept the packet.

So the first fragment will get into nf_conntrack_in(), and I think, if
enough information is there in place, it will get a ct object. Follow
up fragments with offset != 0 which doesn't contain headers will
definitely not get a ct object.

Shouldn't handle case this by attaching a template conntrack?
Currently this IP_NODEFRAG case is going through as invalid traffic.

My impression is that we're handling this case in a sloppy way, am I
missing anything?

> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/ipv4/netfilter/nf_nat_l3proto_ipv4.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c b/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
> index f8aad03d674b..6f5e8d01b876 100644
> --- a/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
> +++ b/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
> @@ -255,11 +255,6 @@ nf_nat_ipv4_fn(void *priv, struct sk_buff *skb,
>  	/* maniptype == SRC for postrouting. */
>  	enum nf_nat_manip_type maniptype = HOOK2MANIP(state->hook);
>  
> -	/* We never see fragments: conntrack defrags on pre-routing
> -	 * and local-out, and nf_nat_out protects post-routing.
> -	 */
> -	NF_CT_ASSERT(!ip_is_fragment(ip_hdr(skb)));
> -
>  	ct = nf_ct_get(skb, &ctinfo);
>  	/* Can't track?  It's not due to stress, or conntrack would
>  	 * have dropped it.  Hence it's the user's responsibilty to
> -- 
> 2.10.2
> 
> --
> 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