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 -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)))
>
