Skip to content

fix(security): reject excessive VRL expression nesting at compile time (OBE-10738, OBE-10740) - #13

Open
JuanMantica45 wants to merge 1 commit into
Sentinel-One:mainfrom
JuanMantica45:fix-obe-10738-vrl-compile-stack-growth
Open

fix(security): reject excessive VRL expression nesting at compile time (OBE-10738, OBE-10740)#13
JuanMantica45 wants to merge 1 commit into
Sentinel-One:mainfrom
JuanMantica45:fix-obe-10738-vrl-compile-stack-growth

Conversation

@JuanMantica45

Copy link
Copy Markdown
Contributor

Why

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 (a guard-page SIGSEGV, 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: u32 field to Compiler, incremented/decremented around compile_expr's recursive descent, rejecting at MAX_EXPR_DEPTH (128) with a compiler diagnostic instead of recursing further.

stacker::maybe_grow — investigated, not adopted here

The review comment on #9 suggested growing the stack with stacker::maybe_grow instead 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:

  • Added stacker as an optional dependency behind the compiler feature.
  • Wrapped compile_expr's recursive call in stacker::maybe_grow(32 * 1024, 8 * 1024 * 1024, ...), per the suggested parameters.
  • Wrote a differential test: parse a program with N nested if-blocks on a normal-stack thread, then run only Compiler::compile against 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 likely Expr::type_info()'s own recursive walk over the just-compiled subtree, invoked from inside compile_expr but 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 stacker actually deliver on the goal would mean auditing and wrapping every recursive path compile_expr triggers (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 nested if 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 on main untouched by this diff).

🤖 Generated with Claude Code

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant