Fix unsoundess arising with deserialize then get_unchecked

Cargo.toml

@@ -4,10 +4,13 @@ members = [
     "vec",
     "set",
     "matrix",
+    "fuzz",
 ]
 
-exclude = [
-    "fuzz",
+default-members = [
+    "vec",
+    "set",
+    "matrix"
 ]
 
 resolver = "2"

RELEASES.md

@@ -1,8 +1,11 @@
-Version 0.10.0 (TO BE RELEASED)
+Version 0.10.0 (VULNERABILITY FIX) (TO BE RELEASED)
 ==========================
 
 <a id="v0.10.0"></a>
 
+- fixed a soundness issue where `deserialize`ing from an untrusted source
+  could make `fn get_unchecked` perform undefined behavior, and could make
+  other functions return incorrect results
 - removed nanoserde support
 
 Version 0.9.1

fuzz/Cargo.toml

@@ -12,15 +12,17 @@ cargo-fuzz = true
 default = ["afl"]
 afl_fuzz = ["afl"]
 honggfuzz_fuzz = ["honggfuzz"]
+# hack to make vscode happy
+serde = []
+miniserde = []
+borsh = []
 
 [dependencies]
 honggfuzz = { version = "0.5", optional = true }
 afl = { version = "0.17", optional = true }
-bit-vec = { path = "../vec/" }
+bit-vec = { path = "../vec/", features = ["serde", "borsh", "miniserde"] }
 bit-set = { path = "../set/" }
 bit-matrix = { path = "../matrix/" }
 # smallvec = "1.15"
-
-[[bin]]
-name = "bit_ops"
-path = "fuzz_targets/bit_ops.rs"
+serde = "1.0"
+serde_json = "1.0"

fuzz/README.md

@@ -5,5 +5,12 @@ Based on fuzzing in `smallvec`.
 # fuzzing
 
 ```sh
-cargo afl build --release --bin bit_ops --features afl && cargo afl fuzz -i in -o out target/release/bit_ops
+cargo afl build --release --features afl && cargo afl fuzz -i in -o out ../target/release/bit-fuzz
+```
+
+It may ask you to prepare the Linux system by running commands such as these:
+```sh
+echo core | sudo tee /proc/sys/kernel/core_pattern
+cd /sys/devices/system/cpu
+echo performance | sudo tee cpu*/cpufreq/scaling_governor
 ```

fuzz/fuzz_targets/bit_ops.rs → fuzz/src/main.rs

@@ -1,7 +1,7 @@
 //! Simple fuzzer testing all available `BitVec`, `BitSet` and `BitMatrix` operations
 
-use bit_vec::{BitBlock, BitVec};
 use bit_set::BitSet;
+use bit_vec::{BitBlock, BitVec};
 // use smallvec::SmallVec;
 
 // There's no point growing too much, so try not to grow
@@ -20,6 +20,17 @@ macro_rules! next_u8 {
     };
 }
 
+macro_rules! next_string {
+    ($b:ident) => {
+        String::from_utf8(
+            (0..next_u8!($b))
+                .map(|_| $b.next().unwrap_or(0) & 0b_01_11_11_11)
+                .collect::<Vec<_>>(),
+        )
+        .expect("why do we have unicode where we shouldn't?")
+    };
+}
+
 fn black_box_bit_vec<T: BitBlock>(s: &BitVec<T>) {
     // print to work as a black_box
     print!("{}", s);
@@ -30,13 +41,15 @@ fn black_box_bit_set<T: BitBlock>(s: &BitSet<T>) {
     print!("{}", s);
 }
 
-fn do_test<T: BitBlock>(data: &[u8]) -> BitVec<T> {
+fn do_test<T: BitBlock + for<'de> serde::Deserialize<'de> + serde::Serialize>(
+    data: &[u8],
+) -> BitVec<T> {
     let mut v = BitVec::<T>::new_general();
 
     let mut bytes = data.iter().copied();
 
     while let Some(op) = bytes.next() {
-        match op % 23 {
+        match op % 25 {
             0 => {
                 v = BitVec::new_general();
             }
@@ -120,6 +133,17 @@ fn do_test<T: BitBlock>(data: &[u8]) -> BitVec<T> {
             22 => {
                 v.fill(true);
             }
+            23 => {
+                let json = serde_json::to_string(&v).unwrap();
+                let deserialized = serde_json::from_str(&json[..]).unwrap();
+                assert_eq!(v, deserialized);
+            }
+            24 => {
+                let input_str = next_string!(bytes);
+                if let Ok(deserialized) = serde_json::from_str(&input_str[..]) {
+                    v = deserialized;
+                }
+            }
             _ => panic!("booo"),
         }
     }
@@ -209,7 +233,7 @@ fn do_test_all(data: &[u8]) {
     do_test_set::<u8>(data);
     do_test_set::<u16>(data);
     do_test_set::<u64>(data);
-    // do_test_set::<u32, SmallVec<[u32; 8]>>(data);
+    // do_test_set::<u32, SmallVec<[str; 8]>>(data);
     // do_test_set::<u16, Vec<u16>>(data);
 }
 

vec/src/lib.rs

@@ -95,6 +95,11 @@
 #[cfg(any(test, feature = "std"))]
 #[macro_use]
 extern crate std;
+
+#[cfg(all(feature = "std", feature = "borsh"))]
+use std::borrow::ToOwned;
+#[cfg(all(feature = "std", feature = "miniserde"))]
+use std::boxed::Box;
 #[cfg(feature = "std")]
 use std::rc::Rc;
 #[cfg(feature = "std")]
@@ -105,13 +110,19 @@ use std::vec::Vec;
 #[cfg(not(feature = "std"))]
 #[macro_use]
 extern crate alloc;
+#[cfg(all(not(feature = "std"), feature = "borsh"))]
+use alloc::borrow::ToOwned;
+#[cfg(all(not(feature = "std"), feature = "miniserde"))]
+use alloc::boxed::Box;
 #[cfg(not(feature = "std"))]
 use alloc::rc::Rc;
 #[cfg(not(feature = "std"))]
 use alloc::string::String;
 #[cfg(not(feature = "std"))]
 use alloc::vec::Vec;
 
+use core::error::Error;
+
 #[cfg(feature = "borsh")]
 extern crate borsh;
 #[cfg(feature = "miniserde")]
@@ -222,22 +233,163 @@ bit_block_impl! {
 /// println!("{:?}", bv);
 /// println!("total bits set to true: {}", bv.iter().filter(|x| *x).count());
 /// ```
-#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
-#[cfg_attr(
-    feature = "borsh",
-    derive(borsh::BorshDeserialize, borsh::BorshSerialize)
-)]
-#[cfg_attr(
-    feature = "miniserde",
-    derive(miniserde::Deserialize, miniserde::Serialize)
-)]
+#[cfg_attr(feature = "serde", derive(serde::Serialize))]
+#[cfg_attr(feature = "borsh", derive(borsh::BorshSerialize))]
+#[cfg_attr(feature = "miniserde", derive(miniserde::Serialize))]
 pub struct BitVec<B = u32> {
     /// Internal representation of the bit vector
     storage: Vec<B>,
     /// The number of valid bits in the internal representation
     nbits: usize,
 }
 
+#[cfg(any(feature = "serde", feature = "borsh", feature = "miniserde"))]
+#[cfg_attr(feature = "serde", derive(serde::Deserialize))]
+#[cfg_attr(feature = "borsh", derive(borsh::BorshDeserialize))]
+#[cfg_attr(feature = "miniserde", derive(miniserde::Deserialize))]
+struct UncheckedBitVec<B = u32> {
+    storage: Vec<B>,
+    nbits: usize,
+}
+
+#[cfg(feature = "serde")]
+impl<'de, B: BitBlock> serde::Deserialize<'de> for BitVec<B>
+where
+    B: serde::Deserialize<'de>,
+{
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        use serde::de::Error;
+        UncheckedBitVec::<B>::deserialize(deserializer).and_then(|unchecked| {
+            let result = BitVec {
+                storage: unchecked.storage,
+                nbits: unchecked.nbits,
+            };
+            if !result.storage_len_matches_nbits() {
+                Err(D::Error::custom(DeserializeError::StorageLenMismatch))
+            } else if !result.is_last_block_fixed() {
+                Err(D::Error::custom(DeserializeError::TrailingBits))
+            } else {
+                Ok(result)
+            }
+        })
+    }
+}
+
+#[cfg(feature = "borsh")]
+impl<B: BitBlock> borsh::BorshDeserialize for BitVec<B>
+where
+    B: borsh::BorshDeserialize,
+{
+    fn deserialize_reader<R: borsh::io::Read>(reader: &mut R) -> borsh::io::Result<Self> {
+        UncheckedBitVec::<B>::deserialize_reader(reader).and_then(|unchecked| {
+            let result = BitVec {
+                storage: unchecked.storage,
+                nbits: unchecked.nbits,
+            };
+            if !result.storage_len_matches_nbits() {
+                Err(borsh::io::Error::other(DeserializeError::StorageLenMismatch))
+            } else if !result.is_last_block_fixed() {
+                Err(borsh::io::Error::other(DeserializeError::TrailingBits))
+            } else {
+                Ok(result)
+            }
+        })
+    }
+}
+
+#[cfg(feature = "miniserde")]
+miniserde::make_place!(Place);
+
+#[cfg(feature = "miniserde")]
+struct BitVecBuilder<'a, B> {
+    storage: Option<Vec<B>>,
+    nbits: Option<usize>,
+    out: &'a mut Option<BitVec<B>>,
+}
+
+#[cfg(feature = "miniserde")]
+impl<B: BitBlock> miniserde::de::Visitor for Place<BitVec<B>>
+where
+    B: miniserde::Deserialize,
+{
+    fn map(&mut self) -> miniserde::Result<Box<dyn miniserde::de::Map + '_>> {
+        Ok(Box::new(BitVecBuilder {
+            storage: None,
+            nbits: None,
+            out: &mut self.out,
+        }))
+    }
+}
+
+#[cfg(feature = "miniserde")]
+impl<B: BitBlock> miniserde::de::Map for BitVecBuilder<'_, B>
+where
+    B: miniserde::Deserialize,
+{
+    fn key(&mut self, k: &str) -> miniserde::Result<&mut dyn miniserde::de::Visitor> {
+        match k {
+            "storage" => Ok(miniserde::Deserialize::begin(&mut self.storage)),
+            "nbits" => Ok(miniserde::Deserialize::begin(&mut self.nbits)),
+            _ => Ok(<dyn miniserde::de::Visitor>::ignore()),
+        }
+    }
+
+    fn finish(&mut self) -> miniserde::Result<()> {
+        let storage = self.storage.take().ok_or(miniserde::Error)?;
+        let nbits = self.nbits.take().ok_or(miniserde::Error)?;
+        let result = BitVec { storage, nbits };
+        if !result.storage_len_matches_nbits() || !result.is_last_block_fixed() {
+            Err(miniserde::Error)
+        } else {
+            *self.out = Some(result);
+            Ok(())
+        }
+    }
+}
+
+#[cfg(feature = "miniserde")]
+impl<B: BitBlock> miniserde::Deserialize for BitVec<B>
+where
+    B: miniserde::Deserialize,
+{
+    fn begin(out: &mut Option<Self>) -> &mut dyn miniserde::de::Visitor {
+        Place::new(out)
+    }
+}
+
+#[derive(Debug)]
+pub enum DeserializeError {
+    StorageLenMismatch,
+    TrailingBits,
+}
+
+impl core::fmt::Display for DeserializeError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(self.simple_description())
+    }
+}
+
+#[cfg(feature = "borsh")]
+impl From<DeserializeError> for String {
+    fn from(value: DeserializeError) -> Self {
+        value.simple_description().to_owned()
+    }
+}
+
+impl Error for DeserializeError {}
+
+impl DeserializeError {
+    fn simple_description(&self) -> &str {
+        match self {
+            DeserializeError::TrailingBits => "some out of bounds trailing bits are set",
+            DeserializeError::StorageLenMismatch => "nbits and storage length isnt same",
+        }
+    }
+}
+
 // FIXME(Gankro): NopeNopeNopeNopeNope (wait for IndexGet to be a thing)
 impl<B: BitBlock> Index<usize> for BitVec<B> {
     type Output = bool;
@@ -587,6 +739,11 @@ impl<B: BitBlock> BitVec<B> {
         }
     }
 
+    /// Checks whether our `nbits` fits within our storage.
+    fn storage_len_matches_nbits(&self) -> bool {
+        self.storage.len() == blocks_for_bits::<B>(self.nbits)
+    }
+
     /// Ensure the invariant for the last block.
     ///
     /// An operation might screw up the unused bits in the last block of the
@@ -597,6 +754,7 @@ impl<B: BitBlock> BitVec<B> {
     #[inline]
     fn ensure_invariant(&self) {
         if cfg!(test) {
+            debug_assert!(self.storage_len_matches_nbits());
             debug_assert!(self.is_last_block_fixed());
         }
     }