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