CVE-2023-28448: Out-of-bounds Read (CWE-125)
value: Versionize is a framework for version tolerant serializion/deserialization of Rust data structures, designed for usecases that need fast deserialization times and minimal size overhead. An issue was discovered in the ‘Versionize::deserialize’ implementation provided by the ‘versionize’ crate for ‘vmm_sys_utils::fam::FamStructWrapper', which can lead to out of bounds memory accesses. The impact started with version 0.1.1. The issue was corrected in version 0.1.10 by inserting a check that verifies, for any deserialized header, the lengths of compared flexible arrays are equal and aborting deserialization otherwise.

CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 7.5
Published: 2023-03-24T20:15Z
Repository: https://github.com/firecracker-microvm/versionize
Language: Rust
Files changed: src/primitives.rs, tests/test.rs
Commit date: 2023-03-24 13:46:54 +0000
Commit message: Add missing bounds check to FamStructWrapper::deserialize

An issue was discovered in the `Versionize::deserialize`
implementation provided by the `versionize` crate for
`vmm_sys_utils::fam::FamStructWrapper`, which can lead to out of
bounds memory accesses. Objects of this type are used to model
structures containing C-style flexible array members [1]. These
structures contain a memory allocation that is prefixed by a header
containing the size of the allocation.

Due to treating the header and the memory allocation as two objects,
`Versionize`'s data format stores the size of the allocation twice:
once in the header and then again as its own metadata of the memory
allocation. A serialized `FamStructWrapper` thus looks as follows:

+------------------------------------------------------------+\
| header (containing length of flexible array member `len1`) |\
+------------------------------------------------------------+\
+---------------------------------------+-----------------------+
| length of flexible array member`len2` | array member contents |
+---------------------------------------+-----------------------+

During deserialization, the library separately deserializes the
header and the memory allocation. It allocates `len2` bytes of
memory, and then prefixes it with the separately deserialized header.
Since `len2` is an implementation detail of the `Versionize`
implementation, it is forgotten about at the end of the deserialize
`function`, and all subsequent operations on the `FamStructWrapper`
assume the memory allocated to have size `len1`. If deserialization
input was malformed such that `len1 != len2`, then this can lead to
(safe) functions on ´FamStructWrapper` to read past the end of
allocated memory (if `len1 > len2`).

The issue was corrected by inserting a check that verifies that these
two lengths are equal, and aborting deserialization otherwise.

[1]: https://en.wikipedia.org/wiki/Flexible_array_member

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

Diff: diff --git a/src/primitives.rs b/src/primitives.rs
index a6ea4bf..f388b0d 100644
--- a/src/primitives.rs
+++ b/src/primitives.rs
@@ -367,10 +367,22 @@ where
         let header = T::deserialize(reader, version_map, app_version)
             .map_err(|ref err| VersionizeError::Deserialize(format!("{:?}", err)))?;
         let entries: Vec<<T as FamStruct>::Entry> =
             Vec::deserialize(reader, version_map, app_version)
                 .map_err(|ref err| VersionizeError::Deserialize(format!("{:?}", err)))?;
+
+        if header.len() != entries.len() {
+            let msg = format!(
+                "Mismatch between length of FAM specified in FamStruct header ({}) \
+                and actual size of FAM ({})",
+                header.len(),
+                entries.len()
+            );
+
+            return Err(VersionizeError::Deserialize(msg));
+        }
+
         // Construct the object from the array items.
         // Header(T) fields will be initialized by Default trait impl.
         let mut object = FamStructWrapper::from_entries(&entries)
             .map_err(|ref err| VersionizeError::Deserialize(format!("{:?}", err)))?;
         // Update Default T with the deserialized header.
diff --git a/tests/test.rs b/tests/test.rs
index 20b48cd..75e3cd1 100644
--- a/tests/test.rs
+++ b/tests/test.rs
@@ -1321,10 +1321,36 @@ impl<T> Versionize for __IncompleteArrayField<T> {
 }
 
 type MessageFamStructWrapper = FamStructWrapper<Message>;
 type Message2FamStructWrapper = FamStructWrapper<Message2>;
 
+#[test]
+fn test_deserialize_famstructwrapper_invalid_len() {
+    let mut vm = VersionMap::new();
+    vm.new_version()
+        .set_type_version(Message::type_id(), 2)
+        .new_version()
+        .set_type_version(Message::type_id(), 3)
+        .new_version()
+        .set_type_version(Message::type_id(), 4);
+
+    // Create FamStructWrapper with len 2
+    let state = MessageFamStructWrapper::new(0).unwrap();
+    let mut buffer = [0; 256];
+
+    state.serialize(&mut buffer.as_mut_slice(), &vm, 2).unwrap();
+
+    // the `len` field of the header is the first serialized field.
+    // Let's corrupt it by making it bigger than the actual number of serialized elements
+    buffer[0] = 255;
+
+    assert_eq!(
+        MessageFamStructWrapper::deserialize(&mut buffer.as_slice(), &vm, 2).unwrap_err(),
+        VersionizeError::Deserialize("Mismatch between length of FAM specified in FamStruct header (255) and actual size of FAM (0)".to_string())
+    );
+}
+
 #[test]
 fn test_versionize_famstructwrapper() {
     let mut vm = VersionMap::new();
     vm.new_version()
         .set_type_version(Message::type_id(), 2)

Fixed code: 
        if header.len() != entries.len() {
            let msg = format!(
                "Mismatch between length of FAM specified in FamStruct header ({}) \
                and actual size of FAM ({})",
                header.len(),
                entries.len()
            );

            return Err(VersionizeError::Deserialize(msg));
        }

#[test]
fn test_deserialize_famstructwrapper_invalid_len() {
    let mut vm = VersionMap::new();
    vm.new_version()
        .set_type_version(Message::type_id(), 2)
        .new_version()
        .set_type_version(Message::type_id(), 3)
        .new_version()
        .set_type_version(Message::type_id(), 4);

    // Create FamStructWrapper with len 2
    let state = MessageFamStructWrapper::new(0).unwrap();
    let mut buffer = [0; 256];

    state.serialize(&mut buffer.as_mut_slice(), &vm, 2).unwrap();

    // the `len` field of the header is the first serialized field.
    // Let's corrupt it by making it bigger than the actual number of serialized elements
    buffer[0] = 255;

    assert_eq!(
        MessageFamStructWrapper::deserialize(&mut buffer.as_slice(), &vm, 2).unwrap_err(),
        VersionizeError::Deserialize("Mismatch between length of FAM specified in FamStruct header (255) and actual size of FAM (0)".to_string())
    );
}