Merged
Conversation
krushimir
reviewed
Feb 11, 2026
Collaborator
There was a problem hiding this comment.
There's an issue regarding deserialization of PartialMerkleTree due to NodeIndex alignment padding.
The tuple doesn't override min_serialized_size, so it falls back to the in-memory size including alignment padding.
For (NodeIndex, Word):
- in-memory:
48bytes (NodeIndex has7bytes of padding between itsu8andu64fields) - serialized:
41bytes (9+32, no padding)
The budget check divides remaining bytes by 48 instead of 41, rejecting valid input. This test fails on this branch:
#[test]
fn deserialize_nonempty_with_budget() {
let mt = MerkleTree::new(VALUES8).unwrap();
let ms = MerkleStore::from(&mt);
let path33 = ms.get_path(mt.root(), NODE33).unwrap();
let pmt = PartialMerkleTree::with_paths([(3, path33.value, path33.path)]).unwrap();
let bytes = pmt.to_bytes();
let parsed = PartialMerkleTree::read_from_bytes_with_budget(&bytes, bytes.len()).unwrap();
assert_eq!(pmt, parsed);
}
InvalidValue("requested 4 elements but reader can provide at most 3")
Contributor
Author
|
@krushimir Fixed in #827 |
krushimir
approved these changes
Feb 12, 2026
Collaborator
krushimir
left a comment
There was a problem hiding this comment.
Verified the fix locally (cherry-picked from next) – works as expected. LGTM!
Replace Vec::with_capacity preallocations with read_many_iter in custom Deserializable implementations to prevent OOM/capacity overflow attacks: - PartialMerkleTree::read_from: use read_many_iter for (NodeIndex, Word) - Smt::read_from: use read_many_iter for (Word, Word) entries - SmtLeaf::read_from: use read_many_iter for (Word, Word) entries Add min_serialized_size() overrides for accurate budget checking: - NodeIndex: 9 bytes (u8 + u64) - Word: 32 bytes (SERIALIZED_SIZE) - PartialMerkleTree: 49 bytes (8 + 9 + 32) - Smt: 65 bytes (1 + 32 + 32) - SmtLeaf: 73 bytes (1 + 8 + 32 + 32) This enables BudgetedReader to enforce tight bounds on allocation sizes before any memory is allocated, preventing malicious inputs from claiming billions of elements while providing only a few bytes of data. Fixes fuzz failures: - smt_serde: no longer OOMs on 6-byte malicious input - merkle: allocation attacks prevented (separate panic in with_leaves on empty input is a pre-existing bug now exposed)
30ef405 to
5869c1b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fix OOMs from untrusted length prefixes, found through fuzzing.
The scheduled fuzz jobs
miden-crypto (merkle)andmiden-crypto (smt_serde)fail with allocation-size-too-big / OOM when random inputs encode absurd length prefixes. Both deserializers preallocate aVecbased on the prefix (Vec::with_capacity), which can request terabytes and abort. After those fixes, fuzzing exposes a pre-existing panic inPartialMerkleTree::with_leaveson empty input (unwrap()onNone).This is technically breaking since we now return an empty tree on depth 0 (instead of panicking).
The fix, how to verify
read_many_iterand accuratemin_serialized_size()boundsPartialMerkleTree::with_leavespanic on empty input (return an empty tree instead)How to verify
cargo +nightly fuzz run merkle -- -max_total_time=60 -runs=10000cargo +nightly fuzz run smt_serde -- -max_total_time=60 -runs=10000cargo test -p miden-crypto merkle::partial_mtcargo test -p miden-crypto merkle::smtcargo test -p miden-crypto merkle::smt::full::tests::test_empty_smt_deserialization_with_budget