fix(security): reject excessive VRL expression nesting at compile time (OBE-10738, OBE-10740) - #13
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.
|
Superseded by #15. Closing this one because measurement showed its premise was wrong, not just its constant. I measured each phase of compilation separately on a 2 MiB thread. The dominant stack consumer is not Two other numbers from this PR's description turned out to be off:
#15 removes the clone (also a straight performance win — it copied the entire AST on every compile) and guards on It carries forward the Note that #15 does not fully close OBE-10738 and says so — beyond ~5,000 levels the process still dies dropping the un-compiled AST remainder, which no reserve can bound. That ticket stays open for a parse-time depth bound. |
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