Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

Fix vulnerable regexp in rule 942490 #1379

Merged
merged 3 commits into from
May 14, 2019
Merged

Conversation

emphazer
Copy link
Contributor

@emphazer emphazer commented May 3, 2019

limit substitution [^\w\s] from + to {1,10}

i tested it against 5276 matches and the results matches are exactly the same.
even {1,2} produced the same results.
i think {1,10} is fairly enough.

according to #1359

 time grep -P -f 942490.rule 942490.payload

real	0m10.631s
user	0m10.630s
sys	0m0.001s



time grep -P -f 942490.test 942490.payload

real	0m0.072s
user	0m0.069s
sys	0m0.002s

@dune73
Copy link
Contributor

dune73 commented May 3, 2019

But does not this invite a bypass via 11 characters?

@emphazer
Copy link
Contributor Author

emphazer commented May 3, 2019

@dune73 try it :-)
here is a payload

` != 0 && `

and here is the bypass

` !"""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""= 0 && `

i wasn't able to bypass it

@emphazer
Copy link
Contributor Author

emphazer commented May 3, 2019

@dune73
i think i got it!
the + is not necessary at all!
you can see it at https://www.debuggex.com/
the [^\w\s] is not supposed to be a match! it's just needed to skip the spaces and to dismatch if a word character appear.

i think the [^\w\s]+ in combination with \W*? couldnt work because you got a 'lazy' and 'greedy' regular expression matching on a non word character next to each other...

@dune73
Copy link
Contributor

dune73 commented May 3, 2019

I'm not deep enough into regexes to really understand this.

@fgs: Could you chime in, please?

@dune73
Copy link
Contributor

dune73 commented May 3, 2019

Ooops. I meant @fgsch.

@fgsch
Copy link
Contributor

fgsch commented May 3, 2019

This looks good but I believe we can go even further and remove the [^\w\s]+ altogether and replace the \W*? with \W+.
We could also drop the non-capture group and the \W*? before the .*?.

Finally, the comment is stale and it should be updated. We should also drop the separate regexp file.

@franbuehler
Copy link
Contributor

franbuehler commented May 3, 2019

I agree with the changes suggested by @fgsch.
I had a look at it, tested it manually and @emphazer's tests (PR #1381) are also still successful.
The resulting regex would look like this (if I understand all the suggestions correctly):

[\"'`][\s\d]*?\W+\d.*?[\"'`\d]

@emphazer
Copy link
Contributor Author

emphazer commented May 3, 2019

I will benchmark both at monday. thank you @fgsch

@emphazer
Copy link
Contributor Author

emphazer commented May 6, 2019

@fgsch
@franbuehler
got the results now.
the match is not the same.

test files:
942490.test
(?:["'][\s\d]*?[^\w\s]\W*?\d\W*?.*?[\"'\d])

942490.test_fgsch
["'][\s\d]*?\W+\d.*?[\"'\d]

echo "' 1=1'" | grep -o --color=always  -P -f 942490.test_fgsch 
' 1=1
echo "' 1=1'" | grep -o --color=always  -P -f 942490.test
' 1=1'

i was not able to get a perfomance win with your suggestion

time grep -c -o --color=never -P -f 942490.test_fgsch payloads.txt
12775

real	0m0.075s
user	0m0.064s
sys	0m0.011s

time grep -c -o --color=never -P -f 942490.test payloads.txt
12773

real	0m0.075s
user	0m0.065s
sys	0m0.010s

the payloads.txt file had 50mb

i had a little different of 0.008 sec with a 500mb file

@fgsch
Copy link
Contributor

fgsch commented May 6, 2019

The performance might be negligible using time but the regexp has 2 steps less.

As for the regexp, you are right, sorry.
Can you try this instead:

[\"'`][\s\d]*\W+\d.*?[\"'`\d]

@emphazer
Copy link
Contributor Author

emphazer commented May 7, 2019

@fgsch

# (?:["'][\s\d]*?[^\w\s]\W*?\d\W*?.*?[\"'\d])
ts=$(date +%s%N)
time grep -c -o --color=never -P -f 942490.test payloads2.txt
tt=$((($(date +%s%N) - $ts)/1000000)) 
echo "Time taken: $tt milliseconds"
35901

real	0m5.223s
user	0m3.644s
sys	0m1.579s
Time taken: 5226 milliseconds



# [\"'`][\s\d]*\W+\d.*?[\"'`\d]
ts=$(date +%s%N)
time grep -c -o --color=never -P -f 942490.test_fgsch2 payloads2.txt
tt=$((($(date +%s%N) - $ts)/1000000))
echo "Time taken: $tt milliseconds"
35910

real	0m5.267s
user	0m3.707s
sys	0m1.560s
Time taken: 5268 milliseconds

echo "' 65"    | grep --color=always -P -f 942490.test_fgsch2
' 65

echo "'971 11" | grep --color=always -P -f 942490.test_fgsch2
'971 11



echo "' 65"    | grep --color=always -P -f 942490.test 
no match

echo "'971 11" | grep --color=always -P -f 942490.test
no match

@emphazer
Copy link
Contributor Author

emphazer commented May 7, 2019

@fgsch
@dune73
i don't see any necessities to do further testing here because the regex in this PR works and is fast!
5.4G payload file

here a comparison with a 5.4gb file
942360 and 942470 takes over 2 minutes here and 942361 takes 8 sec

here a POST payload file with 50mb

942410:
real	0m0.453s
user	0m0.436s
sys	0m0.017s
Time taken: 454 milliseconds


942470:
real	0m0.557s
user	0m0.546s
sys	0m0.010s
Time taken: 558 milliseconds


942490: 
real	0m0.074s
user	0m0.066s
sys	0m0.008s
Time taken: 75 milliseconds

the rule is 7 or 8 time faster here.

@fgsch
Copy link
Contributor

fgsch commented May 9, 2019

Using grep to measure the speed is not the right approach.
For once, this is linear. There is also caching involved and a many other things. But let's stop here.

Remove the non-capturing group and I have no objections.

@emphazer
Copy link
Contributor Author

emphazer commented May 9, 2019

@fgsch
thank you.


util/regression-tests/CRS_Tests.py::test_crs[ruleset2020-942490.yaml -- 942490-1] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2021-942490.yaml -- 942490-2] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2022-942490.yaml -- 942490-3] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2023-942490.yaml -- 942490-4] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2024-942490.yaml -- 942490-5] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2025-942490.yaml -- 942490-6] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2026-942490.yaml -- 942490-7] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2027-942490.yaml -- 942490-8] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2028-942490.yaml -- 942490-9] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2029-942490.yaml -- 942490-10] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2030-942490.yaml -- 942490-11] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2031-942490.yaml -- 942490-12] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2032-942490.yaml -- 942490-13] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2033-942490.yaml -- 942490-14] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2034-942490.yaml -- 942490-15] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2035-942490.yaml -- 942490-16] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2036-942490.yaml -- 942490-17] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2037-942490.yaml -- 942490-18] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2038-942490.yaml -- 942490-19] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2039-942490.yaml -- 942490-20] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2040-942490.yaml -- 942490-21] PASSED
util/regression-tests/CRS_Tests.py::test_crs[ruleset2041-942490.yaml -- 942490-22] PASSED

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants