On Mon, Mar 14, 2022 at 01:27:14AM +0100, Alexander Bluhm wrote:
> On Sun, Mar 13, 2022 at 11:24:33PM +0100, Remi Locherer wrote:
> > Hi,
> >
> > When pf processes a TCP packet with SYN and FIN flags set, it removes
> > the FIN flag and continuous processing it. I propose we change that and
> > let pf drop such a packet. I don't see any legit use for combining these
> > two flags in the same packet.
> >
> > Henning added this comment 7 years ago:
> > XXX why clear instead of drop?
> >
> > Damjan Dimitrov approached me with this. He got a request that his firewall
> > should drop TCP packets with SYN and FIN flags set. But with pf this can
> > currently not be done because the FIN flag is cleared before rule
> > processing.
> >
> > I tested the behaviour with scapy:
> > send(IP(dst="172.24.217.34")/TCP(dport=23,flags="SF"))
> >
> > Opinions? OKs?
>
> RFC 1644 TCP Extensions for Transactions (T/TCP) allows it
> RFC 6247 declares T/TCP historic due to security issues
> RFC 7413 TCP Fast Open (TFO) might reintroduce it
>
> The intension of the clear FIN in pf might be to convert T/TCP into
> regular TCP. But then the data should also be scrubbed. Our stack
> ignores SYN+data and SYN+FIN. I think it puts such a connection
> attempt into the syn-cache. Of course without data and FIN to avoid
> DoS.
This is how OpenBSD responds with pf disabled:
192.168.201.21.20 > 192.168.201.29.22: SF 0:0(0) win 8192
192.168.201.29.22 > 192.168.201.21.20: S 2641340782:2641340782(0) ack 1 win
16384 <mss 1460> (DF)
So pf behaves kind of similar to that.
But even with T/TCP or TFO, I don't a legit use of a TCP packet with
SYN and FIN set together.
If we want to handle TFO then pf should probably inspect the TFO
option header and coocky.
> What about SYN+ACK+data+FIN ? When we ack this, the 3-way handhake
> is complete. I don't see why we should not allow it. Could you
> disable pf and see if our TCP stack can handle this?
With pf disabled:
192.168.201.21.20 > 192.168.201.29.22: SF [tcp sum ok] 0:1000(1000) ack 0 win
8192 (ttl 64, id 1, len 1040)
192.168.201.29.22 > 192.168.201.21.20: R [tcp sum ok] 0:0(0) win 0 (DF) (ttl
64, id 55619, len 40)
The same but without initial ACK and FIN:
192.168.201.21.20 > 192.168.201.29.22: S [tcp sum ok] 0:1000(1000) win 8192
(ttl 64, id 1, len 1040)
192.168.201.29.22 > 192.168.201.21.20: S [tcp sum ok] 689392523:689392523(0)
ack 1 win 16384 <mss 1460> (DF) (ttl 64, id 10777, len 44)
>
> Maybe it should be
>
> /* No transactional TCP */
> if ((flags & (TH_ACK|TH_FIN)) == TH_FIN)
> goto tcp_drop;
>
Did T/TCP specify the combination of SYN and FIN flags?
With TFO a client can send a cookie and data together with the SYN.
But a FIN flag? I did not find a hint for that in the RFC.
> Or should we strip data and FIN from both SYN packets to disable
> TFO?
In the TCP stack or pf? An OpenBSD router with activated pf might be used to
protect hosts with support for TFO. So pf should probably not strip data and
the TFO cookie from SYN packets. But this does not imply that the TCP stack
has to support TFO IMHO.
> > Index: pf_norm.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/pf_norm.c,v
> > retrieving revision 1.223
> > diff -u -p -r1.223 pf_norm.c
> > --- pf_norm.c 10 Mar 2021 10:21:48 -0000 1.223
> > +++ pf_norm.c 13 Mar 2022 15:39:42 -0000
> > @@ -1117,8 +1117,9 @@ pf_normalize_tcp(struct pf_pdesc *pd)
> > if (flags & TH_RST)
> > goto tcp_drop;
> >
> > - if (flags & TH_FIN) /* XXX why clear instead of drop? */
> > - flags &= ~TH_FIN;
> > + /* Illegal packet */
> > + if (flags & TH_FIN)
> > + goto tcp_drop;
> > } else {
> > /* Illegal packet */
> > if (!(flags & (TH_ACK|TH_RST)))
> >
>
