Inadequate locking for multithreading in libModSecurity In-Memory persistent storage

[martinhsv]

This issue does not apply to the main, current use case of libModSecurity with nginx, since that is a multi-process but single-threaded application. This finding is based on code inspection only; I did not build a multi-threaded environment to prove a crash or other tangible errors.

While adding the expirevar support I noticed that, while the in-memory option includes some internal locking, it does so only for updates.

The problem is that if ThreadA is performing a read-only action using an interator and, while that is ongoing, ThreadB makes an update to the data, it could invalidate the iterator that ThreadA is in the midst of using. (Note that the update by ThreadB might be a deletion, but an insertion could also invalidate the iterator that is in-use by ThreadA.)

[airween]

Hi @martinhsv,

thanks for sharing the results of your inspection. I try to write a sample code that will help us to investigate the behavior that you explained, but it would be a help if you could provide a rule set which helps to cause the (possible) behavior?

[airween]

Hi @martinhsv,

thanks again for your report.

I made a small test source which embeds the libmodsecurity3 library to a multi threaded application.

The sad news is that it not necessary to add the expirevar to make a segfault.

If the debug log is turned on, it is guaranteed to cause a segfault.

Thread 3 "multithr" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff3c226c0 (LWP 877)]
0x00007ffff7e6aca3 in modsecurity::RulesSet::debug(int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) () from /lib/x86_64-linux-gnu/libmodsecurity.so.3

If I turn off the debug.log, then I got a segfault in evaluate:

Thread 3 "multithr" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff3c226c0 (LWP 910)]
0x00007ffff7e6af67 in modsecurity::RulesSet::evaluate(int, modsecurity::Transaction*) () from /lib/x86_64-linux-gnu/libmodsecurity.so.3

I'm going to finish that example tool soon (add the correct Makefile.am and so on...), and will add that to examples/ directory (or first just share that through a gist on Github).

[martinhsv]

Yes of course. I never claimed that the problem originated with that 2023 development effort, only that that was when I noticed it. The underlying issue has been present in the code for years.

[airween]

There is a separated branch which has a new, multi-threaded example. With this, I couldn't reproduce this behavior.

@martinhsv please take a look to rules - is that something you thought?

If anyone wants to play with that code, please let me know, I try to help.

[eduar-hte]

I've just created PR #3216 to try and address this.

@martinhsv, it'd be helpful if you could take a look at the changes and offer your opinion. thx!