Page MenuHomeFreeBSD

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

Authored by gnn on Apr 9 2015, 6:38 PM.

Details

Reviewers
kristof
Group Reviewers
network
Summary

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

gnn retitled this revision from to 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..Apr 9 2015, 6:38 PM
gnn updated this object.
gnn updated this revision to Diff 4762.
gnn added a reviewer: network.Apr 9 2015, 6:39 PM
gnn removed a subscriber: imp.

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 added a subscriber: hiren.Apr 10 2015, 4:24 AM
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 added a comment.Apr 10 2015, 12:27 PM

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

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 added a comment.Apr 12 2015, 5:22 PM

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 added a subscriber: emaste.Nov 15 2018, 8:43 PM

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

kristof accepted this revision.Nov 15 2018, 8:54 PM
kristof added a subscriber: kristof.

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.

This revision is now accepted and ready to land.Nov 15 2018, 8:54 PM