Skip to content

Fix crypto/merkle ProofOperators.Verify to check bounds on keypath pa…#2756

Merged
ebuchman merged 3 commits intodevelopfrom
jae/generalmerkle_fix_oob_error
Nov 6, 2018
Merged

Fix crypto/merkle ProofOperators.Verify to check bounds on keypath pa…#2756
ebuchman merged 3 commits intodevelopfrom
jae/generalmerkle_fix_oob_error

Conversation

@jaekwon
Copy link
Contributor

@jaekwon jaekwon commented Nov 5, 2018

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

NOTE: Go 1.11 has issues where despite the lack of bounds checking prior to this PR, it doesn't panic at all due to a bug in 1.11. https://go-review.googlesource.com/c/go/+/132495/ . Upgrade to 1.11.2.

@codecov-io
Copy link

codecov-io commented Nov 5, 2018

Codecov Report

Merging #2756 into develop will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2756      +/-   ##
===========================================
+ Coverage    62.21%   62.22%   +<.01%     
===========================================
  Files          212      212              
  Lines        17136    17185      +49     
===========================================
+ Hits         10661    10693      +32     
- Misses        5577     5592      +15     
- Partials       898      900       +2
Impacted Files Coverage Δ
crypto/merkle/proof.go 40.35% <100%> (+2.16%) ⬆️
libs/events/events.go 93.2% <0%> (-4.86%) ⬇️
libs/db/mem_db.go 82.9% <0%> (-2.39%) ⬇️
consensus/reactor.go 66.78% <0%> (-0.59%) ⬇️
p2p/pex/pex_reactor.go 73.05% <0%> (-0.33%) ⬇️
mempool/mempool.go 74.72% <0%> (+0.54%) ⬆️
blockchain/pool.go 66.43% <0%> (+0.69%) ⬆️

for i, op := range poz {
key := op.GetKey()
if len(key) != 0 {
if len(keys) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh damn, I was obviously wrong, since the keys were getting re-sliced and could run out before we finish looping over the poz. Very nice catch!

@ebuchman ebuchman merged commit 03e42d2 into develop Nov 6, 2018
@ebuchman ebuchman deleted the jae/generalmerkle_fix_oob_error branch November 6, 2018 06:53
maxim-levy pushed a commit to maxim-levy/tendermint that referenced this pull request Nov 13, 2018
* 'master' of https://github.com/tendermint/tendermint: (330 commits)
  Release/v0.26.1 (tendermint#2803)
  fix amino overhead computation for Tx (tendermint#2792)
  p2p: re-check after sleeps (tendermint#2664)
  check the result of `ps.peer.Send` before calling `ps.setHasVote` (tendermint#2787)
  p2p: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode (tendermint#2797)
  test AutoFile#Size (happy path)
  [autofile/group] do not panic when checking size
  openFile creates a file if not exist => ErrNotExist is not possible
  use our logger in autofile/group
  Add tests for ValidateBasic methods (tendermint#2754)
  [docs] improve organization of ABCI docs & fix links (tendermint#2749)
  p2p: peer-id -> peer_id (tendermint#2771)
  mempool: print postCheck error (tendermint#2762)
  Fix crypto/merkle ProofOperators.Verify to check bounds on keypath pa… (tendermint#2756)
  Mempool WAL is still created by default in home directory, leads to permission errors (tendermint#2758)
  mempool: ErrPreCheck and more log info (tendermint#2724)
  Release/v0.26.0 (tendermint#2726)
  [ADR] [DRAFT] pubsub 2.0 (tendermint#2532)
  validate reactor messages (tendermint#2711)
  TMHASH is 32 bytes. Closes tendermint#1990 (tendermint#2732)
  ...
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