Skip to content

Commit ffb791d

Browse files
fix(dpp): bound length-prefixed document field reads before allocating
Document deserialization read a varint length from the (untrusted) serialized bytes and immediately allocated a Vec of that size, for both variable-size string/byte-array fields and nested object payloads. A proved getDocuments response is decoded before its quorum signature is checked, so a self-consistent GroveDB proof carrying a document whose length prefix claims gigabytes made every client that verifies proofs (rs-sdk, wasm-sdk, and the Dash Core embedder) abort on allocation failure from ~70 bytes of input. Both sites now check the declared length against the bytes remaining in the reader and reject with CorruptedSerialization before allocating. A regression test feeds a 2^62 length prefix to both the string and object arms.
1 parent 90543a6 commit ffb791d

1 file changed

Lines changed: 65 additions & 14 deletions

File tree

  • packages/rs-dpp/src/data_contract/document_type/property

‎packages/rs-dpp/src/data_contract/document_type/property/mod.rs‎

Lines changed: 65 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,33 @@ impl DocumentPropertyType {
645645
}
646646
}
647647

648+
/// Reads exactly `len` bytes, refusing up front when the buffer does
649+
/// not hold that many. The length comes from the (possibly untrusted)
650+
/// serialized document itself, so it must never drive an allocation
651+
/// before it has been checked against the bytes actually present.
652+
fn read_exact_bounded(
653+
buf: &mut BufReader<&[u8]>,
654+
len: usize,
655+
what: &str,
656+
) -> Result<Vec<u8>, DataContractError> {
657+
// `BufReader<&[u8]>` advances the underlying slice as it refills, so
658+
// the bytes still readable are whatever is buffered plus whatever the
659+
// slice has left.
660+
let remaining = buf.buffer().len() + buf.get_ref().len();
661+
if len > remaining {
662+
return Err(DataContractError::CorruptedSerialization(format!(
663+
"{what} declares {len} bytes but only {remaining} remain in the serialized document"
664+
)));
665+
}
666+
let mut value = vec![0u8; len];
667+
buf.read_exact(&mut value).map_err(|_| {
668+
DataContractError::CorruptedSerialization(format!(
669+
"error reading {what} of length {len} from serialized document"
670+
))
671+
})?;
672+
Ok(value)
673+
}
674+
648675
fn read_varint_value(buf: &mut BufReader<&[u8]>) -> Result<Vec<u8>, DataContractError> {
649676
let bytes: usize = buf.read_varint().map_err(|_| {
650677
DataContractError::CorruptedSerialization(
@@ -654,14 +681,7 @@ impl DocumentPropertyType {
654681
if bytes == 0 {
655682
Ok(vec![])
656683
} else {
657-
let mut value: Vec<u8> = vec![0u8; bytes];
658-
buf.read_exact(&mut value).map_err(|_| {
659-
DataContractError::CorruptedSerialization(format!(
660-
"error reading varint of length {} from serialized document",
661-
bytes
662-
))
663-
})?;
664-
Ok(value)
684+
Self::read_exact_bounded(buf, bytes, "varint value")
665685
}
666686
}
667687

@@ -834,12 +854,7 @@ impl DocumentPropertyType {
834854
"error reading varint of object length".to_string(),
835855
)
836856
})?;
837-
let mut object_bytes = vec![0u8; object_byte_len];
838-
buf.read_exact(&mut object_bytes).map_err(|_| {
839-
DataContractError::CorruptedSerialization(
840-
"error reading object bytes".to_string(),
841-
)
842-
})?;
857+
let object_bytes = Self::read_exact_bounded(buf, object_byte_len, "object")?;
843858
// Wrap the bytes in a BufReader
844859
let mut object_buf_reader = BufReader::new(&object_bytes[..]);
845860
let mut finished_buffer = false;
@@ -4239,6 +4254,42 @@ mod tests {
42394254
// read_optionally_from() tests
42404255
// -----------------------------------------------------------------------
42414256

4257+
/// A serialized document is untrusted input: a length prefix must never
4258+
/// size an allocation before it has been checked against the bytes that
4259+
/// are actually present. Before this check a two-byte string field
4260+
/// declaring a multi-gigabyte length aborted the process on allocation.
4261+
#[test]
4262+
fn test_read_optionally_from_rejects_length_prefix_longer_than_input() {
4263+
use std::io::BufReader;
4264+
let prop = DocumentPropertyType::String(StringPropertySizes {
4265+
min_length: None,
4266+
max_length: None,
4267+
});
4268+
// varint 2^62 followed by two bytes of payload
4269+
let mut data = vec![0xffu8, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x3f];
4270+
data.extend_from_slice(b"ab");
4271+
let mut reader = BufReader::new(data.as_slice());
4272+
let err = prop
4273+
.read_optionally_from(&mut reader, true)
4274+
.expect_err("oversized length prefix must be rejected, not allocated");
4275+
assert!(
4276+
err.to_string()
4277+
.contains("remain in the serialized document"),
4278+
"unexpected error: {err}"
4279+
);
4280+
4281+
let object = DocumentPropertyType::Object(IndexMap::new());
4282+
let mut reader = BufReader::new(data.as_slice());
4283+
let err = object
4284+
.read_optionally_from(&mut reader, true)
4285+
.expect_err("oversized object length must be rejected, not allocated");
4286+
assert!(
4287+
err.to_string()
4288+
.contains("remain in the serialized document"),
4289+
"unexpected error: {err}"
4290+
);
4291+
}
4292+
42424293
#[test]
42434294
fn test_read_optionally_from_optional_marker_none() {
42444295
use std::io::BufReader;

0 commit comments

Comments
 (0)