Owasp-modsecurity-crs: Rule 942360 false-positive on Keyword alter

[shadow4040]

False Positive because of the keyword: alter (from SQL)
/modsecurity-crs/REQUEST-942-APPLICATION-ATTACK-SQLI.conf"] [line "81"] [id "942360"] [rev "2"] [msg "Detects concatenated basic SQL injection and SQLLFI attempts"] [data "Matched Data: Alter found within ARGS:request.debtors.privatePerson.birthName: Alter XYZ "]

[dune73]

Thank you for submitting @mostworld77.

Unfortunately, I can not reproduce the alert. Could you please submit the exact / minimal payload that triggers the rule. Ideally as a CLI call with curl. Alternatively a complete audit log entry.

[franbuehler]

I was able to reproduce this false positive:
curl localhost:80?name=+Alter+XYZ

The problem is the space before the keyword alter:
Alter XYZ

The matching regex is this one:
^[\W\d]+\s*?alter\b from:
https://github.com/SpiderLabs/owasp-modsecurity-crs/blob/v3.1/dev/util/regexp-assemble/regexp-942360.data#L35

We could extend this regex with table, column and other valid keywords?
alter table or alter column are possible statements. Maybe even more.
But because I'm not an SQL professional, I'm not sure if we open a security hole.

[dune73]

Thank you for digging into this @franbuehler.

So this is a very similar case to #988. It is unfortunate that "Alter" is a German word (here), but the whole group of keywords are English words. I mean that's what SQL is all about.

We may want to cover this FP here with the same PR.

I see where your suggestion is heading. And I also see the inherent risk if we ignore an exotic SQL dialect and boom. Maybe rather work with one of the alternatives you mentioned in #988.

[franbuehler]

Why not move these two regexes to PL2 to a stricter sibling 942361?
^[\W\d]+\s*?alter\b
^[\W\d]+\s*?union\b

AND also extend these regexes on PL1 with possible keywords:
^[\W\d]+\s*?alter\b\s*(table|column)
^[\W\d]+\s*?union\b\s*(all|select)

This way we still have some coverage on PL1.

I could work on a more specific proposal for regexes on PL1.

[dune73]

Yes, I thought of this too. But would not we want to cover the other keywords / regexes too? Are not they equally likely to result in FPs?

^[\W\d]+\s*?create\b
^[\W\d]+\s*?delete\b
^[\W\d]+\s*?desc\b
^[\W\d]+\s*?insert\b
^[\W\d]+\s*?load\b
^[\W\d]+\s*?rename\b
^[\W\d]+\s*?select\b
^[\W\d]+\s*?truncate\b
^[\W\d]+\s*?update\b

And then we suddenly feel like interfering with @csanders-git's plan to revise the complete set of SQLi rules.

Or would it be more suitable to just follow your suggestion as a pragmatic short-term solution for v3.1?

[spartantri]

All these regex seem very false positive, false negative like comments should be mitigated by other rules, all of them include common english words for PL1 don't you think?

I like the @franbuehler suggestion for PL1 to reduce false positives in free text fields, maybe also adding a t:replaceComments,t:cmdLine and change those \s* to a single \b could be a good addition
^[\W\d]+\b?alter\b(table|column)
^[\W\d]+\b?union\b(all|select)

[dune73]

Yes, it's actually surprising there are not more reports of FPs here.

I like the \b before the keyword, but I think we need to allow for a (single) whitespace in front of the bracket. t:cmdLine reduces whitespace to a single char.

Do we need the t:replaceComments? I mean, do SQL parsers accept / understand this? If yes, then we absolutely need t:replaceComments.

[spartantri]

Sounds like a plan, the comments part is not necessary there are other rules to handle such things.

^[\W\d]+\s\balter\b(table|column)
^[\W\d]+\s\bunion\b(all|select)

[dune73]

You still don't do whitespace after the \b. Or do I get this wrong?

But whatever, @franbuehler promised to do a PR based on the agreed course of action; only covering alter / union for the time being.

[spartantri]

yep, you're right, also using non capturing groups maybe better as capturing those keywords may not be really required

[shadow4040]

Is there a workaround for this case? thanks

[dune73]

Multiple variants exist: https://www.netnea.com/cms/apache-tutorial-8_handling-false-positives-modsecurity-core-rule-set/

Quick and dirty: SecRuleRemoveById 942360

[shadow4040]

thanks, but as you discussed, is it not possible to exclude only the keyword "alter" from this rule.
Does the rule disabling not opening other security attack surfaces?

[franbuehler]

I looked at the possible valid keywords for union and alter.
The union case is simple:

^[\W\d]+\s*?union\s*all\b
^[\W\d]+\s*?union\s*select\b
^[\W\d]+\s*?union\s*distinct\b

But alter is really difficult.
I checked different dialects: MySQL, MSSQL, Oracle, PLSQL, PostgreSQL, DB2 and there are 76 valid keywords after alter:

^[\W\d]+\s*?alter\s*aggregate\b
^[\W\d]+\s*?alter\s*application\s*role\b
^[\W\d]+\s*?alter\s*assembly\b
^[\W\d]+\s*?alter\s*asymmetric\s*key\b
^[\W\d]+\s*?alter\s*audit\b
^[\W\d]+\s*?alter\s*authorization\b
^[\W\d]+\s*?alter\s*availability\s*group\b
^[\W\d]+\s*?alter\s*broker\s*priority\b
^[\W\d]+\s*?alter\s*bufferpool\b
^[\W\d]+\s*?alter\s*certificate\b
^[\W\d]+\s*?alter\s*collation\b
^[\W\d]+\s*?alter\s*column\b
^[\W\d]+\s*?alter\s*conversion\b
^[\W\d]+\s*?alter\s*credential\b
^[\W\d]+\s*?alter\s*cryptographic\s*provider\b
^[\W\d]+\s*?alter\s*database\b
^[\W\d]+\s*?alter\s*default\b
^[\W\d]+\s*?alter\s*domain\b
^[\W\d]+\s*?alter\s*endpoint\b
^[\W\d]+\s*?alter\s*extension\b
^[\W\d]+\s*?alter\s*external\b
^[\W\d]+\s*?alter\s*event\b
^[\W\d]+\s*?alter\s*foreign\b
^[\W\d]+\s*?alter\s*fulltext\b
^[\W\d]+\s*?alter\s*function\b
^[\W\d]+\s*?alter\s*group\b
^[\W\d]+\s*?alter\s*histogram\b
^[\W\d]+\s*?alter\s*index\b
^[\W\d]+\s*?alter\s*instance\b
^[\W\d]+\s*?alter\s*language\b
^[\W\d]+\s*?alter\s*large\b
^[\W\d]+\s*?alter\s*logfile\s*group\b
^[\W\d]+\s*?alter\s*login\b
^[\W\d]+\s*?alter\s*mask\b
^[\W\d]+\s*?alter\s*master\s*key\b
^[\W\d]+\s*?alter\s*message\s*type\b
^[\W\d]+\s*?alter\s*method\b
^[\W\d]+\s*?alter\s*module\b
^[\W\d]+\s*?alter\s*nickname\b
^[\W\d]+\s*?alter\s*operator\b
^[\W\d]+\s*?alter\s*package\b
^[\W\d]+\s*?alter\s*partition\b
^[\W\d]+\s*?alter\s*permission\b
^[\W\d]+\s*?alter\s*procedure\b
^[\W\d]+\s*?alter\s*queue\b
^[\W\d]+\s*?alter\s*remote\b
^[\W\d]+\s*?alter\s*resource\b
^[\W\d]+\s*?alter\s*role\b
^[\W\d]+\s*?alter\s*route\b
^[\W\d]+\s*?alter\s*schema\b
^[\W\d]+\s*?alter\s*search\b
^[\W\d]+\s*?alter\s*security\b
^[\W\d]+\s*?alter\s*server\b
^[\W\d]+\s*?alter\s*service\b
^[\W\d]+\s*?alter\s*sequence\b
^[\W\d]+\s*?alter\s*session\b
^[\W\d]+\s*?alter\s*symmetric\s*key\b
^[\W\d]+\s*?alter\s*stogroup\b
^[\W\d]+\s*?alter\s*table\b
^[\W\d]+\s*?alter\s*tablespace\b
^[\W\d]+\s*?alter\s*text\b
^[\W\d]+\s*?alter\s*threshold\b
^[\W\d]+\s*?alter\s*trigger\b
^[\W\d]+\s*?alter\s*trusted\b
^[\W\d]+\s*?alter\s*type\b
^[\W\d]+\s*?alter\s*usage\b
^[\W\d]+\s*?alter\s*user\b
^[\W\d]+\s*?alter\s*view\b
^[\W\d]+\s*?alter\s*work\b
^[\W\d]+\s*?alter\s*workload\b
^[\W\d]+\s*?alter\s*wrapper\b
^[\W\d]+\s*?alter\s*xml\s*schema\b
^[\W\d]+\s*?alter\s*xsrobject\b

This means for us, the source file grows by 76 more lines and the resulting regex is also larger:

(?:^[\W\d]+\s*?(?:alter\s*(?:a(?:
(?:pplication\s*rol|ggregat)e|s(?:ymmetric\s*ke|sembl)y|u(?:thorization|dit)|vailability\s*group)|c(?:r(
?:yptographic\s*provider|edential)|o(?:l(?:latio|um)|nversio)n|ertificate)|s(?:e(?:rv(?:ice|er)|curity|que
nce|ssion|arch)|ymmetric\s*key|togroup|chema)|t(?:(?:abl(?:espac)?
|yp)e|r(?:igger|usted)|hreshold|ext)|m(?:e(?:ssage\s*type|thod)|as(?:ter\s*key|k)|odule)|
(?:r(?:e(?:sourc|mot)|o(?:ut|l))|nicknam|queu)e|p(?:a(?:rtition|ckage)|ermission|rocedure)|l(?:og(?:file
\s*group|in)|a(?:ngua|r)ge)|e(?:xte(?:nsion|rnal)|
(?:ndpoi|ve)nt)|b(?:roker\s*priority|ufferpool)|f(?:u(?:lltext|nction)|oreign)|d(?:atabase|efault|omain)|
x(?:ml\s*schema|srobject)|w(?:ork(?:load)?
|rapper)|in(?:stance|dex)|us(?:age|er)|histogram|operator|group|view)|u(?:nion\s*(?:
(?:distin|sele)ct|all)|pdate)|(?:(?:trunc|cre)at|renam)e|(?:inser|selec)t|de(?:lete|sc)|load)\b|(?:(?:
(?:trunc|cre|upd)at|renam)e|(?:inser|selec)t|de(?:lete|sc)|alter|load)\s+
(?:group_concat|load_file|char)\s?\(?|[\d\W]\s+as\s*?[\"'`\w]+\s*?from|[\s(]load_file\s*?\(|
[\"'`]\s+regexp\W|end\s*?\);)

My Pull Request would be ready now. But I am now not quite sure how to proceed.
With so many possibilities after alter, it is possible that we forget a statement. But at least we have a better coverage than without any alter statement.
Should we really add these 76 alter statements to PL1 @dune73, @spartantri?

If yes, I will do it.

[dune73]

We have FPs in the default installation. Your work allows us to keep this part of the rule in PL1. If we do not do that, I think we need to push it to PL2 which lowers the security at PL1.

There is a fair chance that we miss future development in the DBMS and a new keywords show up. However, we would still cover the basic alter keyword at PL2 as a stricter sibling, so I think we could live with that shortcoming (let's rather cover 98% of an issue than 0% because we can not reach 100%).

If you do the PL, please include the name of the DBMS or maybe even the links to their documentation. That would help with future updates of the list of keywords.

[dune73]

@mostworld77 : No, there is no simple solution to exclude a keyword from a regular expression. That would mean to alter the regular expression and altering the regular expression typically means to edit the rule set, which prevents you from a simple upgrade path.