Re: pf: drop tcp packet when syn AND fin flags are set
On Mon, Mar 14, 2022 at 10:38:46AM +0100, Remi Locherer wrote: > 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 (DF) > > So pf behaves kind of similar to that. This matches what I read in tcp_input(). > But even with T/TCP or TFO, I don't a legit use of a TCP packet with > SYN and FIN set together. You can send a SYN, Data (e.g. HTTP Request) and FIN (EOF) in one packet. Our stack does not support this and ignores Data and FIN. > If we want to handle TFO then pf should probably inspect the TFO > option header and coocky. That's a different story. For now we should stay compatible and improve security. I am not sure what we should do to achieve 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) A misunderstanding. I wanted to see 192.168.201.21.20 > 192.168.201.29.22: S [tcp sum ok] 0:1000(1000) win 8192 (t tl 64, id 1, len 1040) 192.168.201.29.22 > 192.168.201.21.20: SF [tcp sum ok] 689392523:689392523(0) a ck 1 win 16384 (DF) (ttl 64, id 10777, len 44) I would expect the next ACK acknowledges the FIN. As our stack does not support this, it is a bit tricky to generate. When sending scpay packets the stack must not interfere. I solved a somehow simmilar problem in src/regress/sys/net/pf_state. > Did T/TCP specify the combination of SYN and FIN flags? 2.2 Transaction Sequences CLOSED LISTEN #1 SYN-SENT*--> --> CLOSE-WAIT* #2 TIME-WAIT <-- <-- LAST-ACK* > 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. I did not find SYN+FIN in TFO RFC. Probably they only think about HTTP connection keep-alive. But why not? FIN is just end of data. When you can send data, you can also append FIN in the same packet. > > Or should we strip data and FIN from both SYN packets to disable > > TFO? Don't know. Perhaps it is the other way around. Without TFO SYN+FIN looks stange. As T/TCP is obsolete, I guess it is not allowed in the generic initial SYN-Packet. But SYN+ACK+FIN may be different. > In the TCP stack or pf? The stack can do what it supports. pf should protect other stacks. We are talking about normalize TCP. There we can remove everything that looks strange and could confuse foreign TCP stacks. But we should not block things that are used in the wild. Maybe that was the idea to strip the FIN instead of dropping the packet. bluhm
Re: pf: drop tcp packet when syn AND fin flags are set
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 (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 (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 - 1.223
> > +++ pf_norm.c 13 Mar 2022 15:39:42 -
> > @@ -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)))
> >
>
Re: pf: drop tcp packet when syn AND fin flags are set
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.
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?
Maybe it should be
/* No transactional TCP */
if ((flags & (TH_ACK|TH_FIN)) == TH_FIN)
goto tcp_drop;
Or should we strip data and FIN from both SYN packets to disable
TFO?
bluhm
> 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 - 1.223
> +++ pf_norm.c 13 Mar 2022 15:39:42 -
> @@ -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)))
>
pf: drop tcp packet when syn AND fin flags are set
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?
Remi
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 - 1.223
+++ pf_norm.c 13 Mar 2022 15:39:42 -
@@ -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)))
