Skip to content

Commit

Permalink
Add missing bounds check to FamStructWrapper::deserialize
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
roypat committed Mar 24, 2023
1 parent 732eb81 commit a57a051
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 0 deletions.
12 changes: 12 additions & 0 deletions src/primitives.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,18 @@ where
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)
Expand Down
26 changes: 26 additions & 0 deletions tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1323,6 +1323,32 @@ 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();
Expand Down

0 comments on commit a57a051

Please sign in to comment.