Crashes in v3/dev/3.1-experimental branch

owasp-modsecurity / ModSecurity

ModSecurity is an open source, cross platform web application firewall (WAF) engine for Apache, IIS and Nginx. It has a robust event-based programming language which provides protection from a range of attacks against web applications and allows for HTTP traffic monitoring, logging and real-time analysis.

https://www.modsecurity.org

Apache License 2.0

7.7k stars 1.54k forks source link

Crashes in v3/dev/3.1-experimental branch #2376

Closed WGH- closed 3 years ago

WGH- commented 3 years ago

Hello, @zimmerle! I debugged crashes found when testing https://github.com/SpiderLabs/ModSecurity/pull/2374#issuecomment-665990756 a bit.

I found two trivial bugs, here are the patches for them:

diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc
index 738b3d72..82e1bd6a 100644
--- a/src/rule_with_actions.cc
+++ b/src/rule_with_actions.cc
@@ -250,7 +250,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans) const {
         }
     }
     for (auto &a : getMatchActionsPtr()) {
-        if (!dynamic_cast<ActionDisruptive *>(a) != NULL
+        if (dynamic_cast<ActionDisruptive *>(a) != NULL
                 && !(disruptiveAlreadyExecuted
                 && dynamic_cast<actions::Block *>(a))) {
             executeAction(trans, a, false);

diff --git a/src/rules_exceptions.cc b/src/rules_exceptions.cc
index aee9e8c0..8f0c6ef1 100644
--- a/src/rules_exceptions.cc
+++ b/src/rules_exceptions.cc
@@ -46,7 +46,7 @@ bool RulesExceptions::loadUpdateActionById(double id,
         }

         if (dynamic_cast<actions::transformations::Transformation *>(a.get())) {
-            actions::transformations::Transformation *t = dynamic_cast<actions::transformations::Transformation *>(a.get());
+            actions::transformations::Transformation *t = dynamic_cast<actions::transformations::Transformation *>(a.release());
             m_action_transformation_update_target_by_id.emplace(
                 std::pair<double,
                 std::unique_ptr<actions::transformations::Transformation>>(id, std::unique_ptr<actions::transformations::Transformation>(t))

And not-so-trivial UAFs which I was unable to decipher. "freed by" and "allocated by" look really wrong here.

=================================================================
==1494020==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160000528f0 at pc 0x7ffff6fdcf37 bp 0x7fffffff79b0 sp 0x7fffffff79a0
READ of size 8 at 0x6160000528f0 thread T0
    #0 0x7ffff6fdcf36 in modsecurity::variables::Rule_DictElement::msg(modsecurity::Transaction*, modsecurity::RuleWithActions const*, std::vector<modsecurity::VariableValue const*, std::allocator<modsecurity::VariableValue const*> >*) (/usr/local/modsecurity/lib/libmodsecurity.so.3+0x285f36)
    #1 0x7ffff6fdd63b in non-virtual thunk to modsecurity::variables::Rule_DictElement::evaluate(modsecurity::Transaction*, std::vector<modsecurity::VariableValue const*, std::allocator<modsecurity::VariableValue const*> >*) (/usr/local/modsecurity/lib/libmodsecurity.so.3+0x28663b)
    #2 0x7ffff7140c82 in modsecurity::RunTimeString::evaluate[abi:cxx11](modsecurity::Transaction*) /home/wgh/ModSecurity-upstream/src/run_time_string.cc:58
    #3 0x7ffff721258d in modsecurity::actions::ActionWithRunTimeString::getEvaluatedRunTimeString[abi:cxx11](modsecurity::Transaction*) const ../src/actions/action_with_run_time_string.h:53
    #4 0x7ffff721258d in modsecurity::actions::SetVar::execute(modsecurity::Transaction*) const actions/set_var.cc:50
    #5 0x7ffff714f216 in modsecurity::RuleWithActions::executeActionsIndependentOfChainedRuleResult(modsecurity::Transaction*) const /home/wgh/ModSecurity-upstream/src/rule_with_actions.cc:207
    #6 0x7ffff717769c in modsecurity::RuleWithOperator::evaluate(modsecurity::Transaction*) const /home/wgh/ModSecurity-upstream/src/rule_with_operator.cc:341
    #7 0x7ffff711de3d in modsecurity::RulesSet::evaluate(int, modsecurity::Transaction*) /home/wgh/ModSecurity-upstream/src/rules_set.cc:300
    #8 0x7ffff70cfa30 in modsecurity::Transaction::processRequestBody() /home/wgh/ModSecurity-upstream/src/transaction.cc:985
    #9 0x55555555e209 in process_transaction /home/wgh/modsecurity-benchmark/benchmark.cc:103
    #10 0x55555555e209 in main /home/wgh/modsecurity-benchmark/benchmark.cc:194
    #11 0x7ffff69900b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #12 0x555555560a9d in _start (/home/wgh/modsecurity-benchmark/benchmark+0xca9d)

0x6160000528f0 is located 112 bytes inside of 601-byte region [0x616000052880,0x616000052ad9)
freed by thread T0 here:
    #0 0x7ffff769c8df in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x1108df)
    #1 0x7ffff7172c6a in __gnu_cxx::new_allocator<char>::deallocate(char*, unsigned long) /usr/include/c++/9/ext/new_allocator.h:128
    #2 0x7ffff7172c6a in std::allocator_traits<std::allocator<char> >::deallocate(std::allocator<char>&, char*, unsigned long) /usr/include/c++/9/bits/alloc_traits.h:470
    #3 0x7ffff7172c6a in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_destroy(unsigned long) /usr/include/c++/9/bits/basic_string.h:237
    #4 0x7ffff7172c6a in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_dispose() /usr/include/c++/9/bits/basic_string.h:232
    #5 0x7ffff7172c6a in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() /usr/include/c++/9/bits/basic_string.h:658
    #6 0x7ffff7172c6a in modsecurity::RuleWithOperator::evaluate(modsecurity::Transaction*) const /home/wgh/ModSecurity-upstream/src/rule_with_operator.cc:251
    #7 0x7ffff711de3d in modsecurity::RulesSet::evaluate(int, modsecurity::Transaction*) /home/wgh/ModSecurity-upstream/src/rules_set.cc:300
    #8 0x7ffff70cfa30 in modsecurity::Transaction::processRequestBody() /home/wgh/ModSecurity-upstream/src/transaction.cc:985
    #9 0x55555555e209 in process_transaction /home/wgh/modsecurity-benchmark/benchmark.cc:103
    #10 0x55555555e209 in main /home/wgh/modsecurity-benchmark/benchmark.cc:194
    #11 0x7ffff69900b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

previously allocated by thread T0 here:
    #0 0x7ffff769b947 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10f947)
    #1 0x7ffff6cb8bbd in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_mutate(unsigned long, unsigned long, char const*, unsigned long) (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0x142bbd)

SUMMARY: AddressSanitizer: heap-use-after-free (/usr/local/modsecurity/lib/libmodsecurity.so.3+0x285f36) in modsecurity::variables::Rule_DictElement::msg(modsecurity::Transaction*, modsecurity::RuleWithActions const*, std::vector<modsecurity::VariableValue const*, std::allocator<modsecurity::VariableValue const*> >*)
Shadow bytes around the buggy address:
  0x0c2c800024c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c800024d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c800024e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c800024f0: fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c2c80002500: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c2c80002510: fd fd fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd
  0x0c2c80002520: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c80002530: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c80002540: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2c80002550: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c2c80002560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==1494020==ABORTING

Program received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff698e859 in __GI_abort () at abort.c:79
#2  0x00007ffff76b76a2 in ?? () from /usr/lib/x86_64-linux-gnu/libasan.so.5
#3  0x00007ffff76c224c in ?? () from /usr/lib/x86_64-linux-gnu/libasan.so.5
#4  0x00007ffff76a38ec in ?? () from /usr/lib/x86_64-linux-gnu/libasan.so.5
#5  0x00007ffff76a3363 in ?? () from /usr/lib/x86_64-linux-gnu/libasan.so.5
#6  0x00007ffff76a41ab in __asan_report_load8 () from /usr/lib/x86_64-linux-gnu/libasan.so.5
#7  0x00007ffff6fdcf37 in std::__shared_ptr<modsecurity::actions::Msg, (__gnu_cxx::_Lock_policy)2>::operator bool (this=0x6160000528f0) at ../src/variables/rule.h:136
#8  std::operator!=<modsecurity::actions::Msg>(std::shared_ptr<modsecurity::actions::Msg> const&, decltype(nullptr)) (__a=<error reading variable: Cannot access memory at address 0x3038253029657c40>)
    at /usr/include/c++/9/bits/shared_ptr.h:404
#9  modsecurity::RuleWithActions::hasMessageAction (this=0x616000052880) at ../src/rule_with_actions.h:426
#10 modsecurity::variables::Rule_DictElement::msg (t=<optimized out>, rule=0x616000052880, l=<optimized out>) at ../src/variables/rule.h:134
#11 0x00007ffff6fdd63c in non-virtual thunk to modsecurity::variables::Rule_DictElement::evaluate(modsecurity::Transaction*, std::vector<modsecurity::VariableValue const*, std::allocator<modsecurity::VariableValue const*> >*) ()
    at ../src/variables/rule.h:165
#12 0x00007ffff7140c83 in modsecurity::RunTimeString::evaluate[abi:cxx11](modsecurity::Transaction*) (this=<optimized out>, transaction=transaction@entry=0x6250005e8900) at /usr/include/c++/9/bits/shared_ptr_base.h:1309
#13 0x00007ffff721258e in modsecurity::actions::ActionWithRunTimeString::getEvaluatedRunTimeString[abi:cxx11](modsecurity::Transaction*) const (transaction=0x6250005e8900, this=0x60c00002eb40) at /usr/include/c++/9/bits/unique_ptr.h:721
#14 modsecurity::actions::SetVar::execute (this=0x60c00002eb40, t=<optimized out>) at actions/set_var.cc:50
#15 0x00007ffff714f217 in modsecurity::RuleWithActions::executeActionsIndependentOfChainedRuleResult (this=this@entry=0x616000105080, trans=trans@entry=0x6250005e8900) at rule_with_actions.cc:207
#16 0x00007ffff717769d in modsecurity::RuleWithOperator::evaluate (this=0x616000105080, trans=<optimized out>) at rule_with_operator.cc:341
#17 0x00007ffff711de3e in modsecurity::RulesSet::evaluate (this=<optimized out>, phase=phase@entry=3, t=t@entry=0x6250005e8900) at /usr/include/c++/9/bits/shared_ptr_base.h:1309
#18 0x00007ffff70cfa31 in modsecurity::Transaction::processRequestBody (this=<optimized out>) at transaction.cc:985
#19 0x000055555555e20a in process_transaction (it=0x7fffffffdb00, modsecTransaction=0x6250005e8900, j=...) at benchmark.cc:103
#20 main (argc=<optimized out>, argv=<optimized out>) at benchmark.cc:194

I think the problem might be due to incorrect use of smart pointers somewhere where the rules are parsed. For example, I found the following problematic fragment, but that wasn't enough to fix the problem.

void RuleWithActions::addDefaultAction(std::shared_ptr<actions::Action> a) {
// std::dynamic_pointer_cast<actions::Msg> should've been used here to avoid UAF
   } else if (dynamic_cast<actions::Msg *>(a.get())) {
        m_defaultActionMsg.reset(dynamic_cast<actions::Msg *>(a.get()));
// ...

Maybe addAction is also wrong, but it's hard to tell since the incoming argument is not a smart pointer.

zimmerle commented 3 years ago

Well noticed. Let me give a little bit of background on that particular change.

The idea is that 3.1 will be able to reload the rules in place, without the server to restart. For that, it was necessary to reduce the memory footprint of the RuleObject and adjacent. At a certain time, rules will be allocated twice in memory: (a) Adress the active requests; (b) Adress the new ones. That new feature also makes it necessary to clean up the memory properly after a new rule set is un-load.

Having said that, in terms of actions (cited in this issue), we have different approaches: some stuff became a rule property (logging) and some are actions to be used in run time with parameters inherited from the parser. To exemplify: Instead to be an object: nolog, log, auditlog and noauditlog became a bit properties of the class Rule:

https://github.com/SpiderLabs/ModSecurity/blob/5824470768e73d438a5db03b7a6eb24d7c795097/src/rule_with_actions.h#L592-L595

Further, those will become a two-bit property, that will be queried with a bitwise operation:	Porporty	Value
noLog	00
log	01
noAuditLog	10
auditLog	11

The thing with run-time-actions, the ones that are classes, is that they belong to a rule. Still, they can also belong to a rule in a second (or nth) virtual host (a consequence of a merging). Atop of that, there are the DefaultActions, which we are aiming to pre-compute in rules load time -- avoid any but necessary processing at runtime.

The smart pointers have an important role in that architecture as we are offloading the responsibility to disposable the action object to C++. I will have a look to understand better what is going on and let you know.

Thanks for testing and reporting this ;) appreciated!

zimmerle commented 3 years ago

The dynamic_cast<ActionDisruptive *>(a) != NULL issue was already addressed on v3.1-experimental.

WGH- commented 3 years ago

Might not be the same crash you mentioned in https://github.com/SpiderLabs/ModSecurity/pull/2374#issuecomment-671348276, but it's likely related.

Commit b32182940d

To reproduce, use this string in benchmark.cc:

char request_uri[] = "/foobar?message=%22AAAAAAAAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAAA%3DBBBBB%3BAAAAAAAAAAAAAA%3DBBBBB%3BloadAAAAAAAA%3DBBBBB%3BAAAAAAAAAAAAAAA%22"

(gdb) bt
#0  modsecurity::RuleWithActions::getMessage[abi:cxx11](modsecurity::Transaction*) const (this=this@entry=0x555556175e00, t=t@entry=0x5555565bb3e0)
    at rule_with_actions.cc:399
#1  0x00007ffff7e49494 in modsecurity::variables::Rule_DictElement::msg (t=0x5555565bb3e0, rule=0x555556175e00, l=0x7fffffffc720)
    at /usr/include/c++/9/bits/shared_ptr_base.h:1312
#2  0x00007ffff7e4970c in non-virtual thunk to modsecurity::variables::Rule_DictElement::evaluate(modsecurity::Transaction*, std::vector<modsecurity::VariableValue const*, std::allocator<modsecurity::VariableValue const*> >*) () at ../src/variables/rule.h:165
#3  0x00007ffff7ea79ae in modsecurity::RunTimeString::evaluate[abi:cxx11](modsecurity::Transaction*) (this=0x555556177050, transaction=transaction@entry=0x5555565bb3e0)
    at /usr/include/c++/9/bits/shared_ptr_base.h:1309
#4  0x00007ffff7edae66 in modsecurity::actions::ActionWithRunTimeString::getEvaluatedRunTimeString[abi:cxx11](modsecurity::Transaction*) const (
    transaction=0x5555565bb3e0, this=0x555556171e00) at /usr/include/c++/9/bits/unique_ptr.h:721
#5  modsecurity::actions::SetVar::execute (this=0x555556171e00, t=0x5555565bb3e0) at actions/set_var.cc:50
#6  0x00007ffff7eaaafc in modsecurity::RuleWithActions::executeActionsIndependentOfChainedRuleResult (this=this@entry=0x5555565a1970, trans=trans@entry=0x5555565bb3e0)
    at rule_with_actions.cc:207
#7  0x00007ffff7eb33dd in modsecurity::RuleWithOperator::evaluate (this=0x5555565a1970, trans=0x5555565bb3e0) at rule_with_operator.cc:340
#8  0x00007ffff7e9fb7f in modsecurity::RulesSet::evaluate (this=0x5555555aac80, phase=phase@entry=3, t=t@entry=0x5555565bb3e0)
    at /usr/include/c++/9/bits/shared_ptr_base.h:1309
#9  0x00007ffff7e8bbb5 in modsecurity::Transaction::processRequestBody (this=0x5555565bb3e0) at transaction.cc:985
#10 0x0000555555556f2c in main (argc=<optimized out>, argv=<optimized out>) at benchmark.cc:136

==95724==ERROR: AddressSanitizer: heap-use-after-free on address 0x61600006f6f0 at pc 0x7f6f3a19bf27 bp 0x7fffcb419890 sp 0x7fffcb419880
READ of size 8 at 0x61600006f6f0 thread T0
    #0 0x7f6f3a19bf26 in modsecurity::variables::Rule_DictElement::msg(modsecurity::Transaction*, modsecurity::RuleWithActions const*, std::vector<modsecurity::VariableValue const*, std::allocator<modsecurity::VariableValue const*> >*) (/home/wgh/ModSecurity-upstream/src/.libs/libmodsecurity.so.3+0x285f26)
    #1 0x7f6f3a19c62b in non-virtual thunk to modsecurity::variables::Rule_DictElement::evaluate(modsecurity::Transaction*, std::vector<modsecurity::VariableValue const*, std::allocator<modsecurity::VariableValue const*> >*) (/home/wgh/ModSecurity-upstream/src/.libs/libmodsecurity.so.3+0x28662b)
    #2 0x7f6f3a3015c2 in modsecurity::RunTimeString::evaluate[abi:cxx11](modsecurity::Transaction*) /home/wgh/ModSecurity-upstream/src/run_time_string.cc:58
    #3 0x7f6f3a3d41d1 in modsecurity::actions::ActionWithRunTimeString::getEvaluatedRunTimeString[abi:cxx11](modsecurity::Transaction*) const ../src/actions/action_with_run_time_string.h:53
    #4 0x7f6f3a3d41d1 in modsecurity::actions::SetVar::execute(modsecurity::Transaction*) const actions/set_var.cc:50
    #5 0x7f6f3a30fb56 in modsecurity::RuleWithActions::executeActionsIndependentOfChainedRuleResult(modsecurity::Transaction*) const /home/wgh/ModSecurity-upstream/src/rule_with_actions.cc:207
    #6 0x7f6f3a338662 in modsecurity::RuleWithOperator::evaluate(modsecurity::Transaction*) const /home/wgh/ModSecurity-upstream/src/rule_with_operator.cc:340
    #7 0x7f6f3a2de49f in modsecurity::RulesSet::evaluate(int, modsecurity::Transaction*) /home/wgh/ModSecurity-upstream/src/rules_set.cc:300
    #8 0x7f6f3a290092 in modsecurity::Transaction::processRequestBody() /home/wgh/ModSecurity-upstream/src/transaction.cc:985
    #9 0x56129356b4d7 in main /home/wgh/ModSecurity-upstream/test/benchmark/benchmark.cc:136
    #10 0x7f6f399bf0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #11 0x56129343d67d in _start (/home/wgh/ModSecurity-upstream/test/benchmark/.libs/benchmark+0xb67d)

0x61600006f6f0 is located 112 bytes inside of 576-byte region [0x61600006f680,0x61600006f8c0)
freed by thread T0 here:
    #0 0x56129352b3df in operator delete(void*) (/home/wgh/ModSecurity-upstream/test/benchmark/.libs/benchmark+0xf93df)
    #1 0x7f6f3a26259b in std::default_delete<modsecurity::RuleWithActions>::operator()(modsecurity::RuleWithActions*) const /usr/include/c++/9/bits/unique_ptr.h:81
    #2 0x7f6f3a26259b in std::_Sp_counted_deleter<modsecurity::RuleWithActions*, std::default_delete<modsecurity::RuleWithActions>, std::allocator<void>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() /usr/include/c++/9/bits/shared_ptr_base.h:471
    #3 0x7f6f3a254204 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/include/c++/9/bits/shared_ptr_base.h:155
    #4 0x7f6f3a254204 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/include/c++/9/bits/shared_ptr_base.h:730
    #5 0x7f6f3a254204 in std::__shared_ptr<modsecurity::Rule, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() /usr/include/c++/9/bits/shared_ptr_base.h:1169
    #6 0x7f6f3a254204 in std::shared_ptr<modsecurity::Rule>::~shared_ptr() /usr/include/c++/9/bits/shared_ptr.h:103
    #7 0x7f6f3a254204 in void std::_Destroy<std::shared_ptr<modsecurity::Rule> >(std::shared_ptr<modsecurity::Rule>*) /usr/include/c++/9/bits/stl_construct.h:98
    #8 0x7f6f3a254204 in void std::_Destroy_aux<false>::__destroy<std::shared_ptr<modsecurity::Rule>*>(std::shared_ptr<modsecurity::Rule>*, std::shared_ptr<modsecurity::Rule>*) /usr/include/c++/9/bits/stl_construct.h:108
    #9 0x7f6f3a254204 in void std::_Destroy<std::shared_ptr<modsecurity::Rule>*>(std::shared_ptr<modsecurity::Rule>*, std::shared_ptr<modsecurity::Rule>*) /usr/include/c++/9/bits/stl_construct.h:137
    #10 0x7f6f3a254204 in void std::_Destroy<std::shared_ptr<modsecurity::Rule>*, std::shared_ptr<modsecurity::Rule> >(std::shared_ptr<modsecurity::Rule>*, std::shared_ptr<modsecurity::Rule>*, std::allocator<std::shared_ptr<modsecurity::Rule> >&) /usr/include/c++/9/bits/stl_construct.h:206
    #11 0x7f6f3a254204 in std::vector<std::shared_ptr<modsecurity::Rule>, std::allocator<std::shared_ptr<modsecurity::Rule> > >::~vector() /usr/include/c++/9/bits/stl_vector.h:677
    #12 0x7f6f3a254204 in modsecurity::Rules::~Rules() ../headers/modsecurity/rules.h:46
    #13 0x7f6f3a254204 in std::array<modsecurity::Rules, 7ul>::~array() /usr/include/c++/9/array:94
    #14 0x7f6f3a254204 in modsecurity::RulesSetPhases::~RulesSetPhases() ../headers/modsecurity/rules_set_phases.h:44
    #15 0x7f6f3a254204 in modsecurity::Parser::Driver::~Driver() parser/driver.cc:36
    #16 0x7f6f3a254ebe in modsecurity::Parser::Driver::~Driver() parser/driver.cc:42
    #17 0x7f6f3a2e8c0e in modsecurity::RulesSet::loadFromUri(char const*) /home/wgh/ModSecurity-upstream/src/rules_set.cc:102
    #18 0x56129356a41c in main /home/wgh/ModSecurity-upstream/test/benchmark/benchmark.cc:82
    #19 0x7f6f399bf0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

previously allocated by thread T0 here:
    #0 0x56129352a447 in operator new(unsigned long) (/home/wgh/ModSecurity-upstream/test/benchmark/.libs/benchmark+0xf8447)
    #1 0x7f6f3a0d8637 in yy::seclang_parser::parse() /home/wgh/ModSecurity-upstream/src/seclang-parser.yy:1096
    #2 0x7f6f3a26094b in modsecurity::Parser::Driver::parse(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&) parser/driver.cc:210
    #3 0x7f6f3a261abb in modsecurity::Parser::Driver::parseFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) parser/driver.cc:254
    #4 0x7f6f3a2e8b49 in modsecurity::RulesSet::loadFromUri(char const*) /home/wgh/ModSecurity-upstream/src/rules_set.cc:95
    #5 0x56129356a41c in main /home/wgh/ModSecurity-upstream/test/benchmark/benchmark.cc:82
    #6 0x7f6f399bf0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: heap-use-after-free (/home/wgh/ModSecurity-upstream/src/.libs/libmodsecurity.so.3+0x285f26) in modsecurity::variables::Rule_DictElement::msg(modsecurity::Transaction*, modsecurity::RuleWithActions const*, std::vector<modsecurity::VariableValue const*, std::allocator<modsecurity::VariableValue const*> >*)

zimmerle commented 3 years ago

Perfect! I am going to investigate.

RunTimeStrings are strings that accept variables as macro expansions. Those variables sometimes need to have the knowledge of the Rule that it belongs (XML or RULE). In this case, a new variable instance needs to be created for every rule. That scenario became especially confused when applied to rule merging. I will try to create a new set of tests to stress the rule merging focusing on reproduce this kind of error. As collateral, this test suit will highlight if such errors appear as regression.

zimmerle commented 3 years ago

@WGH- ping. Can you assist me with some testing on that matter? I think we have already mitigated that issue + a couple of changes for the benefit of performance && usability. It would be wonderful if I can count on you to test this again.

zimmerle commented 3 years ago

I think that it is no longer an issue in v3.1-experimetal. Therefore I am closing it.

The problem was related to a broken copy constructor for an Action that wasn't updating the reference to a new rule. Leading to an invalid memory access.

WGH- commented 3 years ago

Still crashes for me, although in different place now.

(gdb) where
f#0  0x00007ffff7c7fee2 in __dynamic_cast () from /lib/x86_64-linux-gnu/libstdc++.so.6
#1  0x00007ffff7ebaf1e in modsecurity::RuleWithActions::executeTransformations (this=0x555556642510,
    trans=0x555564d64f70, in=..., results=std::__cxx11::list = {...}) at rule_with_actions.cc:386
#2  0x00007ffff7eb2c32 in modsecurity::RuleWithOperator::evaluate (this=<optimized out>, trans=<optimized out>)
    at rule_with_operator.cc:302
#3  0x00007ffff7ebd353 in modsecurity::RulesSet::evaluate (this=0x5555555bccd0, phase=<optimized out>,
    t=0x555564d64f70) at rules_set.cc:300
#4  0x00007ffff7ecd724 in modsecurity::Transaction::processRequestBody (this=0x555564d64f70) at transaction.cc:956
#5  0x0000555555559b14 in process_transaction (it=0x7fffffffe1c0, modsecTransaction=0x555564d64f70, j=...)
    at benchmark.cc:103
#6  main (argc=<optimized out>, argv=<optimized out>) at benchmark.cc:194
(gdb) frame 1
#1  0x00007ffff7ebaf1e in modsecurity::RuleWithActions::executeTransformations (this=0x555556642510,
    trans=0x555564d64f70, in=..., results=std::__cxx11::list = {...}) at rule_with_actions.cc:386
386         if (dynamic_cast<actions::transformations::None *>(t)) {
(gdb) l
381     // FIXME: It can't be something different from transformation. Sort this
382     //        on rules compile time.
383     auto range = trans->m_rules->m_exceptions.m_action_transformation_update_target_by_id.equal_range(m_ruleId);
384     for (auto it = range.first; it != range.second; ++it) {
385         Transformation *t = it->second.get();
386         if (dynamic_cast<actions::transformations::None *>(t)) {
387             none++;
388         }
389     }
390

8aa3e3439d697e70b09b029b85e8a83a6edb7bc4 is the first certainly bad commit, and 67b08dfe43633e63dfe6d1e386879519d634185f crashes due to apparently unrelated problem:

#0  modsecurity::RuleWithActions::getMessage[abi:cxx11](modsecurity::Transaction*) const (
    this=this@entry=0x5555557ae4c0, t=t@entry=0x555564ceca60) at rule_with_actions.cc:496
#1  0x00007ffff7e5b164 in modsecurity::variables::Rule_DictElement::msg (t=0x555564ceca60, rule=0x5555557ae4c0,
    l=0x7fffffffc710) at /usr/include/c++/9/bits/shared_ptr_base.h:1312
#2  0x00007ffff7e5b3dc in non-virtual thunk to modsecurity::variables::Rule_DictElement::evaluate(modsecurity::Transaction*, std::vector<modsecurity::VariableValue const*, std::allocator<modsecurity::VariableValue const*> >*) ()
    at ../src/variables/rule.h:165
#3  0x00007ffff7eb6414 in modsecurity::RunTimeString::evaluate[abi:cxx11](modsecurity::Transaction*) (
    this=0x5555557af760, transaction=transaction@entry=0x555564ceca60)
    at /usr/include/c++/9/bits/shared_ptr_base.h:1309
#4  0x00007ffff7ee4c21 in modsecurity::actions::ActionWithRunTimeString::getEvaluatedRunTimeString[abi:cxx11](modsecurity::Transaction*) const (transaction=0x555564ceca60, this=0x5555557af6f0) at /usr/include/c++/9/bits/unique_ptr.h:721
#5  modsecurity::actions::SetVar::execute (this=0x5555557af6f0, rule=<optimized out>, t=0x555564ceca60)
    at actions/set_var.cc:50
#6  0x00007ffff7eb9e62 in modsecurity::RuleWithActions::executeActionsIndependentOfChainedRuleResult (
    this=this@entry=0x55555660f690, trans=trans@entry=0x555564ceca60) at rule_with_actions.cc:260
#7  0x00007ffff7ec19e7 in modsecurity::RuleWithOperator::evaluate (this=0x55555660f690, trans=0x555564ceca60)
    at rule_with_operator.cc:340
#8  0x00007ffff7eae1d6 in modsecurity::RulesSet::evaluate (this=0x5555555bccd0, phase=phase@entry=3,
    t=t@entry=0x555564ceca60) at /usr/include/c++/9/bits/shared_ptr_base.h:1309
#9  0x00007ffff7e9cdc3 in modsecurity::Transaction::processRequestBody (this=0x555564ceca60) at transaction.cc:964
#10 0x0000555555559b4c in process_transaction (it=0x7fffffffe1c0, modsecTransaction=0x555564ceca60, j=...)
    at benchmark.cc:103
#11 main (argc=<optimized out>, argv=<optimized out>) at benchmark.cc:194

zimmerle commented 3 years ago

Thanks @WGH-, is this the benchmark utility for ModSec? Which rules are loaded?

Good to know that it is in a different piece of the code, it means that we have solve the first issue.

WGH- commented 3 years ago

No, it's my own private code. I'll try to give you either good ASAN diagnostic, or a simple test case, hopefully tomorrow UTCish.

On October 30, 2020 1:35:04 AM GMT+03:00, Felipe Zimmerle notifications@github.com wrote:

Thanks @WGH-, is this the benchmark utility for ModSec?

zimmerle commented 3 years ago

No, it's my own private code. I'll try to give you either good ASAN diagnostic, or a simple test case, hopefully tomorrow UTCish. … On October 30, 2020 1:35:04 AM GMT+03:00, Felipe Zimmerle @.*> wrote: Thanks @WGH-, is this the benchmark** utility for ModSec?

perfect! thank you. I am merging some other stuff that should not impact with those.

WGH- commented 3 years ago

Making asan give proper diagnostics was harder than I expected. Turned out I have to

1) compile C++ library itself with asan, otherwise non-instrumented dynamic_cast implementation crashes without giving proper diagnostics, 2) increase quarantine memory size with ASAN_OPTIONS=quarantine_size_mb=4096.

==3085008==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000f50c0 at pc 0x7ff1432b53fd bp 0x7ffdc1a1d630 sp 0x7ffdc1a1d628
READ of size 8 at 0x6060000f50c0 thread T0
    #0 0x7ff1432b53fc in __dynamic_cast /home/wgh/prefix_test/usr/src/debug/sys-libs/libcxxabi-11.0.0/libcxxabi/src/private_typeinfo.cpp:618:21
    #1 0x7ff1438a59f7 in modsecurity::RuleWithActions::executeTransformations(modsecurity::Transaction*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::list<modsecurity::TransformationResult, std::__1::allocator<modsecurity::TransformationResult> >&) const /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rule_with_actions.cc:386:13
    #2 0x7ff1438bc8e3 in modsecurity::RuleWithOperator::evaluate(modsecurity::Transaction*) const /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rule_with_operator.cc:302:13
    #3 0x7ff14388c9f0 in modsecurity::RulesSet::evaluate(int, modsecurity::Transaction*) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rules_set.cc:300:19
    #4 0x7ff143865eb0 in modsecurity::Transaction::processRequestBody() /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/transaction.cc:956:20
    #5 0x408280 in process_transaction(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > const&, modsecurity::Transaction*, modsecurity::ModSecurityIntervention_t*) /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:103:24
    #6 0x408280 in main /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:194:13
    #7 0x7ff142f1fe09 in __libc_start_main (/home/wgh/prefix_test/lib64/libc.so.6+0x23e09)
    #8 0x406879 in _start (/home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark+0x406879)

0x6060000f50c0 is located 0 bytes inside of 64-byte region [0x6060000f50c0,0x6060000f5100)
freed by thread T0 here:
    #0 0x7ff143d1bb0d in operator delete(void*) (/home/wgh/prefix_test/usr/lib/clang/11.0.0/lib/linux/libclang_rt.asan-x86_64.so+0xcdb0d)
    #1 0x7ff14390ebf5 in modsecurity::actions::transformations::None::~None() /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/../src/actions/transformations/none.h:34:7
    #2 0x7ff1437f0de4 in std::__1::default_delete<modsecurity::actions::Action>::operator()(modsecurity::actions::Action*) const /home/wgh/prefix_test/usr/include/c++/v1/memory:2262:5
    #3 0x7ff1437fadea in std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >::reset(modsecurity::actions::Action*) /home/wgh/prefix_test/usr/include/c++/v1/memory:2517:7
    #4 0x7ff1437bfe3a in std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >::~unique_ptr() /home/wgh/prefix_test/usr/include/c++/v1/memory:2471:19
    #5 0x7ff1437eec8b in std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > >::destroy(std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >*) /home/wgh/prefix_test/usr/include/c++/v1/memory:1811:92
    #6 0x7ff1437eec78 in void std::__1::allocator_traits<std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >::__destroy<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > >(std::__1::integral_constant<bool, true>, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > >&, std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >*) /home/wgh/prefix_test/usr/include/c++/v1/memory:1703:21
    #7 0x7ff1437eec68 in void std::__1::allocator_traits<std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >::destroy<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > >(std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > >&, std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >*) /home/wgh/prefix_test/usr/include/c++/v1/memory:1544:14
    #8 0x7ff1437eec16 in std::__1::__vector_base<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >::__destruct_at_end(std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >*) /home/wgh/prefix_test/usr/include/c++/v1/vector:428:9
    #9 0x7ff1437eeb8b in std::__1::__vector_base<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >::clear() /home/wgh/prefix_test/usr/include/c++/v1/vector:371:29
    #10 0x7ff1437ee9dc in std::__1::__vector_base<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >::~__vector_base() /home/wgh/prefix_test/usr/include/c++/v1/vector:465:9
    #11 0x7ff1437ee905 in std::__1::vector<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >::~vector() /home/wgh/prefix_test/usr/include/c++/v1/vector:557:5
    #12 0x7ff1437ee8c5 in std::__1::default_delete<std::__1::vector<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > > >::operator()(std::__1::vector<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >*) const /home/wgh/prefix_test/usr/include/c++/v1/memory:2262:5
    #13 0x7ff1437ee7ea in std::__1::unique_ptr<std::__1::vector<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >, std::__1::default_delete<std::__1::vector<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > > > >::reset(std::__1::vector<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >*) /home/wgh/prefix_test/usr/include/c++/v1/memory:2517:7
    #14 0x7ff1437a4dda in std::__1::unique_ptr<std::__1::vector<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > >, std::__1::default_delete<std::__1::vector<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> >, std::__1::allocator<std::__1::unique_ptr<modsecurity::actions::Action, std::__1::default_delete<modsecurity::actions::Action> > > > > >::~unique_ptr() /home/wgh/prefix_test/usr/include/c++/v1/memory:2471:19
    #15 0x7ff143798c09 in yy::seclang_parser::parse() /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/seclang-parser.yy:1515:13
    #16 0x7ff14384f4ca in modsecurity::Parser::Driver::parse(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/parser/driver.cc:213:22
    #17 0x7ff14384f904 in modsecurity::Parser::Driver::parseFile(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/parser/driver.cc:257:12
    #18 0x7ff143889725 in modsecurity::RulesSet::loadFromUri(char const*) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rules_set.cc:95:17
    #19 0x406faf in main /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:180:16
    #20 0x7ff142f1fe09 in __libc_start_main (/home/wgh/prefix_test/lib64/libc.so.6+0x23e09)

previously allocated by thread T0 here:
    #0 0x7ff143d1b2ad in operator new(unsigned long) (/home/wgh/prefix_test/usr/lib/clang/11.0.0/lib/linux/libclang_rt.asan-x86_64.so+0xcd2ad)
    #1 0x7ff1437924f7 in yy::seclang_parser::parse() /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/seclang-parser.yy:2928:9
    #2 0x7ff14384f4ca in modsecurity::Parser::Driver::parse(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/parser/driver.cc:213:22
    #3 0x7ff14384f904 in modsecurity::Parser::Driver::parseFile(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/parser/driver.cc:257:12
    #4 0x7ff143889725 in modsecurity::RulesSet::loadFromUri(char const*) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rules_set.cc:95:17
    #5 0x406faf in main /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:180:16
    #6 0x7ff142f1fe09 in __libc_start_main (/home/wgh/prefix_test/lib64/libc.so.6+0x23e09)

zimmerle commented 3 years ago

Thank you @WGH-. Your feedback is very important. Going to approach that with a different perspective.

Ideally (long-term-plan) we won't have t:none computation on runtime. Rather we want to have it pre-computed on load time. Only exception if the action was added in run-time. The issues that you are facing is maybe a collateral from a middle state. Let me finish that t:none pre-computation and I will ask you to test again.

WGH- commented 3 years ago

It can be easily reproduced with test/benchmark/benchmark.cc and the following rule set:

SecRule ARGS "@rx foo" "id:1"
SecRuleUpdateActionById 1 "t:none,t:utf8toUnicode,t:urlDecodeUni"

This is the fix:

diff --git a/src/rules_exceptions.cc b/src/rules_exceptions.cc
index 97bbdc2e..07623e9c 100644
--- a/src/rules_exceptions.cc
+++ b/src/rules_exceptions.cc
@@ -46,7 +46,7 @@ bool RulesExceptions::loadUpdateActionById(RuleId id,
         }

         if (dynamic_cast<actions::transformations::Transformation *>(a.get())) {
-            actions::transformations::Transformation *t = dynamic_cast<actions::transformations::Transformation *>(a.get());
+            actions::transformations::Transformation *t = dynamic_cast<actions::transformations::Transformation *>(a.release());
             m_action_transformation_update_target_by_id.emplace(
                 std::pair<RuleId,
                 std::unique_ptr<actions::transformations::Transformation>>(id, std::unique_ptr<actions::transformations::Transformation>(t))

martinhsv commented 3 years ago

Hi @WGH- ,

I came to the same conclusion as you a few minutes ago.

The current code essentially results in two different unique_ptr smart pointers 'owning' the same object.

zimmerle commented 3 years ago

thank you guys 👍

WGH- commented 3 years ago

Fixing that, I have a null pointer dereference crash now:

0x00007ffff71e7d0e in modsecurity::RuleWithActions::executeActionsAfterFullMatch(modsecurity::Transaction*) const::$_2::operator()(modsecurity::Transaction*, std::__1::shared_ptr<modsecurity::actions::ActionWithExecution> const&) const (this=<optimized out>, t=0x624000182100, a=...)
    at rule_with_actions.cc:322
322         a->execute(t);

I debugged it a bit, and addGenericMatchAction(std::dynamic_pointer_cast<ActionWithExecution>(a)); line in RuleWithActions::RuleWithActions seems to insert a null pointer into the m_executeIfMatchActions; list.

Here's a rule set to reproduce it with test/benchmark/benchmark:

SecRule REQBODY_PROCESSOR "!@rx (?:URLENCODED|MULTIPART|XML|JSON)" \
    "id:901340,\
    ctl:forceRequestBodyVariable=On

martinhsv commented 3 years ago

Hi @WGH- ,

Thanks for calling attention to that additional problem. I can reproduce it, and I think I see the underlying problem.

martinhsv commented 3 years ago

Just to note that this most recent issue also applies to ctl:auditEngine

martinhsv commented 3 years ago

Regarding the v3.1-experimental crashes caused by ctl:forceRequestBodyVariable and ctl:auditEngine ...

These have never been properly supported in v3, and yet in current v3 code, the standard '... is not supported ...' error behaviour is not triggered. The crash in v3.1 is merely a more obvious symptom than failing silently.

The immediate plan is to have both of these actions trigger the standard '... is not supported ...' error behaviour (like sanitiseArg, and others currently do)

zimmerle commented 3 years ago

Both crashed are addressed by @martinhsv, under QA here - https://travis-ci.org/github/SpiderLabs/ModSecurity/builds/741862888

WGH- commented 3 years ago

It no longer crashes in my benchmark :+1:

It seems to leak memory like crazy though, which is also reported by ASAN (to clarify, it certainly leaks without ASAN as well). It's not just one-time leak on e.g. rule loading, the memory consumption keeps increasing with every processed transaction.

The commit I tested is 15fc0481f17d0527b54dca7987041cdc633c05e5

WGH- commented 3 years ago

There's lots of leaks with slightly different stack traces, but I believe these are the three main types:


Direct leak of 58575456 byte(s) in 406774 object(s) allocated from:
    #0 0x7fef091032ad in operator new(unsigned long) (/home/wgh/prefix_test/usr/lib/clang/11.0.0/lib/linux/libclang_rt.asan-x86_64.so+0xcd2ad)
    #1 0x7fef08c2d4bb in modsecurity::TransactionRuleMessageManagement::messageNew() /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/transaction.cc:88:31
    #2 0x7fef08c2d303 in modsecurity::TransactionRuleMessageManagement::logMatchLastRuleOnTheChain(modsecurity::RuleWithActions const*) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/transaction.cc:83:9
    #3 0x7fef08c9170a in modsecurity::RuleWithOperator::evaluate(modsecurity::Transaction*) const /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rule_with_operator.cc:400:12
    #4 0x7fef08c60d30 in modsecurity::RulesSet::evaluate(int, modsecurity::Transaction*) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rules_set.cc:300:19
    #5 0x7fef08c3240d in modsecurity::Transaction::processRequestHeaders() /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/transaction.cc:593:20
    #6 0x407f47 in process_transaction(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > const&, modsecurity::Transaction*, modsecurity::ModSecurityIntervention_t*) /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:87:24
    #7 0x407f47 in main /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:194:13
    #8 0x7fef082e4e09 in __libc_start_main (/home/wgh/prefix_test/lib64/libc.so.6+0x23e09)

Direct leak of 9914352 byte(s) in 413098 object(s) allocated from:
    #0 0x7fef091032ad in operator new(unsigned long) (/home/wgh/prefix_test/usr/lib/clang/11.0.0/lib/linux/libclang_rt.asan-x86_64.so+0xcd2ad)
    #1 0x7fef08c8e507 in modsecurity::RuleWithOperator::updateMatchedVars(modsecurity::Transaction*, modsecurity::VariableValue const*, bpstd::basic_string_view<char, std::__1::char_traits<char> > const&) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rule_with_operator.cc:85:30
    #2 0x7fef08c91082 in modsecurity::RuleWithOperator::evaluate(modsecurity::Transaction*) const /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rule_with_operator.cc:357:21
    #3 0x7fef08c60d30 in modsecurity::RulesSet::evaluate(int, modsecurity::Transaction*) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rules_set.cc:300:19
    #4 0x7fef08c3240d in modsecurity::Transaction::processRequestHeaders() /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/transaction.cc:593:20
    #5 0x407f47 in process_transaction(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > const&, modsecurity::Transaction*, modsecurity::ModSecurityIntervention_t*) /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:87:24
    #6 0x407f47 in main /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:194:13
    #7 0x7fef082e4e09 in __libc_start_main (/home/wgh/prefix_test/lib64/libc.so.6+0x23e09)

Direct leak of 6208800 byte(s) in 258700 object(s) allocated from:
    #0 0x7fef091032ad in operator new(unsigned long) (/home/wgh/prefix_test/usr/lib/clang/11.0.0/lib/linux/libclang_rt.asan-x86_64.so+0xcd2ad)
    #1 0x7fef08ba3a05 in modsecurity::VariableValue::VariableValue(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const*) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/../headers/modsecurity/variable_value.h:101:17
    #2 0x7fef08ba391e in std::__1::__compressed_pair_elem<modsecurity::VariableValue, 1, false>::__compressed_pair_elem<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&, 0ul, 1ul>(std::__1::piecewise_construct_t, std::__1::tuple<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&>, std::__1::__tuple_indices<0ul, 1ul>) /home/wgh/prefix_test/usr/include/c++/v1/memory:2113:9
    #3 0x7fef08ba3318 in std::__1::__compressed_pair<std::__1::allocator<modsecurity::VariableValue>, modsecurity::VariableValue>::__compressed_pair<std::__1::allocator<modsecurity::VariableValue>&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&>(std::__1::piecewise_construct_t, std::__1::tuple<std::__1::allocator<modsecurity::VariableValue>&>, std::__1::tuple<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&>) /home/wgh/prefix_test/usr/include/c++/v1/memory:2197:9
    #4 0x7fef08ba2cc6 in std::__1::__shared_ptr_emplace<modsecurity::VariableValue, std::__1::allocator<modsecurity::VariableValue> >::__shared_ptr_emplace<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*>(std::__1::allocator<modsecurity::VariableValue>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&) /home/wgh/prefix_test/usr/include/c++/v1/memory:3470:16
    #5 0x7fef08ba0a6b in std::__1::enable_if<!(is_array<modsecurity::VariableValue>::value), std::__1::shared_ptr<modsecurity::VariableValue> >::type std::__1::make_shared<modsecurity::VariableValue, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*&&) /home/wgh/prefix_test/usr/include/c++/v1/memory:4291:26
    #6 0x7fef08ba0544 in modsecurity::variables::VariableModificatorCount::evaluate(modsecurity::Transaction const*, std::__1::vector<std::__1::shared_ptr<modsecurity::VariableValue const>, std::__1::allocator<std::__1::shared_ptr<modsecurity::VariableValue const> > >*) const /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/../src/variables/variable.h:776:22
    #7 0x7fef08c90abb in modsecurity::RuleWithOperator::evaluate(modsecurity::Transaction*) const /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rule_with_operator.cc:284:14
    #8 0x7fef08c60d30 in modsecurity::RulesSet::evaluate(int, modsecurity::Transaction*) /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/rules_set.cc:300:19
    #9 0x7fef08c3240d in modsecurity::Transaction::processRequestHeaders() /home/wgh/prefix_test/modsec/ModSecurity-upstream/src/transaction.cc:593:20
    #10 0x407f47 in process_transaction(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > const&, modsecurity::Transaction*, modsecurity::ModSecurityIntervention_t*) /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:87:24
    #11 0x407f47 in main /home/wgh/prefix_test/modsec/modsecurity-benchmark/benchmark.cc:194:13
    #12 0x7fef082e4e09 in __libc_start_main (/home/wgh/prefix_test/lib64/libc.so.6+0x23e09)

martinhsv commented 3 years ago

Thanks @WGH- for raising those memory leaks.

I have created a pull request (https://github.com/SpiderLabs/ModSecurity/pull/2446) for the first of the three that you mention.

zimmerle commented 3 years ago

notice that there is a memory leak on the matched_vars as stated here: #2428

martinhsv commented 3 years ago

Yes that's right. The 2nd of 3 mentioned by WGH- on Nov. 6 is a duplicate of the '// FIXME: Memory leak.' cited in: #2428

zimmerle commented 3 years ago

all issues reported here are now mitigated -- 587cbf39153fccb9cd129433609835fa83064bc4

WGH- commented 3 years ago

You forgot to add some headers to pkginclude_HEADERS list. Namely, anchored_set_variable_match_vars.h, anchored_set_variable_match_vars_names.h, and anchored_variable_match_var_name.h.

The last leak is still not fixed. The first constructor of VariableValue doesn't free the pointer stored in m_value.

    explicit VariableValue(const std::string *collection,
        const std::string *value = nullptr)
        : m_origin(),
        m_value(new std::string(value != nullptr?*value:"")), // FIXME: no need for to copy here.
        m_valueHolder(nullptr),
        m_key(nullptr),
        m_keyHolder(nullptr),
        m_collection(new std::string(*collection)) // FIXME: no need for to copy here.
    { };

zimmerle commented 3 years ago

Thank you @WGH- going to look into those.

zimmerle commented 3 years ago

@WGH-

[x] VariableValue constructor was changed.
[X] anchored_set_variable_match_vars.h, anchored_set_variable_match_vars_names.h, and anchored_variable_match_var_name.h are now part of pkginclude_HEADERS.

New VariableValue constructor - https://github.com/SpiderLabs/ModSecurity/blob/5f38e37f73f151db49e3f34981adc96d03633889/headers/modsecurity/variable_value.h#L98-L108

_pkgincludeHEADERS - https://github.com/SpiderLabs/ModSecurity/blob/5f38e37f73f151db49e3f34981adc96d03633889/src/Makefile.am#L37-L57

There are some others concerns being discussed on #2469. I will report here whenever we have a concrete position, meaning: testable branch.

Trying to have make check-valgrind as a manual action on GitHub workflows. The idea of having it on a branch is not pleasing GitHub.

zimmerle commented 3 years ago

[X] #2469 is closed.
[ ] #2381 under investigation.

WGH- commented 3 years ago

@zimmerle hey, do you need ASAN memory leak output regarding rules loading? I believe I had quite a lot of reported leaks related to it as well. I didn't show them because per-transaction memory leaks were more concerning at that moment.

zimmerle commented 3 years ago

@WGH- It will be amazing if you manage to share those. We want to mitigate all those memory related issues. Even small leaks can be very annoying depending on the deployment. thanks!

WGH- commented 3 years ago

Commit 81dcf2947d1d1fd757c2cf0d8a39be98605226bf. I didn't do any analysis, but if you need assistance with debugging or fixing, please ping me.

modsec_asan.log

zimmerle commented 3 years ago

@WGH-,

Thank you for providing the asan output; I have changed the Makefiles.am a bit to make easier to use compilation flags via _CXXFLAGS, such as -fsanitize=address -fno-omit-frame-pointer (b7d616a40ce186e6879ec3a0d59247002ede7377) -

https://github.com/SpiderLabs/ModSecurity/blob/b345d1a5a94668e0289efcd65261f27012b9d462/src/Makefile.am#L334-L335

Remove all the leaks from the parser was an interesting task :100: ; In details -

Parser location is now a custom class. It holds a shared pointer with the file name; If the parser fails, the resource is deleted.
To follow the parser change, the Rule class now holds the file name in a shared pointer instead of a unique pointer. As a shared pointer we avoid duplication of the file name in memory, plus, it frees itself when not in use anymore.
Operator init also accepting the filename as a shared pointer.
Driver is treating m_location was privative. Now it holds a std::list<std::shared_ptr> instead of: std::list<yy::seclang_parser::location_type *>.
Fix: addSecRule on Driver() was changed from RuleWithAction to RuleWithOperator.
Minor changes on the regression and rules-check utility to force deletion of resources even when they fail.
Couple of virtual destructors were placed to force the shared pointer decrementing on shared variables.
Deleted constructors for copy were placed for the sake of readability.

All those changes are packed here: b345d1a5a94668e0289efcd65261f27012b9d462 and effectively solve anything related to #2381

An inclusion of a file that does not exist will lead to a leak, but, I don't want to hold that patch due to a very unlikely scenario with a minimal leak size. We can left that for the next cleanup.

I guess we are safe fro another round of tests ;) :rocket: :rocket: :rocket:

WGH- commented 3 years ago

Yep, no leaks! 👍🏻

Thank you for providing the asan output; I have changed the Makefiles.am a bit to make easier to use compilation flags via CXX_FLAGS, such as -fsanitize=address -fno-omit-frame-pointer

Interesting, because I didn't have any problems with enabling sanitizers. Well, caused by ModSecurity build scripts, at least :)

zimmerle commented 3 years ago

Yep, no leaks! 👍🏻

Good to hear :star_struck: :nerd_face: :star_struck: :nerd_face: :star_struck:

Interesting, because I didn't have any problems with enabling sanitizers. Well, caused by ModSecurity build scripts, at least :)

I will try to have the leak check on our QA as well ;) 💯 :sunglasses: