8000 feat: added detection for quote evasion by Xhoenix · Pull Request #3813 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: added detection for quote evasion #3813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Xhoenix
Copy link
Member
@Xhoenix Xhoenix commented Sep 6, 2024

Added detection for shell quote evasion.

@Xhoenix Xhoenix changed the title fix: added detection for quote evasion fix(security): added detection for quote evasion Sep 6, 2024
@Xhoenix Xhoenix requested a review from a team September 6, 2024 08:43
@theseion
Copy link
Contributor
theseion commented Sep 9, 2024

This should already be handled by the existing rules. The existing rules require a prefix to reduce false positives, e.g., ;. Example (;'g'cc ):

curl -H "x-format-output: txt-matched-rules" "https://sandbox.coreruleset.org/?bla=%3B%27g%27cc%20"

@fzipi
Copy link
Member
fzipi commented Sep 13, 2024

@Xhoenix ☝️

@Xhoenix
Copy link
Member Author
Xhoenix commented Sep 13, 2024

This is a bypass without the need of a prefix, something like 932230 at PL 1, i.e Direct command injection.

@fzipi
Copy link
Member
fzipi commented Sep 13, 2024

This is probably very prone to false positives al PL1, right?

@fzipi
Copy link
Member
fzipi commented Sep 13, 2024

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

@Xhoenix
Copy link
Member Author
Xhoenix commented Sep 14, 2024

Yes, I was thinking PL2 would be more appropriate, as some other common bypasses are at PL2. So, should I move this to PL2?

@theseion
Copy link
Contributor

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

@Xhoenix
Copy link
Member Author
Xhoenix commented Sep 14, 2024

I was thinking about rule 932240 at PL2, which I think is way more prone to FPs(e.g $$ or $0) than my rule. This is just common sense.

@fzipi
Copy link
Member
fzipi commented Sep 14, 2024

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

@Xhoenix
Copy link
Member Author
Xhoenix commented 8000 Sep 16, 2024

Looking fwd to tonight’s meeting. 🙂

@fzipi
Copy link
Member
fzipi commented Sep 16, 2024

Do you have numbers for the discussion? Otherwise, how are you going to support it?

@Xhoenix
Copy link
Member Author
Xhoenix commented Sep 18, 2024

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.

@fzipi
Copy link
Member
fzipi commented Sep 18, 2024

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.

@Xhoenix
Copy link
Member Author
Xhoenix commented Sep 19, 2024

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.

@Xhoenix Xhoenix changed the title fix(security): added detection for quote evasion feat: added detection for quote evasion Sep 19, 2024
@fzipi fzipi added the ⚠️ do not merge Additional work or discussion is needed despite passing tests label Sep 19, 2024
Xhoenix and others added 2 commits September 19, 2024 18:33
Co-authored-by: azurit <jozef@sudolsky.sk>
@Xhoenix Xhoenix added ⚠️ do not merge Additional work or discussion is needed despite passing tests and removed ⚠️ do not merge Additional work or discussion is needed despite passing tests labels Sep 21, 2024
@Xhoenix Xhoenix changed the title feat(security): added detection for quote evasion feat: added detection for quote evasion Oct 7, 2024
@fzipi
Copy link
Member
fzipi commented Oct 7, 2024

Sadly we didn't had any response yet on this one. I'll try to include more people next.

Copy link
Contributor
github-actions bot commented Jan 5, 2025

📊 Quantitative test results for language: eng, year: 2023, size: 10K, paranoia level: 1:
🚀 Quantitative testing did not detect new false positives

@Xhoenix
Copy link
Member Author
Xhoenix commented Jan 14, 2025

Is there any updates on the test? Some positive results will get this PR merged. 🙂

@Xhoenix Xhoenix requested a review from a team January 22, 2025 10:16
@theseion
Copy link
Contributor

We're talking to them. But communication is always slow in these instances.

@Xhoenix
Copy link
Member Author
Xhoenix commented Mar 28, 2025

Are we there yet? 😂

@Xhoenix Xhoenix added the release:new-detection In this PR we introduce a new detection label Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:new-detection In this PR we introduce a new detection ⚠️ do not merge Additional work or discussion is needed despite passing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0