diff --git a/README.md b/README.md index 5849875..f84bf65 100644 --- a/README.md +++ b/README.md @@ -434,8 +434,10 @@ summed-slot message, `decode pre-scan: declares more elements than the input can back`: any decoder, and the pre-scan's end-of-input check, rejects that envelope too, so only the exact message proves the slot check ran. `src/read_allocation_probe.rs` (unit tests only) bounds what the size-cap and -ratio reads allocate, with a positive control, and pins what an accepted -msgpack-`bin` `retrieve_data` and `retrieve` below 64 KiB allocate. +ratio reads allocate, with a positive control, pins what an accepted +msgpack-`bin` `retrieve_data` and `retrieve` below 64 KiB allocate, and +checks that a read whose envelope decode fails copies none of a `bin` or `str` +payload of 64 KiB or more. `tests/wire_format_constructed.rs` runs the fixture's constructed 32-bit ratio-product vector natively and, in CI, on `wasm32-unknown-unknown` under `wasm-bindgen-test`; `tests/wire_format_vectors.rs` decodes the constructed diff --git a/SECURITY.md b/SECURITY.md index 208e3a8..d04f8d4 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -194,6 +194,19 @@ input, including every input it would reject, to the pre-scan and `rmp_serde`, so a rejection is theirs: a pre-scan rejection carries the error below, and an `rmp_serde` rejection is `DeserializationFailed` with `rmp_serde`'s message. +Both decodes borrow a `bin` payload under 64 KiB from the input, and copy a +longer one out of it only once the decode has accepted the envelope, so the +decode never copies the payload of an envelope it rejects. +`rejected_envelope_reads_do_not_copy_a_large_payload` in +`src/read_allocation_probe.rs` counts what `retrieve` and `retrieve_data` +request from the allocator on five envelopes that pass the pre-scan, carry a +`bin32` or `str32` payload of 64 KiB or more, and fail the typed decode in a +later field; each read must request less than 64 KiB. An envelope the decode +accepts has its long payload copied even when the size, ratio or checksum +check that follows rejects it. A legacy int-array payload has no bytes to +borrow, so it is decoded into an owned buffer as it is read, rejected or not: +one byte per element, and each element takes at least one byte of input. + The pre-scan rejects: | Rule | Bound | diff --git a/src/byte_storage.rs b/src/byte_storage.rs index 786e5ae..c065011 100644 --- a/src/byte_storage.rs +++ b/src/byte_storage.rs @@ -69,7 +69,7 @@ const MAX_COMPRESSION_RATIO: u64 = 1000; /// (128 KiB), even doubled, so it is never mmap-ed, and removing it cannot /// move either threshold. #[cfg(all(feature = "compression", feature = "checksum", feature = "messagepack"))] -const LARGE_PAYLOAD: usize = 64 * 1024; +pub(crate) const LARGE_PAYLOAD: usize = 64 * 1024; /// Inputs shorter than this are written as one literals-only LZ4 block instead /// of going through `lz4_flex::compress`. @@ -746,6 +746,7 @@ fn decode_envelope_full( ByteStorageError::DeserializationFailed(format!("decode pre-scan: {what}")) })?; rmp_serde::from_slice(envelope_bytes) + .map(borrowed::StorageEnvelope::copy_large_payload) .map_err(|e| ByteStorageError::DeserializationFailed(e.to_string())) } @@ -834,6 +835,19 @@ mod borrowed { extract_payload(&self.compressed_data, &self.checksum, self.original_size) } + /// Copy a borrowed payload from `LARGE_PAYLOAD` up out of the input, + /// as `decode_one_pass` does once it has read the whole envelope. The + /// serde decode borrows every `bin` and `str` payload, and + /// `decode_envelope_full` calls this only after `rmp_serde` has + /// accepted the envelope, so the payload of an envelope it rejects is + /// never copied. + pub(super) fn copy_large_payload(mut self) -> Self { + if let Cow::Borrowed(payload) = self.compressed_data { + self.compressed_data = borrow_below_large(payload); + } + self + } + /// One pass over the shape `rmp_serde` writes for every envelope: a /// fixarray of 4 holding `compressed_data` as bin8, bin16 or bin32; /// `checksum` as a fixarray of 8, each byte a positive fixint or a @@ -920,10 +934,12 @@ mod borrowed { } /// serde_bytes' `ByteBuf` decode, which `StorageEnvelope::compressed_data` - /// uses, except that a `bin` or `str` payload under `LARGE_PAYLOAD` is - /// borrowed. Owned bytes and strings reach `visit_bytes` and `visit_str` - /// through serde's default methods. serde_bytes' own `Cow` decode borrows - /// too, but words its type error differently. + /// uses, except that a `bin` or `str` payload is borrowed, whatever its + /// length: the envelope may still be rejected, and `copy_large_payload` + /// copies one from `LARGE_PAYLOAD` up only once it is accepted. Owned + /// bytes and strings reach `visit_bytes` and `visit_str` through serde's + /// default methods. serde_bytes' own `Cow` decode borrows too, but words + /// its type error differently. fn borrow_payload<'de, D: Deserializer<'de>>( deserializer: D, ) -> Result, D::Error> { @@ -937,11 +953,11 @@ mod borrowed { } fn visit_borrowed_bytes(self, v: &'de [u8]) -> Result { - Ok(borrow_below_large(v)) + Ok(Cow::Borrowed(v)) } fn visit_borrowed_str(self, v: &'de str) -> Result { - Ok(borrow_below_large(v.as_bytes())) + Ok(Cow::Borrowed(v.as_bytes())) } fn visit_bytes(self, v: &[u8]) -> Result { diff --git a/src/read_allocation_probe.rs b/src/read_allocation_probe.rs index 7577430..f0ba1af 100644 --- a/src/read_allocation_probe.rs +++ b/src/read_allocation_probe.rs @@ -20,8 +20,15 @@ //! `LARGE_PAYLOAD` allocates: `retrieve_data` requests only what the LZ4 //! decoder does (the one-pass decode reads these envelopes without //! allocating), and `retrieve` adds the format `String` and nothing else. A -//! legacy int-array payload, and a payload from `LARGE_PAYLOAD` up, -//! are copied out of the input by design. +//! payload from `LARGE_PAYLOAD` up is copied out of the input by design, but +//! only once the envelope decode has accepted the envelope: +//! `rejected_envelope_reads_do_not_copy_a_large_payload` checks that reading +//! five such envelopes, each rejected by the decode in a later field, +//! requests less than `LARGE_PAYLOAD`. An envelope the decode accepts is +//! copied even when `extract_payload` then rejects it (a size cap, the ratio +//! bound, a checksum mismatch). A legacy +//! int-array payload is decoded into an owned buffer as it is read, so it is +//! allocated whether or not the envelope is then accepted. //! //! The same count, [`bytes_requested`], also pins the write side: the //! `byte_storage` unit test `store_allocates_only_its_output_and_block` checks @@ -154,6 +161,78 @@ mod tests { } } + /// The fallback decode copies a `bin` or `str` payload from + /// `LARGE_PAYLOAD` up out of the input only once it has accepted the + /// envelope, as the one-pass decode does. Each envelope here is a complete + /// document, so it passes the pre-scan and reaches `rmp_serde`, which + /// reads the payload before it finds the fault in a later field. + #[test] + fn rejected_envelope_reads_do_not_copy_a_large_payload() { + use crate::byte_storage::LARGE_PAYLOAD; + + // bin32 at the copy threshold and past it, and str32 (zero bytes are + // valid UTF-8), which reaches the visitor's other borrowing method. + for (kind, marker, payload_len) in [ + ("bin32", 0xc6, LARGE_PAYLOAD), + ("bin32", 0xc6, 2 * LARGE_PAYLOAD), + ("str32", 0xdb, LARGE_PAYLOAD), + ] { + let mut payload = vec![marker]; + payload.extend_from_slice(&(payload_len as u32).to_be_bytes()); + payload.resize(payload.len() + payload_len, 0); + let checksum = [0x98, 1, 2, 3, 4, 5, 6, 7, 8]; + let short_checksum = [0x97, 1, 2, 3, 4, 5, 6, 7]; + let size = [0x01]; + let format = [0xa1, b'f']; + for (name, envelope) in [ + ( + "ends before format", + [&[0x93][..], &payload, &checksum, &size].concat(), + ), + ( + "trailing fifth element", + [&[0x95][..], &payload, &checksum, &size, &format, &[0xc0]].concat(), + ), + ( + "integer format", + [&[0x94][..], &payload, &checksum, &size, &[0x07]].concat(), + ), + ( + "invalid UTF-8 format", + [&[0x94][..], &payload, &checksum, &size, &[0xa1, 0xff]].concat(), + ), + ( + "7-byte checksum", + [&[0x94][..], &payload, &short_checksum, &size, &format].concat(), + ), + ] { + let storage = ByteStorage::new(None); + let (full, full_requested) = bytes_requested(|| storage.retrieve(&envelope)); + let (data, data_requested) = bytes_requested(|| storage.retrieve_data(&envelope)); + for (call, err, requested) in [ + ("retrieve", full.err(), full_requested), + ("retrieve_data", data.err(), data_requested), + ] { + let label = format!("[{name}, {payload_len} B {kind} payload] {call}"); + let Some(ByteStorageError::DeserializationFailed(msg)) = err else { + panic!("{label}: expected DeserializationFailed, got {err:?}"); + }; + // A pre-scan rejection never reaches the payload decode, + // so the count below would pass whatever it does. + assert!( + !msg.starts_with("decode pre-scan: "), + "{label}: rejected by the pre-scan, not rmp_serde: {msg}" + ); + assert!( + requested < LARGE_PAYLOAD, + "{label}: requested {requested} B from the allocator, \ + reaching LARGE_PAYLOAD ({msg})" + ); + } + } + } + } + /// The structural pre-scan keeps its nesting stack on the call stack for /// an envelope, current or legacy, at every payload size, so walking one /// requests nothing from the allocator. `retrieve` runs the walk only in