pset: check commitment lengths before FFI parsing - #295
Conversation
|
Hi @ethicnology , thanks for reporting this. |
| // an out-of-bounds read (a segfault on empty input). | ||
| if bytes.len() != 33 { | ||
| return Err(encode::Error::ParseFailed("invalid pedersen commitment length")); | ||
| } |
There was a problem hiding this comment.
Value, Asset, and Nonce from_commitment all forward an arbitrary-length &[u8] to ConfInner::from_slice
Better to put the length check where the unchecked slice actually crosses into FFI — a small validated helper in src/confidential/ used by all three from_commitments (they already have CONFIDENTIAL_LEN = 33 in scope at asset.rs, and have the PSET Deserialize impls go through it. That covers the PSET fields and the public constructors.
There was a problem hiding this comment.
Thanks, addressed in e5a1d37.
The 33-byte validation now lives in a shared helper under src/confidential/, with a single shared CONFIDENTIAL_LEN. Value::from_commitment, Asset::from_commitment, Nonce::from_commitment, and the PSET Deserialize implementations all go through it before calling from_slice.
282869c to
e357ba3
Compare
The PSET input fields `issuance_value_comm` and `issuance_inflation_keys_comm`, and the output fields `amount_comm` and `asset_comm`, deserialize attacker-controlled byte strings via `PedersenCommitment::from_slice` and `Generator::from_slice`. Those wrap FFI functions that take no length argument and unconditionally read 33 bytes from the slice pointer. An empty or short value therefore triggers an out-of-bounds read; on an empty value the dangling pointer (address 0x1) is dereferenced and the process segfaults instead of returning an error. Found by libFuzzer/AddressSanitizer on the `deserialize_pset` target: SEGV on unknown address 0x1, read access. Reproduced on 0.25.3, 0.26.2 and master. Reject any length other than 33 bytes at the PSET deserialization boundary, before reaching FFI. Add regression tests for empty, short and invalid-content inputs, plus the minimized fuzz artifact. The same missing length check exists in the safe `secp256k1-zkp` wrappers themselves; to be reported separately upstream.
e357ba3 to
e5a1d37
Compare
|
IMO we should fix upstream rust-secp256k1-zkp rather than putting length checks in here. |
3 participants
The PSET input fields
issuance_value_commandissuance_inflation_keys_comm, and the output fieldsamount_commandasset_comm, deserialize attacker-controlled byte strings viaPedersenCommitment::from_sliceandGenerator::from_slice. Those wrap FFI functions (secp256k1_pedersen_commitment_parse,secp256k1_generator_parse) that take no length argument and unconditionally read 33 bytes from the slice pointer. An empty or short value therefore triggers an out-of-bounds read; on an empty value the dangling pointer (address 0x1) is dereferenced and the process segfaults instead of returning an error.Found by libFuzzer/AddressSanitizer on the
deserialize_psettarget: SEGV on unknown address 0x1, read access, zero page. Reproduced on 0.25.3, 0.26.2 and current master (8765552). Any application parsing untrusted PSETs (wallets, signing or watch-only services) can be crashed by a malicious PSET: denial of service. The faulting access is a fixed read at 0x1; we did not assess further exploitability.Reject any length other than 33 bytes at the PSET deserialization boundary, before reaching FFI. Adds regression tests for empty, short and invalid-content inputs, plus the minimized 397-byte fuzz artifact. The full test suite passes (160 tests) and a 6.3M-execution ASan run of
deserialize_psetwith the fix found no further crash.The same missing length check exists in the safe
secp256k1-zkp::zkpwrappers themselves (PedersenCommitment::from_slice,Generator::from_slice); to be reported separately to BlockstreamResearch/rust-secp256k1-zkp.