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

Deserialize is not sound for VecStorage (and possibly others) #883

Closed
Andlon opened this issue Apr 29, 2021 · 2 comments · Fixed by #889
Closed

Deserialize is not sound for VecStorage (and possibly others) #883

Andlon opened this issue Apr 29, 2021 · 2 comments · Fixed by #889
Assignees
Labels
bug P-high High priority

Comments

@Andlon
Copy link
Sponsor Collaborator

Andlon commented Apr 29, 2021

VecStorage currently looks like this:

/// A Vec-based matrix data storage. It may be dynamically-sized.
#[repr(C)]
#[derive(Eq, Debug, Clone, PartialEq)]
#[cfg_attr(feature = "serde-serialize", derive(Serialize, Deserialize))]
pub struct VecStorage<T, R: Dim, C: Dim> {
    data: Vec<T>,
    nrows: R,
    ncols: C,
}

VecStorage carries an implicit invariant in that nrows * ncols == data.len(), and I believe there is some unsafe code that relies on this invariant. However, since Deserialize is derived, it does not know about this invariant. Therefore it is possible to break this invariant in safe code by deserializing invalid data. This is a potential security issue, since a malicious attacker might potentially use this to read/write to invalid memory locations.

In order to fix this, we should write a custom Deserialize implementation that validates the input.

There may also be other soundness issues caused by invariants not being upheld by derived Deserialize implementations throughout the library.

@Andlon Andlon added bug P-high High priority labels Apr 29, 2021
@Andlon
Copy link
Sponsor Collaborator Author

Andlon commented Apr 30, 2021

Unfortunately I don't have the bandwidth to address this myself at the moment, but it seems to me that we should try to fix this sooner rather than later. Is there anyone who would be interested in taking this on? I'd be happy to assist/mentor and review code.

In addition to a custom Deserialize implementation, we should also have tests that verify that inputs that break the invariant are rejected during deserialization.

@sebcrozet
Copy link
Member

I will take care of this next week.

@sebcrozet sebcrozet self-assigned this Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug P-high High priority
Projects
None yet
2 participants