Added comments referencing multiple CVEs in tests and production code.

lucash-dev · lucash-dev · commit 38bfca6bb2ad · 2019-06-02T10:25:03.000-07:00
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
diff --git 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 nValueOut = 0;
     for (const auto& txout : tx.vout)
     {
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -2513,7 +2513,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
@@ -334,7 +334,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
                 opcode == OP_MOD ||
                 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 in non-segwit script is rejected even in an unexecuted branch
             if (opcode == OP_CODESEPARATOR && sigversion == SigVersion::BASE && (flags & SCRIPT_VERIFY_CONST_SCRIPTCODE))
@@ -1483,6 +1483,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<std::vector<unsigned char> > 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
@@ -829,15 +829,16 @@
 ["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' 'b'", "CAT", "P2SH,STRICTENC", "DISABLED_OPCODE", "CAT disabled"],
 ["'a' 'b' 0", "IF CAT ELSE 1 ENDIF", "P2SH,STRICTENC", "DISABLED_OPCODE", "CAT disabled"],
 ["'abc' 1 1", "SUBSTR", "P2SH,STRICTENC", "DISABLED_OPCODE", "SUBSTR disabled"],
 ["'abc' 1 1 0", "IF SUBSTR ELSE 1 ENDIF", "P2SH,STRICTENC", "DISABLED_OPCODE", "SUBSTR disabled"],
 ["'abc' 2 0", "IF LEFT ELSE 1 ENDIF", "P2SH,STRICTENC", "DISABLED_OPCODE", "LEFT disabled"],
 ["'abc' 2 0", "IF RIGHT ELSE 1 ENDIF", "P2SH,STRICTENC", "DISABLED_OPCODE", "RIGHT disabled"],
-
-["NOP", "SIZE 1", "P2SH,STRICTENC", "INVALID_STACK_OPERATION"],
-
 ["'abc'", "IF INVERT ELSE 1 ENDIF", "P2SH,STRICTENC", "DISABLED_OPCODE", "INVERT disabled"],
 ["1 2 0 IF AND ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC", "DISABLED_OPCODE", "AND disabled"],
 ["1 2 0 IF OR ELSE 1 ENDIF", "NOP", "P2SH,STRICTENC", "DISABLED_OPCODE", "OR disabled"],
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -1865,7 +1865,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.
@@ -3136,6 +3136,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/feature_block.py b/test/functional/feature_block.py
@@ -787,7 +787,7 @@ def run_test(self):
         #
         # 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
@@ -212,6 +212,7 @@ def run_test(self):
             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
@@ -53,10 +53,11 @@ def run_test(self):
         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,7 +82,7 @@ def run_test(self):
 
         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])

Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe`
`18`	`18`	`if (::GetSerializeSize(tx, PROTOCOL_VERSION \| SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT)`
`19`	`19`	`return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-oversize");`
`20`	`20`
`21`		`- // Check for negative or overflow output values`
	`21`	`+ // Check for negative or overflow output values (see CVE-2010-5139)`
`22`	`22`	`CAmount nValueOut = 0;`
`23`	`23`	`for (const auto& txout : tx.vout)`
`24`	`24`	`{`
Original file line number	Diff line number	Diff line change
`@@ -2513,7 +2513,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr`
`2513`	`2513`	`}`
`2514`	`2514`	`AddOrphanTx(ptx, pfrom->GetId());`
`2515`	`2515`
`2516`		`- // DoS prevention: do not allow mapOrphanTransactions to grow unbounded`
	`2516`	`+ // DoS prevention: do not allow mapOrphanTransactions to grow unbounded (see CVE-2012-3789)`
`2517`	`2517`	`unsigned int nMaxOrphanTx = (unsigned int)std::max((int64_t)0, gArgs.GetArg("-maxorphantx", DEFAULT_MAX_ORPHAN_TRANSACTIONS));`
`2518`	`2518`	`unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTx);`
`2519`	`2519`	`if (nEvicted > 0) {`
Original file line number	Diff line number	Diff line change
`@@ -787,7 +787,7 @@ def run_test(self):`
`787`	`787`	`#`
`788`	`788`	`# Blocks are not allowed to contain a transaction whose id matches that of an earlier,`
`789`	`789`	`# not-fully-spent transaction in the same chain. To test, make identical coinbases;`
`790`		`- # the second one should be rejected.`
	`790`	`+ # the second one should be rejected. See also CVE-2012-1909.`
`791`	`791`	`#`
`792`	`792`	`self.log.info("Reject a block with a transaction with a duplicate hash of a previous transaction (BIP30)")`
`793`	`793`	`self.move_tip(60)`
Original file line number	Diff line number	Diff line change
`@@ -212,6 +212,7 @@ def run_test(self):`
`212`	`212`	`rawtxs=[tx.serialize().hex()],`
`213`	`213`	`)`
`214`	`214`
	`215`	`+ # The following two validations prevent overflow of the output amounts (see CVE-2010-5139).`
`215`	`216`	`self.log.info('A transaction with too large output value')`
`216`	`217`	`tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_reference)))`
`217`	`218`	`tx.vout[0].nValue = 21000000 * COIN + 1`