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

Pablo Neira Ayuso <[email protected]> wrote:
> > +	const struct nf_conn *ct;
> > +
> > +	ct =3D nf_ct_get(e->skb, &ctinfo);
> > +	if (ct && !nf_ct_is_template(ct) && nf_ct_protonum(ct) !=3D proto)
>=20
> 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 theo=
retical nfqueue based
tunneling header insertion, if done in prerouting before conntrack.

> > +	switch (proto) {
> > +	case IPPROTO_TCP: {
> > +		const struct tcphdr *th =3D (const struct tcphdr *)data;
>=20
> 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]).

> > +	case IPPROTO_SCTP:
> > +		return data_len >=3D sizeof(struct sctphdr);
> > +	case IPPROTO_GRE:
> > +		return data_len >=3D sizeof(struct gre_base_hdr);
> > +	case IPPROTO_NONE:
>=20
> 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 !=3D IPPROTO_TCP)
> > +		return true;
> > +
> > +	return priv->offset > doff || priv->offset + priv->len <=3D doff;
>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>=20
> maybe simply check priv->offset >=3D 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.