Skip to content
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

Fix vulnerable regexp in rule 932140 #1355

Open
wants to merge 3 commits into
base: v3.2/dev
from

Conversation

Projects
None yet
4 participants
@s0md3v
Copy link

commented Apr 15, 2019

(?:/[dflr].*)* can be changed to (?:/[dflr].*) because .* at the end of the sub-pattern will match the repetition of this sub-pattern anyway.

Mitigate ReDOS vulnerablity (Fixes #135)
`(?:/[dflr].*)*` can be changed to `(?:/[dflr].*)` because `.*` at the end of the sub-pattern will match the repetition of this sub-pattern anyway.
@theMiddleBlue

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

refer to #1354

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Have you actually reproduced this issue with modsecurity?

@spartantri
Copy link
Collaborator

left a comment

The "fix" don't match common for loops, I made some slight changes and got it working with "@rx \b(?:if(?:/i)?(?: not)?(?: exist\b| defined\b| errorlevel\b| cmdextversion\b|(?: |\().*(?:\bgeq\b|\bequ\b|\bneq\b|\bleq\b|\bgtr\b|\blss\b|==))|for\s?(?:/[dflr])?.* %+[\S]+ in\s?\(.*\)\s?do)"
Sample FOR internal command syntax:

 syntax-FOR-Files
       FOR %%parameter IN (set) DO command 
   
 syntax-FOR-Files-Rooted at Path   
       FOR /R [[drive:]path] %%parameter IN (set) DO command 
   
 syntax-FOR-Folders
       FOR /D %%parameter IN (folder_set) DO command 
   
 syntax-FOR-List of numbers   
       FOR /L %%parameter IN (start,step,end) DO command 
   
 syntax-FOR-File contents   
       FOR /F ["options"] %%parameter IN (filenameset) DO command 
   
       FOR /F ["options"] %%parameter IN ("Text string to process") DO command
   
 syntax-FOR-Command Results 
       FOR /F ["options"] %%parameter IN ('command to process') DO command

s0md3v added some commits Apr 24, 2019

@s0md3v

This comment has been minimized.

Copy link
Author

commented Apr 24, 2019

It should work now.

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

Can you explain why is the .*? needed? I believe changing the * to ? should suffice.

@spartantri

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

Hi @fgsch @theMiddleBlue @franbuehler I think you like better regex, can you please take a look/test, apparently this needs fixing for other use cases, as it is it won't match for loops and some if it is missing spaces so this regular expression has some false negatives as t:cmdline will replace all multiple spaces (including tab, newline, etc.) into one space (which can be missing depending on the payload).

I did my testing with this expression below to take care both the redos and the three false negatives I found:
\b(?:if(?: /i)?(?: not)?(?: exist\b| defined\b| errorlevel\b| cmdextversion\b|(?: |\().*(?:\bgeq\b|\bequ\b|\bneq\b|\bleq\b|\bgtr\b|\blss\b|==))|for(?: /[dflr].*)?.*? %+[^ ]+ in\s\(.*\)\s?do)

But if we use .* after the for switched then it make useless the switches section and it will be better to remove it as it is optional, this will make this regular expression more prone to false positives so we may need to address the for syntax a bit better but the text that would match will look a bit weird.

Test at least with:

FOR %%parameter IN (set) DO command 
FOR /R [[drive:]path] %%parameter IN (set) DO command 
FOR /D %%parameter IN (folder_set) DO command 
FOR /L %%parameter IN (start,step,end) DO command 
FOR /F ["options"] %%parameter IN (filenameset) DO command 
FOR /F ["options"] %%parameter IN ("Text string to process") DO command
FOR /F ["options"] %%parameter IN ('command to process') DO command
FOR it doesn't matter what or how many spaces %until_you_get_here_no_space_permitted in (whatever) do
for getting 50 %: in (english) don't do this
@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

I'd say let's concentrate on dealing with the problem in this PR and fix FNs in a different PR.
I've tested the following list only replacing * with ? and everything is working:

for %variable in (set) do command
for %%variable in (set) do command
for /d %variable in (set) do command
for /r c:\ %variable in (set) do command
for /l %variable in (1,1,2) do command
for /f %variable in (fileset) do command
for /f %variable in ("string") do command
for /f %variable in ('command') do command
for /f "usebackq" %variable in (`command`) do command

I've added spaces in different places to no avail.
I will add this to the test suite so we have something we can work on as currently there are no tests for this rule.

fgsch added a commit that referenced this pull request Apr 24, 2019

@fgsch fgsch changed the title Mitigate ReDOS vulnerablity (Fixes #1354) Vulnerable regexp in rule 932140 Apr 29, 2019

@fgsch fgsch changed the title Vulnerable regexp in rule 932140 Fix vulnerable regexp in rule 932140 Apr 29, 2019

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

I think if we change the for(?:/[dflr].*)* %+[^ ]+ in\(.*\)\s?do part to for(?:/[dflr].*)? %+[^ ]+ in\(.*\)\s?do we are good to go.

Any objections? @s0md3v can you update it?

csanders-git pushed a commit to j0k2r/owasp-modsecurity-crs that referenced this pull request May 6, 2019

csanders-git pushed a commit to j0k2r/owasp-modsecurity-crs that referenced this pull request May 6, 2019

Fix indentation
Migrate from python2 to python3
Python 2.7 will not be maintained past 2020

Migrating back to Python2 and abiding by PEP8

Updating whitespace element

Update README.md

add missing OWASP_CRS tags to 921xxx rules

Fix transform name pointed out by secrules_parsing

Whitespace

Add tests for the for command

Ref SpiderLabs#1355

Oneliner test

Revert "Oneliner test"

This reverts commit 7ca2c8e.

Switch to the latest image for now

This should fix the tests while the old image is reinstated.
Found by many people, including studersi via PR SpiderLabs#1350.

Revert back to 2.9-apache-ubuntu for now

Too many things changed in 2-apache and latest is nginx based now.

Added Gobuster

Alphabeticalized gobuster

Adding 21 new positive tests for 942260

Replaced descriptions

added 21 regression tests for rule 942490

Remove regression tests for non-existent rule.

Update regression tests for 931110.

Update regression tests for 931120.

added 36 regression tests for rule 942190

added 36 regression tests for rule 942160

added 16 regression tests for rule 942140

Adding 19 tests for 942150 (none so far)

Adding 14 tests for 942100 (none so far)

added 11 regression tests for rule 942240 (none so far)

Also handle dot variant of X_Filename

PHP will transform dots to underscore in variable names since dot is
invalid.

Content-Type is case insensitive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.