Page MenuHomePhabricator

Merge #9980: Fix mem access violation merkleblock
ClosedPublic

Authored by jasonbcox on Fri, May 10, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Fri, May 10, 00:30
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, May 10, 00:30
markblundeberg accepted this revision.Fri, May 10, 01:35
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.Fri, May 10, 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.

Fabien accepted this revision.Tue, May 14, 08:35
This revision was automatically updated to reflect the committed changes.