Re: [PATCH nf v2 1/1] netfilter: validate L4 headers after userspace packet writes

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

On Thu, Jul 30, 2026 at 01:06:27PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > +	const struct nf_conn *ct;
> > > +
> > > +	ct = nf_ct_get(e->skb, &ctinfo);
> > > +	if (ct && !nf_ct_is_template(ct) && nf_ct_protonum(ct) != proto)
> > 
> > I think it should be easier to disallow protocol number mangling in
> > the IP header (layer 3 restrictions), if not done already.
> 
> How?  nfqueue is whole-replace, not a delta.
> Or do you mean checking ip_hdr(skb) vs. the protocol field in userspace
> provided buffer?
> 
> I would prefer this solution (i.e. check ct protocol), it still allows theoretical nfqueue based
> tunneling header insertion, if done in prerouting before conntrack.

OK

> > > +	switch (proto) {
> > > +	case IPPROTO_TCP: {
> > > +		const struct tcphdr *th = (const struct tcphdr *)data;
> > 
> > This needs to use skb_header_pointer() here, you cannot assume the tcp
> > header is in a linear area.
> 
> data is a linear buffer coming from userspace
> (nla_data(nfqa[NFQA_PAYLOAD]).

Indeed.

> > > +	case IPPROTO_SCTP:
> > > +		return data_len >= sizeof(struct sctphdr);
> > > +	case IPPROTO_GRE:
> > > +		return data_len >= sizeof(struct gre_base_hdr);
> > > +	case IPPROTO_NONE:
> > 
> > Remove this and make it part of default and return true if protocol is
> > unknown.
> 
> Hmm.  Its likely safe to accept unknown headers, here.
> 
> Perhaps next iteration should indeed do what you suggest but also check
> check ESP and AH.
> 
> > Default is false for an unknown protocol, should be true.
> 
> I suggested it this way, i.e. don't permit unknown l4 protocols, but
> maybe its too restrictive.
> 
> > > +	if (pkt->tprot != IPPROTO_TCP)
> > > +		return true;
> > > +
> > > +	return priv->offset > doff || priv->offset + priv->len <= doff;
> >                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > maybe simply check priv->offset >= doff here?
> 
> Could you elaborate?  The above LGTM.  priv->offset > doff is already
> tested?  I mean, write is ok either if offset exceeds doff (lhs)
> or if offset + length doesn't touch doff area (rhs).
> 
> Did you mean "just reject everything exceeding doff"?
> 
> Patch LGTM, except perhaps switching to "allow unknowns" in nfqueue.

OK, thanks for explaining.