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

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

On Tue, Feb 21, 2017 at 03:40:19PM +0100, Florian Westphal wrote:
> 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.

With NAT in place, this also means the first packet of a flow gets
mangled, while follow up don't.

Probably change that spot above to use ip_is_fragment()?

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

AFAIK, only IP_NODEFRAG locally generated packets would get affected.
I wonder how this option is used (network testing?). I cannot come up
with any reasonable stateful ruleset that may work with this. With a
stateful ruleset in place, the first packet will go through and follow
up would be INVALID. There are tons of rulesets outthere simply
logging and dropping invalid ones.

Look, the first packet create an entry in SYN_SENT state, that just
expires later on.

> I also don't see how alternative handling is 'better'.

We could just handle all the packets in a flow in the same way, so
they all go through INVALID.