Bug 229477
| Summary: | [PATCH] fail-policy changes cause delays on synproxy packets | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Andre Albsmeier <mail> | ||||
| Component: | kern | Assignee: | freebsd-pf (Nobody) <pf> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Some People | CC: | cy, kp, vegeta | ||||
| Priority: | --- | Keywords: | patch | ||||
| Version: | 11.2-STABLE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
Just to add that I am referring to https://svnweb.freebsd.org/base?view=revision&sortby=date&revision=335798 Can you describe your setup and what the problem looks like exactly? I've not looked at this in any detail yet, but I hope to be able to write a test case so we won't accidentally break things again. That's easy (this is just a cut-down excerpt of my real rules I used on a test machine to address this bug):
set ruleset-optimization none
set block-policy return
set skip on lo0
set debug misc
set timeout tcp.established 432000
set limit { states 2000, src-nodes 1000, frags 2000, table-entries 30000 }
scrub in on e0 all fragment reassemble
scrub out on e0 all random-id set-tos 0xB8
scrub on e0 all reassemble tcp
pass out quick on e0 all no state allow-opts
pass in quick on e0 proto tcp from any to any port 1234 synproxy state
pass in quick on e0 all no state
Now run some "nc -l DEST 1234" on host DEST and connect to 1234 with and without the synproxy rule...
You can probably just change the 'action != PF_PASS' into 'action == PF_DROP' rather than adding an extra if statement. I don't think so but maybe I am just interpreting your idea badly. How about a code snippet or patch? (In reply to Andre Albsmeier from comment #5) No, I've checked again, you're right. I forgot about the 'return (action)'. I think your patch is right. I'm still looking at writing an automated test case, but hopefully I'll be able to commit both test and fix soon. OK, but as I said: Regarding #2 I am unsure. We can also leave it out and wait if someone who actually uses "fail-policy return" complains.... I am the person responsible for fail-policy and I can test this behaviour with fix for synproxy. I'm sorry for not testing it with synproxy initially, but synproxy is broken for route-to rules anyway. I'll get back to you soon. I ran some tests and the patch seems correct.
If I understand correctly, my patch prevented "return(action)" to be called for pf_create_state returning with synproxy and this one restores this behaviour while still allowing pf_return for really failed rules.
Unfortunately I found out that fail-policy does not really work for rdr rules, probably because they are not really normal rules with rule number and so on, even if they create a state ("rdr pass"). I assume fixing that should be my own job in another bug report.
A commit references this bug: Author: kp Date: Sat Jul 14 10:14:59 UTC 2018 New revision: 336275 URL: https://svnweb.freebsd.org/changeset/base/336275 Log: pf: Fix synproxy Synproxy was accidentally broken by r335569. The 'return (action)' must be executed for every non-PF_PASS result, but the error packet (TCP RST or ICMP error) should only be sent if the packet was dropped (i.e. PF_DROP) and the return flag is set. PR: 229477 Submitted by: Andre Albsmeier <mail AT fbsd.e4m.org> MFC after: 1 week Changes: head/sys/netpfil/pf/pf.c A commit references this bug: Author: kp Date: Sat Jul 21 07:00:22 UTC 2018 New revision: 336575 URL: https://svnweb.freebsd.org/changeset/base/336575 Log: MFC r336275: pf: Fix synproxy Synproxy was accidentally broken by r335569. The 'return (action)' must be executed for every non-PF_PASS result, but the error packet (TCP RST or ICMP error) should only be sent if the packet was dropped (i.e. PF_DROP) and the return flag is set. PR: 229477 Submitted by: Andre Albsmeier <mail AT fbsd.e4m.org> Changes: _U stable/11/ stable/11/sys/netpfil/pf/pf.c |
Created attachment 194842 [details] Patch to bring back synproxy functionality I was wondering why synproxy rules performed so badly after the recent pf changes implementing the fail-policy. Please have a look at my patch: 1. Shouldn't the return(action) statement be performed ALWAYS if action != PF_PASS? 2. If I understand pf_return() correctly, a RST is sent in case of TCP connections. But it's probably not OK to send a RST if "action" came back with PF_SYNPROXY_DROP. Is it OK to catch this with my additional "action == PF_DROP" condition or do we need something more sophisticated? While I am sure about 1., I am unsure about 2. (I just had a quick look at pf_return() so maybe I missed something).