diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index 09e3710e3c..6c1c1b9429 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -18,7 +18,7 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT) return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-oversize"); - // Check for negative or overflow output values + // Check for negative or overflow output values (see CVE-2010-5139) CAmount nValueOutExplicit = 0; for (const auto& txout : tx.vout) { diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 93a98974eb..34d349e8e9 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2559,7 +2559,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 nMaxOrphanTx = (unsigned int)std::max((int64_t)0, gArgs.GetArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS)); unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTx); if (nEvicted > 0) { diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 84c292fa2e..ce0beb656f 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -345,7 +345,7 @@ bool EvalScript(std::vector >& stack, const CScript& opcode == OP_DIV || opcode == OP_MOD ) { - 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 in non-segwit script is rejected even in an unexecuted branch @@ -1886,6 +1886,8 @@ bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, const C 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 ded29bbf51..d8ac79546d 100644 --- a/src/test/data/script_tests.json +++ b/src/test/data/script_tests.json @@ -828,6 +828,7 @@ ["NOP", "SIZE 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"], +["TEST DISABLED OP CODES (CVE-2010-5137)"], ["2 0 IF 2MUL ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC", "DISABLED_OPCODE", "2MUL disabled"], ["2 0 IF 2DIV ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC", "DISABLED_OPCODE", "2DIV disabled"], ["2 2 0 IF MUL ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC", "DISABLED_OPCODE", "MUL disabled"], diff --git a/src/validation.cpp b/src/validation.cpp index 9a5e4b50fd..8356fa2af8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1970,7 +1970,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. @@ -3305,6 +3305,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-multiple", "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, true)) return state.Invalid(state.GetReason(), false, state.GetRejectCode(), state.GetRejectReason(), diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index c0422ba0af..b247def424 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -21,10 +21,27 @@ Invalid tx cases not covered here can be found by running: """ import abc -from test_framework.messages import CTransaction, CTxIn, CTxOut, COutPoint +from test_framework.messages import CTransaction, CTxIn, CTxOut, COutPoint, COIN from test_framework import script as sc from test_framework.blocktools import create_tx_with_script, MAX_BLOCK_SIGOPS - +from test_framework.script import ( + CScript, + OP_CAT, + OP_SUBSTR, + OP_LEFT, + OP_RIGHT, + OP_INVERT, + OP_AND, + OP_OR, + OP_XOR, + OP_2MUL, + OP_2DIV, + OP_MUL, + OP_DIV, + OP_MOD, + OP_LSHIFT, + OP_RSHIFT +) basic_p2sh = sc.CScript([sc.OP_HASH160, sc.hash160(sc.CScript([sc.OP_0])), sc.OP_EQUAL]) @@ -181,7 +198,47 @@ class TooManySigops(BadTxTemplate): script_pub_key=lotsa_checksigs, amount=self.spend_avail) +def getDisabledOpcodeTemplate(opcode): + """ Creates disabled opcode tx template class""" + def get_tx(self): + tx = CTransaction() + vin = self.valid_txin + vin.scriptSig = CScript([opcode]) + tx.vin.append(vin) + tx.vout.append(CTxOut(49 * COIN, basic_p2sh)) + tx.vout.append(CTxOut(COIN)) # ELEMENTS fee + tx.calc_sha256() + return tx + + return type('DisabledOpcode_' + str(opcode), (BadTxTemplate,), { + 'reject_reason': "disabled opcode", + 'expect_disconnect': True, + 'get_tx': get_tx, + 'valid_in_block' : True + }) + +# Disabled opcode tx templates (CVE-2010-5137) +# ELEMENTS: many of these are reenabled +DisabledOpcodeTemplates = [getDisabledOpcodeTemplate(opcode) for opcode in [ +# OP_CAT, +# OP_SUBSTR, +# OP_LEFT, +# OP_RIGHT, +# OP_INVERT, +# OP_AND, +# OP_OR, +# OP_XOR, + OP_2MUL, + OP_2DIV, + OP_MUL, + OP_DIV, + OP_MOD, +# OP_LSHIFT, +# OP_RSHIFT +]] + def iter_all_templates(): """Iterate through all bad transaction template types.""" return BadTxTemplate.__subclasses__() + diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index c52d260947..7d2a389e4d 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -815,7 +815,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 e740bde651..af9f6c8bc2 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -219,6 +219,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 = CTxOutValue(21000000 * COIN + 1) diff --git a/test/functional/p2p_invalid_block.py b/test/functional/p2p_invalid_block.py index 4b9e9cca97..965295785c 100755 --- a/test/functional/p2p_invalid_block.py +++ b/test/functional/p2p_invalid_block.py @@ -53,18 +53,21 @@ 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) block_time += 1 # b'0x51' is OP_TRUE - tx1 = create_tx_with_script(block1.vtx[0], 0, script_sig=b'\x51', amount=50 * COIN) - tx2 = create_tx_with_script(tx1, 0, script_sig=b'\x51', amount=50 * COIN) + # ELEMENTS: scriptpubkeys can't be empty or else we interpret them as fee outputs, + # so we modify the Core test to move the OP_TRUEs from scriptSig to scriptPubKey + tx1 = create_tx_with_script(block1.vtx[0], 0, script_pub_key=b'\x51', amount=50 * COIN) + tx2 = create_tx_with_script(tx1, 0, script_pub_key=b'\x51', amount=50 * COIN) block2.vtx.extend([tx1, tx2]) block2.hashMerkleRoot = block2.calc_merkle_root() @@ -81,25 +84,16 @@ class InvalidBlockRequestTest(BitcoinTestFramework): node.p2p.send_blocks_and_test([block2], node, success=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, reject_reason='bad-txns-inputs-duplicate') - - # Check transactions for duplicate inputs - 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, 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.") @@ -115,5 +109,31 @@ class InvalidBlockRequestTest(BitcoinTestFramework): node.p2p.send_blocks_and_test([block3], node, success=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()