Skip to content

Inadequate locking for multithreading in libModSecurity In-Memory persistent storage #3054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
martinhsv opened this issue Feb 7, 2024 · 6 comments
Open

Inadequate locking for multithreading in libModSecurity In-Memory persistent storage #3054

martinhsv opened this issue Feb 7, 2024 · 6 comments
Labels
3.x Related to ModSecurity version 3.x

Comments

@martinhsv
Copy link
Contributor

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
Copy link
Member

airween commented Feb 7, 2024

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
Copy link
Member

airween commented Mar 3, 2024

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
Copy link
Contributor Author

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
Copy link
Member

airween commented Mar 15, 2024

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.

@airween airween mentioned this issue Aug 6, 2024
Merged
@eduar-hte eduar-hte mentioned this issue Aug 7, 2024
Open
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 7, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
a6683de 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 recursive mutex
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 7, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
329232a 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 recursive mute
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 7, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
63b476c 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
  - Provides exclusive/shared lock access so that multiple readers can
    access the data at the same time, but only one writer. this is used
    to favor query performance by allowing more concurrent access to the
    data until an update needs to be performed.
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
  - NOTE: Because std::shared_mutex is not recursive, calls to another
    function that tries to acquire the lock will fail. Introduced
    __store & __updateFirst helper methods to workaround this.
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 7, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
df5c8b2 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
  - Provides exclusive/shared lock access so that multiple readers can
    access the data at the same time, but only one writer. this is used
    to favor query performance by allowing more concurrent access to the
    data until an update needs to be performed.
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
  - NOTE: Because std::shared_mutex is not recursive, calls to another
    function that tries to acquire the lock will fail. Introduced
    __store & __updateFirst helper methods to workaround this.
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 7, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
40408b6 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
  - Provides exclusive/shared lock access so that multiple readers can
    access the data at the same time, but only one writer. this is used
    to favor query performance by allowing more concurrent access to the
    data until an update needs to be performed.
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
  - NOTE: Because std::shared_mutex is not recursive, calls to another
    function that tries to acquire the lock will fail. Introduced
    __store & __updateFirst helper methods to workaround this.
@eduar-hte eduar-hte mentioned this issue Aug 7, 2024
Merged
@eduar-hte
Copy link
Contributor

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!

eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 7, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
15e9992 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
  - Provides exclusive/shared lock access so that multiple readers can
    access the data at the same time, but only one writer. this is used
    to favor query performance by allowing more concurrent access to the
    data until an update needs to be performed.
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
  - NOTE: Because std::shared_mutex is not recursive, calls to another
    function that tries to acquire the lock will fail. Introduced
    __store & __updateFirst helper methods to workaround this.
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 7, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
f73fb0e 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
  - Provides exclusive/shared lock access so that multiple readers can
    access the data at the same time, but only one writer. this is used
    to favor query performance by allowing more concurrent access to the
    data until an update needs to be performed.
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
  - NOTE: Because std::shared_mutex is not recursive, calls to another
    function that tries to acquire the lock will fail. Introduced
    __store & __updateFirst helper methods to workaround this.
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 7, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
2c79137 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
  - Provides exclusive/shared lock access so that multiple readers can
    access the data at the same time, but only one writer. this is used
    to favor query performance by allowing more concurrent access to the
    data until an update needs to be performed.
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
  - NOTE: Because std::shared_mutex is not recursive, calls to another
    function that tries to acquire the lock will fail. Introduced
    __store & __updateFirst helper methods to workaround this.
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 8, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
b4f9981 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
  - Provides exclusive/shared lock access so that multiple readers can
    access the data at the same time, but only one writer. this is used
    to favor query performance by allowing more concurrent access to the
    data until an update needs to be performed.
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
  - NOTE: Because std::shared_mutex is not recursive, calls to another
    function that tries to acquire the lock will fail. Introduced
    __store & __updateFirst helper methods to workaround this.
- Updates to InMemoryPerProcess::resolveFirst
  - Updated the code to store the expired var in 'expiredVars' to delete
    them after iterating over the range (and releasing the read lock, as
    'delIfExpired' needs to acquire it for exclusive access), as the
    current call to 'delIfExpired' would invalidate the range triggering
    undefined behaviour on the following iteration.
  - Noticed that in commit 118e1b3 the call to 'delIfExpired' in this
    function is done using 'it->second.getValue()'' instead of
    'it->first', which seems incorrect (based on similar code in other
    resolveXXX functions).
- Updated InMemoryPerProcess::delIfExpired to use 'std::find_if' (with a
  lambda that matches both the key and the 'isExpired' condition)
  because the data structure is a multimap. The version introduced in
  commit 118e1b3 could find an entry (not necessarily the first, because
  the map is unordered) where 'isExpired' is 'false' and exit, while
  another entry could be expired.
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 9, 2024
@eduar-hte
Adding multithreaded example from issue owasp-modsecurity#3054 (by ai…
436cf35 
…rween)

- Rewritten to use C++ libModSecurity API and std::thread (instead of
  pthreads)
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 9, 2024
@eduar-hte
Adding multithreaded example from issue owasp-modsecurity#3054 (by ai…
8bb28b9 
…rween)

- Rewritten to use C++ libModSecurity API and std::thread (instead of
  pthreads)
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 9, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
5ad47f1 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
  - Provides exclusive/shared lock access so that multiple readers can
    access the data at the same time, but only one writer. this is used
    to favor query performance by allowing more concurrent access to the
    data until an update needs to be performed.
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
  - NOTE: Because std::shared_mutex is not recursive, calls to another
    function that tries to acquire the lock will fail. Introduced
    __store & __updateFirst helper methods to workaround this.
- Updates to InMemoryPerProcess::resolveFirst
  - Updated the code to store the expired var in 'expiredVars' to delete
    them after iterating over the range (and releasing the read lock, as
    'delIfExpired' needs to acquire it for exclusive access), as the
    current call to 'delIfExpired' would invalidate the range triggering
    undefined behaviour on the following iteration.
  - Noticed that in commit 118e1b3 the call to 'delIfExpired' in this
    function is done using 'it->second.getValue()'' instead of
    'it->first', which seems incorrect (based on similar code in other
    resolveXXX functions).
- Updated InMemoryPerProcess::delIfExpired to use 'std::find_if' (with a
  lambda that matches both the key and the 'isExpired' condition)
  because the data structure is a multimap. The version introduced in
  commit 118e1b3 could find an entry (not necessarily the first, because
  the map is unordered) where 'isExpired' is 'false' and exit, while
  another entry could be expired.
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 9, 2024
@eduar-hte
Adding multithreaded example from issue owasp-modsecurity#3054 (by ai…
1dac6f3 
…rween)

- Rewritten to use C++ libModSecurity API and std::thread (instead of
  pthreads)
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 9, 2024
@eduar-hte
Adding multithreaded example from issue owasp-modsecurity#3054 (by ai…
df25d02 
…rween)

- Rewritten to use C++ libModSecurity API and std::thread (instead of
  pthreads)
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 9, 2024
@eduar-hte
Adding multithreaded example from issue owasp-modsecurity#3054 (by ai…
da2bc50 
…rween)

- Rewritten to use C++ libModSecurity API and std::thread (instead of
  pthreads)
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 9, 2024
@eduar-hte
Prevent concurrent access to data structure in resolve methods
e2b3c95 
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are
  not acquiring a lock/mutex to prevent concurrent access to the data
  structures that may be modified at the same time from other threads,
  and thus triggering undefined behaviour.
- Replace inheritance of std::unordered_multimap data structure with
  data member to prevent potential clients to use it without acquiring
  the mutex to protect concurrent access.
- Replace pthreads lock with std C++11 std::shared_mutex
  - Provides exclusive/shared lock access so that multiple readers can
    access the data at the same time, but only one writer. this is used
    to favor query performance by allowing more concurrent access to the
    data until an update needs to be performed.
  - Simplifies acquisition and disposal of lock/mutex with
    std::lock_guard, which has RAII semantics.
  - NOTE: Because std::shared_mutex is not recursive, calls to another
    function that tries to acquire the lock will fail. Introduced
    __store & __updateFirst helper methods to workaround this.
- Updates to InMemoryPerProcess::resolveFirst
  - Updated the code to store the expired var in 'expiredVars' to delete
    them after iterating over the range (and releasing the read lock, as
    'delIfExpired' needs to acquire it for exclusive access), as the
    current call to 'delIfExpired' would invalidate the range triggering
    undefined behaviour on the following iteration.
  - Noticed that in commit 118e1b3 the call to 'delIfExpired' in this
    function is done using 'it->second.getValue()'' instead of
    'it->first', which seems incorrect (based on similar code in other
    resolveXXX functions).
- Updated InMemoryPerProcess::delIfExpired to use 'std::find_if' (with a
  lambda that matches both the key and the 'isExpired' condition)
  because the data structure is a multimap. The version introduced in
  commit 118e1b3 could find an entry (not necessarily the first, because
  the map is unordered) where 'isExpired' is 'false' and exit, while
  another entry could be expired.
eduar-hte added a commit to eduar-hte/ModSecurity that referenced this issue Aug 9, 2024
@eduar-hte
Adding multithreaded example from issue owasp-modsecurity#3054 (by ai…
4bf9616 
…rween)

- Rewritten to use C++ libModSecurity API and std::thread (instead of
  pthreads)
@eduar-hte
Copy link
Contributor

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

@airween: This issue can now be closed now that the PR has been merged.

@marcstern marcstern added the 3.x Related to ModSecurity version 3.x label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@airween @marcstern @martinhsv @eduar-hte