I can find no reason to allow packets with both SYN and FIN bits set past this point in the code. The packet should be dropped and not massaged as it is here.

[gnn]

Summary

Suggested by: eri
Sponsored by: Rubicon Communications (Netgate)

[wollman]

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.

[hiren]

In D2266#5, @wollman wrote:

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.

I couldn't find the spec/rfc saying that. Can you please share it here? Based on what we decide, we may have to fix code in tcp_input() too which we handle with drop_synfin sysctl.

[gnn]

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.

[gnn]

Also: http://www.juniper.net/techpubs/software/junos-es/junos-es93/junos-es-swconfig-security/tcp-headers-with-syn-and-fin-flags-set.html

[rwatson]

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

[allanjude]

In D2266#11, @rwatson wrote:

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

The PF fingerprinting you are thinking of is passive, not active (send a SYN+FIN and see if the host drops it), so should be unaffected by this change.

[gnn]

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?

[emaste]

Independent of the main stack change in D2267 it seems reasonable for firewalls to drop these packets.

[kp]

LGTM.

For what it's worth, OpenBSD don't drop here but henning did add a /* XXX why clear instead of drop? */ comment a few years ago.

[pstef]

This has been committed as 916e17fd5678.