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

x/bank/types: AddressFromBalancesStore address length checking condition false positive #9111

Closed
4 tasks
cuonglm opened this issue Apr 14, 2021 · 3 comments · Fixed by #9112
Closed
4 tasks

Comments

@cuonglm
Copy link
Contributor

cuonglm commented Apr 14, 2021

Summary of Bug

Coming here from #9060, the current address length checking condition in AddressFromBalancesStore has false positive:

if len(key[1:]) < int(addrLen) {

First, we check for len(key[1:]) < int(addrLen), but later, we do slice slicing with key[1 : addrLen+1].

The problem is that addrLen is encoded in a byte, so addrLen+1 can be overflow. It seems to me that we can fix this by using `int(addrLen)+1 instead.

Version

All versions.

Steps to Reproduce

_ = banktypes.AddressFromBalancesStore([]byte("\xff000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000")

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 14, 2021

cc @odeke-em

@odeke-em
Copy link
Collaborator

Nice catch @cuonglm and good to see that a fuzzer also caught it! Please help send a PR for it. Thank you!

@cuonglm
Copy link
Contributor Author

cuonglm commented Apr 14, 2021

Nice catch @cuonglm and good to see that a fuzzer also caught it! Please help send a PR for it. Thank you!

Added in #9112

odeke-em pushed a commit that referenced this issue Apr 15, 2021
…9112)

addrLen is encoded in a byte, so it's an uint8. The code in
AddressFromBalancesStore cast it to int for bound checking, but wrongly uses "addrLen+1", which can be overflow.

To fix this, just cast addrLen once and use it in all places.

Found by fuzzing added in #9060.

Fixes #9111
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 a pull request may close this issue.

2 participants