feat: added detection for quote evasion#3813
Conversation
|
This should already be handled by the existing rules. The existing rules require a prefix to reduce false positives, e.g., |
|
@Xhoenix ☝️ |
|
This is a bypass without the need of a prefix, something like 932230 at PL 1, i.e Direct command injection. |
|
This is probably very prone to false positives al PL1, right? |
|
I mean, we are very careful trying to get only some words (that's why we have lists of words). The regular expression you created there it is only going to give problems for a generic match. So we need to do better or everyone will disable this rule. 🤷 |
|
Yes, I was thinking PL2 would be more appropriate, as some other common bypasses are at PL2. So, should I move this to PL2? |
|
@Xhoenix I think this needs to be part of a more general discussion. Currently, we avoid generic matching rules for RCE on purpose, to prevent FPs. We have these rules for all PL levels, so the example given above will also work at PL2 for the list of words we check for at that level. Even with the prefix checks we experience high numbers of FPs for some strings, so if we wanted to start going prefix-less, that would have to be a group discussion, best based on metrics. If you can prove that this rule will not create more FPs (which I doubt you can because we don't even know the rate for the current rules), then we could certainly adopt it. |
|
I was thinking about rule 932240 at PL2, which I think is way more prone to FPs(e.g |
|
Common sense is not tests. Tests are proofs. Let's see what we can do, but without numbers it is very difficult to make an informed decision... |
|
Looking fwd to tonight’s meeting. 🙂 |
|
Do you have numbers for the discussion? Otherwise, how are you going to support it? |
|
I think this PR is perfect for PL 2, without any modifications. Expecting people to make typos like "no space after quotes" is just not practical in the sense the rule is at PL 2 and targets ARGS. So, LGTM. |
|
QQ: what do we think ARGS has? Because I see possible a lot of text coming to that variable, and that is how you get your FPs. |
|
Then, as per what I wrote above, rule 932240 shouldn't exist either. It's atleast twice as probable to cause FPs than this simple rule. You can check the regex-assembly file 932240, which lists the payloads in details. |
Co-authored-by: azurit <jozef@sudolsky.sk>
|
Would these be false positives? |
|
Okay, PL 3 then. |
|
If it were up to me, I'd say defer this until after the LTS release, and look into the likely false positives I posted. |
|
@coreruleset/core-developers Any status updates? |
|
Ping @coreruleset/core-developers |
EsadCetiner
left a comment
There was a problem hiding this comment.
Can you add some negative tests? @dkegel-fastly has a few examples here #3813 (comment)
Also your current version of the rule doesn't catch payloads like 'curl' or "curl"
|
@Xhoenix Are you able to work through the last open issue and linter error? |
|
@Xhoenix Can you address the feedback I gave you in this comment: #3813 (review) I can't approve this PR as there still are outstanding issues |
Those should cause FPs, that's why this rule was moved to PL 3. Adding negative tests mean changing the rule entirely. |
|
Although not mandatory, general English writing convention is to quote a single quote inside double quotes and vice versa. Some of those FPs cannot be avoided that's why there are rule exclusions and this rule is on PL3. 🙂 |
You don't need to change the rule, just add those tests so the false positives are at least documented, and maybe we can fix them in the future. |
|
@EsadCetiner Tests added. |
|
@Xhoenix Thanks, can you look at my last suggestion as well:
Should be good to go after that. |
Added detection for shell quote evasion.