Crashes in v3/dev/3.1-experimental branch #2376
Assignees
Labels
3.x
Related to ModSecurity version 3.x
Comments
|
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: Further, those will become a two-bit property, that will be queried with a bitwise operation:
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! |
|
The |
|
Might not be the same crash you mentioned in #2374 (comment), but it's likely related. Commit b321829 To reproduce, use this string in 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" |
|
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
added a commit
that referenced
this issue
Sep 18, 2020
This is an effort towards better understanding the issues reported on #2376
zimmerle
added a commit
that referenced
this issue
Sep 24, 2020
This is an effort towards better understanding the issues reported on #2376
zimmerle
added a commit
that referenced
this issue
Oct 14, 2020
This is an effort towards better understanding the issues reported on #2376
|
@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. |
|
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. |
zimmerle
added a commit
that referenced
this issue
Oct 21, 2020
This is an effort towards better understanding the issues reported on #2376
zimmerle
added a commit
that referenced
this issue
Oct 21, 2020
This is an effort towards better understanding the issues reported on #2376
zimmerle
added a commit
that referenced
this issue
Oct 23, 2020
This is an effort towards better understanding the issues reported on #2376
zimmerle
added a commit
that referenced
this issue
Oct 29, 2020
This is an effort towards better understanding the issues reported on #2376
|
Still crashes for me, although in different place now. 8aa3e34 is the first certainly bad commit, and 67b08df crashes due to apparently unrelated problem: |
|
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. |
perfect! thank you. I am merging some other stuff that should not impact with those. |
|
Making asan give proper diagnostics was harder than I expected. Turned out I have to
|
|
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. |
|
It can be easily reproduced with 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)) |
zimmerle
added a commit
that referenced
this issue
Nov 23, 2020
Delay the variable name resolution till last minute. Fix one of the issues raised in #2376
zimmerle
added a commit
that referenced
this issue
Nov 30, 2020
This is an effort towards better understanding the issues reported on #2376
zimmerle
added a commit
that referenced
this issue
Nov 30, 2020
Delay the variable name resolution till last minute. Fix one of the issues raised in #2376
|
all issues reported here are now mitigated -- 587cbf3 |
|
You forgot to add some headers to The last leak is still not fixed. The first constructor of 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.
{ }; |
|
Thank you @WGH- going to look into those. |
zimmerle
added a commit
that referenced
this issue
Dec 7, 2020
Delay the variable name resolution till last minute. Fix one of the issues raised in #2376
New VariableValue constructor - pkginclude_HEADERS - 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 |
zimmerle
added a commit
that referenced
this issue
Dec 10, 2020
This is an effort towards better understanding the issues reported on #2376
zimmerle
added a commit
that referenced
this issue
Dec 10, 2020
Delay the variable name resolution till last minute. Fix one of the issues raised in #2376
|
|
@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. |
|
@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! |
|
Commit 81dcf29. I didn't do any analysis, but if you need assistance with debugging or fixing, please ping me. |
zimmerle
added a commit
that referenced
this issue
Dec 23, 2020
This is an effort towards better understanding the issues reported on #2376
zimmerle
added a commit
that referenced
this issue
Dec 23, 2020
Delay the variable name resolution till last minute. Fix one of the issues raised in #2376
|
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 Remove all the leaks from the parser was an interesting task 💯 ; In details -
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 ;) 🚀 🚀 🚀 |
zimmerle
added a commit
that referenced
this issue
Jan 13, 2021
This is an effort towards better understanding the issues reported on #2376
zimmerle
added a commit
that referenced
this issue
Jan 13, 2021
Delay the variable name resolution till last minute. Fix one of the issues raised in #2376
|
Yep, no leaks! 👍🏻
Interesting, because I didn't have any problems with enabling sanitizers. Well, caused by ModSecurity build scripts, at least :) |
Good to hear 🤩 🤓 🤩 🤓 🤩
I will try to have the leak check on our QA as well ;) 💯 😎 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hello, @zimmerle! I debugged crashes found when testing #2374 (comment) a bit.
I found two trivial bugs, here are the patches for them:
And not-so-trivial UAFs which I was unable to decipher. "freed by" and "allocated by" look really wrong here.
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.
Maybe
addActionis also wrong, but it's hard to tell since the incoming argument is not a smart pointer.The text was updated successfully, but these errors were encountered: