serde_bser/mincode: validate wire-supplied lengths before indexing

BSER and mincode deserializers take lengths directly from the encoded
data and use them to index into buffers without validation, panicking
on malformed input instead of returning an error.

serde_bser:
- Reject negative bytestring/utf8string lengths in read_bytes instead of
  casting them to a huge usize.
- Grow the IoRead scratch buffer in bounded chunks instead of
  pre-allocating the wire length, avoiding a capacity-overflow panic when
  the declared length is far larger than the stream.

mincode:
- Reject string/bytes lengths that exceed the remaining input before
  calling split_at.
- Reject empty or truncated chars instead of indexing out of bounds.

Add regression tests for each panic.

Author: rootkiller6788

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
+    );
+}