Rewrite/rethink rule 942260 with re2 compatibility

[fzipi]

Motivation

Rule 942260 is using ++ (possesive quantifier) and it is not supported.

[\"'`]\s*?[^?\w\s=.,;)(]++\s*?[(@\"'`]*?\s*?\w+\W+\w

We need to see if we can find a viable alternative or that can be dropped somehow.

Proposed solution

None yet.

Alternatives

1. Just remove possessive quantifier. re2 matches should be guaranteed to run in time linear in the size of the input (but we are testing it).

Additional context

Possessive matching isn't supported in re2 ((?>re) | possessive match of re (NOT SUPPORTED)).

• RE2 syntax

[theseion]

I would drop it. Fortunately, the [(@\"'`]*? part is optional, so that removing the possessive quantifier will not change the semantics (note that [^?\w\s=.,;)(] overlaps with [(@\"'`]).
Performance shouldn't be impacted if the engine is smart (there are no nested quantifiers), no ReDos either.

[fzipi]

Ok, but we can't drop it for pcre, right? So the solution is to have two different rules? Or just drop it for good?

[theseion]

I don't see why we'd need it for PCRE. As I wrote above: no ReDOS, and performance shouldn't be impacted (in general; implementation dependent of course).

[theseion]

I retract that comment. I did not realise that the expression in the rule was much longer. Removing the possessive quantifier actually completely destroys performance. This has already been discussed in SpiderLabs/owasp-modsecurity-crs#1359.

I'm trying to understand what it is that we are trying to match. The description says "SQL authentication bypass"... Unfortunately, there's no test for this rule that tests what [\"'`]\s*?[^?\w\s=.,;)(]++\s*?[(@\"'`]*?\s*?\w+\W+\w is supposed to do. If we knew what we are looking at we may be able to come up with an alternative.

Side note: getting rid of ++ on its own will probably not be enough. The ++ will eat up many things that would be matched by [(@\"'`]*? and that expression itself suffers from backtracking.

Let's try to analyse possible inputs.

1. The input has to start with ["'`]
2. Optional white space characters (why the lazy match??)
3. The next part leaves a lot of room. Here's a selection of possible tokens: !|\/{}[]+@#$%^&*-><'"`~™§ˆ¶ºª•§¢
4. Optional white space characters (why the lazy match??)
5. Zero or more [@"'`]. This part overlaps with the tokens from 3, so when there are no space characters between the two character classes, the overlapping characters will be matched by the first class.
6. Optional white space characters (why the lazy match??)
7. At least one word character
8. This part (\W+) is close to what was matched by 3. In addition to 3 it also matches [=?\s.,;)(]
9. The last part looks funny. It could be there to reduce false positives, e.g. ensuring that the input doesn't end with =

It is likely that the excluded characters from 3 are supposed to be matched in 8. For example: ...##...=....

I went looking for actual payloads and the most promising source (https://github.com/payloadbox/sql-injection-payload-list) had ```'&&SLEEP(5)&&'1` as pretty much the only thing that looks promising. This kind of injection should be handled by other rules though, I believe, so I'm not sure if this really is what we're looking for.

@theMiddleBlue any ideas?

[fzipi]

Thanks for this analysis Max! We need to keep digging and something will come up. Will take a deeper look also.

[theseion]

@jptosso any ideas?

[fzipi]

Taking a look at this one. Ugh. This rule is nothing but pain.

1. History.

We don't really know what is exactly trying to match. There is no clear documentation on the rule, other than the msg:'Detects basic SQL authentication bypass attempts 2/3'.

2. Testing.

Tests in 942260 from 1-6 looks reasonable. But then from 7-21 they have nothing to do with "authentication bypass attempts". Mostly they are about "Embedded shell execution" and "Advanced embedded shell execution", and there is an XSS test at the end. Isn't that relevant to other rules?

Can you enlighten us a bit on this @dune73 ? Maybe you are aware, maybe not.

Moving forward

So there is some room here. First, we need to move the tests on shell execution to a different rule, probably. This one should not be, IMHO, about shell execution, right? With that, we might be able to trim this regexp a bit, or move some parts to a different rule.

I am thinking on moving in particular these two:

[\"'`]\s*?\*\s*?\w+\W+[\"'`]
[\"'`]\s*?[^?\w\s=.,;)(]++\s*?[(@\"'`]*?\s*?\w+\W+\w

From reading the whole data file, looks like they don't belong here.

[theseion]

Good thinking. Maybe those statements aren't for SQL injection. Or are the SQL dialects that allow shell calls as part of a query?

[fzipi]

Even if there is that possibility, I would treat those as shell injections, not SQL injections. We should mostly block anything that targets shell execution. But on the "shell side", not on the "SQL side" I mean.

[dune73]

OK.

Here is the oldest version of the rule I have: CRS 2.2.7, 2012-12-19

SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "(?i:(?:union\s*?(?:all|distinct|[(!@]*?)?\s*?[([]*?\s*?select\s+)|(?:\w+\s+like\s+[\"'`´’‘])|(?:like\s*?[\"'`´’‘]\%)|(?:[\"'`´’‘]\s*?like\W*?[\"'`´’‘\d])|(?:[\"'`´’‘]\s*?(?:n?and|x?x?or|div|like|between|and|not |\|\||\&\&)\s+[\s\w]+=\s*?\w+\s*?having\s+)|(?:[\"'`´’‘]\s*?\*\s*?\w+\W+[\"'`´’‘])|(?:[\"'`´’‘]\s*?[^?\w\s=.,;)(]+\s*?[(@\"'`´’‘]*?\s*?\w+\W+\w)|(?:select\s+?[\[\]()\s\w\.,\"'`´’‘-]+from\s+)|(?:find_in_set\s*?\())" "phase:2,capture,t:none,t:urlDecodeUni,block,msg:'Detects basic SQL authentication bypass attempts 2/3',id:'981245',tag:'OWASP_CRS/WEB_ATTACK/SQL_INJECTION',logdata:'Matched Data: %{TX.0} found within %{MATCHED_VAR_NAME}: %{MATCHED_VAR}',severity:'2',setvar:'tx.msg=%{rule.id}-%{rule.msg}',setvar:tx.sql_injection_score=+1,setvar:tx.anomaly_score=+%{tx.critical_anomaly_score},setvar:'tx.%{tx.msg}-OWASP_CRS/WEB_ATTACK/SQLI-%{matched_var_name}=%{tx.0}'"

Somehow similar, but not quite.

0 documentation about the rule in said release. Likely one of the early rules Ofer Shezaf added when we started with CRS in 2007 by googling for SQL injections. I inquired if we had anything backed up from back then, but he responded negative after checking some old disks.

So we need to do with what we have.

The sad truth about the tests of this rule and may others is that we were lacking tests for a lot of rules and I just grabbed alerts that I had for certain rules. From a big body of alerts that a 4.5M request burp run created.

In my defense I can say that many of the SQL rules are not overly clear cut. They also catch non-SQL stuff, and apparently this rule also catches RCEs, so I am simply testing the status quo. However, I agree there is no reason to continue to trigger non-SQL stuff with this particular regex, namely if we can confirm other rules catch a certain non-sqli-payload. If that's the case the test could be moved, or simply removed if need be.

[theseion]

Thanks @dune73! I checked your old rule and the current rule is just written a bit nicer but the basic building blocks are the same (no additional or removed parts, AFAICT).

I agree with @fzipi that we should move those two statements out of the rule, possibly adding them somewhere else (although I highly suspect that our shell injection rules are much better than what those two statements can cover). Then we move the tests that have nothing to do with SQL injection to where they make sense and, finally, add tests that cover the different cases represented by the rule.

[dune73]

The disassembly was done by @franbuehler.

The splitting and removal of nonsense certainly makes sense. If you feel our existing shell rules cover everything present here, then I see no reason to keep it around. Redundant coverage is not a core feature. :)

[theseion]

There is no test that covers [\"']\s*?*\s*?\w+\W+["'], removing it has no effect on tests.

The following tests cover [\"']\s*?[^?\w\s=.,;)(]++\s*?[(@"']*?\s*?\w+\W+\w, but actually do a few different things:

• 6: batch script?
• 7-10: shell injection
• 11-14: exec on fs (sqlserver)
• 15-16: data dump (mysql)
• 17: batch script
• 18: XSS
• 19: sql injection (mysql)
• 20: sql injection (sqlserver)
• 21: batch script
• 22: XSS

As of now I think that we need to have a statement that covers 19 and 20. The others don't belong in this rule.

[theseion]

Oh and BTW, we should do the same deconstruction for 942180 and 942340, which are both part of the same "sql authentication bypass" group.

[theseion]

The tests are covered as follows:

• 6: batch script? -> covered by multiple sqli rules, and also 941130
• 7-10: shell injection -> covered by 932105, 932100, 932130
• 11-14: exec on fs (sqlserver) -> covered by 942190
• 15-16: data dump (mysql) -> covered by 942480
• 17: batch script -> covered by 932115
• 18: XSS -> no real XSS payload, not detected
• 19: sql injection (mysql) -> covered by 942100-14
• 20: sql injection (sqlserver) -> covered by 942280
• 21: batch script -> covered by 941180
• 22: XSS -> covered by 941160

[fzipi]

942180 and 942340,

Is this for v4?

[theseion]

942180 and 942340,

Is this for v4?

Not strictly, but I though it would make sense to clean those up as well. Since I'm in the flow... ;)

[theseion]

I've also removed matches for find_in_set and union ... all, since they are already covered by other rules:

find_in_set\s*?( -> covered by 942150
union\s*?\s*?[([]?\s?select\s+
union\s*?all\s*?[([]?\s?select\s+
union\s*?distinct\s*?[([]?\s?select\s+
union\s*?[(!@]?\s?[([]?\s?select\s+ -> all covered by 942480

[theseion]

@fzipi you're right. It's not a good idea to do everything now. I've opened a separate issue for clean up rules 942180 and 942340.