fix(dpp): bound document value validation depth#4115
Conversation
📝 WalkthroughWalkthroughAdds a versioned maximum document value depth, iterative nested-value traversal, and consensus validation that rejects documents exceeding the configured limit. Tests cover protocol configuration, depth boundaries, recursive map keys, oversized data, and validation errors. ChangesDocument depth validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Blockers found — Sonnet deferred (commit aa3edcf) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4115 +/- ##
=============================================
+ Coverage 65.42% 87.52% +22.09%
=============================================
Files 26 2667 +2641
Lines 2707 337319 +334612
=============================================
+ Hits 1771 295231 +293460
- Misses 936 42088 +41152
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact HEAD 22a91fc, the new depth check remains unreachable for some under-limit attacker payloads because recursive state-transition decoding can abort first; this is a blocking denial of service. The field-size helper also retains a direct root-array clone failure, but the current DPP caller reports a shallow map key and applies the v12 depth guard first, so that issue is nonblocking. Targeted tests passed, and isolated probes against the actual crates reproduced both aborts.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/methods/validate_document/v0/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/data_contract/methods/validate_document/v0/mod.rs:50-59: Enforce the depth limit during decoding
This validation runs after attacker-controlled document values have already undergone recursive processing. An actual document-batch state transition containing 4,000 single-element arrays encoded to 8,149 bytes, below the 20,480-byte transition limit, and aborted with a stack overflow inside `StateTransition::deserialize_from_bytes`. `Value` derives recursive `Decode`, so the transition never reaches this deterministic `ValueError`. Values that survive decoding are also recursively cloned while create/replace actions are built and again when `self.data().into()` prepares validation. Enforce the depth budget in a custom or iterative decoder, then check borrowed transition data before action transformation performs recursive clones.
In `packages/rs-platform-value/src/lib.rs`:
- [SUGGESTION] packages/rs-platform-value/src/lib.rs:1401: Avoid cloning a recursive root-array result
For a root array, `reported_value` is its first child. If an oversized scalar occurs beneath that child, `reported_value.cloned()` recursively clones the remaining subtree and defeats the helper's stack-independent traversal. An actual 8,000-level `Value::Array` ending in oversized text aborted here, while the new deep test uses `Null` and never enters this result path. The current DPP production caller wraps document properties in a root map, preserving a shallow field key, and v12 depth-checks first, so this is not a second blocking document-validation exploit. It still violates the changed public helper's stated behavior; represent the reported field without cloning a recursive `Value` and add a deeply nested oversized root-array test.
| if let Some(max_depth) = platform_version.system_limits.max_document_value_depth { | ||
| if let Some(actual_depth) = value.first_depth_exceeding(max_depth as usize) { | ||
| return Ok(SimpleConsensusValidationResult::new_with_error( | ||
| ConsensusError::BasicError(BasicError::ValueError( | ||
| ValueError::new_from_string(format!( | ||
| "document value depth {actual_depth} exceeds system maximum {max_depth}" | ||
| )), | ||
| )), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Enforce the depth limit during decoding
This validation runs after attacker-controlled document values have already undergone recursive processing. An actual document-batch state transition containing 4,000 single-element arrays encoded to 8,149 bytes, below the 20,480-byte transition limit, and aborted with a stack overflow inside StateTransition::deserialize_from_bytes. Value derives recursive Decode, so the transition never reaches this deterministic ValueError. Values that survive decoding are also recursively cloned while create/replace actions are built and again when self.data().into() prepares validation. Enforce the depth budget in a custom or iterative decoder, then check borrowed transition data before action transformation performs recursive clones.
source: ['codex']
| }; | ||
|
|
||
| if let Some(actual_size) = actual_size { | ||
| return Some((reported_value.cloned(), actual_size)); |
There was a problem hiding this comment.
🟡 Suggestion: Avoid cloning a recursive root-array result
For a root array, reported_value is its first child. If an oversized scalar occurs beneath that child, reported_value.cloned() recursively clones the remaining subtree and defeats the helper's stack-independent traversal. An actual 8,000-level Value::Array ending in oversized text aborted here, while the new deep test uses Null and never enters this result path. The current DPP production caller wraps document properties in a root map, preserving a shallow field key, and v12 depth-checks first, so this is not a second blocking document-validation exploit. It still violates the changed public helper's stated behavior; represent the reported field without cloning a recursive Value and add a deeply nested oversized root-array test.
source: ['codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head aa3edcf, both prior findings are fixed: decoding is iterative and version-scoped, and oversized-field reporting no longer clones recursive values. One blocking consensus-path inconsistency remains: over-depth documents are charged and nonce-bumped only when their nested Value depth remains within the decoder ceiling; deeper instances become unpaid invalid encodings.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/state_transition/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/mod.rs:800-807: Keep all over-depth documents on the same consensus path
The decoder applies `max_document_value_depth` to each `Value`, while document validation includes the enclosing `BTreeMap<String, Value>` as depth one. A property containing 256 nested arrays therefore has document depth 257 but Value depth 256: it decodes, reaches `first_data_depth_exceeding`, returns `BasicError::ValueError`, and on protocol v12 produces a `BumpIdentityDataContractNonce` action and paid error. With 257 nested arrays, the decoder instead raises `DecodeError::OtherString`; deserialization maps that to `PlatformDeserializationError`, raw-transition decoding maps it to `SerializedObjectParsingError`, and processing returns an unpaid error without the nonce action. `prepare_proposal` consequently retains the first transition but removes the second. This contradicts the PR's stated deterministic `ValueError` behavior and lets equivalent violations select different fee and nonce semantics. Preserve the iterative safety boundary, but coordinate it with document-depth validation through a typed depth failure or a separate defensive ceiling so every protocol depth violation follows the intended consensus path.
| let max_value_depth = platform_version | ||
| .system_limits | ||
| .max_document_value_depth | ||
| .map(usize::from); | ||
| let state_transition = | ||
| platform_value::with_value_decode_depth_limit(max_value_depth, || { | ||
| StateTransition::deserialize_from_bytes(bytes) | ||
| })?; |
There was a problem hiding this comment.
🔴 Blocking: Keep all over-depth documents on the same consensus path
The decoder applies max_document_value_depth to each Value, while document validation includes the enclosing BTreeMap<String, Value> as depth one. A property containing 256 nested arrays therefore has document depth 257 but Value depth 256: it decodes, reaches first_data_depth_exceeding, returns BasicError::ValueError, and on protocol v12 produces a BumpIdentityDataContractNonce action and paid error. With 257 nested arrays, the decoder instead raises DecodeError::OtherString; deserialization maps that to PlatformDeserializationError, raw-transition decoding maps it to SerializedObjectParsingError, and processing returns an unpaid error without the nonce action. prepare_proposal consequently retains the first transition but removes the second. This contradicts the PR's stated deterministic ValueError behavior and lets equivalent violations select different fee and nonce semantics. Preserve the iterative safety boundary, but coordinate it with document-depth validation through a typed depth failure or a separate defensive ceiling so every protocol depth violation follows the intended consensus path.
source: ['codex']
|
Addressed the remaining blocker in be42949. The split came from an off-by-one between the two depth counts: the decoder bounds each decoded Both DPP depth checks now give each property value the full 256-container budget, identical to the decoder's per-value ceiling (and consistent with the contract-schema rule, whose root map is a real |
The decoder applies max_document_value_depth to each decoded Value, while document validation counted the enclosing BTreeMap data wrapper as depth one. A property with 256 nested containers therefore decoded but failed validation with a paid ValueError plus nonce bump, while 257 nested containers failed decoding as an unpaid parse error — letting equivalent depth violations select different fee and nonce semantics. Give each property value the full depth budget in both DPP depth checks, matching the decoder's per-value ceiling (and the existing contract schema rule, whose root map is a real Value container). No decodable payload can now violate the depth rule, so every depth violation uniformly fails as an undecodable transition; the validation-side checks remain as defense in depth for values that do not arrive via the wire decoder. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Protocol v12 is already active on live networks, so activating the new depth rule in SYSTEM_LIMITS_V2 would retroactively change an active version's consensus behavior for both historical replay and mixed-version validation. Move the limit to a new SYSTEM_LIMITS_V3 wired into protocol v13 (the in-development version on v4.1-dev) and restore v12 to None. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
be42949 to
f781c97
Compare
|
Rebased onto current v4.1-dev and moved the activation gate: protocol v12 is already active on live networks, so the depth limit now lives in a new |
2 participants
Summary
Valueiteratively with a version-scoped container-depth ceiling so nesting depth can never consume attacker-controlled stackValue::has_data_larger_thanas an iterative traversal that borrows (never clones) the reported fieldRoot cause
Document create and replace validation passed decoded, attacker-controlled properties to
has_data_larger_thanbefore JSON/schema validation.Valuederived a recursiveDecode, so deeply nested payloads could overflow the stack duringStateTransition::deserialize_from_bytesbefore any validation ran, and the oversized-array error path repeatedly cloned nested subtrees while unwinding.Impact
On protocol v13,
Valuedecoding rejects more than 256 nested containers per value, so an over-depth document value is an undecodable transition — the same deterministic, unpaid rejection as any other malformed encoding, applied identically at every depth. Because each document property value receives the same depth budget in the decoder and in validation (the plainBTreeMapdata wrapper is not counted, matching the existing contract-schema rule whose root map is a realValuecontainer), no decodable payload can violate the depth rule: equivalent violations can no longer split between paid and unpaid fee/nonce semantics. The validation-side depth checks invalidate_document_propertiesand the batch transformer remain as defense in depth for values that do not arrive via the wire decoder. Protocol versions before v13 retain their prior consensus rule, while the shared field-size helper and theValuedecoder are iterative for all callers.Validation
DocumentFieldMaxSizeExceededErrorcargo test -p platform-value --all-features— all passedcargo test -p platform-version -q— passedcargo test -p dpp --all-features— 3,870 passed, 6 ignoredcargo check -p drive-abci— passedcargo clippy -p platform-value --all-targets -- -D warnings— passedcargo clippy -p dpp --all-features --lib -- -D warnings— passedcargo clippy -p dpp --all-features --tests -- -D warnings -A clippy::needless_borrows_for_generic_args— passedcargo fmt --all -- --checkandgit diff --check— passedThe unmodified strict DPP test-target Clippy command is currently blocked by an unrelated pre-existing
needless_borrows_for_generic_argswarning indata_contract/factory/v0/mod.rs.