[feature request] Macro expansion when counting/comparing variables

[std0ut]

With the changes to CRS_41_SQLi a few versions ago where multiple rules were combined into a single regex, I've often found it would be useful to be able to do this:

SecRule &TX:973338-OWASP_CRS/WEB_ATTACK/XSS-ARGS:%{tx.1} "@ge 1" "setvar:tx.anomaly_score=-%{tx.critical_anomaly_score}"

For example, in rule 973338, there are several regex branches we want to keep, but one of them is overly broad for our purposes (i.e. [\s\S]style[\s\S]), and I'd like to write a score adjustment only if that rule is matched, and the args value is "\wstyle_sheet.jsp$."

These are the current work-arounds I know of, but none of them are ideal:

1.) I could remove the rule, but that prevents other less aggressive branches from matching as well.
2.) I could write a score adjustment without checking that the rule matches, but this opens the possibility that future CRS changes may change the rule and points would be given back arbitrarily.
3.) I could rewrite the rule (e.g. remove in CRS_99 and add modified rule to CRS_15), but that seems like a kludge, and if I only want to rewrite for specific URIs, ARGS, etc, it gets even messier (end up adding three or more rules rules, instead of one score adjustment).

If there is a way to do this in 2.9.X and I'm just messing up the syntax, PLEASE let me know, but debug suggests it's not being expanded. I've run into this issue 5-10 times over the last year or two and I figured it's finally time to just submit a request.

[csanders-git]

So we went down a rabbit hole over on the CRS side and ended up at this problem.
SpiderLabs/owasp-modsecurity-crs#400

Ivan even says in his book "If you encounter a part of the rule language that does not support this feature (expansion). If that happens, you are advised to report the problem to the issue tracker. Initially the support was added only to places where it was needed. By popular demand the support expanded over time."

It looks like both variables and actions add support for this by implementing expand_macros(). In theory it shouldn't be outrageous to add this functionality to what i'll call collection indexes. Might take a stab at it tmrw or if you want to that would also be nice.

[marcstern]

ipMatch & validateByteRange also don't accept expansion on operators.
Is there any reason?
Can we add this to 2.9 also?

[victorhora]

@marcstern I would suggest submitting a pull request. I think the main concern is indeed performance as @zimmerle pointed out. If there's no clear performance impact when handling a ruleset that uses macro expansion somewhat heavily (e.g. CRS).

If performance remains a concern, I think we could have this as a compile time option (i.e. --extended-macro-expansion-operators) and then adding this to all remaining operators as optional.

Some references:

https://github.com/SpiderLabs/ModSecurity/blob/v3/master/src/operators/str_match.h#L29-L35
https://github.com/SpiderLabs/ModSecurity/blob/v3/master/src/operators/operator.cc#L105-L124

[marcstern]

Actually, expansion should be performed only if we detected - at config time - that there is something to expand. This would be as effective as the compile flag, more flexible/generic, and would speed up parsing of currently expanded operators.

[marcstern]

Before submitting a PR (actually 2 PR), I'd like to have some feedback about my proposal.
Logic (for v2):

1. in msre_rule_create(), we check if the operator param contains "%{" and set a flag accordingly
2. in all exec() functions, we only try to expand if the flag is set
   Does anybody see a problem with this?

Structural detail: this is merely what the rule->re_precomp means.
Three solutions:

1. we generalise this flag (the name would become a bit illogical)
2. we add another flag named "possible_expansion"
3. if we add the other flag, we also remove the "re_precomp" one
   Solution 3 seems the cleanest one

Opinions?

[victorhora]

Actually, expansion should be performed only if we detected - at config time - that there is something to expand. This would be as effective as the compile flag, more flexible/generic, and would speed up parsing of currently expanded operators.

@marcstern if I'm not mistaken, this is already the case with v3. Macro-expansion is handled at the parser and only performed if there really is something to expand.

[marcstern]

Actually, expansion should be performed only if we detected - at config time - that there is something to expand. This would be as effective as the compile flag, more flexible/generic, and would speed up parsing of currently expanded operators.

@marcstern if I'm not mistaken, this is already the case with v3. Macro-expansion is handled at the parser and only performed if there really is something to expand.

possible, but not in v2