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

fix decodeBytes() integer overflow on 32-bit systems #340

Merged
merged 1 commit into from
Dec 13, 2020

Conversation

erikgrinaker
Copy link
Contributor

Fixes #339.

@erikgrinaker erikgrinaker force-pushed the erik/decodebytes-32bit-check branch from b2539a9 to 237d17e Compare December 12, 2020 14:04
Copy link
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @erikgrinaker! Minor nit: if we could adapt the test in the repro would be nice. Also when this is approved, could you please cut a new release, and let's mail an update for all dependencies for cosmos/iavl? Thank you.

@erikgrinaker erikgrinaker merged commit 903e53d into master Dec 13, 2020
@erikgrinaker erikgrinaker deleted the erik/decodebytes-32bit-check branch December 13, 2020 09:36
@erikgrinaker
Copy link
Contributor Author

Minor nit: if we could adapt the test in the repro would be nice.

I don't have access to a 32-bit system at the moment, we'll look into this next week.

could you please cut a new release, and let's mail an update for all dependencies for cosmos/iavl?

I'll cut a new release and ping the SDK people, but don't feel like we need to announce this more broadly as 32-bit is fairly niche and supported on a best-effort basis.

@odeke-em
Copy link
Contributor

Thank you for the fix @erikgrinaker, and thank you for the review @marbar3778!

I'll cut a new release and ping the SDK people, but don't feel like we need to announce this more broadly as 32-bit is fairly niche and supported on a best-effort basis.

As I commented in #339 (comment), I just re-ran some thoughts and also fuzzed and alas identified a vector that crashes on 64-bit systems, as well with the same runtime out of range error when the length is saved as 9223372036854775806 and it cleverly circumvents the checks given that the code

 if len(bz) < n+int(size) { 
 	return nil, n, fmt.Errorf("insufficient bytes decoding []byte of length %v", size) 
 } 

n+int(9223372036854775806) on 64-bit systems --> -X
e.g. if n = 9, we get size 9223372036854775806 int(size) 9223372036854775806 n 9 len(bz) 9 -9223372036854775801.

This PR fixes also the 32-bit problems as well as for 64-bit systems, thus I recommend that we should perhaps issue updates to code using this package ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:bug Type: Bug (Confirmed)
Projects
None yet
3 participants