Skip to content

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

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

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.
@JuanMantica45

Copy link
Copy Markdown
Contributor Author

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 compile_expr — it is ast.clone() in compile_with_state, which deep-copies the whole AST so it survives for the unused-expression check afterwards. That derived Clone recurses per nesting level and overflows at depth ~700, before Compiler::compile is ever called. So the MAX_EXPR_DEPTH = 128 counter proposed here could never have fired on the public compile() entry point.

Two other numbers from this PR's description turned out to be off:

  • The parser is not the expensive part. It is table-driven LR and does 130 levels in ~8 KB, not the 32 MB claimed here. The compiler is ~80x more expensive per level (5,030 B release / 10,850 B debug vs 255 B).
  • A fixed depth of 128 is wrong in both directions: it exceeds what a 512 KiB thread can hold (~104 levels) while rejecting at 3.25x margin on the 2 MiB stack tokio actually gives Vector's workers.

#15 removes the clone (also a straight performance win — it copied the entire AST on every compile) and guards on stacker::remaining_stack() with a reserve expressed as a fraction of available stack rather than a byte count, since the work owed when the guard fires scales with the depth reached, which scales with the stack.

It carries forward the stacker-not-maybe_grow conclusion from this PR's investigation, which was correct and saved real time.

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.

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