Add isolated PCRE match limits as a layer of ReDoS defense by brandonpayton · Pull Request #2736 · owasp-modsecurity/ModSecurity (original) (raw)

There are a few design assumptions here that I think might be worth considering.

The population of your two new variables is 'first wins' -- i.e. they only get set if they have not already been set.

In general ModSecurity has a 'last wins' approach. This is true for variables like MATCHED_VAR and for pcre API matches as well (a repeated group like (a.c)* with input 'abcadc' will return only 'adc' for that capture group). Using 'last wins' would also give the admin slightly more flexibility for collecting information. I.e. it's possible for the admin to either do something with the changing variable periodically, whereas with 'first wins' that additional avenue of inquiry doesn't work.

If we continue with only holding a single value, I would suggest we use 'last wins' instead. Counterarguments are welcome.

But this bleeds into some bigger questions.

I'm somewhat hesitant about the proposed new variable RX_ERROR_RULE_ID.

This is not something ModSecurity has done before. Default rule-specific processing information tends to be communicated through ModSecurity's Debug Log.

I'm not necessarily rejecting the concept of maybe providing this sort of information through a variable. After all, some installations may prefer to run without the debug log engaged, and having some basic information available in such cases may be helpful.

However, if we do wish to pursue that goal, we might wonder if we really want to restrict the admin to only knowing about 1 of possibly multiple failures. Also, we may want to think more broadly: a processing failure that is related to a specific rule may be relevant to other types of operators besides rx. Maybe we should really be considering a new variable in the form of a collection like RULE_ERROR where each key is the rule id where a failure was seen, and the value is a fixed text describing the error.

I'm not sure I've thought this through fully; and implementing an idea like that might be better done separately, as then it would have better visibility with (and potential input from) the community.

As an immediate measure to get your central goal moving forward, I'm wondering if it might be fruitful to limit the near term change to the existing model: set a single signalling variable (in ModSecurity v2, this was called MSC_PCRE_LIMITS_EXCEEDED) and leave additional information communication to the Debug Log.

Thoughts and counterarguments are welcome.