Should we cancel Macro Expansion support to tag action?

[weliu]

This Macro Expansion support was discussed in #324, but I found that the example in that issue is not very convincing:
tag:'http://www.owasp.org/index.php?title=ModSecurity_CRS_RuleID-%{rule.id}
The rule id can be determined during writing this rule, so there is no need to get its value at run time.

I think we may cancel this Macro Expansion support to tag action because:
A. It impacts performance. If we remove this support, it can reduce the overhead of tag action with each hit rules, and SecRuleRemoveByTag/ruleRemoveByTag and SecRuleUpdateTargetByTag/ruleRemoveTargetByTag can be more efficient.

B. The scenario to use this Macro Expansion in tag is very rare. I only see two place in CRS:
# SecDefaultAction "phase:1,log,auditlog,redirect:'http://%{request_headers.host}/',tag:'Host: %{request_headers.host}'"
\ # SecDefaultAction "phase:2,log,auditlog,redirect:'http://%{request_headers.host}/',tag:'Host: %{request_headers.host}'"

C. If we do need this, we can use msg action instead which also supports Macro Expansion.

I can submit a PR if we decide to cancel Macro Expansion support to tag action eventually.

[zimmerle]

Hi @weliu,

For sure removing the run time calculation on the tags will speed up things, not sure however if it will be a big boost or not. As we don't consider the macro expansion on SecRule(*)ByTag.

I complete agree with your argument. I don't see why have the tags run-time-computed. But I also don't want to brake the compatibility fro those who are using. Do you thing you can have it as a compilation flag? Something like: --enable-tags-with-macros or --enable-dynamic-tags we can make it disable by default.

[weliu]

Hi @zimmerle ,

I am afraid it would be difficult for me to add such a compilation flag. Because changing the behavior of tag action involves parser modification. I am no expert on bison, is it possible to add such a flag in seclang-scanner.ll and seclang-parser.yy?
How about adding a new action 'dtag' which means dynamic tag, and change current 'tag' action to configuration kind? Those who need macro expansion in tag only need a minor change in their rules.

[zimmerle]

Hi @weliu,

Sorry for the long delay. Let me share a little bit of my thinking specifically on how to improve tags.

Currently implementation

Currently we save the tags as a text string for each rule. Here is the detailed steps on the tags life cycle:

Loading

• The parser reads a tag action:
  https://github.com/SpiderLabs/ModSecurity/blob/1ecd9713061c3534626bf6a5f59d1c928c0c52bb/src/parser/seclang-scanner.ll#L567
• The semantic value of the tag is added here:
  https://github.com/SpiderLabs/ModSecurity/blob/1ecd9713061c3534626bf6a5f59d1c928c0c52bb/src/parser/seclang-parser.yy#L2842-L2845
• In the sequence tag is initialized here:
  https://github.com/SpiderLabs/ModSecurity/blob/v3/master/headers/modsecurity/actions/action.h#L41-L49

Execution

• The action is executed here:
  https://github.com/SpiderLabs/ModSecurity/blob/1ecd9713061c3534626bf6a5f59d1c928c0c52bb/src/actions/tag.cc#L59-L67
• Saving the tag name here:
  https://github.com/SpiderLabs/ModSecurity/blob/1ecd9713061c3534626bf6a5f59d1c928c0c52bb/headers/modsecurity/rule_message.h#L116

Rule removal

• Update target by tag:
  https://github.com/SpiderLabs/ModSecurity/blob/1ecd9713061c3534626bf6a5f59d1c928c0c52bb/src/rule.cc#L463-L475
• Remove target by tag:
  https://github.com/SpiderLabs/ModSecurity/blob/1ecd9713061c3534626bf6a5f59d1c928c0c52bb/src/rule.cc#L529-L537
  https://github.com/SpiderLabs/ModSecurity/blob/1ecd9713061c3534626bf6a5f59d1c928c0c52bb/src/rule.cc#L713-L723

Others

• The contains tag method:
  https://github.com/SpiderLabs/ModSecurity/blob/1ecd9713061c3534626bf6a5f59d1c928c0c52bb/src/rule.cc#L865-L872

The problem

The current implementation is not optimal for several reasons, including:

• The same given tag 'X' is allocated in memory for each rule increasing the general memory usage footprint.
• The comparison to check whenever a rule contains a tag or not is based on a text string compassion (slow).
• Tags strings are duplicated to be saved on the Rule Message, creating fragmentation problem.

How can we make it better

Given all that said, lets see how can we improve the code... Lets consider two different approaches:

1. Tags contains a macro expansion.
2. Tags does not contains a macro expansion.

We can use two complete different approaches based on the items listed above. Lets tackle the easiest one first.

Tags does not contains a macro expansion

The basic idea is to have the tags mapped to a number. Lets say:

Wordpress -> 1
Joomla -> 2
Magento -> 4
Magicart -> 8
MyCuteLittleApp -> 16

That will give us the opportunity to save within the rule instance a single number that will represents all the tags that a given rule contains, for instance:

SecRule ARGS "test" "id:1,tag:Wordpress"
SecRule ARGS "test" "id:2,tag:Wordpress,tag:Magicart"
SecRule ARGS "test" "id:3,tag:Wordpress,tag:Magicart,tag:Joomla"

In the rule with the id 1, the value of the m_tagNum will be: 1. The rule id 2 will be 9. Rule #3 will be 11.
The m_tagNum can be calculated by the bitwise operator | (or). Further to check if a rule contains a given tag or not, we don't have to iterate over the list of strings making comparatives. Instead, we can use the bitwise operator & (and) which will return zero whenever the rule does not contains o tag.

The number associated to each tag can be computed during the rules load. Where a table can be created to map each index to the tag name. The tag name can be a std::shared_ptr, so, while passing this information to the rule message, we don't have to duplicate the string avoiding memory fragmentation.

That modification will bring three benefits: reduce memory usage, avoid memory fragmentation, decrease the processing time.

Tags contains a macro expansion

I will write about it later. :)

That is not a big modification, but it is quite a challenging. Let me know if you are in the mood for this challenge. I can help you via hangout.

[weliu]

Hi @zimmerle, it is a great idea. In my opinion, we need to address 2 problems:

1. Since we have to go through all the rules to know all the tags, we must introduce a phase after all rules being parsed, so that we can build such a tag table. That means we have to add an API in rules.h, which brights a compatibility issue.
2. The total size of the tag table can only be get in that phase, so the standard bitset could not be used here. We have to implement our own run time bitset or introduce boost::dynamic_bitset.

Still we have to decide how to handle macro expansion in tag action, any comments on those issues?

[zimmerle]

it may be relate to #1734

[zimmerle]

Hi @weliu,

Hi @zimmerle, it is a great idea. In my opinion, we need to address 2 problems:

1. Since we have to go through all the rules to know all the tags, we must introduce a phase after all rules being parsed, so that we can build such a tag table. That means we have to add an API in rules.h, which brights a compatibility issue.

Not really. Regarding rules manipulation in general, we have two different use-cases:

1. When they are loaded.
2. In execution.

We better keep the heavier processing time to the item "1". The item "2" is executed per-request, so it means that it has a directly relation to latency and it is also executed repetitively.

2. The total size of the tag table can only be get in that phase, so the standard bitset could not be used here. We have to implement our own run time bitset or introduce boost::dynamic_bitset.

Hum... i am not sure about boost. We are avoiding to add it just for the sake of reduce the amount of dependencies.

Still we have to decide how to handle macro expansion in tag action, any comments on those issues?

Not yet ;) have some ideas, but i am still thinking the better approach.

[zimmerle]

As of 3.1-experimetal the tag computation is built on rules load time. The idea of having binary operations to identify tag is still valid and most likely will add a performance benefit.

We don't support macro expansion in the TAGs. The need for that is still questionable, given the amount of computation that is involved.

I am closing this issue because of changes in the code; the technical details listed here are no longer accurate. If you still need macro support expansion in TAGs, please open a new issue.