Fix crash bug with duplicate inputs within a transaction

TheBlueMatt commented at 9:57 pm on September 17, 2018: member

Introduced by #9049

b8f801964f

[qa] Test for duplicate inputs within a transaction 9b4a36effc

laanwj commented at 10:10 pm on September 17, 2018: member

tested ACK 9b4a36effcf642f3844c6696b757266686ece11a

jamesob commented at 10:10 pm on September 17, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/14247/commits/9b4a36effcf642f3844c6696b757266686ece11a

sdaftuar commented at 10:18 pm on September 17, 2018: member

ACK

laanwj added the label Consensus on Sep 17, 2018

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.

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).

ryanofsky commented at 10:55 pm on September 17, 2018: member

Slightly tested ACK 9b4a36effcf642f3844c6696b757266686ece11a, just ran python test with & without fix.

gmaxwell commented at 10:59 pm on September 17, 2018: contributor

ACK

laanwj referenced this in commit c64128df58 on Sep 17, 2018

laanwj merged this on Sep 17, 2018

laanwj closed this on Sep 17, 2018

laanwj referenced this in commit d926a87fde on Sep 17, 2018

laanwj referenced this in commit 696b936aa3 on Sep 17, 2018

laanwj referenced this in commit a765d575dd on Sep 18, 2018

laanwj referenced this in commit f37124f2a7 on Sep 18, 2018

laanwj referenced this in commit 71c5900381 on Sep 18, 2018

laanwj referenced this in commit 52965fbaef on Sep 18, 2018

laanwj referenced this in commit 4b8a3f5d23 on Sep 18, 2018

practicalswift commented at 3:21 pm on September 18, 2018: contributor

utACK 9b4a36effcf642f3844c6696b757266686ece11a

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

luke-jr commented at 6:09 pm on September 18, 2018: member

For reference, this issue was assigned CVE-2018-17144

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?

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.

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.

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.

cryptapus referenced this in commit d018a9b73e on Sep 18, 2018

deadalnix commented at 0:23 am on September 19, 2018: none

We tried to coordinate timelines for release […]

Tell us more about this.

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.

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.

biblepay commented at 9:44 am on September 19, 2018: none

Thanks.

globaltoken referenced this in commit 5a201f2af0 on Sep 19, 2018

dartdart26 referenced this in commit b1fa9eb160 on Sep 19, 2018

lateminer referenced this in commit f8062c3b57 on Sep 19, 2018

sidhujag referenced this in commit 8c462d09fb on Sep 19, 2018

expocash referenced this in commit 0c37829493 on Sep 20, 2018

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!

isghe commented at 9:44 pm on September 20, 2018: contributor

Is this bug reproducible in regtest or testnet? Thanks

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?

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.

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

cryptomeow referenced this in commit dffb8c270a on Sep 22, 2018

litebitcoins referenced this in commit 4cc80c81cf on Sep 23, 2018

tpruvot referenced this in commit 954c382f9a on Sep 23, 2018

Stouse49 referenced this in commit 947f668d9b on Sep 24, 2018

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

MarcoFalke locked this on Sep 26, 2018

Fix crash bug with duplicate inputs within a transaction #14247