Fix: Denial of Service (Stack Overflow & O(N^2) CPU) in OptionConverter#590
Open
OxBat wants to merge 3 commits intoapache:masterfrom
Open
Fix: Denial of Service (Stack Overflow & O(N^2) CPU) in OptionConverter#590OxBat wants to merge 3 commits intoapache:masterfrom
OxBat wants to merge 3 commits intoapache:masterfrom
Conversation
…ic Complexity) The variable substitution logic lacked a recursion depth limit, leading to Stack Overflow crashes with deeply nested variables. The cycle detection was also implemented using a linked list with O(N^2) complexity. This patch replaces the linked list with a std::vector and enforces a recursion depth limit of 20.
Contributor
|
can you add a unit test to ensure that this is working correctly? This unit test can be added to the already existing |
Contributor
|
there's actually already a test case for recursive: Why is that test not failing, or is that test not properly catching the problem that this is fixing? |
Added a test to ensure recursion stops at the defined depth limit.
Author
|
The existing test only catches circular dependencies (loops). My fix targets stack overflows caused by deep recursion without loops. I just added |
Added #include <log4cxx/helpers/stringhelper.h> to fix 'identifier not found' error on Windows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I identified a Denial of Service vulnerability in
OptionConverter::substVars.The component handles variable substitution (e.g.,
${key}) recursively.The Issues:
substVarsSafelyis recursive but had no depth limit. A configuration with deeply nested variables (e.g., 20,000 levels) caused a Segmentation Fault.LogStringChain) that required traversing the parent chain at every step, resulting in O(N^2) complexity.The Fix:
I refactored the internal
substVarsSafelyfunction:LogStringChainlinked list with astd::vector<LogString>for history tracking (faster lookup).MAX_SUBST_DEPTH(20). If recursion exceeds this depth, it stops and logs a warning, preventing the crash.