Fix PSBT key deserialisation byte size
What the developer wrote
Authored by Mitchell Bagot
Fix PSBT key deserialisation byte size
The PSBT Key decoding makes an assumption that the key type will always
encoding in a single byte in it's compact form. While this is often
correct, this is not correct according to the spec and may fail in
cases where key type values are large.
Fix the encode and decode implementations on Key to correctly calculate
the type value size to add to the key length size.
What changed, and why it matters
This commit fixes a bug in how Partially Signed Bitcoin Transactions (PSBTs) read and write their internal 'key' fields. The old code assumed the key's type number always fits in one byte when encoded as a compact size. That assumption is wrong for large type numbers, so the code could misread the key's length and either reject valid PSBTs or, in the worst case, allocate too much memory. The fix makes the encoder and decoder correctly account for the actual number of bytes the type number occupies.
Review whether the decode path can be induced to allocate an oversized vector or read out-of-bounds before the fix, and consider adding regression tests with key type values > 0xFC. Users should upgrade if they parse untrusted PSBTs.
Security signals we found
Incorrect length calculation in deserialization
Possible oversized vector allocation due to wrong size subtraction
Spec non-compliance with BIP-174 compact-size encoding
Memory allocation check bypass risk
Evidence from the diff
In bitcoin/src/psbt/raw.rs, Key::decode previously read the total byte size, subtracted 1 as the type-value length, and then read the type value. This is only correct when type_value <= 0xFC (one-byte compact size). For larger type values (3, 5, or 9 bytes), the computed key_data length is wrong, leading to oversized/undersized reads and incorrect MAX_VEC_SIZE checks. Key::serialize similarly always added 1 instead of the real encoded size. The patch computes the actual compact-size encoded length of type_value and uses it in both encode and decode. It also changes the length conversion to saturate at usize::MAX rather than using ToU64.
Changed components
bitcoin/src/psbt/raw.rsPSBT Key serialization/deserializationInspect captured patch +31 / −10
diff --git a/bitcoin/src/psbt/raw.rs b/bitcoin/src/psbt/raw.rs
index 91d50590..7773f78e 100644
--- a/bitcoin/src/psbt/raw.rs
+++ b/bitcoin/src/psbt/raw.rs
@@ -9,7 +9,6 @@ use core::fmt;
#[cfg(feature = "arbitrary")]
use arbitrary::{Arbitrary, Unstructured};
-use internals::ToU64 as _;
use io::{BufRead, Write};
use super::serialize::{Deserialize, Serialize};
@@ -68,25 +67,39 @@ impl fmt::Display for Key {
impl Key {
pub(crate) fn decode<R: BufRead + ?Sized>(r: &mut R) -> Result<Self, Error> {
- let byte_size = r.read_compact_size()?;
+ // Convert compact size to usize, saturating at max.
+ // If this value exceeds MAX_VEC_SIZE (a usize), we'll error down below, so it's fine
+ // to discard any higher value.
+ let byte_size: usize = r.read_compact_size()?
+ .try_into()
+ .unwrap_or(usize::MAX);
if byte_size == 0 {
return Err(Error::NoMorePairs);
}
- let key_byte_size: u64 = byte_size - 1;
-
- if key_byte_size > MAX_VEC_SIZE.to_u64() {
+ // byte_size is the length of key_data + the length of type_value encoding
+ let type_value = r.read_compact_size()?;
+ // The protocol abuses a compact size here to encode a value that is never used in
+ // relation to memory so conversion to a usize cannot be done (and thus
+ // CompactSizeEncoder::encoded_size can't be used)
+ let type_size = match type_value {
+ 0..=0xFC => 1,
+ 0xFD..=0xFFFF => 3,
+ 0x10000..=0xFFFF_FFFF => 5,
+ _ => 9,
+ };
+ let key_byte_size = byte_size - type_size;
+
+ if key_byte_size > MAX_VEC_SIZE {
return Err(encode::Error::Parse(encode::ParseError::OversizedVectorAllocation {
- requested: key_byte_size as usize,
+ requested: key_byte_size,
max: MAX_VEC_SIZE,
})
.into());
}
- let type_value = r.read_compact_size()?;
-
- let mut key_data = Vec::with_capacity(key_byte_size as usize);
+ let mut key_data = Vec::with_capacity(key_byte_size);
for _ in 0..key_byte_size {
key_data.push(Decodable::consensus_decode(r)?);
}
@@ -98,7 +111,15 @@ impl Key {
impl Serialize for Key {
fn serialize(&self) -> Vec<u8> {
let mut buf = Vec::new();
- buf.emit_compact_size(self.key_data.len() + 1).expect("in-memory writers don't error");
+
+ // First compact size value is the length of key_data + the length of the encoded type_value
+ let type_size = match self.type_value {
+ 0..=0xFC => 1,
+ 0xFD..=0xFFFF => 3,
+ 0x10000..=0xFFFF_FFFF => 5,
+ _ => 9,
+ };
+ buf.emit_compact_size(self.key_data.len() + type_size).expect("in-memory writers don't error");
buf.emit_compact_size(self.type_value).expect("in-memory writers don't error");
Why this scored 50/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.