Replace NodeJS Rule Set #2163
Replace NodeJS Rule Set #2163
Conversation
|
hmm, the check has an error on a renamed yaml... am I missed something? |
|
Meeting decision August 2021: @fzipi will check the failing tests and merge the PR when the tests are ok. (@dune73 says: Feel free to merge as soon as the tests pass. The PR looks good to me.) |
tests/regression/tests/REQUEST-934-APPLICATION-ATTACK-GENERIC/934110.yaml
Outdated
Show resolved
Hide resolved
tests/regression/tests/REQUEST-934-APPLICATION-ATTACK-GENERIC/934110.yaml
Outdated
Show resolved
Hide resolved
|
yep, thanks! I'll send an update asap |
383330e to
97f0df3
Compare
|
@fzipi have I changed it right? I see that tests still failing |
|
@theMiddleBlue Thanks. For the yaml part, they fail because the empty lines need to be really empty (I tend to do the same and indent the whole line). If you remove all spaces in that line it should work in go-ftw and the yaml linter. |
8e8ee42 to
d3cbbc1
Compare
|
And for the diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index c684fbf..e41b68c 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -30,7 +30,7 @@ jobs:
REQUEST-931-APPLICATION-ATTACK-RFI,
REQUEST-932-APPLICATION-ATTACK-RCE,
REQUEST-933-APPLICATION-ATTACK-PHP,
- REQUEST-934-APPLICATION-ATTACK-NODEJS,
+ REQUEST-934-APPLICATION-ATTACK-GENERIC,
REQUEST-941-APPLICATION-ATTACK-XSS,
REQUEST-942-APPLICATION-ATTACK-SQLI,
REQUEST-943-APPLICATION-ATTACK-SESSION-FIXATION, |
|
Hey @theMiddleBlue, would you have time to execute the requested changes before the meeting on Monday? Would be cool if we could merge before that. |
|
already done with the previous commit (I guess) but I can't figure out why tests fail |
|
TBH, me neither. Something is not working, meaning the rule is not being fired by our tests. |
|
How about GH not taking Does it work with go-ftw? |
|
No, it bails with both 🤔 |
|
What is failing is the multipart in modsecurity. I'm trying to find out why in this particular test. |
tests/regression/tests/REQUEST-934-APPLICATION-ATTACK-GENERIC/934110.yaml
Outdated
Show resolved
Hide resolved
d3cbbc1 to
6b4ea56
Compare
|
This is makes me very curious. The suspense is rising. |
|
I'm still testing this one. Seems like a modsec problem (or a test generation problem) for the multipart/form-data. |
|
🤦 Found it. Of course. 👉 @theMiddleBlue This works perfectly in modsec3-nginx. Right now I've made it to no errors in Apache, but no matches either. I think it is being discarded, but need to look at debug logs. I think this is not working in apache because of owasp-modsecurity/ModSecurity#2193 and/or owasp-modsecurity/ModSecurity#2417. Probably @airween (of course) can chime in to see what's next. |
|
Additional tests:
Just in case:
@theMiddleBlue Maybe the problem is that per https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-%28v2.x%29#request_body: Anyway, even after adding End of debug log: Weird things
Making it kind of "binary". After filtering using
Good things
|
|
I've made some researches, and found these issues:
The patch is here. Maybe we should replace the What do you think? |
|
Thanks @airween for your comments. I think that the test In particular, I've changed |
|
Sure - but test is just a part of the problem. The real question is what should we use in these cases? Keep the |
|
Meeting decision Oct 4: @theMiddleBlue will change REQUEST_BODY to FULL_REQUEST so that it works for both v2 and v3. Add some tests that proves that. |
fd8bd08 to
b3d5c5d
Compare
|
Looks like we need to merge this one before #2259 |
|
Yes, and it seems to be stuck. |
b3d5c5d to
04774b5
Compare
|
Did a new pass on this one today. With the fixed go-ftw binary, and using the Don't know what else to try. Looks like the actual request body in the FULL_REQUEST Isn't properly loaded :/ |
|
@airween I'm resorting to you on this one. Maybe I did something in a hurry, but I don't know by know.....can you help me figuring out what's needed here? |
|
@airween: Could you cast an eye here? |
|
Meeting decision December: @airween will look into the failing tests and shepherd this PR to merging. |
|
I started to investigate this issue. This the output of gdb, explanation under the block: Breakpoint 1, var_full_request_generate (msr=0x7ffff5e03790, var=0x7ffff3857640, rule=0x7ffff38572a8, vartab=0x7ffff447b408, mptmp=0x7ffff447b028) at re_variables.c:2081
2081 return var_simple_generate_ex(var, vartab, mptmp, full_request,
(gdb) p msr->msc_reqbody_buffer
$1 = 0x0
(gdb) p msr->msc_reqbody_length
$2 = 359
(gdb) p full_request
$3 = 0x555555688910 "POST / HTTP/1.1\n\nHost: localhost\nUser-Agent: curl/7.74.0\nAccept: */*\nContent-Length: 359\nContent-Type: multipart/form-data; boundary=", '-' <repeats 24 times>, "e7b37752c8d6cdb3\n\n"As you can see, the behavior of variable I tried to turn on the variable without any results - the I need more investigation, especially how mod_sec2 handles the request body. |
|
I did some more research about how The bad news is that I'm afraid we can't do this what @theMiddleBlue wants, namely inspect the whole request (body). As you know, mod_securit2 (and libmodsecurity3 too - but it does not matter now) has the variable Mod_security2 (and 3 too) has 4 kind of request body processors: As the documentation says, I supposed the These zeroes are the allocated spaces for the body, which isn't available - therefore they remain empty... I didn't find any other solution, so I'm very sorry, but unfortunately I can say now, this rule won't work. |
|
I reviewed again with a minimalist rule set: and really looks like if the Result of the urlencoded request Result of the multipart request The rule with target May be this rule can work as plugin through Lua - as the antivirus-plugin works. But I'm afraid we can't do anything else. |
|
Outcome issue meeting December 2021: We close the PR and create a new PR with just the renaming. Also open an issue with the rule you are killing. |
|
Closing this in favour of discussion of #2291. |
this replaces #2061 removing NodeJS rule set and adds a new generic rule set.