From e5a1d371d9a64b2c15b2faa9134c77cd009cf2be Mon Sep 17 00:00:00 2001 From: ethicnology Date: Tue, 4 Aug 2026 17:42:35 -0400 Subject: [PATCH] pset: check commitment lengths before FFI parsing 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. --- src/confidential/asset.rs | 5 ++--- src/confidential/mod.rs | 17 +++++++++++++++++ src/confidential/nonce.rs | 6 +++--- src/confidential/value.rs | 5 ++--- src/pset/mod.rs | 26 ++++++++++++++++++++++++++ src/pset/serialize.rs | 8 ++++++-- 6 files changed, 56 insertions(+), 11 deletions(-) diff --git a/src/confidential/asset.rs b/src/confidential/asset.rs index 5636b4bf..a8a24ff1 100644 --- a/src/confidential/asset.rs +++ b/src/confidential/asset.rs @@ -10,7 +10,7 @@ use secp256k1_zkp::{self, Generator, Secp256k1, Signing, Tweak, ZERO_TWEAK}; #[cfg(feature = "serde")] use serde::{Deserialize, Deserializer, Serialize, Serializer}; -use super::CommitmentEncoder; +use super::{checked_commitment_slice, CommitmentEncoder, CONFIDENTIAL_LEN}; use crate::encode::{self, Decodable, Encodable}; use crate::encoding; use crate::issuance::AssetId; @@ -19,7 +19,6 @@ type ExplicitInner = AssetId; type ConfInner = Generator; const EXPLICIT_LEN: usize = 32; -const CONFIDENTIAL_LEN: usize = 33; const CONFIDENTIAL_LEN_LESS_PREFIX: usize = CONFIDENTIAL_LEN - 1; const CONF_PREFIX_1: u8 = 0x0a; const CONF_PREFIX_2: u8 = 0x0b; @@ -57,7 +56,7 @@ impl Asset { /// Create from commitment. pub fn from_commitment(bytes: &[u8]) -> Result { - Ok(Self::Confidential(ConfInner::from_slice(bytes)?)) + Ok(Self::Confidential(ConfInner::from_slice(checked_commitment_slice(bytes)?)?)) } /// Check if the object is null. diff --git a/src/confidential/mod.rs b/src/confidential/mod.rs index 22fe9542..bfbdac99 100644 --- a/src/confidential/mod.rs +++ b/src/confidential/mod.rs @@ -51,6 +51,16 @@ pub use self::value::{ use crate::issuance::AssetId; use crate::{encode, encoding}; +const CONFIDENTIAL_LEN: usize = 33; + +pub(crate) fn checked_commitment_slice(bytes: &[u8]) -> Result<&[u8], encode::Error> { + // The upstream FFI parsers take no length and unconditionally read 33 bytes. + if bytes.len() != CONFIDENTIAL_LEN { + return Err(encode::Error::ParseFailed("invalid confidential commitment length")); + } + Ok(bytes) +} + #[derive(Clone, Debug)] enum CommitmentEncoder<'e> { Null(u8), @@ -323,6 +333,13 @@ mod tests { #[test] fn commitments() { + for len in [0usize, 1, 32, 34] { + let bytes = vec![0u8; len]; + assert!(Value::from_commitment(&bytes).is_err()); + assert!(Asset::from_commitment(&bytes).is_err()); + assert!(Nonce::from_commitment(&bytes).is_err()); + } + let x = Value::from_commitment(&VALUE_COMMITMENT1).unwrap(); let commitment = x.commitment().unwrap(); let mut commitment = commitment.serialize(); diff --git a/src/confidential/nonce.rs b/src/confidential/nonce.rs index 12310e0c..49185a65 100644 --- a/src/confidential/nonce.rs +++ b/src/confidential/nonce.rs @@ -10,7 +10,7 @@ use secp256k1_zkp::{self, PublicKey, Secp256k1, SecretKey, Signing}; #[cfg(feature = "serde")] use serde::{Deserialize, Deserializer, Serialize, Serializer}; -use super::CommitmentEncoder; +use super::{checked_commitment_slice, CommitmentEncoder, CONFIDENTIAL_LEN}; use crate::encode::{self, Decodable, Encodable}; use crate::encoding; use crate::hashes::sha256d; @@ -19,7 +19,6 @@ type ExplicitInner = [u8; 32]; type ConfInner = PublicKey; const EXPLICIT_LEN: usize = 32; -const CONFIDENTIAL_LEN: usize = 33; const CONFIDENTIAL_LEN_LESS_PREFIX: usize = CONFIDENTIAL_LEN - 1; const CONF_PREFIX_1: u8 = 0x02; const CONF_PREFIX_2: u8 = 0x03; @@ -99,7 +98,8 @@ impl Nonce { /// Create from commitment. pub fn from_commitment(bytes: &[u8]) -> Result { Ok(Self::Confidential( - ConfInner::from_slice(bytes).map_err(secp256k1_zkp::Error::Upstream)?, + ConfInner::from_slice(checked_commitment_slice(bytes)?) + .map_err(secp256k1_zkp::Error::Upstream)?, )) } diff --git a/src/confidential/value.rs b/src/confidential/value.rs index 930e20c3..c992c821 100644 --- a/src/confidential/value.rs +++ b/src/confidential/value.rs @@ -14,7 +14,7 @@ use secp256k1_zkp::{ #[cfg(feature = "serde")] use serde::{Deserialize, Deserializer, Serialize, Serializer}; -use super::CommitmentEncoder; +use super::{checked_commitment_slice, CommitmentEncoder, CONFIDENTIAL_LEN}; use crate::confidential::AssetBlindingFactor; use crate::encode::{self, Decodable, Encodable}; use crate::encoding; @@ -24,7 +24,6 @@ type ExplicitInner = u64; type ConfInner = PedersenCommitment; const EXPLICIT_LEN: usize = 8; -const CONFIDENTIAL_LEN: usize = 33; const CONFIDENTIAL_LEN_LESS_PREFIX: usize = CONFIDENTIAL_LEN - 1; const CONF_PREFIX_1: u8 = 0x08; const CONF_PREFIX_2: u8 = 0x09; @@ -78,7 +77,7 @@ impl Value { /// Create from commitment. pub fn from_commitment(bytes: &[u8]) -> Result { - Ok(Self::Confidential(ConfInner::from_slice(bytes)?)) + Ok(Self::Confidential(ConfInner::from_slice(checked_commitment_slice(bytes)?)?)) } /// Check if the object is null. diff --git a/src/pset/mod.rs b/src/pset/mod.rs index d4f254be..d8861477 100644 --- a/src/pset/mod.rs +++ b/src/pset/mod.rs @@ -807,6 +807,32 @@ mod tests { assert_eq!(encode::serialize_hex(&pset), pset_hex); } + #[test] + fn deserialize_commitments_rejects_bad_lengths() { + use crate::pset::serialize::Deserialize as _; + + // The upstream FFI parsers read 33 bytes without a length argument; + // empty and short inputs must be rejected before reaching FFI. + for len in [0usize, 1, 32, 34] { + let bytes = vec![0u8; len]; + assert!(secp256k1_zkp::PedersenCommitment::deserialize(&bytes).is_err()); + assert!(secp256k1_zkp::Generator::deserialize(&bytes).is_err()); + } + // A 33-byte input reaches FFI and is rejected on content, not by crash. + let bytes = vec![0u8; 33]; + assert!(secp256k1_zkp::PedersenCommitment::deserialize(&bytes).is_err()); + assert!(secp256k1_zkp::Generator::deserialize(&bytes).is_err()); + } + + #[test] + fn deserialize_malformed_pset_returns_error() { + // Minimized libFuzzer artifact (397 bytes): deserializing this input + // segfaulted (read at address 0x1) instead of returning an error. + let bytes = hex::decode_to_vec("70736574ff01050a02ffffc50070736574ff010204ccbeff0001fb040200000027ff030000feff010000000500005808080032000081040808080808080808080808080808080f080808736574ff01fb0104ff0001fb040200000027ff030000feff0100000005000058080808080808080808080808080808080072700008736574ff01fb0104010000d670736574ff000005000070736574ff030000020001000005002929220a0a0202027073654545454545454547474747474747471111111100ff1111111111116574d0ffffff011107fc04707365740100001111111111111111111111111111010074ff0a0a02b102020229292929292929220a0a0202027073654545454545454574ff02b80202020a0a454545454545454545ffe2e2e20002006511111111111111111111040200000027ff030000feff0100ffffffffffffffffffff080808080808080808080f0808087347474747474747471111111100ff1111111111116574d0ffffff011107fc047073657401000011110100000000000000111111110100").unwrap(); + let result: Result = encode::deserialize(&bytes); + assert!(result.is_err()); + } + #[test] fn test_pset() { tx_pset_rtt("010000000001715df5ccebaf02ff18d6fae7263fa69fed5de59c900f4749556eba41bc7bf2af0000000000000000000201230f4f5d4b7c6fa845806ee4f67713459e1b69e8e60fcee2e4940c7a0d5de1b2010000000124101100001f5175517551755175517551755175517551755175517551755175517551755101230f4f5d4b7c6fa845806ee4f67713459e1b69e8e60fcee2e4940c7a0d5de1b2010000000005f5e100000000000000"); diff --git a/src/pset/serialize.rs b/src/pset/serialize.rs index cf39eaca..8523556c 100644 --- a/src/pset/serialize.rs +++ b/src/pset/serialize.rs @@ -235,7 +235,9 @@ impl Serialize for secp256k1_zkp::PedersenCommitment { impl Deserialize for secp256k1_zkp::PedersenCommitment { fn deserialize(bytes: &[u8]) -> Result { - let comm = secp256k1_zkp::PedersenCommitment::from_slice(bytes)?; + let comm = secp256k1_zkp::PedersenCommitment::from_slice( + confidential::checked_commitment_slice(bytes)?, + )?; Ok(comm) } } @@ -248,7 +250,9 @@ impl Serialize for secp256k1_zkp::Generator { impl Deserialize for secp256k1_zkp::Generator { fn deserialize(bytes: &[u8]) -> Result { - let comm = secp256k1_zkp::Generator::from_slice(bytes)?; + let comm = secp256k1_zkp::Generator::from_slice( + confidential::checked_commitment_slice(bytes)?, + )?; Ok(comm) } }