8000 fix: Added t:urlDecodeUni for REQUEST_URI / REQUEST_BASENAME checks in phase 1 (921240 PL1, 920440 PL1, 920201 PL2, 920202 PL4) by dune73 · Pull Request #3411 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: Added t:urlDecodeUni for REQUEST_URI / REQUEST_BASENAME checks in phase 1 (921240 PL1, 920440 PL1, 920201 PL2, 920202 PL4) #3411

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 22, 2024
Merged

fix: Added t:urlDecodeUni for REQUEST_URI / REQUEST_BASENAME checks in phase 1 (921240 PL1, 920440 PL1, 920201 PL2, 920202 PL4) #3411

merged 1 commit into from
Jan 22, 2024

Conversation

dune73
Copy link
Member
@dune73 dune73 commented Dec 12, 2023

The problem

This applies to variables for REQUEST_URI, REQUEST_BASENAME and REQUEST_FILENAME.

ModSec3 does an implicit URL decoding on phase 1 and the same on phase 2.

ModSec2 does no implicit URL decoding on phase 1, but it does it on phase 2.

This allows an attacker to URL encode a URI and bypass detection on phase 1 rules on ModSec2.

Example: 920440 PL1 should detect this: /dump%2Erdb as an encoded version of dump.rdb.
However it fails do to so in the v4 dev tree.

It used to work up to CRS v3.3, because rule 920440 used to run in phase 2, but when we introduced early blocking for CRS v4, we shifted 920440 and several other rules from phase 2 to phase 1 and the implicit decoding of the payload (see above) was removed for ModSec 2 and we no longer detected the attack.

Unfortunately, this behavior difference did not surface, since we apparently did not have any test for URL encoded payloads where it mattered.

The Solution

This is a behavior difference that would ideally be fixed in the engine, but it is not quite clear what the right behavior would be given the difference between ModSec2 and ModSec3 and chances are waiting for a fix could delay CRS v4.

So this PR adds the t:urlDecodeUni transformation to the rules inspecting REQUEST_URI and REQUEST_BASENAME in phase 1. None of the rules targetting REQUEST_FILENAME is affected, though.

Implementation

I added the t:urlDecodeUni transformation following other rules where this is already done. For the 4 rules affected, I also did a new test with an encoded payload that is now detected on ModSec2.

When thinking about URL encoding, namely of the URI, it makes sense to also look into 920220, which I cover in a separate PR (#3410). This rule looks for invalid URL encoding. The rule is working on REQUEST_URI, but think this is wrong. The PR mentioned shifts the target to REQUEST_URI_RAW.

Once CRSv4 is out, we may want to look into getting the engines in sync again and looking into systematic coverage of these encoding issues while also addressing double-encoding (an area where we have spotty coverage now).

But for the time being, I think this has got us covered for CRS v4.

I'm adding the do not merge label, so we can discuss this or alternative approaches to this problem in the next CRS chat.

List of rules carrying any of the targets named

Rules with target REQUEST_URI

920260 PL1      not affected (phase 2)
920270 PL1      not affected (phase 2)
920450 PL1      not affected (phase 2)
920271 PL2      not affected (phase 2)
920272 PL3      not affected (phase 2)
920460 PL4      not affected (phase 2)
921240 PL1      affected, fixed, new test 921240-2
930100 PL1      not affected (phase 2)
930110 PL1      not affected (phase 2)
933131 PL3      not affected (phase 2)

Rules with target REQUEST_BASENAME
920440 PL1      affected, fixed, new test 920440-6
920200 PL2      not affected (REQUEST_BASENAME is only used to ignore .pdf)
920201 PL2      affected, fixed, new test 920201-2
920202 PL4      affected, fixed, new test 920202-2
942101 PL2      not affected (t:urlDecodeUni already applied)
942160 PL1      not affected (t:urlDecodeUni already applied)

Rules with target REQUEST_FILENAME
920250 PL1      not affected (phase 2)
920500 PL1      not affected (t:urlDecodeUni already applied) 
921190 PL1      not affected (t:urlDecodeUni already applied)
930130 PL1      not affected (t:urlDecodeUni already applied)
931131 PL1      not affected (t:urlDecodeUni already applied)
933150 PL1      not affected (phase 2)
933160 PL1      not affected (phase 2)
933180 PL1      not affected (phase 2)
933210 PL1      not affected (phase 2)
933151 PL2      not affected (phase 2)
933161 PL3      not affected (phase 2)
933211 PL3      not affected (phase 2)
934100 PL1      not affected (phase 2)
934110 PL1      not affected (phase 2)
934160 PL1      not affected (phase 2)
934170 PL1      not affected (phase 2)
934101 PL2      not affected (phase 2)
934120 PL2      not affected (phase 2)
941010 ADMIN    not affected (rules does not apply when it encounters % in URI)
941110 PL1      not affected (phase 2)
941130 PL1      not affected (phase 2)
941140 PL1      not affected (phase 2)
941160 PL1      not affected (phase 2)
941170 PL1      not affected (phase 2)
941180 PL1      not affected (phase 2)
941190 PL1      not affected (phase 2)
941200 PL1      not affected (phase 2)
941210 PL1      not affected (phase 2)
941220 PL1      not affected (phase 2)
941230 PL1      not affected (phase 2)
941240 PL1      not affected (phase 2)
941250 PL1      not affected (phase 2)
941260 PL1      not affected (phase 2)
941270 PL1      not affected (phase 2)
941280 PL1      not affected (phase 2)
941290 PL1      not affected (phase 2)
941300 PL1      not affected (phase 2)
941310 PL1      not affected (phase 2)
941350 PL1      not affected (phase 2)
941360 PL1      not affected (phase 2)
941370 PL1      not affected (phase 2)
941390 PL1      not affected (phase 2)
941400 PL1      not affected (phase 2)
941101 PL2      not affected (t:urlDecodeUni already applied)
941120 PL2      not affected (phase 2)
941150 PL2      not affected (phase 2)
941181 PL2      not affected (phase 2)
941320 PL2      not affected (phase 2)
941330 PL2      not affected (phase 2)
941340 PL2      not affected (phase 2)
941380 PL2      not affected (phase 2)
942550 PL1      not affected (phase 2)
942110 PL2      not affected (phase 2)
942120 PL2      not affected (phase 2)
942101 PL2      not affected (t:urlDecodeUni already applied)
944130 PL2      not affected (phase 2)

@dune73 dune73 added the ⚠️ do not merge Additional work or discussion is needed despite passing tests label Dec 12, 2023
theMiddleBlue
theMiddleBlue approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor
@theMiddleBlue theMiddleBlue left a comment

Choose a reason for hiding this comment

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

LGTM

@dune73
Copy link
Member Author
dune73 commented Dec 12, 2023

Thank you for your review, @theMiddleBlue.

@airween
Copy link
Contributor
airween commented Dec 12, 2023

Note, that you can find a script here which helps to find the affected rules.

This script iterates through rules and print the rule's id if the rule:

  • uses any of variable which listed in line 39
  • has any of transformation which listed in line 51
  • the transformation belongs that variable (eg. in case of chained rule the tfn could belong the next rule)
  • rule is on phase:1

We should inspect all identified rules, because not all of them are affected. Eg. 920200 is not affected because the second rule uses a negated condition.

@dune73 dune73 mentioned this pull request Dec 18, 2023
Closed
@dune73 dune73 changed the title fix: Added t:urlDecodeUni for REQUEST_URI / REQUEST_BASENAME checks in phase 1 (921240 PL1, 920440 PL1, 920201 PL2, 920202 PL4) (Christian Folini) fix: Added t:urlDecodeUni for REQUEST_URI / REQUEST_BASENAME checks in phase 1 (921240 PL1, 920440 PL1, 920201 PL2, 920202 PL4) Dec 18, 2023
@dune73 dune73 mentioned this pull request Jan 8, 2024
Closed
@dune73 dune73 removed the ⚠️ do not merge Additional work or discussion is needed despite passing tests label Jan 17, 2024
@airween airween merged commit d68277b into v4.0/dev Jan 22, 2024
@github-actions github-actions bot mentioned this pull request Jan 23, 2024
Merged
@dune73 dune73 mentioned this pull request Feb 5, 2024
Closed
@fzipi fzipi deleted the urlDecode-phase-1 branch February 14, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theMiddleBlue theMiddleBlue theMiddleBlue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dune73 @airween @theMiddleBlue
0