fix: fixed tests and descriptions #3201
Conversation
|
Failed tests have the following payloads. Except for the last one, there's no match at PL1 payload: fopen //comment\r\n (blah) curl -s 'https://sandbox.coreruleset.org/?foo=fopen%20%2f%2fcomment%0d%0a%20%28blah%29' \
-H 'x-crs-paranoia-level: 1' \
-H 'x-format-output: txt-matched-rules' payload: fopen #comment\r\n (blah) curl -s 'https://sandbox.coreruleset.org/?foo=fopen%20%23comment%0d%0a%20%28blah%29' \
-H 'x-crs-paranoia-level: 1' \
-H 'x-format-output: txt-matched-rules' payload: fopen \t #\n () curl -s 'https://sandbox.coreruleset.org/?foo=fopen%20%09%20%23%0a%20%28%29' \
-H 'x-crs-paranoia-level: 1' \
-H 'x-format-output: txt-matched-rules'payload: fopen \t/**/\t () curl -s 'https://sandbox.coreruleset.org/?foo=fopen%20%09%2f%2a%2a%2f%09%20%28%29' \
-H 'x-crs-paranoia-level: 1' \
-H 'x-format-output: txt-matched-rules'payload: fopen\t/foo\r\nbar/\t () curl -s 'https://sandbox.coreruleset.org/?foo=fopen%09%2f%2afoo%0d%0abar%2a%2f%09%20%28%29' \
-H 'x-crs-paranoia-level: 1' \
-H 'x-format-output: txt-matched-rules'payload: /eval(base64_decode('JGNoZWNrID... curl -s 'https://sandbox.coreruleset.org/eval%28base64_decode%28%27JGNoZWNrID...' \
-H 'x-crs-paranoia-level: 1' \
-H 'x-format-output: txt-matched-rules'
933150 PL1 PHP Injection Attack: High-Risk PHP Function Name Found
934100 PL1 Node.js Injection Attack 1/2
941390 PL1 Javascript method detected
949110 PL1 Inbound Anomaly Score Exceeded (Total Score: 15)
980170 PL1 Anomaly Scores: (Inbound Scores: blocking=15, detection=15, per_pl=15-0-0-0, threshold=5) - (Outbound Scores: blocking=0, detection=0, per_pl=0-0-0-0, threshold=4) - (SQLI=0, XSS=5, RFI=0, LFI=0, RCE=5, PHPI=5, HTTP=0, SESS=0, COMBINED_SCORE=15)Testing at PL2, all payload are blocked by other rules. Should we accept this and remove the failing tests? |
|
There's an error in the regular expression that you uncovered. Line 13 of |
tests/regression/tests/REQUEST-933-APPLICATION-ATTACK-PHP/933160.yaml
Outdated
Show resolved
Hide resolved
tests/regression/tests/REQUEST-933-APPLICATION-ATTACK-PHP/933160.yaml
Outdated
Show resolved
Hide resolved
regex changes fixed almost all failing tests, seems that 933160-23 still failing. This is the response from sandbox: curl -s 'https://sandbox.coreruleset.org/get/eval%28base64_decode%28%27JGNoZWNrID...' \
-H 'x-crs-paranoia-level: 1' \
-H 'x-format-output: txt-matched-rules'
933150 PL1 PHP Injection Attack: High-Risk PHP Function Name Found
934100 PL1 Node.js Injection Attack 1/2
941390 PL1 Javascript method detected
949110 PL1 Inbound Anomaly Score Exceeded (Total Score: 15)
980170 PL1 Anomaly Scores: (Inbound Scores: blocking=15, detection=15, per_pl=15-0-0-0, threshold=5) - (Outbound Scores: blocking=0, detection=0, per_pl=0-0-0-0, threshold=4) - (SQLI=0, XSS=5, RFI=0, LFI=0, RCE=5, PHPI=5, HTTP=0, SESS=0, COMBINED_SCORE=15) |
|
The issue appears to be that the parentheses the URL does not include a closing parenthesis: |
That's true, thanks! Should I change the payload? Idk if setting no_log_contains makes sense here... |
Yes, I would change the payload. |
|
BTW, do you think it's a mistake that |
I didn't notice it! Yes and maybe |
Yes, I think that's ok. |
|
Do we have a status here? Is this ready to be merged? What is needed here? |
|
Looks good to me. Once my change requests are addressed, this can be merged IMO. |
tests/regression/tests/REQUEST-933-APPLICATION-ATTACK-PHP/933160.yaml
Outdated
Show resolved
Hide resolved
|
Note that the second test failure (980170) is due to a race condition in go-ftw. |
|
@theMiddleBlue I'd like to get this merged. When do you have the time to polish this? |
3552c75 to
942f896
Compare
|
it should be done |
|
LGTM. Thanks for the update. @theseion do you agree? If yes, please merge. |
|
ouch: there're a lot of conflicts |
|
Ah, yes, sucks. You were overtaken by the PHP function names generator PR. Maybe better start over from scratch. |
|
Are you still working on this @theMiddleBlue ? |
A missing alternation in the regular expression for matching characters after PHP commands caused `#` comments and whitespace to be matched improperly. This issue was discovered as part of coreruleset#3201.
|
The error in the regular expression is fixed with #3432. |
622bfab to
5c6278b
Compare
|
I've rebased and updated / moved tests. |
|
@theMiddleBlue I think I got everything, can you confirm? |
5c6278b to
b36e95a
Compare
|
Closed in favour of #3462. |
Regression tests for 933160 seem to be inconsistent (#3198) . I've fixed the following problems:
I've tested all payloads against the sandbox, and after my fixes, we've some test that doesn't pass correctly due to whitespaces in the payload. Maybe the rule regex should consider it.