Re: NPF: broken checksums

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 04/04/2018 à 09:42, Maxime Villard a écrit :
> Le 04/04/2018 à 09:30, Michael van Elst a écrit :
> > max%m00nbsd.net@localhost (Maxime Villard) writes:
> >
> > > It's not correct; when we call npf_fetch_tcpopts to only read the TCP options,
> > > we shouldn't modify the packet. Otherwise the TCP checksum becomes invalid
> > > (we're not recomputing it), and the AH signature too (if any).
> >
> > Please also look at where packets are filtered.
> >
> > On input, the checksum is already validated and trashing it afterwards
> > has little effect.
>
> Mmh no, the TCP checksum is validated in tcp_input(), which is *after* NPF
> gets invoked. So modifying the packet does have an effect. Perhaps you are
> mixing IP checksum and TCP checksum? The IP one is indeed validated before
> NPF.
>
> > On output, pfil_run_hooks is called after the TCP header (without opts!)
> > checksum has been calculated and before the remaining partial checksum
> > (with header opts) is added. Adjusting the checksum for changes in
> > the TCP options would make the final checksum invalid.
> >
> > On forwarding, the most likely case, it's filtered on input and output,
> > so the filter rules are matched once for PFIL_IN, once for PFIL_OUT.
> > PFIL_IN is then ignored because of nbuf_cksum_barrier(), but PFIL_OUT isn't.
>
> Not sure what your point is. In all cases the check should be
> !nbuf_cksum_barrier, no?

So I ended up committing my patch.

My patch makes sense to me, and with it in place, the packets aren't dropped
anymore.

If somehow the original code was intentional and functional, then please
explain how and why. As I said, my tests showed that the packets were
systematically dropped, so it didn't work.

Now, a different issue; in the course of testing the implementation (with
crafted packets), I came across another problem that appears to be related to
checksumming and max-mss clamping.

Let's say you put in the packet a MAXSEG option of 30000, and that your NPF
configuration allows only an MSS of 15000. You send a packet with:

	nptr[0] = TCPOPT_MAXSEG
	nptr[1] = TCPOLEN_MAXSEG
	nptr[2,3] = the maxseg option = 30000

Here everything is fine, NPF will change 30000->15000, will recompute the
checksum, and the kernel won't drop the packet.

But if you send a packet with

	nptr[0] = TCPOPT_NOP
	nptr[1] = TCPOPT_MAXSEG
	nptr[2] = TCPOLEN_MAXSEG
	nptr[3,4] = the maxseg option = 30000

The checksum NPF computes becomes incorrect, and the packet is dropped in
tcp_input.

And then again, if you send a packet with

	nptr[0] = TCPOPT_NOP
	nptr[1] = TCPOPT_NOP
	nptr[2] = TCPOPT_MAXSEG
	nptr[3] = TCPOLEN_MAXSEG
	nptr[4,5] = the maxseg option = 30000

Everything works correctly, back to normal.

It looks like there is a 16byte alignment problem somewhere in NPF. RFC793
says that "An option may begin on any octet boundary", so NPF should handle
that.

Maxime