fix(security): raise OBE-10735 array-index cap from 32,768 to 2^20 - #12
fix(security): raise OBE-10735 array-index cap from 32,768 to 2^20#12JuanMantica45 wants to merge 1 commit into
Conversation
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>
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>
0eb853f to
b812285
Compare
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>
* 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>
|
Superseded by #14. #12 conflicts with The one thing I dropped: #12's byte-budget framing. Deriving the cap as @ajayshekar-s1 — #14 answers your objection from #7 directly in the description. Short version: the cap cannot be removed, because assigning to index N materialises N + 1 elements and that null padding is observable VRL semantics ( |
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-s1requested 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 unboundedVec::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 theisize::MINoverflow fix (unsigned_abs()instead of(-index) as usize, since negatingisize::MINoverflows) 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
MAX_ARRAY_INDEXconstant insrc/value/value/crud/mod.rs, set to1_048_576(2^20).insert_valuerejects (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, andindex.unsigned_abs()instead of(-index) as usizefor the negative case.lib/tests/tests/issues/obe_10735_array_index_cap.vrlexercising the cap boundary.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: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 aResultthroughValueCollection::insert_value,crud::insert, and the publicValue::insertAPI, andcompiler::expression::assignment::Target::insertcurrently 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 --libagainstmain: 1717 passed, 0 failedcargo runinlib/tests(the.vrlfixture runner):obe 10735 array index cap ... OK🤖 Generated with Claude Code