Suggested by: eri
Sponsored by: Rubicon Communications (Netgate)
There's nothing actually wrong with a SYN+FIN packet; the TCP spec unambiguously defines how it's supposed to be handled, and the code does it correctly as is (by turning off the FIN bit), which certainly seems to be within the spirit of what "scrub" is supposed to do. I'm not sure I see what the problem this is trying to solve is.
A change like this addresses Postel's curse. While once upon a time the admonition to "be conservative in what you do, be liberal in what you accept from others." made sense that time is long past. A SYN/FIN packet has no use and, as such should not be accepted, it should be dropped.
SYN|FIN packets are often used for network-stack fingerprinting. If the policy goal is to prevent fingerprinting, then dropping SYN|FIN makes sense. However, fingerprinting is sometimes useful (e.g., PF actually supports using fingerprinting to control end-host policy in a useful way, if I recall) and so while having it enabled when (for example) normalising packets might be OK, but I think you wouldn't want the TCP stack to do this itself by default (subject of another patch).
For this case (as opposed to the one in the main stack under review as 2267) I think it really makes the most sense to drop early. PF is a firewall, why should it handle a packet that it clearly in error?