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

Unsoundness in nalgebra's serde Deserialize implementation #880

Closed
Andlon opened this issue Apr 30, 2021 · 5 comments
Closed

Unsoundness in nalgebra's serde Deserialize implementation #880

Andlon opened this issue Apr 30, 2021 · 5 comments

Comments

@Andlon
Copy link

Andlon commented Apr 30, 2021

We recently discovered a soundness issue related to the Deserialize implementation of nalgebra::VecStorage, which is the backing storage for any dynamically sized matrix/vector. nalgebra relies on a certain invariant so that it's sound to avoid some bounds checks in unsafe code. This is at the very least a soundness issue, but I believe it is possible for a malicious attacker to exploit this if an application is using serde to deserialize matrices/vectors from an untrusted source (for example, one can imagine a game or application using serde serialization over the network). By manipulating the input it is possible to induce unsound out-of-bounds reads and writes. As far as I know, this would apply to a wide range of nalgebra versions going back a long time.

Unfortunately I'm strapped for time and not able to file a formal advisory. I hope nonetheless that this issue is useful and adheres to the spirit of this project.

@thomcc
Copy link
Contributor

thomcc commented Jun 5, 2021

(cough https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_derive_deserialize should probably not be an allow-by-default lint cough)

@Andlon
Copy link
Author

Andlon commented Jun 6, 2021

@thomcc: was not aware of this lint! It's great that it's implemented, but, as you say, if it's not linted by default then many people are likely to be unaware of its existence...

@Andlon
Copy link
Author

Andlon commented Jun 6, 2021

This source of unsoundness should have been resolved in nalgebra version 0.27.1.

I don't know whether to close the issue or not, since it still affects older versions. I'm not sure exactly how issues here are supposed to be used. (I suppose ideally there'd be a proper advisory but I have no experience with them and I am still unable to devote the time to look into it)

@austinhartzheim
Copy link
Contributor

@Andlon I have opened PR #931 to add an advisory. I believe I have captured the substance of this issue, but I would be happy to update it if you have further suggestions.

(Likewise for anyone from RustSec. This is the first advisory I have added.)

@Andlon
Copy link
Author

Andlon commented Jun 6, 2021

@austinhartzheim: that's awesome, thanks! I think with this I can close this issue.

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

No branches or pull requests

3 participants