Skip to content

fix(security): raise OBE-10735 array-index cap from 32,768 to 2^20 - #12

Open
JuanMantica45 wants to merge 1 commit into
Sentinel-One:mainfrom
JuanMantica45:raise-obe-10735-array-index-cap
Open

fix(security): raise OBE-10735 array-index cap from 32,768 to 2^20#12
JuanMantica45 wants to merge 1 commit into
Sentinel-One:mainfrom
JuanMantica45:raise-obe-10735-array-index-cap

Conversation

@JuanMantica45

@JuanMantica45 JuanMantica45 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Context

Split out from #7 to unblock that PR's merge on an unresolved review disagreement, without holding up the panic/OOM fixes it contains.

On #7, ajayshekar-s1 requested changes, arguing the cap on array-index assignment (.foo[N] = value) introduced for OBE-10735 — originally 32,768 in either direction — should be removed outright, since there could be a legitimate use case for indexing beyond that.

I pushed back on removing the cap entirely: OBE-10735 was a real, exploitable OOM. Before that fix, an event-controlled index (.foo[40000000] = 1, where the index comes from untrusted event data or an operator-authored VRL program) triggered an unbounded Vec::with_capacity(index + 1) allocation and null-padding loop — attacker-controlled memory exhaustion. Removing the cap reopens that vulnerability outright. But "the cap might be too conservative for real use cases" is a fair, separate concern from "the cap shouldn't exist," so this PR raises it instead of removing it.

This PR is now self-contained against main — it introduces the array-index cap directly at its final value (2^20) rather than depending on #7's intermediate 32,768 version, so this diff isn't entangled with #7's other 8, unrelated panic fixes. It also carries the isize::MIN overflow fix (unsigned_abs() instead of (-index) as usize, since negating isize::MIN overflows) that jsbalis1 found in review — that fix was pulled back out of #7 and lives here now, alongside the cap raise, since both touch the same capacity-calculation code.

What changed

  • New MAX_ARRAY_INDEX constant in src/value/value/crud/mod.rs, set to 1_048_576 (2^20). insert_value rejects (and logs a warning for) any array-index write outside ±MAX_ARRAY_INDEX.
  • src/value/value/crud/insert.rs's preallocation capacity calc uses the same shared constant, and index.unsigned_abs() instead of (-index) as usize for the negative case.
  • New regression fixture lib/tests/tests/issues/obe_10735_array_index_cap.vrl exercising the cap boundary.
  • Unit tests for: rejecting an index beyond the cap (positive and negative), accepting an index at the cap boundary, and not panicking on isize::MIN.

Why 2^20 specifically

std::mem::size_of::<Value>() is 40 bytes (checked directly on this branch). That makes the worst-case single-allocation cost of one indexed write:

Cap Elements Worst-case allocation
Unbounded (pre-fix) unbounded unbounded — the original OBE-10735 vulnerability
32,768 (original fix) 32,769 ~1.3 MB
2^20 (this PR) 1,048,577 ~42 MB

2^20 gives 32x more headroom than the original cap — enough that it's very unlikely to reject a legitimate pipeline's array usage — while keeping the worst case for a single indexed write bounded in the tens-of-MB range rather than unbounded. It's still a fixed, hardcoded ceiling, not user-configurable — if a concrete use case surfaces that needs more than this, that's a separate conversation with real data behind it, not a reason to remove the guardrail speculatively.

Note on VRL-level error surfacing

The out-of-range write currently no-ops silently (with a warn! log added here for observability) rather than returning a VRL runtime error. Making it a real error requires threading a Result through ValueCollection::insert_value, crud::insert, and the public Value::insert API, and compiler::expression::assignment::Target::insert currently discards the insert result outright — VRL array-index assignment is architecturally infallible today. That's a real, separate feature, tracked as a follow-up rather than bundled here.

Test plan

  • cargo test --lib against main: 1717 passed, 0 failed
  • cargo run in lib/tests (the .vrl fixture runner): obe 10735 array index cap ... OK

🤖 Generated with Claude Code

JuanMantica45 added a commit to JuanMantica45/vrl that referenced this pull request Aug 18, 2026
Reverts this PR's changes to src/value/value/crud/insert.rs and removes
lib/tests/tests/issues/obe_10735_array_index_cap.vrl. Both now live in
Sentinel-One#12, which raises the array-index cap (OBE-10735) from
32,768 to 2^20 in response to ajayshekar-s1's review feedback, and
carries the isize::MIN unsigned_abs() fix and its regression test
forward from this branch.

Note: this reintroduces the isize::MIN overflow bug in insert.rs's
capacity calculation that jsbalis1 flagged on this PR (negating
isize::MIN overflows). It is fixed in Sentinel-One#12, which is stacked on this
branch and should follow shortly. mod.rs's MAX_ARRAY_INDEX cap and
warn! log (jsbalis1's other two comments, plus the safe_binop
consolidation in arithmetic.rs) are unaffected and stay in this PR.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
JuanMantica45 added a commit to JuanMantica45/vrl that referenced this pull request Aug 18, 2026
Reverts this PR's changes to src/value/value/crud/insert.rs and removes
lib/tests/tests/issues/obe_10735_array_index_cap.vrl. Both now live in
Sentinel-One#12, which raises the array-index cap (OBE-10735) from
32,768 to 2^20 in response to ajayshekar-s1's review feedback, and
carries the isize::MIN unsigned_abs() fix and its regression test
forward from this branch.

Note: this reintroduces the isize::MIN overflow bug in insert.rs's
capacity calculation that jsbalis1 flagged on this PR (negating
isize::MIN overflows). It is fixed in Sentinel-One#12, which is stacked on this
branch and should follow shortly. mod.rs's MAX_ARRAY_INDEX cap and
warn! log (jsbalis1's other two comments, plus the safe_binop
consolidation in arithmetic.rs) are unaffected and stay in this PR.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Assigning to a large array index (`.foo[N] = value`, N from untrusted
event data or an operator-authored VRL program) padded the target array
with `Value::Null` up to that index with no cap, and preallocated
`Vec::with_capacity(index + 1)` up front -- an event-controlled index
was enough to exhaust memory.

Caps growth to 2^20 (1,048,576) elements in either direction. `Value` is
40 bytes, so this bounds a single indexed write's worst-case
preallocation to ~42MB instead of unbounded. Also uses
`index.unsigned_abs()` rather than `(-index) as usize` for the negative
case, since negating `isize::MIN` overflows.

Logs a warning when a write is dropped for exceeding the cap. VRL-side
array-index assignment is currently infallible end-to-end
(compiler::expression::assignment::Target::insert discards the insert
result), so surfacing this as a proper VRL runtime error is a separate,
larger change -- this at least makes a dropped write observable instead
of a silent no-op.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@JuanMantica45
JuanMantica45 force-pushed the raise-obe-10735-array-index-cap branch from 0eb853f to b812285 Compare August 18, 2026 22:59
JuanMantica45 added a commit to JuanMantica45/vrl that referenced this pull request Aug 19, 2026
Reverts this PR's changes to src/value/value/crud/insert.rs and removes
lib/tests/tests/issues/obe_10735_array_index_cap.vrl. Both now live in
Sentinel-One#12, which raises the array-index cap (OBE-10735) from
32,768 to 2^20 in response to ajayshekar-s1's review feedback, and
carries the isize::MIN unsigned_abs() fix and its regression test
forward from this branch.

Note: this reintroduces the isize::MIN overflow bug in insert.rs's
capacity calculation that jsbalis1 flagged on this PR (negating
isize::MIN overflows). It is fixed in Sentinel-One#12, which is stacked on this
branch and should follow shortly. mod.rs's MAX_ARRAY_INDEX cap and
warn! log (jsbalis1's other two comments, plus the safe_binop
consolidation in arithmetic.rs) are unaffected and stay in this PR.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ajayshekar-s1 pushed a commit that referenced this pull request Aug 19, 2026
* fix(security): prevent 9 panic/OOM vectors in VRL runtime (OBE-10722..10743 batch J)

Close all panics and DoS-by-OOM paths identified in the batch-J security audit:

- OBE-10722 find(): clamp negative `from` to 0 before usize cast; guard
  find_regex_in_str against offset > haystack.len() (regex::find_at panic).
- OBE-10723 format_number(): replace .expect("not NaN") with fallible
  Decimal::from_f64 conversion; returns VRL error for ±∞ and out-of-range floats.
- OBE-10724 format_number(): reject negative scale; cap scale at 1024 to prevent
  unbounded push('0') OOM loop; type_def changed to fallible().
- OBE-10727 arithmetic: add safe_mul/safe_add/safe_rem helpers mirroring safe_sub;
  replace NotNan::mul/add/rem calls that panic on NaN result (e.g. ∞ * 0).
- OBE-10731 parse_xml(): filter single-child path to element/text nodes; prevents
  Comment/PI child from reaching the unreachable!() arm in process_node.
- OBE-10733 starts_with(): fix hand-rolled Chars iterator — treat width==0 (stray
  continuation bytes) and truncated multi-byte sequences as error bytes; fix
  off-by-one in the Err arm that read past the advanced pos.
- OBE-10734 lex.rs: add b'}' => '}' arm to unescape_string_literal; the lexer
  already accepted \} via escape_code but the unescaper had no matching arm,
  hitting unimplemented!().
- OBE-10735 array insert: cap insert_value index at ±32768 to bound Null-padding
  loop; cap Vec::with_capacity in crud/insert.rs to the same limit.
- OBE-10743 parse_grok(): wrap pattern.match_against in catch_unwind to convert
  Oniguruma retry-limit panics to VRL errors (mirrors existing parse_groks guard).

All 1680 lib tests pass. New regression tests added for each fixed panic path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(security): cover the 5 untested batch-J fixes, fix CI gates

Batch J fixed 9 panic/OOM paths but only 4 of them shipped a regression test.
Add tests for the remaining 5 and prove each one actually protects its guard.

Unit tests (11 new):
- OBE-10727 arithmetic.rs: new tests module covering safe_mul/safe_add/safe_rem —
  inf*0, inf+-inf and inf%inf return errors, and overflow to inf stays valid.
- OBE-10731 parse_xml: comment-only child, PI-only child, and a comment beside an
  element child.
- OBE-10734 lex.rs: unescape_string_literal handles `\}` (and `\{\}`).
- OBE-10735 crud/insert.rs: indices beyond ±32768 are rejected and leave the array
  untouched; index 32768 still works.

VRL source-level tests (4 new, lib/tests/tests/issues/): the same four defects
driven through compile+run, which is the path operator-authored VRL actually takes.
Each was confirmed to panic (or, for OBE-10735, to allocate an unbounded array)
against a pre-fix build of the CLI.

OBE-10743 is deliberately left untested: grok 2.4.1's onig backend already converts
Oniguruma errors to `None` via `unwrap_or_default()` (see grok src/onig.rs:53), so
the retry-limit panic the catch_unwind guards is not reachable with the pinned
dependency. No exploit input could be constructed.

Also fixes gates the original commit broke, none of which `cargo test --lib` runs:
- format_number's documented example no longer compiled after type_def became
  fallible, failing the generated `functions/format_number` test — now uses
  `format_number!`.
- `cargo fmt --check` flagged three hunks in arithmetic.rs and xml.rs.
- `clippy::all` (denied in src/value/mod.rs) flagged the MAX_ARRAY_INDEX check as
  manual_range_contains.
- Added the changelog fragments CI requires, including a `breaking` entry for
  format_number becoming fallible.

cargo test --workspace: 1692 passed, 0 failed.
vrl-tests: 765 passed, 0 failed (761 before).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore: drop changelog fragments from batch-J PR

Removes changelog.d/7.security.md and changelog.d/7.breaking.md.

Note: scripts/check_changelog_fragments.sh requires at least one fragment per PR,
so PR #7 now needs the 'no-changelog' GitHub label to pass that CI check. The
breaking change the fragment documented still stands: format_number is now
fallible, so programs calling it without `!` or `??` will no longer compile.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(security): address batch-J review feedback on OBE-10727/10735

- Use isize::unsigned_abs() instead of (-index) as usize when computing
  the array preallocation capacity: negating isize::MIN overflows, which
  reintroduces a panic on the same input class OBE-10735 was meant to
  close (jsbalis1).
- Deduplicate the array-index cap into a single MAX_ARRAY_INDEX constant
  shared by insert.rs and crud/mod.rs instead of two separate magic
  numbers that had to be kept in sync by hand (jsbalis1).
- Consolidate safe_add/safe_mul/safe_rem/safe_sub into one safe_binop
  helper parameterized by the operator (jsbalis1).
- Log a warning when an array-index write is dropped for exceeding the
  cap, since VRL assignment is currently infallible end-to-end
  (compiler::expression::assignment::Target::insert discards the result)
  and turning this into a proper runtime error is a larger, separate
  change tracked as a follow-up.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(security): move array-index-cap follow-up work to a separate PR

Reverts this PR's changes to src/value/value/crud/insert.rs and removes
lib/tests/tests/issues/obe_10735_array_index_cap.vrl. Both now live in
#12, which raises the array-index cap (OBE-10735) from
32,768 to 2^20 in response to ajayshekar-s1's review feedback, and
carries the isize::MIN unsigned_abs() fix and its regression test
forward from this branch.

Note: this reintroduces the isize::MIN overflow bug in insert.rs's
capacity calculation that jsbalis1 flagged on this PR (negating
isize::MIN overflows). It is fixed in #12, which is stacked on this
branch and should follow shortly. mod.rs's MAX_ARRAY_INDEX cap and
warn! log (jsbalis1's other two comments, plus the safe_binop
consolidation in arithmetic.rs) are unaffected and stay in this PR.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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