fix(security): reject excessive VRL expression nesting at compile time (OBE-10738, OBE-10740) - #13
Open
JuanMantica45 wants to merge 1 commit into
Conversation
…at compile time Compiler::compile_expr recurses once per AST expression-nesting level with no depth counter, so a crafted VRL program can drive unbounded native-stack recursion during compilation and crash the process (SIGSEGV, not catchable). This also covers OBE-10740 (runtime resolver): a program that can't compile past this cap never reaches the runtime resolver, whose recursion follows the same nesting depth. Add a depth field to Compiler, incremented/decremented around compile_expr's recursive descent, rejecting at MAX_EXPR_DEPTH (128) with a compiler diagnostic rather than recursing further. Split out of the batch-C PR (Sentinel-One#9) at review request, to isolate the stacker::maybe_grow question (see PR description) from that PR's unrelated fixes.
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.
Why
Compiler::compile_exprrecurses once per AST expression-nesting level with no depth counter, so a crafted VRL program can drive unbounded native-stack recursion during compilation and crash the process (a guard-pageSIGSEGV, not a catchable panic). This also covers OBE-10740 (runtime resolver): a program that can't compile past this cap never reaches the runtime resolver, whose recursion follows the same nesting depth.Split out of #9 at @jsbalis1's request so this specific design question (see below) doesn't block that PR's unrelated fixes.
What changed
Add a
depth: u32field toCompiler, incremented/decremented aroundcompile_expr's recursive descent, rejecting atMAX_EXPR_DEPTH(128) with a compiler diagnostic instead of recursing further.stacker::maybe_grow— investigated, not adopted hereThe review comment on #9 suggested growing the stack with
stacker::maybe_growinstead of (or alongside) a hard reject, so that legitimate deeply-nested programs on a constrained worker-thread stack wouldn't be spuriously rejected. I implemented and tested this:stackeras an optional dependency behind thecompilerfeature.compile_expr's recursive call instacker::maybe_grow(32 * 1024, 8 * 1024 * 1024, ...), per the suggested parameters.if-blocks on a normal-stack thread, then run onlyCompiler::compileagainst the pre-built AST on a thread with a deliberately small (256 KB) stack — isolating the compiler's own recursion from the separately-recursive parser.Result: it still overflowed the stack and crashed, at both 60 and 120 levels of nesting — with several MB of the freshly-grown 8 MB segment still reported free at the time of the crash. Wrapping only
compile_expr's own frame isn't sufficient: something else that scales with nesting depth — most likelyExpr::type_info()'s own recursive walk over the just-compiled subtree, invoked from insidecompile_exprbut not itself wrapped — also consumes stack proportional to depth and isn't protected by the same mechanism. The crash occurred right at the deepest point of recursion (processing the leaf node) in both trials, which doesn't match a simple "ran out of the grown segment" failure mode, so there may be more going on here than a single unwrapped call site.Making
stackeractually deliver on the goal would mean auditing and wrapping every recursive pathcompile_exprtriggers (type inference, possibly others), not just its own frame — a materially larger and riskier change than "add a dependency and grow the stack," and the kind of broad-surface change the OBE-10732 design doc explicitly flagged as deserving its own dedicated review rather than a drive-by addition. Given that, this PR keeps the simple, already-empirically-safe hard cap (128, chosen against a 2 MB worker-thread stack per the original design measurement) as the sole fix. Raising the cap, or making stack-growth actually work end-to-end, is a larger follow-up if it turns out 128 is overly conservative for real customer programs in practice.Test plan
compiler::compiler::tests::test_expression_depth_limit_obe10738— program with 130 nestedif true { }blocks rejected at compile time (run on a 32 MB thread stack, since the LALRPOP parser is itself recursive and needs headroom just to produce the AST).cargo test --lib: 1754 passed, 0 failed.cargo clippy --lib: no new warnings (3 pre-existing, unrelated failures onmainuntouched by this diff).🤖 Generated with Claude Code