Page MenuHomePhabricator

Merge #9980: Fix mem access violation merkleblock
ClosedPublic

Authored by jasonbcox on May 10 2019, 00:30.

Details

Summary

8276e70de Adding assert to avoid a memory access violation inside of PartialMerkleTree::CalcHash() (Chris Stewart)

Pull request description:

Fixing a possible memory access violation in CPartialMerkleTree::CalcHash().

This can happen if we some how a merkle tree with zero txids. I don't think this can happen in practice as we only send merkle block messages on the p2p network as of now -- we cannot receive them.

This was found with #8469, specifically using this [generator](https://github.com/Christewart/bitcoin/blob/rapidcheck/src/test/gen/merkleblock_gen.h#L52-L77) which will cause a memory access violation on [this test case](https://github.com/Christewart/bitcoin/blob/rapidcheck/src/test/merkleblock_properties.cpp#L48).

Tree-SHA512: b95904ec45ea3f082c7722161d93ee06b24c706fbffa909a6e995ed14788aed2830f91b626da6f0347660c45874a0735dab61c9440b59c949c690af4165c83fb

Backport of Core PR9980
https://github.com/bitcoin/bitcoin/pull/9980/files

Reviewer note: This seems like a vulnerability waiting to happen, so I carefully looked over it's usage. Short summary:

  • It cannot be exploited at present.
  • The node does not handle receiving CMerkleBlock, it only contructs and sends them from (and this is the important part) blocks that have been validated.
  • Potential future vectors to attack this assert could include:
    • Invalid block reflection attack if net_processing ever stores an invalid, 0-tx block in most_recent_block
    • Any new code that accepts CMerkleBlock from peers.
Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
pr9980
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5788
Build 9638: Bitcoin ABC Buildbot (legacy)
Build 9637: arc lint + arc unit

Event Timeline

markblundeberg added a subscriber: markblundeberg.

still exists in this form in Core.

I notice the comment in merkleblock.h in ABC is noticeably different from Core, did we put that in or missing backport?

This revision is now accepted and ready to land.May 10 2019, 01:35

still exists in this form in Core.

I notice the comment in merkleblock.h in ABC is noticeably different from Core, did we put that in or missing backport?

I put that in a while back because it wasn't clear.

This revision was automatically updated to reflect the committed changes.