Created attachment 237962 [details] git diff for adding the new patch file pfctl -k <ip> never gets called, meaning the abusing IP can keep it up fast as long as the state is active. UDP, this is basically indefinitely as long as the keep the active. For TCP protocols such as HTTPS2 this means as long as the connection is up, they can continue launching attacks against the server as it is a multiplexed connection, meaning more than a single request can be made for a single TCP session. As it currently stands the current implementation relies on luck or the assumption that most people are not using it for UDP and for when it comes to TCP it is largely not being used for services with multiplexed connections. The patch allows the old insecure behaviour to be kept, but makes it secure by default. Old behavior can be kept via adding no_kick=true. This is documented in the config as well as explanation of what the choice means for either. Also while there, document how to set multiple protocols for the protocol setting.
Created attachment 237963 [details] diff for the package msg and the make file
Can you submit this as a phabricator review? Reason being is that the first patch file contains some spelling errors. The pkg-message addition assumes everyone uses pf, which is false. IT should be reworded to say, pf users using pf.conf must be aware that ... Reviewing the new pf.conf file: The phrase "This will ensure the ban actually takes affect" implies that the ban never takes effect. Is this true or is it that it will eventually take effect when the session is finally removed from the state table? Wouldn't this worded better as, "this will cause the ban to take immediate effect." Further on in the paragraph there are simple typos like "strenght" should be spelled "strength." I think this should be worded differently. The comments are as important as the code because the comments tell the user what lines can be uncommented and/or changed to suit the user's requirements. Anyhow. please submit a phabricator review. This way not only I can review this patch but others. An alternative would be to submit a pull request to https://github.com/fail2ban/fail2ban. They will do a proper review before incorporating this patch into their upstream repository. This probably makes more sense than a phabricator review. Another thing to keep in mind is avoid the word "you" or any other personal pronoun. The comments are not a personal letter but inline documentation to help the user to customize the action for their needs. Or you can fix the issues and resubmit the patches. The code itself looks good though. Thinking about this further, the proper plan of action should be to submit a pull request to the upstream fail2ban. Work with them to have the patch incorporated into the upstream repo. Ping me with the URL of the pull request or put it here. I will join in the discussion with the fail2ban developers. Regarding the FreeBSD Makefile and pkg-message changes, I can work with you on those in parallel with your submitting the pull request to the fail2ban development team. Does this sound good to you?
I see you already have submitted a pull request. Let's discuss the patch there.
(In reply to Cy Schubert from comment #2) No they will not. The main dev for Fail2ban has shut down any attempt to fix this bug going on 5 years now. They have been aware and have literally made a point of not even documenting it or offering a secure on by default with a optional insecure one for those who choose to use it. Honestly was a bit livid when their first reply back to me about this was this as a known issue they had not cared to fix. Even a bit more so when they danced around attempting to justify ignoring the issue and keeping it insecure by default. Basically the Fail2ban's main dev's take on it is is safer to risk data exfiltration, a DDOS, or some sort of other compromise than the occasional inconvenience of a innocent connection being forced to re-establish. And yes, that is true. The ban will not meaningfully take affect. A attacker can keep a UDP state active indefinitely by just keeping the attack up. Sure it will eventually take affect, but the question is how long. For all effective purposes it is not meaningfully taking affect. The big question is how to get this fixed because as is this is a security bug with very notable implications.
(In reply to Cy Schubert from comment #3) Sure. I'm game. Even though their reply was basically telling me to screw off as they've known about it for 5 years now and don't want to fix it.
BTW sorry about the typos there. Thanks for the tips as well and though on that a bit of the wording there. I was reading that as not assuming one is using pf as it was right after a bit also about pf. :) But yeah, if you want to chime in on the github one that would be awesome. :) Basically if you have some idea how to bring some sort of sanity to that conversation on Github, it would be most welcome. My though on requiring "no_kick" is it is like how some commands require adding additional args to confirm you are really about to do something that may very well be a bad idea.
Added my comments to the upstream pull request review. Please review them there or we can discuss using phabricator. Otherwise I have no interest in committing either patch without the requested changes. I have no problem with the code itself but the comments paint a picture that pf is buggy where in fact you have come across what is naturally in stateful firewall behavior because the state table is referenced before rules are searched. Not to mention that stateless TCP rules inspect only the ACK bit. Your comments misrepresent the problem, painting a false narrative of what and why works the way it does. We can either review this on github or in phabricator where each line can be reviewed and fixed. Or you can resubmit the patches after each review, which will invariably miss something. This will be painful for you and frustrating for me, resulting in no update to the port. As it stands the proposed patches will not be committed without the requested changes.
(In reply to Cy Schubert from comment #7) Ohh, completely understood and thanks for the pointers. :) Also thanks! BTW if you don't mind me saying, mind if I ask what made it sound like pf was buggy? I find that take from it interesting as I was going for how it is being used here is buggy with very real dangers from not cleaning up the state table post change.
It's the choice of words. Like my other half says, with her degree in linguistics, words matter. They do. We're not talking about a weakness (flaw) but a limitation. A logical and natural limitation because the state table is searched before rules are matched. This is the same limitation that ipfilter has. Iptables, firewalld, npf, pf, ipfw, and every commercial firewall I've worked on has this same limitation. It's a logical limitation that pf thankfully has a way of removing entries from the state table that all the others (except for maybe NetBSD's npf) do not.
(In reply to Cy Schubert from comment #9) Once again thanks for being sane and easy to work with. PF is fine, just the issue is how it is being used in this instance. That said notably updated version there and I look forward to hearing your feed back on it. But aye, they are important and I think how me and you are reading it raises a interesting difference how context will be parsed, in this case if the issue is PF v. how PF is being used. =^.^=
(In reply to Zane C. Bowers-Hadley from comment #10) If you're suggesting that it be committed as is, the answer is no, it will not be committed as is. If you're suggesting I reword it, the answer is also no, I do not have the time for this. Please make the suggested changes.
(In reply to Cy Schubert from comment #11) I'm talking about the bit on github. I've migrated that to here yet and the requested changes were made there. I also never suggested you re-word anything. I was just pointing out a interesting quick in the difference of the parsing, in regards to where the emphasis is being read as being put. :) Sorry if I offtended. Was just trying to be pleasant and conversational. It struck me as interesting.
That said, I'll get the stage bit of the diff updated. I just though I would ask here, given you mentioned the github bit. Honestly sort of avoiding there currently as I don't want to talk with the guy again or honestly really deal with him give just how emotionally exhuasting it has been. If your fine with dealing with here instead, I am perfectly happy to.