Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing bounds check to <FamStructWrapper as Versionize>::deserialize #53

Merged
merged 2 commits into from
Mar 24, 2023

Conversation

roypat
Copy link
Contributor

@roypat roypat commented Mar 24, 2023

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.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any user-facing changes are mentioned in CHANGELOG.md.

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>
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat merged commit 3385d79 into firecracker-microvm:main Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants