Added libinjection SQLi rule for last path segment #1329

theseion · 2019-03-21T09:38:22Z

The new rule is a sibling of rule 942100 that adds a libinjection
of the last path segment

The new rule is a sibling of rule 942100 that adds a libinjection of the last path segment

theMiddleBlue

I've understood the problem and this seems to be a good start. You just need to fix a bit the rule, and idk if this is suitable for PL1 (let's see what others dev say about it).

theMiddleBlue · 2019-03-21T09:53:02Z

rules/REQUEST-942-APPLICATION-ATTACK-SQLI.conf

+    setvar:'tx.msg=%{rule.msg}',\
+    setvar:'tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/SQL_INJECTION-%{MATCHED_VAR_NAME}=%{MATCHED_VAR}',\
+    chain"
+    SecRule TX:1 "@detectSQLi"


missing action on the chained SecRule (3rd argument)

Thanks. I've just pushed a commit where the setvar actions have been moved to the chained rule, as they would be evaluated every time otherwise.

I thought about putting the rule in PL1 but I don't know what the implications are for FP's.

theseion · 2019-03-21T10:12:31Z

I don't understand why the test fails. Chained rules don't need ID's AFAIK.

theMiddleBlue · 2019-03-21T10:31:33Z

if I'm not wrong, it always needs an action. for example, you can move setvar to the chained one

- only log when actually blocking and not on every request - remove all slashes from the beginning of REQUEST_FILENAME (there may be more than one)

theseion · 2019-03-21T13:09:29Z

Sorry about the noise. This should conclude any updates to the rule for now, I have it running in production.

spartantri · 2019-03-21T13:54:03Z

Since this uses REQUEST_FILENAME and will not include the query string which is the one that usually triggers the false positives. The chained rule do not need multimatch as it is not doing any anti-evasion transformation over TX.1

@spartantri

Suggested by @spartantri, as the chained rule deosn't perform anti-evasion transformations.

theseion · 2019-03-21T14:44:35Z

Thanks. I've pushed an update with multiMatch removed.

fgsch · 2019-03-21T16:14:39Z

Tests please. Tons of them 😄

fgsch · 2019-03-21T16:20:52Z

rules/REQUEST-942-APPLICATION-ATTACK-SQLI.conf

+    severity:'CRITICAL',\
+    chain"
+    SecRule TX:1 "@detectSQLi" \
+        "setvar:'tx.anomaly_score_pl1=+%{tx.critical_anomaly_score}',\


If this is PL2 you need to increment pl2.

Fixed, thanks.

@fgsch

Suggested by @fgsch

fgsch · 2019-03-21T17:46:22Z

rules/REQUEST-942-APPLICATION-ATTACK-SQLI.conf

+#
+# This is a sibling of rule 942100 that adds checking of the last path segment.
+#
+SecRule REQUEST_FILENAME "@rx ^/+(.*)$" \


I don't think this do what you think it does. REQUEST_FILENAME will have the whole path, not the last segment.
Please update the comment.

Can you explain a bit more why you are using rx and not simply passing the path to detectSQLi?

Thanks for pointing that out. I don't think there is a variable then, which holds that information, is there? I'll update the regular expression to parse out the last segment then.

libinjection is more likely to fail when passing the path, at least that is what my tests showed. E.g. the following string produces a match 999999.1 union select unhex(hex(version())) -- and 1=1 while this doesn't /999999.1 union select unhex(hex(version())) -- and 1=1 (tested directly with libinjection). So to be safe I don't want the slashes in there.

Maybe I could pass the entire path with the slashes replaced... But I don't know what that would do to the detection capabilities.

You have https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-%28v2.x%29#REQUEST_BASENAME.

Sorry, I pasted the wrong link. I've updated it now.
Can you use that instead and squash the commits?

emphazer · 2019-03-21T19:22:35Z

put it in PL3 pls. this will lead to much FP. we run PL2 as default. for example: aggregation on js or css wont be supported anymore.

theseion · 2019-03-21T19:24:55Z

Sure. Out of curiosity, could you give an example of what you mean by aggregation?

emphazer · 2019-03-21T22:26:25Z

@theseion sure
look at this path here. its an aggregation of different java script files.

/sites/default/files/advagg_js/js__8Cefk7UudAKRWCZ23BiLQNs5GDGwHszGZfd5m3bBkno__2XMzkOnobbuTwYFViZn-Z2zlJnQLv6vUbBpBt-8Bpsw__dwrExSTRu6UuvuQxBPkJ0DQAptvitAf5_8JXXxvP9nQ.js

i can give you a lot of pages where stuff like that is very common.
its often used in drupal or typo3 for example.

take a look at the url https://www.land.nrw/ its the page for the state north rhine westphalia in germany hosted by the government.

fgsch · 2019-03-21T23:55:13Z

@emphazer will this FP on sqli or xss?

@fgsch

- improved comment to state why we only use the last path segment (@fgsch) - fixed regular expression to expect an arbitrary path (REQUEST_FILENAME holds the full path) (@fgsch) - moved rule to PL3 (@emphazer)

theseion · 2019-03-22T05:54:57Z

Thanks for the reviews and explanations evryone! I've pushed an update where all your concerns should be addressed.

fgsch · 2019-03-22T10:35:24Z

I think this is on the right track but I'd like to see some tests.
Can you add a few positive and negative ones? Let us know if you need any help with that.

emphazer · 2019-03-22T15:25:43Z

@fgsch sqli

@theseion thx for changes.

fgsch · 2019-03-22T15:48:33Z

@emphazer are you saying libinjection will detect js__8Cefk7UudAKRWCZ23BiLQNs5GDGwHszGZfd5m3bBkno__2XMzkOnobbuTwYFViZn-Z2zlJnQLv6vUbBpBt-8Bpsw__dwrExSTRu6UuvuQxBPkJ0DQAptvitAf5_8JXXxvP9nQ.js as a sql injection?

emphazer · 2019-03-22T18:10:37Z

@fgsch no, thats just an example.
this urls are always different they change several times a day.

fgsch · 2019-03-22T18:25:10Z

@emphazer The format for this aggregator is fixed (js__[A-Za-z0-9-_]{43}__[A-Za-z0-9-_]{43}__[A-Za-z0-9-_]{43}.js(\.gz|\.br)). I don't see how this will ever FP.
I'd rather put it in PL2 and move it to PL3 if it indeed FP.

emphazer · 2019-03-22T18:40:10Z

here are 2 examples which matched

Matched Data: 1c found within ARGS:token: 9--6nVTjusWqO8_f84sNRMwXlmDvt9J_JDLV6rWzoCc

Matched Data: 1c found within ARGS:AesGroup[_token]: 9--CeqpjEHcaUuZKr24FPQNqGeTfcl4yDkQitZKd_KQ

fgsch · 2019-03-22T18:57:17Z

@emphazer I stand corrected. You are right.

fgsch · 2019-03-29T15:54:57Z

@theseion have you seen my comment at #1329 (comment)?
Also, any chance you can add some tests?

theseion · 2019-03-29T16:20:58Z

@theseion have you seen my comment at #1329 (comment)?
Also, any chance you can add some tests?

Yes but I hadn't seen the update. That edit makes more sense ;)
I'm in a bit of a pickle at the moment but I'll try to provide some tests ASAP.

fgsch · 2019-03-29T17:18:44Z

@theseion great, thank you! let us know if you need a hand or two :)

lifeforms · 2019-04-01T18:45:01Z

Very nice contribution @theseion, we'd love to include this in the next release. Paranoia Level 3 seems alright to prevent false positives in the default install. Let us know if you have any trouble with the tests.

fgsch · 2019-04-16T21:10:37Z

@theseion any updates on this?

theseion · 2019-04-17T12:26:14Z

Not yet, sorry. It's on my list though.

fgsch · 2019-07-05T10:31:23Z

Hey @theseion. Any way we can entice you to complete this? 😉

theseion · 2019-07-05T11:48:27Z

Hm... Wine? 😜 I know, I should be working on this... I haven't forgotten, I promise.

fgsch · 2019-07-05T12:54:59Z

Here we go:

dune73 · 2019-07-17T15:01:22Z

This is a welcome PR and it's time we merge this.

Summing up the remaining issues:

Instead of a chained rule, the new rule should just use REQUEST_BASENAME (thanks @fgs)
We need tests

If you have time to do this within the next week, please do @theseion. If not, I'll take it over and probably push a new PR with the same functionality.

theseion · 2019-07-17T15:41:56Z

No, I won't have time this week, sorry.

Thanks for your help!

dune73 · 2019-07-18T08:44:01Z

OK.

Feel free to bring the wine to the next CRS meetup. ;)

dune73 · 2019-08-02T13:28:30Z

I have re-implemented this PR addressing the two item above in #1492.

Thank you for the PR @theseion.

Closing this in favour of #1492.

theseion added 2 commits March 21, 2019 10:31

Added libinjection SQLi rule for last path segment

3370e52

The new rule is a sibling of rule 942100 that adds a libinjection of the last path segment

Moved actions to chained rule

4ba4096

theMiddleBlue suggested changes Mar 21, 2019

View reviewed changes

theseion added 5 commits March 21, 2019 11:36

added missing backslash

52121b5

added missing quotes

8722358

Made some minor improvements to the new libinjection rule

356fb82

- only log when actually blocking and not on every request - remove all slashes from the beginning of REQUEST_FILENAME (there may be more than one)

Moved multiMatch action to chained rule

06c8584

Moved meta actions back to chain starter rule

c63ad16

Removed multiMatch action

3b373c8

Suggested by @spartantri, as the chained rule deosn't perform anti-evasion transformations.

fixed missing quote

ff42ca0

fgsch reviewed Mar 21, 2019

View reviewed changes

Increased tx.anomaly_score_ to level 2.

f4ea7a8

Suggested by @fgsch

fgsch reviewed Mar 21, 2019

View reviewed changes

Updated rules as suggested

9a517fa

- improved comment to state why we only use the last path segment (@fgsch) - fixed regular expression to expect an arbitrary path (REQUEST_FILENAME holds the full path) (@fgsch) - moved rule to PL3 (@emphazer)

dune73 mentioned this pull request Apr 1, 2019

Monthly Chat Agenda April 2019 (2019-04-01) #1333

Closed

dune73 added the Needs action label May 5, 2019

dune73 self-requested a review July 1, 2019 19:57

dune73 self-assigned this Jul 17, 2019

dune73 mentioned this pull request Aug 2, 2019

Add libinjection check on last path segment #1492

Merged

dune73 closed this Aug 2, 2019

dune73 mentioned this pull request Aug 2, 2019

No libinjection check on last path segment #1328

Closed

CRS-migration-bot mentioned this pull request May 13, 2020

Added libinjection SQLi rule for last path segment coreruleset/coreruleset#1329

Closed

franbuehler mentioned this pull request Dec 12, 2020

Simple request header SQL injection not triggered untill paranoia level 3 coreruleset/coreruleset#1904

Closed

Added libinjection SQLi rule for last path segment #1329

Added libinjection SQLi rule for last path segment #1329

Uh oh!

Conversation

theseion commented Mar 21, 2019

Uh oh!

theMiddleBlue left a comment

Choose a reason for hiding this comment

Uh oh!

theMiddleBlue Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

theseion Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

theseion commented Mar 21, 2019

Uh oh!

theMiddleBlue commented Mar 21, 2019

Uh oh!

theseion commented Mar 21, 2019

Uh oh!

spartantri commented Mar 21, 2019

Uh oh!

theseion commented Mar 21, 2019

Uh oh!

fgsch commented Mar 21, 2019

Uh oh!

fgsch Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

theseion Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

fgsch Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theseion Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

fgsch Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgsch Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

emphazer commented Mar 21, 2019

Uh oh!

theseion commented Mar 21, 2019

Uh oh!

emphazer commented Mar 21, 2019

Uh oh!

fgsch commented Mar 21, 2019

Uh oh!

theseion commented Mar 22, 2019

Uh oh!

fgsch commented Mar 22, 2019

Uh oh!

emphazer commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgsch commented Mar 22, 2019

Uh oh!

emphazer commented Mar 22, 2019

Uh oh!

fgsch commented Mar 22, 2019

Uh oh!

emphazer commented Mar 22, 2019

Uh oh!

fgsch commented Mar 22, 2019

Uh oh!

fgsch commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theseion commented Mar 29, 2019

Uh oh!

fgsch commented Mar 29, 2019

Uh oh!

lifeforms commented Apr 1, 2019

fgsch Mar 21, 2019 •

edited

Loading

fgsch Mar 21, 2019 •

edited

Loading

emphazer commented Mar 22, 2019 •

edited

Loading

fgsch commented Mar 29, 2019 •

edited

Loading

fgsch commented Apr 16, 2019 •

edited

Loading