From 452d182739a86c5b5a95dff1b7d99c609f89092e Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 18 Sep 2019 15:53:36 +0200 Subject: [PATCH] Merge #14696: qa: Add explicit references to related CVE's in p2p_invalid_block test. 0c62e3aa73839e97e65a3155e06a98d84b700a1e New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev) 38bfca6bb2ad68719415e9c54a981441052da072 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 0c62e3aa73839e97e65a3155e06a98d84b700a1e, checked the CVE numbers, thanks for adding documentation Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9 --- src/consensus/tx_check.cpp | 2 +- src/net_processing.cpp | 2 +- src/script/interpreter.cpp | 4 ++- src/test/data/script_tests.json | 6 ++-- src/validation.cpp | 3 +- test/functional/feature_block.py | 2 +- test/functional/mempool_accept.py | 1 + test/functional/p2p_invalid_block.py | 44 +++++++++++++++++++++++----- 8 files changed, 48 insertions(+), 16 deletions(-) diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index 5fc58c0cb6..76f24ac9d0 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -28,7 +28,7 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state) if (tx.vExtraPayload.size() > MAX_TX_EXTRA_PAYLOAD) return state.DoS(100, false, REJECT_INVALID, "bad-txns-payload-oversize"); - // Check for negative or overflow output values + // Check for negative or overflow output values (see CVE-2010-5139) CAmount nValueOut = 0; for (const auto& txout : tx.vout) { if (txout.nValue < 0) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index fe89b16c23..a3998240f4 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3220,7 +3220,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr } AddOrphanTx(ptx, pfrom->GetId()); - // DoS prevention: do not allow mapOrphanTransactions to grow unbounded + // DoS prevention: do not allow mapOrphanTransactions to grow unbounded (see CVE-2012-3789) unsigned int nMaxOrphanTxSize = (unsigned int)std::max((int64_t)0, gArgs.GetArg("-maxorphantxsize", DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE)) * 1000000; unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTxSize); if (nEvicted > 0) { diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 93a4381152..e871fc4f85 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -338,7 +338,7 @@ bool EvalScript(std::vector >& stack, const CScript& opcode == OP_MUL || opcode == OP_LSHIFT || opcode == OP_RSHIFT) - return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE); // Disabled opcodes. + return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE); // Disabled opcodes (CVE-2010-5137). // With SCRIPT_VERIFY_CONST_SCRIPTCODE, OP_CODESEPARATOR is rejected even in an unexecuted branch if (opcode == OP_CODESEPARATOR && sigversion == SigVersion::BASE && (flags & SCRIPT_VERIFY_CONST_SCRIPTCODE)) @@ -1599,6 +1599,8 @@ bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigne return set_error(serror, SCRIPT_ERR_SIG_PUSHONLY); } + // scriptSig and scriptPubKey must be evaluated sequentially on the same stack + // rather than being simply concatenated (see CVE-2010-5141) std::vector > stack, stackCopy; if (!EvalScript(stack, scriptSig, flags, checker, SigVersion::BASE, serror)) // serror is set diff --git a/src/test/data/script_tests.json b/src/test/data/script_tests.json index 48c4ddb5f6..68eaf777df 100644 --- a/src/test/data/script_tests.json +++ b/src/test/data/script_tests.json @@ -827,6 +827,9 @@ ["NOP", "2SWAP 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"], ["1", "2 3 2SWAP 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"], +["NOP", "SIZE 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"], + +["TEST DISABLED OP CODES (CVE-2010-5137)"], ["'a' 'bc'", "CAT", "P2SH,STRICTENC", "DISABLED_OPCODE"], ["'abc' 'a' 'bc'", "CAT EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK"], ["'' '' ''", "CAT EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK"], @@ -909,9 +912,6 @@ ["0x05 0x0100800080", "BIN2NUM -8388609 EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK", "Ensure significant zero bytes are retained"], ["0x05 0x01000f0000", "BIN2NUM 983041 EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK", "Ensure significant zero bytes are retained"], ["0x05 0x01000f0080", "BIN2NUM -983041 EQUAL", "P2SH,STRICTENC,DIP0020_OPCODES", "OK", "Ensure significant zero bytes are retained"], - -["NOP", "SIZE 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"], - ["Disabled opcodes"], ["'abc'", "IF INVERT ELSE 1 ENDIF", "P2SH,STRICTENC", "DISABLED_OPCODE", "INVERT disabled"], ["2 0 IF 2MUL ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC", "DISABLED_OPCODE", "2MUL disabled"], diff --git a/src/validation.cpp b/src/validation.cpp index 4c506db36a..d38e336b3f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2052,7 +2052,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl // If such overwrites are allowed, coinbases and transactions depending upon those // can be duplicated to remove the ability to spend the first instance -- even after // being sent to another address. - // See BIP30 and http://r6.ca/blog/20120206T005236Z.html for more information. + // See BIP30, CVE-2012-1909, and http://r6.ca/blog/20120206T005236Z.html for more information. // This logic is not necessary for memory pool transactions, as AcceptToMemoryPool // already refuses previously-known transaction ids entirely. // This rule was originally applied to all blocks with a timestamp after March 15, 2012, 0:00 UTC. @@ -3739,6 +3739,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P return state.DoS(100, false, REJECT_INVALID, "bad-cb-multiple", false, "more than one coinbase"); // Check transactions + // Must check for duplicate inputs (see CVE-2018-17144) for (const auto& tx : block.vtx) if (!CheckTransaction(*tx, state)) return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(), diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index 7232a8d073..2cddb81fac 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -765,7 +765,7 @@ class FullBlockTest(BitcoinTestFramework): # # Blocks are not allowed to contain a transaction whose id matches that of an earlier, # not-fully-spent transaction in the same chain. To test, make identical coinbases; - # the second one should be rejected. + # the second one should be rejected. See also CVE-2012-1909. # self.log.info("Reject a block with a transaction with a duplicate hash of a previous transaction (BIP30)") self.move_tip(60) diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index c7154d101a..5f994b9837 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -215,6 +215,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework): rawtxs=[tx.serialize().hex()], ) + # The following two validations prevent overflow of the output amounts (see CVE-2010-5139). self.log.info('A transaction with too large output value') tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_reference))) tx.vout[0].nValue = 21000000 * COIN + 1 diff --git a/test/functional/p2p_invalid_block.py b/test/functional/p2p_invalid_block.py index 7c2497c770..570244b70a 100755 --- a/test/functional/p2p_invalid_block.py +++ b/test/functional/p2p_invalid_block.py @@ -54,10 +54,11 @@ class InvalidBlockRequestTest(BitcoinTestFramework): block_time = best_block["time"] + 1 # Use merkle-root malleability to generate an invalid block with - # same blockheader. + # same blockheader (CVE-2012-2459). # 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/consensus/merkle.cpp. self.log.info("Test merkle root malleability.") block2 = create_block(tip, create_coinbase(height), block_time) @@ -81,15 +82,16 @@ class InvalidBlockRequestTest(BitcoinTestFramework): assert block2_orig.vtx != block2.vtx node.p2p.send_blocks_and_test([block2], node, success=False, request_block=False, reject_reason='bad-txns-duplicate') - # Check transactions for duplicate inputs + # Check transactions for duplicate inputs (CVE-2018-17144) self.log.info("Test duplicate input block.") - block2_orig.vtx[2].vin.append(block2_orig.vtx[2].vin[0]) - block2_orig.vtx[2].rehash() - block2_orig.hashMerkleRoot = block2_orig.calc_merkle_root() - block2_orig.rehash() - block2_orig.solve() - node.p2p.send_blocks_and_test([block2_orig], node, success=False, request_block=False, reject_reason='bad-txns-inputs-duplicate') + block2_dup = copy.deepcopy(block2_orig) + block2_dup.vtx[2].vin.append(block2_dup.vtx[2].vin[0]) + block2_dup.vtx[2].rehash() + block2_dup.hashMerkleRoot = block2_dup.calc_merkle_root() + block2_dup.rehash() + block2_dup.solve() + node.p2p.send_blocks_and_test([block2_dup], node, success=False, reject_reason='bad-txns-inputs-duplicate') self.log.info("Test very broken block.") @@ -104,5 +106,31 @@ class InvalidBlockRequestTest(BitcoinTestFramework): node.p2p.send_blocks_and_test([block3], node, success=False, request_block=False, reject_reason='bad-cb-amount') + # Complete testing of CVE-2012-2459 by sending the original block. + # It should be accepted even though it has the same hash as the mutated one. + + self.log.info("Test accepting original block after rejecting its mutated version.") + node.p2p.send_blocks_and_test([block2_orig], node, success=True, timeout=5) + + # Update tip info + height += 1 + block_time += 1 + tip = int(block2_orig.hash, 16) + + # Complete testing of CVE-2018-17144, by checking for the inflation bug. + # Create a block that spends the output of a tx in a previous block. + block4 = create_block(tip, create_coinbase(height), block_time) + tx3 = create_tx_with_script(tx2, 0, script_sig=b'\x51', amount=50 * COIN) + + # Duplicates input + tx3.vin.append(tx3.vin[0]) + tx3.rehash() + block4.vtx.append(tx3) + block4.hashMerkleRoot = block4.calc_merkle_root() + block4.rehash() + block4.solve() + self.log.info("Test inflation by duplicating input") + node.p2p.send_blocks_and_test([block4], node, success=False, reject_reason='bad-txns-inputs-duplicate') + if __name__ == '__main__': InvalidBlockRequestTest().main()