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

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 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.

ipv4_get_l4proto():
       if (iph->frag_off & htons(IP_OFFSET))
              return -NF_ACCEPT;

so yes, you are right, first packet will be tracked in this case.

> 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?

What would you do instead?

We currently have a suboptimal handling of such cases, but I don't see
how we can change it without (possibly) breaking existing setups.
I also don't see how alternative handling is 'better'.

Tagging it as UNTRACKED seems wrong because its used for cases where
we could track but decide against it, e.g. due to -j NOTRACK or explicit
tracker whitelist (icmpv6 neigh for instance).

Documentation says (iptables-extensions):

  INVALID The packet is associated with no known connection.
  UNTRACKED  The packet is not tracked at all, which happens if  you
  explictly untrack it by using -j CT --notrack in the raw table.

(XXX: needs a sentence wrt. icmpv6...)

So current behaviour at least appears consistent with documentation.

> > diff --git a/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c b/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
> > ick_inndex 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)));
> > -

We could make this a explicit test+return but that seems weird too,
we would track the first fragment but would not nat.

However, changing test to if (iph->frag_off) return -NF_ACCEPT seems
wrong too because we have enough info to track. OTOH, this only happens
with HDRINCL+raw socket so perhaps we shouldn't care about this and
just change ipv4 l3 tracker to ignore all packets w. iph->frag_off set.