fix: prevent invalid commands matches on 5 characters or less (932220 PL-2, 932230 PL-1, 932232 PL-3, 932235 PL-1, 932236 PL-2, 932237 PL-3, 932238 PL-3, 932239 PL-2, 932250 PL-1, 932260 PL-1) #3735
Conversation
… PL-1, 932235 PL-1, 932236 PL-2, 932237 PL-3, 932239 PL-2, 932250 PL-1, 932260 PL-1)
|
I'll need a couple of days before I can look at this PR in detail. |
Just as a note: |
Maybe using a word boundary instead? Like |
I've further simplified this by just using a word boundry and getting rid of the optional non-capturing group. The effect is pretty much the same, detection might be slightly better, but the generated regexes have reduced in size dramatically with this change (especially 932236). For some reason only using a word boundry for rule 932237 crashes Apache, I couldn't figure out the cause so I just added a workaround and comment. |
|
Thank you very much @EsadCetiner for this PR. I think the idea of looking for word boundaries is great. What I don't like is that we now need to add I have an idea for a different approach that I think makes more sense in the long run. I think we should simplify the command line processor in The result would look as follows (simplified): This still leaves This would result in the following (simplified): This would match In conclusion:
@EsadCetiner Please open a separate PR with the additions (I saw a couple |
|
I realise your PR solves an issue we have now and will prevent more issues from being filed. I propose to go ahead with your PR and implement my proposal later on. To complete your PR, @EsadCetiner, you'll have to also adapt |
Agreed, I had a general idea of something like this but I wasn't sure on how to approach this.
4.5.0 is coming out in 3 weeks or so, I'll try and address all of your feedback before then. I'd rather get it out of the way now so it's not forgotten.
These commands are already detected in the current release, but they stopped being detected once I prevented the invalid match. Shouldn't those additions be part of this PR so those commands are still covered, therefore avoiding some regressions?
I'm not entirely clear on what this does, I see what looks to be I assume you want me to modify the unix evasion suffix to this, then remove all references to unix: |
- [\s<>,&|)].*
+ \bI think your saying here:
To modify the affected regex assembly files to something like this, while keeping I hope this wasn't too convoluted, is this the general idea of what you want me to do? |
Yes.
Modify, yes. We still need
You can ignore that part. That's my proposal for the change of the processor. It has nothing to do with your PR. |
|
@theseion done, ready for review |
|
I've gone back to my original solution of resolving this false positive without a word boundry |
|
Do you think the |
|
I've checked over my work again this morning and I think this PR is ready to be merged if nobody else has objections. |
|
Ok. I think it makes sense but I think 5 might be too conservative. For example, Please also update the |
|
@theseion these look machine generated, is there another file I'm supposed to edit then compile the regex or should I just replace them completely? It's hard to read the regex and understand what it's trying to match. |
|
No, I created them manually. But you're right. I'll handle it. |
Match at most 10 consecutive characters
256d481 to
20ecdd8
Compare
|
I've updated the expressions @EsadCetiner, please take a look. The expressions mainly consist of the |
|
@theseion I think I have a better understanding of the regex now, the extra comments helped clear up some confusion. I've taken a look at your changes and found a regression with the I've pushed a fix, but the fix felt a little too easy, can you double check it? |
|
Duh. You're right. |
|
LGTM |
|
@EsadCetiner are you waiting for me to perform the merge? |
|
@theseion no, I was waiting for somebody else to review it since it's a big change. I'll merge in a few days if nobody has any objections. |
There have been many reports of false positives with the 932260 family of rules, most of them stem from invalid commands being matched (such as identity matching id). This pr solves most of these issues by preventing matches of invalid commands on 5 characters or less, I didn't prevent invalid matches for all commands to avoid the regex size exploding in size. I've modified the definition of
@from[\s<>&|)]to(?:[\s<>&|)]|$)and adding@to all commands with 5 characters or less. This results in either matching a command without arguments, or an command with an argument without matching permutations of a commands (For example, sudo matching sudoaaaa).Since this is a big change, I likely may have missed some commands that were intended to be matched (For example, ip matching iptables and ip6tables) but were shortened for performance or other reasons.
Some new attacks are detected with this PR, but some false negatives are introduced. The unix rules need a complete overhaul however this PR should be fine as an short to medium term fix.
Fixes false positives with english words:
identity
unique
time express
This should fix false positives with random data such as UUID, session cookies, tokens, and base64 data.
This might cause some new false positives because some commands are being matched with no arguments when they previously weren't, but this shouldn't be as bad as the false positives it fixes. I've excluded some commands to try and prevent the most common instances of these false positives (including some old ones, for example matching
id=1).closes: #3711
closes: #3631
closes: #3725
closes: #3812
closes: #3985