Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash bug with duplicate inputs within a transaction #14247

Merged
merged 2 commits into from Sep 17, 2018

Conversation

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Sep 17, 2018

No description provided.

@laanwj
Copy link
Member

@laanwj laanwj commented Sep 17, 2018

tested ACK 9b4a36e

@jamesob
Copy link
Member

@jamesob jamesob commented Sep 17, 2018

utACK 9b4a36e

@sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Sep 17, 2018

ACK

@laanwj laanwj added the Consensus label Sep 17, 2018
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Sep 17, 2018

utACK 9b4a36e. Checked that the test fails without the fix and passes with the fix.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

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

@@ -3122,7 +3122,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P

// Check transactions
for (const auto& tx : block.vtx)
if (!CheckTransaction(*tx, state, false))
if (!CheckTransaction(*tx, state, true))

This comment has been minimized.

@ryanofsky

ryanofsky Sep 17, 2018
Contributor

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 (#14258 (comment)).

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Sep 17, 2018

ACK

laanwj added a commit that referenced this pull request Sep 17, 2018
…nsaction

0d49c82 [qa] backport: Test for duplicate inputs within a transaction (Suhas Daftuar)
833180f Fix crash bug with duplicate inputs within a transaction (Suhas Daftuar)

Pull request description:

  This is a backport of #14247.

Tree-SHA512: 4d3b6244d501a48f56a728c571dac9a346019a671434edac943f4f535ef8f94ec6bfd569a0585ad5e23a6e488ecd7e0079510cbb10a2a22f576eb36d73accb0c
@laanwj laanwj merged commit 9b4a36e into bitcoin:master Sep 17, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Sep 17, 2018
9b4a36e [qa] Test for duplicate inputs within a transaction (Suhas Daftuar)
b8f8019 Fix crash bug with duplicate inputs within a transaction (Suhas Daftuar)

Pull request description:

Tree-SHA512: 8c7ea34c7fa44188d86c04a690a7cbf8e9deda71ab1f7ca6d11de1f2abb3dd7222627071f86d0d39689a8b302ba9af142f0202466a67e30cd54aed3a08d4eb14
laanwj added a commit that referenced this pull request Sep 17, 2018
…nsaction

9bd08fd [qa] backport: Test for duplicate inputs within a transaction (Suhas Daftuar)
d1dee20 Fix crash bug with duplicate inputs within a transaction (Suhas Daftuar)

Pull request description:

  This is a backport of #14247 to 0.16.

Tree-SHA512: f11b2b0f2d8089bbac7542f78a0f14fc15c693604cb1168ef5ea71852a206da7eb53b6e420376ed1380583961176ba2d283e409e19d783c7a68c3407933a89b0
laanwj added a commit that referenced this pull request Sep 18, 2018
Github-Pull: #14247

Tree-SHA512: fa0b11e245b66a9334ca3aa8c99d541f5835e5c60d372c852fc44a0ee8b930112e967ade535a3ca0a51bf40c8e9becfb5aeab3807abaddd7afb259cfa3844b32
laanwj added a commit that referenced this pull request Sep 18, 2018
Github-Pull: #14247

Tree-SHA512: 149dfe70406c8e013fd3e1ffad61dd1b4980a630f4f752a3520d366dd9b3848125dc0865eef80c8e04388310172d6a92e4414fa90059b2e14aa77a2c6d89d83d
laanwj added a commit that referenced this pull request Sep 18, 2018
Introduced by #9049

Github-Pull: #14247

Tree-SHA512: 54ccf896e4c816ba8532644affc984a091ed801d8387bb01a836953c9ec4a345359d98fb58dd5f929617afd42bce0cc40293fecf943a1584207c82dd78da0ea5
laanwj added a commit that referenced this pull request Sep 18, 2018
Introduced by #9049

Github-Pull: #14247

Tree-SHA512: 2815312a3da8ef4a93dbc26b71d9147e34d1fed794aa7188ec12670579d2e45380212cf1e23526a7b5e339a185a73637fc2f342e0699b687c920244bc2edc124
@practicalswift
Copy link
Member

@practicalswift practicalswift commented Sep 18, 2018

utACK 9b4a36e

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

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Sep 18, 2018

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

@ftrader
Copy link

@ftrader ftrader commented Sep 18, 2018

Could @TheBlueMatt / @sdaftuar outline the responsible disclosure procedures, if any, that were attempted to other projects before publishing these pull requests?

@TheBlueMatt
Copy link
Contributor Author

@TheBlueMatt TheBlueMatt commented Sep 18, 2018

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
Copy link

@zquestz zquestz commented Sep 18, 2018

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
Copy link
Contributor

@PierreRochard PierreRochard commented Sep 18, 2018

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 added a commit to cryptapus/bitcoin that referenced this pull request Sep 18, 2018
Introduced by bitcoin#9049

Github-Pull: bitcoin#14247

Tree-SHA512: 54ccf896e4c816ba8532644affc984a091ed801d8387bb01a836953c9ec4a345359d98fb58dd5f929617afd42bce0cc40293fecf943a1584207c82dd78da0ea5
@deadalnix
Copy link

@deadalnix deadalnix commented Sep 19, 2018

We tried to coordinate timelines for release [...]

Tell us more about this.

@sickpig
Copy link

@sickpig sickpig commented Sep 19, 2018

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
Copy link
Member

@Sjors Sjors commented Sep 19, 2018

Post merge slightly tested 9b4a36e, ran python test without fix which produced the assert.

@biblepay
Copy link

@biblepay biblepay commented Sep 19, 2018

Thanks.

globaltoken added a commit to globaltoken/globaltoken that referenced this pull request Sep 19, 2018
- Fixed a bug, that can cause a node to crash if a block includes duplicate transactions. (bitcoin#14247)
dartdart26 added a commit to projectpai/paicoin that referenced this pull request Sep 19, 2018
Merged the following PR from bitcoin repository: bitcoin/bitcoin#14247 .
Merged the actual fix only, the functional test is not yet merged in:

bitcoin:
Merge #14247: Fix crash bug with duplicate inputs within a transaction

b8f801964f59586508ea8da6cf3decd76bc0e571 Fix crash bug with duplicate inputs within a transaction (Suhas Daftuar)

Pull request description:

Tree-SHA512: 8c7ea34c7fa44188d86c04a690a7cbf8e9deda71ab1f7ca6d11de1f2abb3dd7222627071f86d0d39689a8b302ba9af142f0202466a67e30cd54aed3a08d4eb14
lateminer added a commit to lateminer/bitcoin-abc that referenced this pull request Sep 19, 2018
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 19, 2018
expocash added a commit to expocash/expocash that referenced this pull request Sep 20, 2018
@practicalswift
Copy link
Member

@practicalswift practicalswift commented Sep 20, 2018

I would like to thank the researcher who invested time into finding this issue. Impressive finding! Thanks!

@isghe
Copy link
Contributor

@isghe isghe commented Sep 20, 2018

Is this bug reproducible in regtest or testnet? Thanks

@Sjors
Copy link
Member

@Sjors Sjors commented Sep 21, 2018

@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
Copy link
Member

@promag promag commented Sep 21, 2018

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

@gmaxwell already suggested it, also agree.

@ftrader
Copy link

@ftrader ftrader commented Sep 22, 2018

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 pushed a commit to cryptomeow/bitcoinfibre that referenced this pull request Sep 22, 2018
litebitcoins pushed a commit to litebitcoins/litebitcoin that referenced this pull request Sep 23, 2018
tpruvot added a commit to LUX-Core/lux that referenced this pull request Sep 23, 2018
Follow bitcoin possible issue bitcoin/bitcoin#14247

Even if there were other tests prevening that in code at several levels (mempool)
Stouse49 added a commit to goldcoin/goldcoin that referenced this pull request Sep 24, 2018
Original Bitcoin Fix:
bitcoin#14247
brouwerQ added a commit to brouwerQ/bitcoin that referenced this pull request Sep 25, 2018
brouwerQ added a commit to brouwerQ/bitcoin that referenced this pull request Sep 25, 2018
Trivial: update comment in CheckTransaction bitcoin#14247
@isghe
Copy link
Contributor

@isghe isghe commented Sep 26, 2018

It looks they created with success 0.1 tBTC from nothing, exploiting this bug in testnet3 block 1414411, hash 00000000eba3f43a8624750f39e4520a1678c0dbdf8707bfa4854a12fbf086c5

@bitcoin bitcoin locked and limited conversation to collaborators Sep 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet