SpiderLabs / owasp-modsecurity-crs

OWASP ModSecurity Core Rule Set (CRS) Project (Official Repository)
https://modsecurity.org/crs
Apache License 2.0
2.45k stars 725 forks source link

Improve SQL error leakage rule (970003/951100) #322

Closed zinoe closed 4 years ago

zinoe commented 8 years ago

As part of the CRS3 paranoia project, we will be discussing possible stricter siblings for some rules. The goal of this series of issues is to discuss the exact details.

This issue brings up rule 970003 (2.2.x) / 951100 (3.0.0-rc1). Information on the proposed changes can be found here.

Please add your thoughts, proposals or concerns in the comment section.

csanders-git commented 8 years ago

This one can go either way. My concern is that in general all of the words in sql-errors.data should be accounted for as part of the following rules. Really the only time this shouldn't trigger is when the user has turned off a specific sql backend in the setup. As a result it should ALWAYS increase the anomaly score if 951100 is triggered unless a user has overridden such events occurring.

dune73 commented 8 years ago

This rule is special as it is a pre-check using @pmf with more granular followup checks using regexes following in subsequent rules.

After a chat session on Aug 2, 2016, @csanders-git summarised the resolution as follows:

#322 - Improve SQL error leakage rule (970003/951100)
We are updating this file to remove elements as part of setup.conf updates this stricter sibling will be moved 3.1 and add a comment about restructuring the file

Full chat coverage for this issue:

22:16 < dune73> That's a fairly simple split of a data file. Simply takes some time but otherwise doable.
22:16 < csanders_> i don't think it is really that useful
22:16 < lifeforms> at this point it's a no-op I think
22:16 < csanders_> esp cause the deafult now is that all those rules will be on
22:16 < lifeforms> yes
22:16 < lifeforms> exactly
22:17 < dune73> I do not really follow you ...
22:17 < lifeforms> in PL1, sql-errors.data is used as a pmf pre-check
22:17 < lifeforms> and afterwards there are some regexps to let the correct rule match, i.e. msaccess, oracle, db2...
22:18 < lifeforms> I think it's a little bit pointless to separate all those errors by DB engine anyway
22:18 < csanders_> we're going to add them as tags so they can be removed more efficently via SecruleRemoveRuleByID
22:18 < lifeforms> so this stricter sibling would just block instantly even if the user has excluded the msaccess/oracle rules in pl1
22:18 < dune73> I see. I underestimated the complexity. Probably a complete rewrite of the whole rulefile is due.
22:19 < dune73> RuleByTag, you mean?
22:19 < lifeforms> I would probably just use the data file and get rid of the regexps at all
22:19 < lifeforms> just, check this data file, done
22:19 < lifeforms> why would I go to the trouble of NOT matching postgres errors
22:19 < lifeforms> if i'm not running it, my response won't have those errors
22:19 < dune73> But then you can't tag them anymore. Split the rule file by software?
22:19 < csanders_> well the thing is then we can't remove by tag
22:20 < csanders_> we could split the rule files out
22:20 < csanders_> *data
22:20 < lifeforms> yeah I know, but, why would you feel the need to spend time of your work day into NOT blocking that error for that software that you don't run so you'll never have the FP anyway
22:20 < csanders_> but I think at this point we should probably move it to 3.1 and just remove the checks from the setup.conf
22:20 < dune73> I agree that @pmf is really fast.
22:21 < lifeforms> oh, it's not like this
22:21 < lifeforms> I am wrong
22:21 < lifeforms> sql-errors.data is not a complete pre-check with all the errors
22:21 < lifeforms> the regexps are much more specific
22:21 < lifeforms> sql-errors.data has stuff like
22:21 < lifeforms> sybase
22:21 < lifeforms> Error
22:21 < lifeforms> Driver
22:21 < lifeforms> so....
22:21 < lifeforms> it's the regexps that really do th heavy lifting right now
22:21 < lifeforms> in 951
22:22 < lifeforms> so this rule file is probably fine
22:22 < lifeforms> the data file just skips the regexps for performance
22:22 < lifeforms> and we shouldn't block on it
22:22 < csanders_> mmhm, and that is probably due for an update we get those regexs from our DB team
22:22 < lifeforms> so, I'll do a minimal incision on the 951 file to tag the rules and remove the tx vars, and not do this sibling
22:22 < csanders_> i agree
22:23 < csanders_> dune73?
22:23 < dune73> How about the naked @pmf is the sibling?
22:23 < lifeforms> otherwise we'll have to really rework the data file to remove small lines from it
22:23 < lifeforms> yeah but it has
22:23 < lifeforms> 'Driver'
22:23 < lifeforms> 'Warning'
22:23 < lifeforms> :")
22:23 < dune73> Yes, remove the small lines you mention.
22:23 < lifeforms> I don't know. seems bad
22:23 < lifeforms> yeah but then well need another dataa file for them
22:23 < lifeforms> and patch the PL1 rules
22:23 < lifeforms> because we'll break our pre-check
22:24 < lifeforms> if we remove those little lines
22:24 < lifeforms> so then we have two data files
22:24 < lifeforms> well, could be intresting
22:24 < dune73> Redundant data files. ;(
22:24 < lifeforms> it would be a guard for us having bugs in 951
22:24 < lifeforms> a possible bug is that we put 'supplied argument is not a valid MySQL' in the data file but not in a regexp
22:24 < lifeforms> that could definitely use some review
22:24 < lifeforms> but we'd have 2 .data files
22:24 < lifeforms> anyway, could be useful if our regexps suck, which is definitely a possibility, but would complicate maintenance
22:25 < lifeforms> though in a pmf you can add comments
22:25 < csanders_> i'm going to move the ticket to 3.1 and add a comment about restructuring the file
22:25 < lifeforms> so, a copy-paste of this data file could be useful in future
22:25 < dune73> It's really a special form of a rule file already with the pre-check. Let's move it to 3.1 if ever.
22:25 < lifeforms> but I think we better just add some tests for 951 and call it a day
22:25 < dune73> Agreed @lifeforms.
22:26 < lifeforms> I wonder if pmf is really so much faster
22:26 < dune73> Faster than @rx? Yes, I think it is.
22:26 < csanders_> its quite fast compared
22:26 < csanders_> aho-korsik is a very fast algorithem
22:26 < csanders_> (spelled that wrong)
22:27 < csanders_> PCRE regex setup takes lots of time to setup
dune73 commented 7 years ago

The plan is to restructure the data file and maybe add a stricter sibling for 3.1 or so.

fgsch commented 4 years ago

This issue has timed out as it has not received any update in over 2 years. If this is still a problem please open a new issue.