Skip to content

feat: added detection for quote evasion#3813

Open
Xhoenix wants to merge 38 commits into
coreruleset:mainfrom
Xhoenix:fix-quote-evasion
Open

feat: added detection for quote evasion#3813
Xhoenix wants to merge 38 commits into
coreruleset:mainfrom
Xhoenix:fix-quote-evasion

Conversation

@Xhoenix
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

fzipi commented Sep 13, 2024

@Xhoenix ☝️

@Xhoenix
Copy link
Copy Markdown
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
Copy Markdown
Member

fzipi commented Sep 13, 2024

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

@fzipi
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

Xhoenix commented Sep 16, 2024

Looking fwd to tonight’s meeting. 🙂

@fzipi
Copy link
Copy Markdown
Member

fzipi commented Sep 16, 2024

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

@Xhoenix
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Comment thread regex-assembly/932290.ra Outdated
@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 temporarily deployed to quantitative-testing-approval March 24, 2026 11:28 — with GitHub Actions Inactive
@Xhoenix Xhoenix deployed to quantitative-testing-approval March 24, 2026 11:33 — with GitHub Actions Active
@dkegel-fastly
Copy link
Copy Markdown

Would these be false positives?

''that's the joke''.jpeg
"circum"stances, eh? c;
"Ham"azing. Ah? Aaaaahhhh?
* O'O'O'O'Reilly ^O'^Reilly Autoparts **YEOOW**
&gt; O'keefe's
^^^it's_a_joke_please_don't_yell_at_me
All y'all stop touchin y'all's pee-pees :)
An O'Reiley's Autoparts store.
At least he got 'you're' right, I guess.
BLOW THA WHISTLE PRRR''PRR''RRR'RRRR
I'm 6'2'' 205~ and my bulk is 3300...idfk.
I've seen a lot of 'London's.
It's OK. I too liked Boston's 'Don't Look Back'.
It's'a me, mazzio, and I'ma gonna win!
M'danke m'm'eme, 4-Chanster!
M'madam, m'ma'am.
˝No, no, 'e's uh,...he's resting.˝
Quick! What's today's' winning lottery numbers?
Riding in the back of M'other's car
Ser'ra'i Malina - Cactuar server
Size: 8"x11"
That's two "O"s in goose.
Tis ne'er e'er o'er m'lo'er o' clo'er?
Toke'n'stroke
wouldn't it be 'u'r'?
Yaw swea'a's on backwads!
You notice the 'seller's profile'?
You should watch the N"if"L

@fzipi
Copy link
Copy Markdown
Member

fzipi commented Mar 24, 2026

@Xhoenix @theseion's suggestion was to add at PL3. We are all afraid of FPs here, with no enough testing.

I would NOT add this to an LTS, honestly.

@Xhoenix
Copy link
Copy Markdown
Member Author

Xhoenix commented Mar 24, 2026

Okay, PL 3 then.

@dkegel-fastly
Copy link
Copy Markdown

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.

@fzipi fzipi added ⚠️ do not merge Additional work or discussion is needed despite passing tests and removed v4 LTS Needed for v4 LTS labels Mar 24, 2026
@Xhoenix
Copy link
Copy Markdown
Member Author

Xhoenix commented Apr 4, 2026

@coreruleset/core-developers Any status updates?

@Xhoenix
Copy link
Copy Markdown
Member Author

Xhoenix commented May 3, 2026

Ping @coreruleset/core-developers
Already on PL 3, waiting for approval.

Copy link
Copy Markdown
Member

@EsadCetiner EsadCetiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Comment thread rules/REQUEST-932-APPLICATION-ATTACK-RCE.conf Outdated
@EsadCetiner
Copy link
Copy Markdown
Member

@Xhoenix Are you able to work through the last open issue and linter error?

@Xhoenix Xhoenix removed the ⚠️ do not merge Additional work or discussion is needed despite passing tests label May 25, 2026
@EsadCetiner
Copy link
Copy Markdown
Member

@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

@Xhoenix
Copy link
Copy Markdown
Member Author

Xhoenix commented May 26, 2026

Can you add some negative tests? @dkegel-fastly has a few examples here #3813 (comment)

Those should cause FPs, that's why this rule was moved to PL 3. Adding negative tests mean changing the rule entirely.

@Xhoenix
Copy link
Copy Markdown
Member Author

Xhoenix commented May 26, 2026

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

@EsadCetiner
Copy link
Copy Markdown
Member

@Xhoenix

Those should cause FPs, that's why this rule was moved to PL 3. Adding negative tests mean changing the rule entirely.

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.

@Xhoenix
Copy link
Copy Markdown
Member Author

Xhoenix commented May 26, 2026

@EsadCetiner Tests added.

@EsadCetiner
Copy link
Copy Markdown
Member

@Xhoenix Thanks, can you look at my last suggestion as well:

Also your current version of the rule doesn't catch payloads like 'curl' or "curl"

Should be good to go after that.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants