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