Rule 920150 (Unmatched multipart boundry) is redundent to modsecurity.conf-recc

[csanders-git]

It is also more of a modsec rule than a CRS rule i'd vote to remove it.

Within modsecurity.conf-reccomended

SecRule MULTIPART_UNMATCHED_BOUNDARY "!@eq 0" \
"id:'200004',phase:2,t:none,log,deny,msg:'Multipart parser detected a possible unmatched boundary.'"

Within CRS 3

#
# Multipart Unmatched Boundary Check
#
# -=[ Rule Logic ]=-
# Check for the MULTIPART_UNMATCHED_BOUNDARY flag and alert
#
# -=[ References ]=-
# https://sourceforge.net/apps/mediawiki/mod-security/index.php?title=Reference_Manual#MULTIPART_UNMATCHED_BOUNDARY
#
SecRule MULTIPART_UNMATCHED_BOUNDARY "!@eq 0" \
  "msg:'Multipart parser detected a possible unmatched boundary.',\
  severity:'CRITICAL',\
  id:920150,\
  ver:'OWASP_CRS/3.0.0',\
  rev:'1',\
  maturity:'8',\
  accuracy:'8',\
  phase:request,\
  block,\
  t:none,\
  tag:'application-multi',\
  tag:'language-multi',\
  tag:'platform-multi',\
  tag:'attack-protocol',\
  tag:'OWASP_CRS/PROTOCOL_VIOLATION/INVALID_REQ',\
  tag:'CAPEC-272',\
  setvar:'tx.msg=%{rule.msg}',\
  setvar:tx.anomaly_score=+%{tx.critical_anomaly_score},\
  setvar:'tx.%{rule.id}-OWASP_CRS/PROTOCOL_VIOLATION/INVALID_REQ-%{matched_var_name}=%{matched_var}'"

[dune73]

I do not think we should rely on the 200K range of recommended rules to be active at all. Even more so as the selection of recommended rules distributed with ModSec is completely arbitrary (and alternative implementations of the rule engine might not have the same recommended rules.

I'd suggest to do a SecRuleRemoveByID instead. That might be controversal as well, though.

P.S. This one is not the only redundant one by the way.

[csanders-git]

From a performance standpoint its ridiculous to evaluate this again. Additionally, if a user has configured their modsecurity using the recommended they might have unblocked that action only to have it blocked again. My feeling is to leave things that deal with processing and internal processing errors to modsecurity.conf and have protocol anomaly and security rules here.

From a testing perspective the above means that in most environments we'll never be able to trigger this rule.

[lifeforms]

A single variable comparison won't have any effect on performance. I don't mind this rule, we have bigger challenges. If there's any chance of people not having it in the standard conf, it's nice to keep it.

[dune73]

@csanders-git : I see, the selection of recommended rules is not completely arbitrary then. (Just not completely obvious :)

Still I agree with @lifeforms: It is a vital rule and we need to be certain it is really active. We do not lose anything by issuing SecRuleRemoveByID on the 2000X rules. It will feel a bit bossy, but it does away with the "blocked again" problem.

[csanders-git]

I really don't agree here. If this was the case we should be enabling XML parsers with CRS, etc. The CRS rules even state that they don't configure ModSecurity in the setup.conf. Explicit removal of a previous rule which a user hopefully configured seems horrible. Like, you thought u fixed that false positive but wait, it's back.

[lifeforms]

I think we shouldn't go hacking around with SecRuleRemoveById, that is ugly and hard to comprehend.

I don't have a strong opinion though. What about just adding in the documentation + install instructions that we depend on modsecurity.conf-recommended. And if people are upgrading from an old install, check that their modsecurity.conf is up to date (I had to find out by accident that I had an old one). If we do that, then fine to remove this rule.

[dune73]

@csanders-git : You are right, this is a can of worms.

Need to think.

[csanders-git]

I'm going to mark it beyond 3.0.0 give us some time to stew :)

[dune73]

Good plan.

[lifeforms]

I noticed that setup.conf.example contains:

# You should use the modsecurity.conf-recommended file that comes with the
# ModSecurity source code archive.

So I think it's fine to rely on the rule in that file and delete this one.

[csanders-git]

Alright, so i'll take a look at removing this for v3.2

[dune73]

During the monthly CRS chat, we decided to drop this issue as we are not enough committed to really come up with a clean solution. Seems we can live with the situation as is.

Meeting minutes: #1671 (comment)

If somebody has the time and interest to solve this for good, then please reopen.