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

Check invariants after deserialization #9

Merged
merged 1 commit into from
Mar 19, 2020

Conversation

Mrmaxmeier
Copy link
Contributor

@Mrmaxmeier Mrmaxmeier commented Mar 15, 2020

Xref: solana-labs/solana#8873

The travis failure seems unrelated.

@ryoqun
Copy link

ryoqun commented Mar 16, 2020

@tov Hi, thanks for maintaining this crate! To add more context, this is a DoS remote-exploitable vulnerability when directly exposed to an untrusted byte stream upon the consumption of such badly-deserialized BitVecs.

When len is abnormally out of range of bits (which could happen currently by deserializing a mal-formed BitVec), a BitVec illegally accesses memory causing a SEGV like this. So, we're currently [patch]-ing this PR, hoping to be fiexed upstream in some near future. For that purpose, I'm really eager to lend some hands like writing tests.

@Mrmaxmeier Last but least, thanks for writing a PR for us. :) I've confirmed this PR actually fixes the security hole and I think this is a correct solution to it.

@tov
Copy link
Owner

tov commented Mar 16, 2020

@Mrmaxmeier, thanks for the PR and nice catch @ryoqun, thanks for the explanation. I’m going to read the changes and look into the CI failure, and then presumably merge this and release.

@ryoqun
Copy link

ryoqun commented Mar 17, 2020

@Mrmaxmeier I think rebasing this on the recently-merged commit solves the CI failure. :)

@Mrmaxmeier Mrmaxmeier force-pushed the deserialize-check-invariants branch from ec84d94 to 9055386 Compare March 17, 2020 09:44
@ryoqun
Copy link

ryoqun commented Mar 17, 2020

@Mrmaxmeier I think rebasing this on the recently-merged commit solves the CI failure. :)

@Mrmaxmeier Thanks for doing so!

@tov Our side's job got done; Now please merge and release in your convenient time. :) Or is there other concerns?

@tov tov merged commit bac626a into tov:master Mar 19, 2020
@tov
Copy link
Owner

tov commented Mar 19, 2020

Sorry for being slow—it’s the end of the quarter. I was hoping to write some tests, but I will release now and do that later. Thanks!

@mvines
Copy link

mvines commented Mar 19, 2020

woot, thanks @tov!

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.

4 participants