serde_bser/mincode: validate wire-supplied lengths before indexing

eden/scm/lib/mincode/src/de.rs

@@ -34,6 +34,13 @@ impl<'de> Deserializer<'de> {
     #[inline]
     fn read_slice(&mut self) -> Result<&'de [u8]> {
         let len = Deserialize::deserialize(&mut *self)?;
+        if len > self.bytes.len() {
+            return Err(Error::new(format!(
+                "length {} exceeds remaining input of {} bytes",
+                len,
+                self.bytes.len()
+            )));
+        }
         let (slice, rest) = self.bytes.split_at(len);
         self.bytes = rest;
         Ok(slice)
@@ -103,13 +110,23 @@ impl<'de, 'a> serde::Deserializer<'de> for &'a mut Deserializer<'de> {
     where
         V: Visitor<'de>,
     {
+        if self.bytes.is_empty() {
+            return Err(Error::new("EOF while deserializing char"));
+        }
         let width = utf8_char_width(self.bytes[0]);
         if width == 1 {
             return visitor.visit_char(self.bytes[0] as char);
         }
         if width == 0 {
             return Err(Error::new("invalid char"));
         }
+        if width > self.bytes.len() {
+            return Err(Error::new(format!(
+                "truncated char: expected {} bytes, only {} remain",
+                width,
+                self.bytes.len()
+            )));
+        }
         let res = match str::from_utf8(&self.bytes[..width]) {
             Ok(s) => s.chars().next().unwrap(),
             Err(err) => {

eden/scm/lib/mincode/src/tests.rs

@@ -37,3 +37,39 @@ quickcheck! {
         foo == foo_deserialized
     }
 }
+
+#[test]
+fn string_length_longer_than_input_is_rejected() {
+    // VLQ length 5 followed by a single byte: the declared string is longer
+    // than the remaining input.
+    let bytes = [0x05, b'a'];
+    let r: Result<String, _> = crate::deserialize(&bytes);
+    assert!(
+        r.is_err(),
+        "expected an error for an oversized string length, got {:?}",
+        r
+    );
+}
+
+#[test]
+fn empty_input_for_char_is_rejected() {
+    let r: Result<char, _> = crate::deserialize(&[]);
+    assert!(
+        r.is_err(),
+        "expected an error for an empty char, got {:?}",
+        r
+    );
+}
+
+#[test]
+fn truncated_multibyte_char_is_rejected() {
+    // 0xC3 is the lead byte of a two-byte UTF-8 sequence; the continuation
+    // byte is missing.
+    let bytes = [0xC3];
+    let r: Result<char, _> = crate::deserialize(&bytes);
+    assert!(
+        r.is_err(),
+        "expected an error for a truncated char, got {:?}",
+        r
+    );
+}

watchman/rust/serde_bser/src/de/bunser.rs

@@ -90,6 +90,11 @@ where
     /// Return a borrowed or copied version of the next n bytes.
     #[inline]
     pub fn read_bytes<'s>(&'s mut self, len: i64) -> Result<Reference<'de, 's, [u8]>> {
+        if len < 0 {
+            // A negative wire-supplied length would cast to a huge `usize`
+            // below and panic in the underlying reader.
+            return Err(Error::DeNegativeLength { len });
+        }
         let len = len as usize;
         self.read
             .next_bytes(len, &mut self.scratch)

watchman/rust/serde_bser/src/de/read.rs

@@ -10,8 +10,8 @@ use std::fmt;
 use std::io;
 use std::result;
 
-use anyhow::Context as _;
 use anyhow::bail;
+use anyhow::Context as _;
 use byteorder::ByteOrder;
 use byteorder::NativeEndian;
 
@@ -210,18 +210,24 @@ where
         len: usize,
         scratch: &'s mut Vec<u8>,
     ) -> anyhow::Result<Reference<'de, 's, [u8]>> {
-        scratch.resize(len, 0);
-        let mut idx = 0;
-        if self.peeked.is_some() {
-            idx += 1;
-        }
-        if idx < len {
-            self.reader.read_exact(&mut scratch[idx..len])?;
-            debug_bytes!("{:x}", ByteBuf(&scratch[idx..len]));
-            self.read_count += len - idx;
-        }
+        // Grow the scratch buffer in bounded chunks instead of pre-allocating
+        // `len` bytes, so a wire-supplied length that is far larger than the
+        // stream can ever provide yields an EOF error rather than a capacity
+        // overflow panic.
+        scratch.clear();
         if let Some(peeked) = self.peeked.take() {
-            scratch[0] = peeked;
+            scratch.push(peeked);
+        }
+        let mut buf = [0u8; 4096];
+        while scratch.len() < len {
+            let want = (len - scratch.len()).min(buf.len());
+            let n = self.reader.read(&mut buf[..want])?;
+            if n == 0 {
+                bail!("eof while reading bytes/string");
+            }
+            debug_bytes!("{:x}", ByteBuf(&buf[..n]));
+            scratch.extend_from_slice(&buf[..n]);
+            self.read_count += n;
         }
         Ok(Reference::Copied(&scratch[0..len]))
     }

watchman/rust/serde_bser/src/errors.rs

@@ -29,6 +29,9 @@ pub enum Error {
     #[error("Expected {} bytes read, but only read {} bytes", .expected, .read)]
     DeEof { expected: usize, read: usize },
 
+    #[error("while deserializing BSER: negative length {}", .len)]
+    DeNegativeLength { len: i64 },
+
     #[error("Invalid magic header: {:?}", .magic)]
     DeInvalidMagic { magic: Vec<u8> },
 

watchman/rust/serde_bser/tests/negative_length.rs

@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+//! Regression tests for deserializing BSER payloads with a negative
+//! bytestring/utf8string length. The wire length is a signed integer and used
+//! to index into the buffer, so a negative value must be rejected instead of
+//! being reinterpreted as a huge `usize`.
+
+use std::io::Cursor;
+
+use serde_bser::de::from_reader;
+use serde_bser::de::from_slice;
+
+fn negative_len_bytestring() -> Vec<u8> {
+    // BSER magic + capabilities + PDU length (INT8 = 3) + a bytestring whose
+    // length is INT8 = -1 (0xFF).
+    let mut msg = vec![0x00, 0x02];
+    msg.extend_from_slice(&[0x00, 0x00, 0x00, 0x00]);
+    msg.extend_from_slice(&[0x03, 0x03]);
+    msg.extend_from_slice(&[0x02, 0x03, 0xFF]);
+    msg
+}
+
+#[test]
+fn negative_bytestring_length_is_rejected() {
+    let msg = negative_len_bytestring();
+    let r: Result<String, _> = from_slice(&msg);
+    assert!(
+        r.is_err(),
+        "expected a deserialization error for a negative bytestring length, got {:?}",
+        r
+    );
+}
+
+#[test]
+fn negative_bytestring_length_is_rejected_from_reader() {
+    let msg = negative_len_bytestring();
+    let r: Result<String, _> = from_reader(Cursor::new(msg));
+    assert!(
+        r.is_err(),
+        "expected a deserialization error for a negative bytestring length, got {:?}",
+        r
+    );
+}