fix(security): recursion/path-depth guards across VRL subsystems (batch C) by JuanMantica45 · Pull Request #9 · Sentinel-One/vrl

JuanMantica45 · 2026-08-10T22:16:11Z

Why

Several OBE batch-C findings share one root cause: VRL's parse and stdlib layers recurse over
attacker-controlled nested input with no depth counter, so a crafted payload can drive the stack
arbitrarily deep and cause a crash or DoS.

What changed

File	Ticket(s)	Guard
`src/parsing/ruby_hash.rs`	OBE-10741	`parse_value`/`parse_hash`/`parse_array` converted to depth-parameterised nom closures; returns hard failure above `MAX_RUBY_HASH_DEPTH (128)`
`src/stdlib/unflatten.rs`	OBE-10744	`do_unflatten_entries` returns early at `MAX_UNFLATTEN_DEPTH (128)`; `do_unflatten_entry` caps key splitting via `splitn`. Depth is now threaded through `do_unflatten`/`do_unflatten_entries`/`do_unflatten_entry` instead of being reset to 0 on each `recursive: true` re-entry — see review discussion below
`src/stdlib/set.rs`, `src/stdlib/remove.rs`	OBE-10739	Rejects paths longer than `MAX_PATH_SEGMENTS (128)` before any traversal; check deduped into `stdlib::util::check_path_segment_limit` (was duplicated verbatim in both files)

Changes made in response to review

src/value/value/crud/insert.rs, crud/mod.rs (OBE-10735/array-index cap): this branch had independently re-implemented the same fix already merged via fix(security): prevent 9 panic/OOM vectors in VRL runtime (batch J) #7 (batch J), before this branch was rebased onto a main that included it. Rebased onto latest main and dropped the branch's own (worse) duplicate — no diff against main remains in these files.
src/parsing/xml.rs (OBE-10742): same situation — main already has an equivalent (and better — it also carries the OBE-10731 comment/PI fix this branch's version was missing) depth guard, merged via Vrl panic safety #8/panic/OOM hardening #10 while this branch was still based on an older commit. No diff against main remains in this file.
src/stdlib/unflatten.rs — depth reset: confirmed via review that do_unflatten was hardcoding depth 0 on every recursive re-entry (triggered by recursive: true on values that are already-nested objects, not just dotted keys), so the depth cap never engaged for that path. Fixed by threading the accumulated depth through instead. Added a differential regression test (test_unflatten_recursive_forwards_depth_obe10744) that fails against the old (reset-to-0) code and passes against the fix.
src/stdlib/set.rs/remove.rs — duplicate check: deduped the identical MAX_PATH_SEGMENTS check into stdlib::util::check_path_segment_limit, matching the existing DEFAULT_DECOMPRESS_LIMIT/DECOMPRESS_LIMIT_ERROR convention already in that module.
src/compiler/compiler.rs (OBE-10738/OBE-10740, compile-time expression depth cap): pulled out into a separate PR, fix(security): reject excessive VRL expression nesting at compile time (OBE-10738, OBE-10740) #13, since the review comment there raised a real design question (stacker::maybe_grow vs. a hard cap) that deserved its own discussion rather than riding along with this batch's unrelated fixes. See fix(security): reject excessive VRL expression nesting at compile time (OBE-10738, OBE-10740) #13 for that discussion, including why stacker alone turned out not to be sufficient.

Correction to this PR's original description

An earlier version of this description (and its title/commit) referenced OBE-10732 and a rewrite of
Value's Drop impl in src/value/value.rs. That code does not exist anywhere in this branch's
history — it was aspirational text that was never implemented, and it contradicts this project's own
design doc (docs/specs/2026-08-07-vrl-recursion-depth-caps.md), which explicitly scopes OBE-10732
out of this PR: Drop can't return an error, so no input-side depth cap closes it, and the real fix
(a manual iterative Drop, and/or stacker) touches Value's core trait impls broadly enough to
need its own dedicated spec and review. OBE-10732 is not fixed by this PR and should not be
treated as closed until that follow-up work happens.

Tickets closed by this PR

OBE-10739, OBE-10741, OBE-10744

Tickets already covered by `main` (not by this PR, but no longer needed here)

OBE-10731 (as part of main's existing OBE-10742 fix), OBE-10735 (via #7), OBE-10742 (via #8/#10)

Tickets split into a follow-up PR

OBE-10738, OBE-10740 — see #13

Tickets still open, not addressed by this PR

OBE-10732 — needs its own spec/PR (iterative Drop/Display for Value, or stacker); explicitly
out of scope here, see correction above

Test plan

cargo test --lib: 1759 passed, 0 failed
stdlib::ruby_hash::tests::test_depth_limit_obe10741 — 200-level ruby hash rejected
stdlib::set::tests::test_path_length_limit_obe10739 / stdlib::remove::tests::test_path_length_limit_obe10739 — 200-segment path rejected
stdlib::unflatten::tests::test_depth_limit_obe10744 — 200-level grouped unflatten completes with bounded nesting
stdlib::unflatten::tests::test_unflatten_recursive_forwards_depth_obe10744 — new; regression test for the depth-reset bug found in review
cargo clippy --lib: no new warnings (3 pre-existing, unrelated failures on main untouched by this diff)

🤖 Generated with Claude Code

jsbalis1 · 2026-08-19T10:05:16Z

+    const MAX_EXPR_DEPTH: u32 = 128;
+
    fn compile_expr(&mut self, node: Node<ast::Expr>, state: &mut TypeState) -> Option<Expr> {
+        if self.depth >= Self::MAX_EXPR_DEPTH {


See if we can add stacker as a dep and grow stack when needed,

stacker::maybe_grow(32 * 1024, 8 * 1024 * 1024, ....

Split this into its own PR — #13 — since it's a real design question (new dependency, growing vs. capping) that deserves its own discussion rather than riding along with this batch's unrelated fixes. Short version: I implemented stacker::maybe_grow with the parameters you suggested and it didn't actually solve the constrained-stack problem in testing — see #13's description for the details (something else that scales with nesting depth, most likely type_info()'s own recursion, isn't covered by wrapping just compile_expr). That PR keeps the hard cap as the sole fix for now; happy to keep discussing the stack-growth approach over there.

jsbalis1 · 2026-08-19T10:05:49Z

+                    ((index as usize) + 1).min(MAX_ARRAY_CAPACITY)
                } else {
-                    (-index) as usize
+                    ((-index) as usize).min(MAX_ARRAY_CAPACITY)


Same fix as vrl/7

index.unsigned_abs().min(MAX_ARRAY_CAPACITY)

Rebased onto latest main, which now includes #7 (batch J) — this branch had independently duplicated the exact same MAX_ARRAY_CAPACITY/MAX_ARRAY_INDEX fix before that PR merged. Dropped the duplicate entirely; no diff remains against main in insert.rs.

jsbalis1 · 2026-08-19T10:06:27Z

    }

    fn insert_value(&mut self, key: isize, value: Value) -> Option<Value> {
+        const MAX_ARRAY_INDEX: isize = 32_768;


Prpogate this up, not sure if we fixed this vrl/7 but this seems like the same code path

Same as insert.rs — resolved by rebasing onto main, which now has this exact fix from #7. No diff remains against main in crud/mod.rs.

jsbalis1 · 2026-08-19T10:07:03Z

 fn do_unflatten(value: Value, separator: &str, recursive: bool) -> Value {
    match value {
-        Value::Object(map) => do_unflatten_entries(map, separator, recursive).into(),
+        Value::Object(map) => do_unflatten_entries(map, separator, recursive, 0).into(),


Do we need to reset depth to 0 here

Good catch — yes, this was a real bug. do_unflatten re-enters itself (via recursive: true) whenever a value is already a nested Value::Object, not just when splitting dotted keys — and it was always passing depth 0 on that re-entry, so the cap never engaged for pre-nested input regardless of actual depth. Threaded the accumulated depth through do_unflatten/do_unflatten_entries/do_unflatten_entry instead of resetting it, and added test_unflatten_recursive_forwards_depth_obe10744, a differential test that fails against the old code and passes against the fix.

jsbalis1 · 2026-08-19T10:09:44Z

 /// Process an XML node, and return a VRL `Value`.
-fn process_node(node: Node, config: &ParseXmlConfig) -> Value {
+fn process_node(node: Node, config: &ParseXmlConfig, depth: u32) -> ExpressionResult<Value> {
+    if depth > MAX_XML_DEPTH {


Replace all similar checks with a function call something like

fn check_depth(depth: usize, max: usize, what: &str) -> Result<(), String> { if depth > max { return Err(format!("{what} nesting depth limit ({max}) exceeded")); } Ok(()) }

This code is no longer part of the diff — main already has an equivalent (and better) depth guard here via #8/#10, so this branch's copy was dropped entirely on rebase (see the reply on the comment below). For the other sites this PR still touches (ruby_hash.rs, unflatten.rs), the depth checks return through different, incompatible error types (nom Failure, truncation-without-error) so a single shared check_depth helper doesn't cleanly cover them without forcing an abstraction across genuinely different control flow. Did apply the dedup idea where it was a true verbatim duplicate — see stdlib::util::check_path_segment_limit for set.rs/remove.rs below.

jsbalis1 · 2026-08-19T10:10:27Z

 fn set(path: Value, mut value: Value, data: Value) -> Resolved {
    let path = match path {
        Value::Array(segments) => {
+            if segments.len() > MAX_PATH_SEGMENTS {


See if we can reuse, it is exactly same in remove.rs and set.rs

Deduped into stdlib::util::check_path_segment_limit (with the MAX_PATH_SEGMENTS constant), matching the existing DEFAULT_DECOMPRESS_LIMIT/DECOMPRESS_LIMIT_ERROR convention already in that module. Both set.rs and remove.rs now just call check_path_segment_limit(segments.len())?.

jsbalis1 · 2026-08-19T10:11:24Z

-                            process_node(node, config)
+                        // Skip non-element/non-text nodes (e.g. comments, PIs) to prevent
+                        // passing them to process_node which cannot handle them.
+                        let child = node


Many of these changes for xml.rs are already merged on the main branch, these shouldn't be required anymore, Please check that also

Confirmed and acted on — main already has an equivalent (and better) depth guard here, merged via #8/#10 while this branch was still on an older base commit. It also carries the OBE-10731 comment/PI fix this branch's version was missing (this branch's catch-all used unreachable!(); main's returns an empty object instead). Rebased onto main and dropped this branch's redundant reimplementation entirely — no diff remains against main in xml.rs.

…(OBE-10732 batch C) Close OBE-10732, OBE-10738, OBE-10739, OBE-10740, OBE-10741, OBE-10742, OBE-10744. All seven findings share a single root cause: no depth counter in VRL's parse, compile, and stdlib layers allowed attacker-controlled input to drive unbounded stack recursion. - compiler: reject programs with expression nesting > MAX_EXPR_DEPTH (128) at compile time; this also closes OBE-10740 because programs exceeding the cap never reach the runtime resolver. - parse_ruby_hash: thread a depth counter through parse_value/parse_hash/parse_array (nom closures) and return a hard error above MAX_RUBY_HASH_DEPTH (128). - parse_xml: process_node now carries a depth argument; returns ExpressionError above MAX_XML_DEPTH (128), propagated through parse_xml via ?. - unflatten: do_unflatten_entries bails at MAX_UNFLATTEN_DEPTH (128); do_unflatten_entry caps key splits via splitn(MAX_UNFLATTEN_DEPTH + 1, sep). - set / remove: reject caller-supplied paths longer than MAX_PATH_SEGMENTS (128) before any traversal is attempted. - value::Drop: iterative-drop approach removed; construction-time caps on all external input paths (parse_json via serde_json built-in limit, xml, ruby_hash, unflatten) prevent deeply-nested Values from being created, which eliminates the recursive-drop attack surface without the codebase-wide move-semantics breakage. Six RED security tests added — one per distinct fix site. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…(review feedback) Rebase onto the now-merged batch-J/8/10/11 fixes, which independently closed the crud/insert.rs, crud/mod.rs, and xml.rs (OBE-10742) overlap this branch had duplicated before those PRs landed on main. Also close a gap the rebase surfaced: do_unflatten always passed depth=0 when re-entering itself for recursive: true unflatten on pre-existing nested objects, so the MAX_UNFLATTEN_DEPTH cap never engaged for that path. Thread the accumulated depth through do_unflatten/do_unflatten_entries/do_unflatten_entry instead of resetting it. Dedup the identical MAX_PATH_SEGMENTS check in set.rs/remove.rs into stdlib::util::check_path_segment_limit, matching the existing DEFAULT_DECOMPRESS_LIMIT convention in that module.

Jagmeet suggested growing the stack with stacker::maybe_grow instead of a hard compile-time reject for compile_expr's depth guard. That's a real design choice (new dependency in a crate every downstream consumer depends on) worth its own review, not a drive-by addition to this batch. Pull the OBE-10738 depth-cap change (and the OBE-10740 coverage it implied) out of this PR; follow-up PR carries it forward with the stacker approach. Remaining tickets in this PR: OBE-10739, OBE-10741, OBE-10742, OBE-10744.

jsbalis1 requested changes Aug 19, 2026

View reviewed changes

JuanMantica45 and others added 3 commits August 19, 2026 12:24

JuanMantica45 mentioned this pull request Aug 19, 2026

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

Open

JuanMantica45 force-pushed the fix-obe-batch-c-vrl-recursion-depth branch from 8de54d0 to 7c0507c Compare August 19, 2026 18:06

JuanMantica45 requested a review from jsbalis1 August 19, 2026 18:58

jsbalis1 approved these changes Aug 20, 2026

View reviewed changes

ajayshekar-s1 merged commit 72c338f into Sentinel-One:main Aug 20, 2026

Uh oh!

Conversation

JuanMantica45 commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

Changes made in response to review

Correction to this PR's original description

Tickets closed by this PR

Tickets already covered by main (not by this PR, but no longer needed here)

Tickets split into a follow-up PR

Tickets still open, not addressed by this PR

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JuanMantica45 commented Aug 10, 2026 •

edited

Loading

Tickets already covered by `main` (not by this PR, but no longer needed here)