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

crypto/types: check for overflow and unreasonably large element count #9163

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

odeke-em
Copy link
Collaborator

Ensure that we don't pass overflowed values into make, because
a clever attacker could see that to cause:

(bits+7)/8

to become negative, they just have to make (bits+7) become negative
simply by >=maxint-6

but also reject unreasonably large element count like >2**32, which
while arbitrary is super duper large for a bit array.

Fixes #9162


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@odeke-em
Copy link
Collaborator Author

/cc @cuonglm

Ensure that we don't pass overflowed values into make, because
a clever attacker could see that to cause:

    (bits+7)/8

to become negative, they just have to make (bits+7) become negative
simply by >=maxint-6

but also reject unreasonably large element count like >2**32, which
while arbitrary is super duper large for a bit array.

Fixes #9162
@odeke-em odeke-em force-pushed the crypto-types-CompactBitArray-fix-bits-overflow branch from 361b2a6 to 5ed97d5 Compare April 22, 2021 00:42
@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #9163 (5ed97d5) into master (603e895) will increase coverage by 0.07%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9163      +/-   ##
==========================================
+ Coverage   58.82%   58.89%   +0.07%     
==========================================
  Files         583      585       +2     
  Lines       32769    32801      +32     
==========================================
+ Hits        19275    19318      +43     
+ Misses      11216    11199      -17     
- Partials     2278     2284       +6     
Impacted Files Coverage Δ
x/gov/types/hooks.go 0.00% <0.00%> (ø)
x/gov/keeper/keeper.go 73.46% <50.00%> (+22.35%) ⬆️
crypto/types/compact_bit_array.go 75.65% <100.00%> (+0.65%) ⬆️
simapp/app.go 83.09% <100.00%> (+0.33%) ⬆️
x/gov/abci.go 89.23% <100.00%> (+0.34%) ⬆️
x/gov/keeper/deposit.go 70.58% <100.00%> (+1.54%) ⬆️
x/gov/keeper/hooks.go 100.00% <100.00%> (ø)
x/gov/keeper/proposal.go 78.64% <100.00%> (+5.11%) ⬆️
x/gov/keeper/vote.go 89.28% <100.00%> (+0.19%) ⬆️
... and 5 more

@odeke-em
Copy link
Collaborator Author

Thank you for the reviews @cuonglm and @alessio. Please note that this is a security fix so a judgment will be needed for backporting if necessary.

@odeke-em odeke-em merged commit 417832f into master Apr 22, 2021
@odeke-em odeke-em deleted the crypto-types-CompactBitArray-fix-bits-overflow branch April 22, 2021 03:09
alessio pushed a commit that referenced this pull request Apr 22, 2021
@alessio
Copy link
Contributor

alessio commented Apr 22, 2021

0.42 backport PR available here: #9173

alessio pushed a commit that referenced this pull request Apr 22, 2021
…#9173)

From: #9163
Fixes: #9162
Thanks: @odeke-em for the patch.

Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants