ModSecurity: Crashes in v3/dev/3.1-experimental branch
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.
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Reactions: 1
- Comments: 36 (36 by maintainers)
Commits related to this issue
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Using a custom VariableMatch* implementation Delay the variable name resolution till last minute. Fix one of the issues raised in #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Using a custom VariableMatch* implementation Delay the variable name resolution till last minute. Fix one of the issues raised in #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Having RunTimeString in a better shape This is an effort towards better understanding the issues reported on #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Using a custom VariableMatch* implementation Delay the variable name resolution till last minute. Fix one of the issues raised in #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
- Using a custom VariableMatch* implementation Delay the variable name resolution till last minute. Fix one of the issues raised in #2376 — committed to owasp-modsecurity/ModSecurity by zimmerle 4 years ago
Yep, no leaks! 👍🏻
Interesting, because I didn’t have any problems with enabling sanitizers. Well, caused by ModSecurity build scripts, at least 😃
Commit 81dcf2947d1d1fd757c2cf0d8a39be98605226bf. I didn’t do any analysis, but if you need assistance with debugging or fixing, please ping me.
modsec_asan.log
@WGH-
New VariableValue constructor - https://github.com/SpiderLabs/ModSecurity/blob/5f38e37f73f151db49e3f34981adc96d03633889/headers/modsecurity/variable_value.h#L98-L108
pkginclude_HEADERS - 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-valgrindas a manual action on GitHub workflows. The idea of having it on a branch is not pleasing GitHub.You forgot to add some headers to
pkginclude_HEADERSlist. Namely,anchored_set_variable_match_vars.h,anchored_set_variable_match_vars_names.h, andanchored_variable_match_var_name.h.The last leak is still not fixed. The first constructor of
VariableValuedoesn’t free the pointer stored inm_value.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
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.
There’s lots of leaks with slightly different stack traces, but I believe these are the three main types:
It no longer crashes in my benchmark 👍
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
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)
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.
It can be easily reproduced with
test/benchmark/benchmark.ccand the following rule set:This is the fix:
Making asan give proper diagnostics was harder than I expected. Turned out I have to
dynamic_castimplementation crashes without giving proper diagnostics,ASAN_OPTIONS=quarantine_size_mb=4096.Still crashes for me, although in different place now.
8aa3e3439d697e70b09b029b85e8a83a6edb7bc4 is the first certainly bad commit, and 67b08dfe43633e63dfe6d1e386879519d634185f crashes due to apparently unrelated problem:
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: