Set TX:MSC_PCRE_LIMITS_EXCEEDED variable is limits exceeded

[airween]

There was a PR (#2737) which fixes an earlier shortcoming, namely that the @rx (and @rxGlobal) operator(s) do not handle PCRE limit issues.

I didn't follow it, but unfortunately seems it implements a different behavior from the other engine (mod_security2).

In mod_security2's reference the relevant behavior is explained as:

MSC_PCRE_LIMITS_EXCEEDED: Set to nonzero if PCRE match limits are exceeded. See SecPcreMatchLimit and SecPcreMatchLimitRecursion for more information.

May be the documentation is a bit ambiguous, but it means the TX.MSC_PCRE_LIMITS_EXCEEDED will be set, not the "regular" MSC_PCRE_LIMITS_EXCEEDED variable.

This patch corrects this behavior.

Please note, that the introduced variable is not mentioned in v3's documentation.

Why is this important?

The OWASP Core Rule Set team has a plan for the rule set to handle these types of errors. Without the compatibility, we can't do that.

[airween]

Cc @brandonpayton.

[brandonpayton]

Thanks for noticing this, @airween. I was not aware of it. Perhaps we should entirely remove the standalone variables added in #2736 and replace them with TX collection entries. What do you think?

[airween]

Thanks for noticing this, @airween. I was not aware of it. Perhaps we should entirely remove the standalone variables added in #2736 and replace them with TX collection entries. What do you think?

I do not know yet. If a rule writer can make a rule both for mod_security2 and libmodsecurity3 (the two are compatible), then - I think - it's not distracting.

[brandonpayton]

I think it is a little confusing to have two different sets of vars indicating MSC PCRE errors. At least I personally find it confusing. :)

[dune73]

I do not see a need why compatibility had to be broken here. Anything bringing it back is good from my perspective.

[airween]

@brandonpayton, @dune73 - thanks guys.

In the meantime, a new issue appeared: #2902, so I think there are three votes to remove the regular variable.

@martinhsv - what would be the better: should I add a new commit to this PR which removes the variable by #2736, or (if this PR is fine) after merging this, I will create a new PR?

[martinhsv]

I have made some comments in #2902 .

In brief: to me it would seem confusing to have pcre error flags set by the engine to use TX:, when we don't do that with any other such errors (REQBODY_ERROR, the many MULTIPART_ errors, etc.)

In other words, the existing v3 functionality may reasonably be considered an improvement.

With the functionality as implemented in #2737 , TX: variables continue to be reserved for transaction variables that are created by individual rules -- either through direct naming or via the capture action).

(Of course, if left as is, the v3 doc and rule 200005 in modsecurity.conf-recommended need updates.)

For anyone wishing to have a single rule work in both v2 and v3, writing a single SecRule to do so is pretty straightforward.

[airween]

For anyone wishing to have a single rule work in both v2 and v3, writing a single SecRule to do so is pretty straightforward.

Sorry for the request, but could you show an example? mod_security2 does not support MSC_PCRE_LIMITS_EXCEEDED variable, only inside of the TX collection. That's our problem, that's why I created this patch.

[martinhsv]

Ugh. I see your point. I didn't think that through fully from the perspective of a v2 installation. Handling both v2 and v3 might indeed be annoying to try to do with a single SecRule. I'll give that a bit more thought.

[airween]

Ugh. I see your point.

No problem.

I'll give that a bit more thought.

Thanks. Let me know if I can help you anything.

[martinhsv]

All right. There is a way to do the test without creating this TX variable within the code. However, the simplest, non-contrived way that I could think of doing so, involves creating a TX variable within a chained rule.

Having a synonym for backwards-compatibility reasons isn't terrible. I'm ok with this and will proceed to merge.

[airween]

@martinhsv thanks.

Now what should we do with the "regular" MSC_PCRE_LIMITS_EXCEEDED variable? As the author of that patch mentioned, he would revoke that.

Should I prepare a PR?

[marcstern]

A good way to handle such inconsistencies would be to add the version number in a variable.
I personnally added it in an environment variable in hook_request_early (v2):
apr_table_set(r->subprocess_env, "ModSecVersion", MODSEC_MODULE_VERSION);
This allowed me to have rules analyzing MULTIPART_PART_HEADERS compatible with all versions.

Using an environment variable also allows to perform some checks with mod_header.