Fix crash bug with duplicate inputs within a transaction #14247

pull TheBlueMatt wants to merge 2 commits into bitcoin:master from TheBlueMatt:2018-09-checktransaction changing 2 files +11 −1
  1. TheBlueMatt commented at 9:57 pm on September 17, 2018: member
  2. Fix crash bug with duplicate inputs within a transaction 
    Introduced by #9049
    b8f801964f
  3. [qa] Test for duplicate inputs within a transaction 9b4a36effc
  4. laanwj commented at 10:10 pm on September 17, 2018: member
    tested ACK 9b4a36effcf642f3844c6696b757266686ece11a
  6. sdaftuar commented at 10:18 pm on September 17, 2018: member
    ACK
  7. laanwj added the label Consensus on Sep 17, 2018
  8. MarcoFalke commented at 10:34 pm on September 17, 2018: member
    utACK 9b4a36effcf642f3844c6696b757266686ece11a. Checked that the test fails without the fix and passes with the fix.
  9. in src/validation.cpp:3125 in 9b4a36effc 
    3121@@ -3122,7 +3122,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P
3122 
3123     // Check transactions
3124     for (const auto& tx : block.vtx)
3125-        if (!CheckTransaction(*tx, state, false))
3126+        if (!CheckTransaction(*tx, state, true))
    ryanofsky commented at 10:52 pm on September 17, 2018:

    ~Just noting it looks like this argument can be removed entirely in a future cleanup PR.~

    Update: Removing the argument was attempted in #14258, but actually turns out not to be a good idea because the optimization will probably be reintroduced in the future (https://github.com/bitcoin/bitcoin/pull/14258#issuecomment-422795765).

  10. ryanofsky commented at 10:55 pm on September 17, 2018: member
    Slightly tested ACK 9b4a36effcf642f3844c6696b757266686ece11a, just ran python test with & without fix.
  11. gmaxwell commented at 10:59 pm on September 17, 2018: contributor
    ACK
  12. laanwj referenced this in commit c64128df58 on Sep 17, 2018
  13. laanwj merged this on Sep 17, 2018
  14. laanwj closed this on Sep 17, 2018
  15. laanwj referenced this in commit d926a87fde on Sep 17, 2018
  16. laanwj referenced this in commit 696b936aa3 on Sep 17, 2018
  17. laanwj referenced this in commit a765d575dd on Sep 18, 2018
  18. laanwj referenced this in commit f37124f2a7 on Sep 18, 2018
  19. laanwj referenced this in commit 71c5900381 on Sep 18, 2018
  20. laanwj referenced this in commit 52965fbaef on Sep 18, 2018
  21. laanwj referenced this in commit 4b8a3f5d23 on Sep 18, 2018
  22. practicalswift commented at 3:21 pm on September 18, 2018: contributor

    utACK 9b4a36effcf642f3844c6696b757266686ece11a

    Very nice find @sdaftuar! How did you find this issue?

  23. luke-jr commented at 6:09 pm on September 18, 2018: member
    For reference, this issue was assigned CVE-2018-17144
  24. ftrader commented at 6:30 pm on September 18, 2018: none
    Could @TheBlueMatt / @sdaftuar outline the responsible disclosure procedures, if any, that were attempted to other projects before publishing these pull requests?
  25. TheBlueMatt commented at 7:45 pm on September 18, 2018: member
    The bug was disclosed to other projects simultaneously to it being disclosed to us. We tried to coordinate timelines for release but sadly didn’t want to wait much past EST business hours to get people patched so ran out of time.
  26. zquestz commented at 8:19 pm on September 18, 2018: none
    Where can I find a detailed description of the impact of this bug? When is the CVE getting published so I can get more clarity on the exact impact? I am seeing all sorts of reports on Twitter and would like to get an accurate assessment of the issue.
  27. PierreRochard commented at 9:31 pm on September 18, 2018: contributor

    Where can I find a detailed description of the impact of this bug? When is the CVE getting published so I can get more clarity on the exact impact? I am seeing all sorts of reports on Twitter and would like to get an accurate assessment of the issue.

    Release notes: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.16.3.md#denial-of-service-vulnerability

    More color in today’s OpTech newsletter ( https://mailchi.mp/cc96f82565eb/bitcoin-optech-newsletter-13?e=cbe2b70569 ):

    Upgrade to Bitcoin Core 0.16.3 to fix denial-of-service vulnerability: a bug introduced in Bitcoin Core 0.14.0 and affecting all subsequent versions through to 0.16.2 will cause Bitcoin Core to crash when attempting to validate a block containing a transaction that attempts to spend the same input twice. Such blocks would be invalid and so can only be created by miners willing to lose the allowed income from having created a block (at least 12.5 XBT or $80,000 USD).

    Patches for master and 0.16 branches were submitted for public review yesterday, the 0.16.3 release has been tagged containing the patch, and binaries will be available for download as soon as a sufficient number of well-known contributors have reproduced the deterministic build—probably later today (Tuesday). Immediate upgrade is highly recommended.

  28. cryptapus referenced this in commit d018a9b73e on Sep 18, 2018
  29. deadalnix commented at 0:23 am on September 19, 2018: none

    We tried to coordinate timelines for release […]

    Tell us more about this.

  30. sickpig commented at 7:46 am on September 19, 2018: none

    We tried to coordinate timelines for release but sadly didn’t want to wait much past EST business hours to get people patched so ran out of time.

    I was among the people who received the anonymous report, even though BU was not affected, and I’m not aware of any kind of effort to “coordinate timelines”.

    As far as I can tell the list of people to which the bug was disclosed was not exhaustive, not by a long shot, so make sure to have a proper way to tell projects/devs that their code is at risk is a must IMHO.

    It would be great to know what were the actions taken to put in places the aforementioned coordination process. The aim is to improve the process in case of new disclosures that could happen in the future.

  31. Sjors commented at 8:57 am on September 19, 2018: member
    Post merge slightly tested 9b4a36e, ran python test without fix which produced the assert.
  32. biblepay commented at 9:44 am on September 19, 2018: none
    Thanks.
  33. globaltoken referenced this in commit 5a201f2af0 on Sep 19, 2018
  34. dartdart26 referenced this in commit b1fa9eb160 on Sep 19, 2018
  35. lateminer referenced this in commit f8062c3b57 on Sep 19, 2018
  36. sidhujag referenced this in commit 8c462d09fb on Sep 19, 2018
  37. expocash referenced this in commit 0c37829493 on Sep 20, 2018
  38. practicalswift commented at 2:11 pm on September 20, 2018: contributor
    I would like to thank the researcher who invested time into finding this issue. Impressive finding! Thanks!
  39. isghe commented at 9:44 pm on September 20, 2018: contributor
    Is this bug reproducible in regtest or testnet? Thanks
  40. Sjors commented at 10:27 am on September 21, 2018: member

    @isghe the bug is reproducible on regtest using the Python test from this PR. You need to change CheckTransaction(*tx, state, true) back to CheckTransaction(*tx, state, false) and then run test/functional/p2p_invalid_block.py. The test contains the kind of invalid block that can crash an (unpatched) node. You’ll see a crash caused by an assert.

    CVE-2018-17144 Full Disclosure, which includes the inflation bug not mentioned above: https://bitcoincore.org/en/2018/09/20/notice/

    I assume we’ll add a test case for the inflation bug once the dust has settled?

  41. promag commented at 10:47 am on September 21, 2018: member

    I assume we’ll add a test case for the inflation bug once the dust has settled? @gmaxwell already suggested it, also agree.

  42. ftrader commented at 4:04 am on September 22, 2018: none

    The discoverer of CVE-2018-17144 has come forward (for those wishing to express their gratitude, there is an included donation address in the post):

    https://medium.com/@awemany/600-microseconds-b70f87b0b2a6

  43. cryptomeow referenced this in commit dffb8c270a on Sep 22, 2018
  44. litebitcoins referenced this in commit 4cc80c81cf on Sep 23, 2018
  45. tpruvot referenced this in commit 954c382f9a on Sep 23, 2018
  46. Stouse49 referenced this in commit 947f668d9b on Sep 24, 2018
  47. isghe commented at 11:18 pm on September 26, 2018: contributor
    It looks they created with success 0.1 tBTC from nothing, exploiting this bug in testnet3 block 1414411, hash 00000000eba3f43a8624750f39e4520a1678c0dbdf8707bfa4854a12fbf086c5
  48. MarcoFalke locked this on Sep 26, 2018


TheBlueMatt laanwj jamesob sdaftuar MarcoFalke ryanofsky gmaxwell practicalswift luke-jr ftrader zquestz PierreRochard deadalnix sickpig Sjors biblepay isghe promag

Labels
Consensus

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me