fix(byte-storage): copy a large payload only after the fallback decode accepts the envelope (LAB-12208)

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

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 |

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<Cow<'de, [u8]>, D::Error> {
@@ -937,11 +953,11 @@ mod borrowed {
             }
 
             fn visit_borrowed_bytes<E: Error>(self, v: &'de [u8]) -> Result<Self::Value, E> {
-                Ok(borrow_below_large(v))
+                Ok(Cow::Borrowed(v))
             }
 
             fn visit_borrowed_str<E: Error>(self, v: &'de str) -> Result<Self::Value, E> {
-                Ok(borrow_below_large(v.as_bytes()))
+                Ok(Cow::Borrowed(v.as_bytes()))
             }
 
             fn visit_bytes<E: Error>(self, v: &[u8]) -> Result<Self::Value, E> {

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