fix(core): reject trailing bytes in manual decoders (#450)#455
Merged
Conversation
Operation::from_bytes and SerializableMerkleProof::from_bytes parsed a valid prefix and returned success while silently ignoring trailing bytes. That broke canonicalization: two distinct wire byte strings could decode to the same value, enabling signature/representation malleability (a signature verified over a re-derived canonical form would accept both encodings). Require full byte exhaustion: Operation::from_bytes now returns Err if any input remains after a complete parse; SerializableMerkleProof::from_bytes returns None unless the cursor consumed the entire buffer. Audited every call site of both decoders across dsm, dsm_sdk, the JNI bridge, and storage: all pass exactly-sized buffers (protobuf bytes fields, to_bytes() output, or length-prefixed reads), so enforcing exhaustion breaks no caller. Sub-operation decodes are length-delimited, so no inner exhaustion check fires. Tests: trailing-byte rejection for Operation (Transfer with signature, and Create) and for SerializableMerkleProof. Full dsm (1436) and dsm_sdk (1506) lib suites pass; clippy clean.
Third hand-rolled decoder with the same gap: it length-checked each field but never required full byte exhaustion, so trailing bytes were silently dropped. Add an `offset + count * 32 != data.len()` check before constructing the proof. Swept every from_bytes/decode in dsm/src: all others already exhaust (Operation and SerializableMerkleProof fixed in the prior commits; spv, device_tree, kyber, capsule, smt_replace_witness, GenesisHash strict; tombstone/policy protobuf). This decoder has no production callers — only the new unit test and the smt_tripwire_vectors integration test, both passing exact `to_bytes()` output. Tests: trailing-byte rejection for SmtInclusionProof. Full dsm lib suite (1437) and smt_tripwire_vectors (25) pass; clippy clean.
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.
What
Closes #450. Three hand-rolled decoders parsed a valid prefix and returned success while silently ignoring
trailing bytes, breaking canonicalization: two distinct wire byte strings could decode to the same value,
enabling signature/representation malleability (a signature verified over a re-derived canonical form would
accept both encodings).
How
Require full byte exhaustion in each decoder:
Operation::from_bytes— returnsErrif any input remains after a complete parse.SerializableMerkleProof::from_bytes— returnsNoneunless the cursor consumed the entire buffer.SmtInclusionProof::from_bytes— returnsNoneunlessoffset + count * 32 == data.len().Safety audit
Swept every
from_bytes/decode indsm/srcand every call site of the three decoders acrossdsm,dsm_sdk, the JNI bridge, and storage:to_bytes()output, or length-prefixedreads), so enforcing exhaustion breaks no caller. Sub-operation decodes are length-delimited, so no inner
exhaustion check fires.
GenesisHash) or are protobuf/fixed-width (tombstone, policy).
SmtInclusionProof::from_byteshas no production callers — only unit + integration tests, both passingexact
to_bytes()output.Tests / verification
cargo test -p dsm --lib— 1437 passed.cargo test -p dsm_sdk --lib(single-threaded) — 1506 passed.cargo test -p dsm --test smt_tripwire_vectors— 25 passed.cargo clippy -p dsm -p dsm_sdk --all-features— clean.Related
trailing-byte malleability on that path; this PR closes it globally for all consumers.
op_idbinding (separate track).