Loading Rule is not thread safe

[iosetek]

Describe the bug

A seclang_parser used for loading rules in ModSecurity uses global variables.
Due to that I was able to create a simple test proving that loading rules by different rule sets at the same time with multiple threads causes some random issues

https://github.com/SpiderLabs/ModSecurity/pull/2535/files

Logs and dumps

For trying to load the same rule SecAction "id:900000,phase:1,pass,nolog,setvar:tx.paranoia_level=1" multiple times I get different errors:

Examples:
'Rules error. File: <>. Line: 1. Column: 88. Expecting an action, got: SecAction "id:900000,phase:1,pass,nolog,setvar:tx.paranoia_level=1"'
'Rules error. File: <>. Line: 1. Column: 27. syntax error, unexpected Phase '
'Rules error. File: <>. Line: 1. Column: 1. syntax error, unexpected "," '

To Reproduce

Steps to reproduce the behavior:

Download the branch from my forked repository: https://github.com/iosetek/ModSecurity/tree/load_rule_multiple_threads
Build the project, enter test directory and run ./unit_tests
The example output looks like this:

Executing thread tests.
Test: 'Test 1 thread' succeeded.
Test: 'Test 3 overlapping load rule threads' failed.
thread [0] returned: ''
thread [1] returned: 'Rules error. File: <<reference missing or not informed>>. Line: 1. Column: 88. Expecting an action, got:  SecAction "id:900000,phase:1,pass,nolog,setvar:tx.paranoia_level=1"'
thread [2] returned: ''
Test: 'Test 3 non overlapping load rule threads (delay between)' succeeded.

Expected behavior

The expected behaviour is for loading rules to be thread safe so the failing test from my fork would work.

[zimmerle]

@iosetek The bison LALR parser is not thread safe. I don't think it is meant to be thread safe. Why do want to load it with multiple threads/process?

[iosetek]

@zimmerle Multiple threads are meant to be spawned in the environment to scan the upcoming requests. I couldn't figure out how to copy an existing ModSecurity::Rules object as it contains multiple raw pointers.

1. How would you suggest to have a multiple threads where each of them has one ModSecurity::Rules instance.
2. Is it the only place where ModSecurity is not thread safe? How about parsing multiple requests by a fixed number of threads. Can we use ModSecurity to scan two requests on two different threads at the same time?

[zimmerle]

@iosetek

The modSecurity life cycle is divided into different stages. The stage that the rules are loaded is not threading safe by design. The rules are meant to be loaded in a serial fashion, one file at a time, line by line. As the order of the rules matter, it is important to load it in the user-specified sequence. Changing the order expressed by the user may lead to a behavior that was not what the user expects. Therefore, as the rules need to be loaded sequentially, parallelism on that process may not add a benefit **. SecRule is a script-ish language, not a pattern match; one may say it is turing complete.

Once the rules are loaded (and pre-compiled?), multiple requests can share the same RulesSet object. Leading to parallelism while addressing different requests in different processes or threads. If the rules are loaded in the main process, before the fork, the physical memory will be shared among the different processes.

In the project source, we have different examples that illustrate some utilization of ModSecurity; you may want to have a look at multiprocess_c, there we treat a use case very similar to the one that you want to address -

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/examples/multiprocess_c/multi.c#L111-L131

** To a benefit, external downloads could be parallelized and cached. Not related to this particular issue.

[iosetek]

@zimmerle Could I reach you on slack or any other chat? I feel it would be quicker to solve this and I could paste the conclusion here.

I am afraid that ModSecurity Rules are not thread safe not only during the process of reading files.

In ModSecurity/src/transaction.cc we can see that there is called m_rules->incrementReferenceCount(); in two places. This method looks like this:

void Rules::incrementReferenceCount(void) {
    this->m_referenceCount++;
}

This is a simple int so I don't think it's thread safe here.

Apart from that in my case I have multiple RuleSet instances. Each thread has its own instance. Even though those are different instances ModSecurity won't allow loading rules by different rule sets at the same time. You can check that in the PR I attached.

I believe we could share one RuleSet instance among all our threads but for now it seems that RuleSet is not thread safe completely (looking at the Transaction example).

Could I reach you on the chat?

[zimmerle]

@zimmerle Could I reach you on slack or any other chat? I feel it would be quicker to solve this and I could paste the conclusion here.

Sure - http://modsecurity.slack.com/ -- you can contact me over email as well: felipe@zimmerle.org

---

For the reference, here is the m_referenceCount being used on Transaction (v3.0.4) -

https://github.com/SpiderLabs/ModSecurity/blob/753145fbd1d6751a6b14fdd700921eb3cc3a1d35/src/transaction.cc#L134-L139

On v3/master we no longer use the reference counter, in general the reference was replaced with std::unique|sharde_ptr -

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/transaction.cc#L165-L172

On v3.1-experimental things are more clear as - almost - everything is constified -

https://github.com/SpiderLabs/ModSecurity/blob/9f47f1473c88a5b105ced2f11c87204a0abba424/src/transaction.cc#L189-L201

---

I am not sure if I understood your use case. You want to load two different ruleset on parallel? Complete not related to each other?

[zimmerle]

I am closing this issue. @iosetek has found a workaround by having a critical section on the rules load.

He does have a good and yet exciting use case for rules load. I am going to try to handle it on a new issue.

[atulkaushik22]

Hi @zimmerle,

Is this handled inside ModSecurity library?
We have a similar used case where we need to load multiple different rulesets in parallel. This is causing crash inside seclang-scanner.cc while accessing global variables.

Note - We are using ModSecurity library version 3.0.10.

Thanks,
Atul