feat(ssrf): adds rules to check for IP based SSRF #2259
Conversation
|
This is gonna be nice rules! There are some small formatting problems to resolve. |
|
For clarity, can "(covered by RFI)" be changed to something like "(already covered by RFI rule 931100)"? |
95a5a5b to
96e0907
Compare
|
Hi, it seems the regex is not anchored to any schema, so the odds of false positives aren't good. |
|
For the record, this is a work-in-progress and we'll add more rules to this file? I'm asking because I am unsure we need a separate rules file for this. The 93x and 94x are usually meant for application defects and this is more a protocol attack in the 92x range, possibly even integrated into 920 or 921. |
Yeah, I'm adding negative tests and seeing that. Probably related to the script for building regexp. |
95445e7 to
cebe925
Compare
|
I'd like to see this into the rules file that #2163 renames. |
|
Is this is a generic attack now, or is it because two rules are not too much to throw in new files? |
|
What about the new |
|
I'm personally not a fan of the separate scores, but some people are. In this particular case I doubt it makes much sense, unless it's really a separate rule group. Generic or not: There are a lot of different rule categories. Adding a new rule category / group brings at least 8 administrative rules. That's a lot if it's only going to be 2-3 rules. So I think we need a group for "various rules". In my eyes, the new generic rules group was such a place. |
|
Meeting decision December: This PR will be shifted into the new category "generic attacks" after #2163 is merged. |
cebe925 to
9ffe1fb
Compare
|
SSRF rules, 🎄 edition! 🎉
Before merge, we just need to think:
|
ee1d977 to
e891353
Compare
|
Meeting decision January: @franbuehler will review it and pass it to @lifeforms for testing. |
|
We're you able to review this one @franbuehler ? |
|
Hi @fzipi, |
tests/regression/tests/REQUEST-934-APPLICATION-ATTACK-GENERIC/934120.yaml
Outdated
Show resolved
Hide resolved
- extends the original IPs handled in RFI rules to IPv6 and other IPv4 evasions - added the regular expressions used to data file - tests check the expressions - add negative tests Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
There was a problem hiding this comment.
If it's just me getting this error while generating the regex (I wanted to check the output against the rule), this MR is ready to be tested by @lifeforms.
Nice rule, a nice extension, thank you!
|
I will run the rule on traffic starting next Wednesday. |
|
So far no false positives reported! 🥳 Let's wait for a few more days. I plan to merge this on Saturday, and then continue work on my prototype pollution PR which will conflict with this. |
|
Can you give us an estimate on the number of requests for this? |
|
I should improve my logging, but if I look at some access_logs from yesterday and extrapolate, I think something like 2 million requests. |
|
Thank you. And I can recommend this format: https://www.netnea.com/cms/apache-tutorial-5_extending-access-log/#step_4_configuring_the_new,_extended_log_format Comes with a handy set of aliases too: https://raw.githubusercontent.com/Apache-Labor/labor/master/bin/.apache-modsec.alias |
|
Merged 🍴 because this rule is cool 😎 and I've got no false positives. (Sadly also no true positives) |
Signed-off-by: Felipe Zipitria felipe.zipitria@owasp.org