qa: Add explicit references to related CVE's in p2p_invalid_block test #14696

lucash-dev · 2018-11-09T05:55:07Z

This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out.
Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation.
This improves developer experience by making understanding the tests easier.

ken2812221 · 2018-11-09T09:31:03Z

Concept ACK.

practicalswift · 2018-11-09T13:58:36Z

Concept ACK

Good idea!

@lucash-dev Would you be able to match other CVE:s against tests and include them in this PR? That would be really useful to have. It would be nice to be able to do git grep CVE-N on all issued Bitcoin Core CVE:s to be able to find the corresponding test case and/or fix

Perhaps Bitcoin CVE guru @luke-jr can help in this matching? :-)

lucash-dev · 2018-11-09T15:49:24Z

I’ll do that. Sounds like a good learning opportunity too.

practicalswift · 2018-11-09T17:39:16Z

test/functional/p2p_invalid_block.py

        # Manufacture a block with 3 transactions (coinbase, spend of prior
        # coinbase, spend of that spend).  Duplicate the 3rd transaction to
        # leave merkle root and blockheader unchanged but invalidate the block.
+        # For more information on merkle-root malleability see src/consesus/merkle.cpp.


Should be "consensus" :-)

test/functional/p2p_invalid_block.py

Empact · 2018-11-13T10:29:37Z

Concept ACK

DrahtBot · 2018-11-13T15:22:51Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15045 ([test] Apply maximal flags to tx_valid tests and minimal flags to tx_invalid tests by jl2012)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2018-11-22T18:07:16Z

Needs rebase (silent conflict)

laanwj · 2018-11-23T12:04:43Z

Thanks for adding these tests!
Concept ACK

lucash-dev · 2018-12-03T06:52:21Z

Coudn't find much info on CVE-2012-4682, CVE-2012-4683. @luke-jr could you provide any pointers?

lucash-dev · 2019-01-07T06:47:10Z

Rebased. Also refactored to use the new invalid tx template.

fanquake · 2019-01-07T06:54:07Z

@lucash-dev Could you cleanup the trailing whitespace. That's what's causing Travis to fail:

./test/functional/data/invalid_txs.py:204:1: W293 blank line contains whitespace
./test/functional/data/invalid_txs.py:205:1: W293 blank line contains whitespace
./test/functional/data/invalid_txs.py:210:10: E131 continuation line unaligned for hanging indent

laanwj · 2019-01-31T13:42:36Z

utACK a342ecd
Could squash some commits and fix spelling in commit messages

lucash-dev · 2019-02-01T01:54:10Z

utACK a342ecd
Could squash some commits and fix spelling in commit messages

Sorry, should've prefixed the PR with "WIP" while I was working adding more comments for more CVEs. It's been a while since I've had time to work on this PR, though. It has also already grown far beyond the original intention (just a couple comments). So probably better if I just fix the commit history, and open a new PR later on when I manage to add comments/tests for the remaining CVEs.
Thanks for reviewing!

lucash-dev · 2019-02-01T05:43:41Z

Fixed the commit history.

practicalswift · 2019-02-01T19:57:05Z

test/functional/data/invalid_txs.py

+        tx.calc_sha256()
+        return tx
+
+


Drop one of the newlines here :-)

🤦‍♂️

practicalswift · 2019-02-01T19:57:35Z

utACK 203c328 modulo nit and squash

lucash-dev · 2019-02-02T18:58:53Z

utACK 203c328 modulo nit and squash

Fixed nit. Further squashed to 2 commits. one for adding comments, other for new tests. Let me know if you'd like just one commit. just thought it would make sense to separate these two, but it's all the same to me.
And thanks for reviewing!

This commit adds comments referencing multiple CVEs both in production and test code. CVEs covered in this commit: CVE-2010-5137 CVE-2010-5139 CVE-2010-5141 CVE-2012-1909 CVE-2012-2459 CVE-2012-3789 CVE-2018-17144

…0-5137. CVE-2018-17144 and CVE-2012-2459 are only partially tested for regression. - CVE-2018-17144 is not tested for the inflation bug. - CVE-2012-2459 is only tested for the mutated block being rejected, not for the original block being accepted afterwards. This commit fixes that limitation. Also added functional test for CVE-2010-5137.

lucash-dev · 2019-06-04T00:08:43Z

Rebased.

Empact · 2019-06-05T11:11:44Z

utACK 38bfca6

laanwj · 2019-09-18T13:53:13Z

ACK 0c62e3a, checked the CVE numbers, thanks for adding documentation

…alid_block test. 0c62e3a New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev) 38bfca6 Added comments referencing multiple CVEs in tests and production code. (lucash-dev) Pull request description: This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out. Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation. This improves developer experience by making understanding the tests easier. ACKs for top commit: laanwj: ACK 0c62e3a, checked the CVE numbers, thanks for adding documentation Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9

…p2p_invalid_block test. 0c62e3a New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev) 38bfca6 Added comments referencing multiple CVEs in tests and production code. (lucash-dev) Pull request description: This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out. Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation. This improves developer experience by making understanding the tests easier. ACKs for top commit: laanwj: ACK 0c62e3a, checked the CVE numbers, thanks for adding documentation Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9

…s and production code. Summary: > This commit adds comments referencing multiple CVEs both in production and test code. > CVEs covered in this commit: > > CVE-2010-5137 > CVE-2010-5139 > CVE-2010-5141 > CVE-2012-1909 > CVE-2012-2459 > CVE-2012-3789 > CVE-2018-17144 Backport of Core [[bitcoin/bitcoin#14696 | PR14696]] - part 1 of 2 Commit bitcoin/bitcoin@38bfca6 Test Plan: `ninja && ninja check` (just to make sure the src/test/data/script_tests.json change didn't break anything) Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8018

…012-2459, and CVE-2010-5137. Summary: CVE-2018-17144 and CVE-2012-2459 are only partially tested for regression. - CVE-2018-17144 is not tested for the inflation bug. - CVE-2012-2459 is only tested for the mutated block being rejected, not for the original block being accepted afterwards. This commit fixes that limitation. Also added functional test for CVE-2010-5137. This concludes backport of [[bitcoin/bitcoin#14696 | PR14696]] - part 2 of 2 Depends on D8018 Test Plan: `ninja && test/functional/test_runner.py p2p_invalid_block.py p2p_invalid_tx.py` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox, deadalnix Subscribers: deadalnix, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8026

…tx functional tests coverage. 2d994c4 qa: adding test case for output value > input value out of range (furszy) 6165c80 qa: update and enable p2p_invalid_tx.py functional test. (furszy) bb62699 Backport chainparams and validation `fRequireStandard` flags functionality + init 'acceptnonstdtxn' option to skip (most) "non-standard transaction" checks, for testnet/regtest only. (furszy) 0280c39 New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev) ec7d0b0 [qa] Test for duplicate inputs within a transaction (Suhas Daftuar) a501fe1 qa: update and enable p2p_invalid_block.py. (furszy) f83d140 qa: Read reject reasons from debug log, not p2p messages (furszy) 8a50165 [tests] Add P2PDataStore class (furszy) ebed910 qa: Correct coinbase input script height. (furszy) cae2d43 [tests] Reduce NodeConn connection logging from info to debug (John Newbery) Pull request description: Introduced the `P2PDataStore` class into the functional test suite and updated it up to a point where was able to enable the `p2p_invalid_block.py` and `p2p_invalid_tx.py` functional tests. Plus updated both tests to a much recent version, expanding its testing coverage as well. Now the test suite is covering the primary CVE that btc had in the past. As both tests were simply unusable before and we have a different synchronization process than upstream, plus we didn't have some params and init/global flags, this work isn't following a PR by PR back port path. Had to made my own way up to the complete tests support. Main pilar PRs from upstream from where this work was based: * dashpay#6329. * bitcoin#11771. * bitcoin#14119 (only `p2p_invalid_tx.py`, `p2p_invalid_block.py` and `mininode.py` changes). * bitcoin#14247 (only 9b4a36e). * bitcoin#14696. ACKs for top commit: random-zebra: ACK 2d994c4 Tree-SHA512: 548830b5fbf8b7f383832a7eea96179d089a4c9c222ef34007846f53736b07c873e37084235f1575eac157b63308c00ab3be78b71d3ed6520f4f73f1c8de81a5

…p2p_invalid_block test. 0c62e3a New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev) 38bfca6 Added comments referencing multiple CVEs in tests and production code. (lucash-dev) Pull request description: This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out. Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation. This improves developer experience by making understanding the tests easier. ACKs for top commit: laanwj: ACK 0c62e3a, checked the CVE numbers, thanks for adding documentation Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9

fanquake added the Tests label Nov 9, 2018

maflcko added the Docs label Nov 9, 2018

maflcko changed the title ~~trivial: Add explicit references to related CVE's in p2p_invalid_block test.~~ doc: Add explicit references to related CVE's in p2p_invalid_block test. Nov 9, 2018

practicalswift reviewed Nov 9, 2018

View reviewed changes

maflcko reviewed Nov 9, 2018

View reviewed changes

test/functional/p2p_invalid_block.py Show resolved Hide resolved

maflcko changed the title ~~doc: Add explicit references to related CVE's in p2p_invalid_block test.~~ qa: Add explicit references to related CVE's in p2p_invalid_block test. Nov 10, 2018

DrahtBot added the Needs rebase label Nov 13, 2018

DrahtBot removed the Needs rebase label Nov 21, 2018

DrahtBot added the Needs rebase label Jan 3, 2019

fanquake removed the Needs rebase label Jan 7, 2019

practicalswift reviewed Feb 1, 2019

View reviewed changes

DrahtBot added the Needs rebase label Apr 10, 2019

lucash-dev added 2 commits June 2, 2019 10:25

Added comments referencing multiple CVEs in tests and production code.

38bfca6

This commit adds comments referencing multiple CVEs both in production and test code. CVEs covered in this commit: CVE-2010-5137 CVE-2010-5139 CVE-2010-5141 CVE-2012-1909 CVE-2012-2459 CVE-2012-3789 CVE-2018-17144

DrahtBot removed the Needs rebase label Jun 2, 2019

maflcko mentioned this pull request Sep 16, 2019

doc: Update tx-size-small comment with relevant CVE disclosure #16885

Merged

laanwj changed the title ~~qa: Add explicit references to related CVE's in p2p_invalid_block test.~~ qa: Add explicit references to related CVE's in p2p_invalid_block test Sep 18, 2019

laanwj merged commit 0c62e3a into bitcoin:master Sep 18, 2019

laanwj mentioned this pull request Sep 18, 2019

False positive (?) in lint-python-dead-code.sh #16906

Closed

furszy mentioned this pull request Apr 27, 2021

[QA] Test suite update + P2P invalid blocks and invalid tx functional tests coverage. PIVX-Project/PIVX#2346

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

qa: Add explicit references to related CVE's in p2p_invalid_block test #14696

qa: Add explicit references to related CVE's in p2p_invalid_block test #14696

Uh oh!

Conversation

lucash-dev commented Nov 9, 2018

Uh oh!

ken2812221 commented Nov 9, 2018

Uh oh!

practicalswift commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucash-dev commented Nov 9, 2018

Uh oh!

practicalswift Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Empact commented Nov 13, 2018

Uh oh!

DrahtBot commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Nov 22, 2018

Uh oh!

laanwj commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucash-dev commented Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucash-dev commented Jan 7, 2019

Uh oh!

fanquake commented Jan 7, 2019

Uh oh!

laanwj commented Jan 31, 2019

Uh oh!

lucash-dev commented Feb 1, 2019

Uh oh!

lucash-dev commented Feb 1, 2019

Uh oh!

practicalswift Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

lucash-dev Feb 2, 2019

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 1, 2019

Uh oh!

lucash-dev commented Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucash-dev commented Jun 4, 2019

Uh oh!

Empact commented Jun 5, 2019

Uh oh!

laanwj commented Sep 18, 2019

Uh oh!

Uh oh!

practicalswift commented Nov 9, 2018 •

edited

Loading

DrahtBot commented Nov 13, 2018 •

edited

Loading

laanwj commented Nov 23, 2018 •

edited

Loading

lucash-dev commented Dec 3, 2018 •

edited

Loading

lucash-dev commented Feb 2, 2019 •

edited

Loading