diff options
-rw-r--r-- | depends/packages/boost.mk | 5 | ||||
-rw-r--r-- | src/consensus/tx_check.cpp | 19 | ||||
-rw-r--r-- | src/consensus/tx_check.h | 2 | ||||
-rw-r--r-- | src/test/compress_tests.cpp | 73 | ||||
-rw-r--r-- | src/test/fuzz/transaction.cpp | 7 | ||||
-rw-r--r-- | src/validation.cpp | 10 |
6 files changed, 92 insertions, 24 deletions
diff --git a/depends/packages/boost.mk b/depends/packages/boost.mk index 5df49b2af8..766b8d224e 100644 --- a/depends/packages/boost.mk +++ b/depends/packages/boost.mk @@ -10,15 +10,14 @@ $(package)_config_opts_debug=variant=debug $(package)_config_opts=--layout=tagged --build-type=complete --user-config=user-config.jam $(package)_config_opts+=threading=multi link=static -sNO_BZIP2=1 -sNO_ZLIB=1 $(package)_config_opts_linux=threadapi=pthread runtime-link=shared -$(package)_config_opts_darwin=--toolset=darwin-4.2.1 runtime-link=shared +$(package)_config_opts_darwin=--toolset=clang-darwin runtime-link=shared $(package)_config_opts_mingw32=binary-format=pe target-os=windows threadapi=win32 runtime-link=static $(package)_config_opts_x86_64_mingw32=address-model=64 $(package)_config_opts_i686_mingw32=address-model=32 $(package)_config_opts_i686_linux=address-model=32 architecture=x86 $(package)_toolset_$(host_os)=gcc $(package)_archiver_$(host_os)=$($(package)_ar) -$(package)_toolset_darwin=darwin -$(package)_archiver_darwin=$($(package)_libtool) +$(package)_toolset_darwin=clang-darwin $(package)_config_libraries=chrono,filesystem,system,thread,test $(package)_cxxflags=-std=c++11 -fvisibility=hidden $(package)_cxxflags_linux=-fPIC diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index 1206035839..6793f871cf 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -7,7 +7,7 @@ #include <primitives/transaction.h> #include <consensus/validation.h> -bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fCheckDuplicateInputs) +bool CheckTransaction(const CTransaction& tx, CValidationState& state) { // Basic checks that don't depend on any context if (tx.vin.empty()) @@ -31,14 +31,15 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-txouttotal-toolarge"); } - // Check for duplicate inputs - note that this check is slow so we skip it in CheckBlock - if (fCheckDuplicateInputs) { - std::set<COutPoint> vInOutPoints; - for (const auto& txin : tx.vin) - { - if (!vInOutPoints.insert(txin.prevout).second) - return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-inputs-duplicate"); - } + // Check for duplicate inputs (see CVE-2018-17144) + // While Consensus::CheckTxInputs does check if all inputs of a tx are available, and UpdateCoins marks all inputs + // of a tx as spent, it does not check if the tx has duplicate inputs. + // Failure to run this check will result in either a crash or an inflation bug, depending on the implementation of + // the underlying coins database. + std::set<COutPoint> vInOutPoints; + for (const auto& txin : tx.vin) { + if (!vInOutPoints.insert(txin.prevout).second) + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-inputs-duplicate"); } if (tx.IsCoinBase()) diff --git a/src/consensus/tx_check.h b/src/consensus/tx_check.h index bcfdf36bf9..6f3f8fe969 100644 --- a/src/consensus/tx_check.h +++ b/src/consensus/tx_check.h @@ -15,6 +15,6 @@ class CTransaction; class CValidationState; -bool CheckTransaction(const CTransaction& tx, CValidationState& state, bool fCheckDuplicateInputs=true); +bool CheckTransaction(const CTransaction& tx, CValidationState& state); #endif // BITCOIN_CONSENSUS_TX_CHECK_H diff --git a/src/test/compress_tests.cpp b/src/test/compress_tests.cpp index e8f149470e..c6a08b293f 100644 --- a/src/test/compress_tests.cpp +++ b/src/test/compress_tests.cpp @@ -4,6 +4,7 @@ #include <compressor.h> #include <test/setup_common.h> +#include <script/standard.h> #include <stdint.h> @@ -61,4 +62,76 @@ BOOST_AUTO_TEST_CASE(compress_amounts) BOOST_CHECK(TestDecode(i)); } +BOOST_AUTO_TEST_CASE(compress_script_to_ckey_id) +{ + // case CKeyID + CKey key; + key.MakeNewKey(true); + CPubKey pubkey = key.GetPubKey(); + + CScript script = CScript() << OP_DUP << OP_HASH160 << ToByteVector(pubkey.GetID()) << OP_EQUALVERIFY << OP_CHECKSIG; + BOOST_CHECK_EQUAL(script.size(), 25); + + std::vector<unsigned char> out; + bool done = CompressScript(script, out); + BOOST_CHECK_EQUAL(done, true); + + // Check compressed script + BOOST_CHECK_EQUAL(out.size(), 21); + BOOST_CHECK_EQUAL(out[0], 0x00); + BOOST_CHECK_EQUAL(memcmp(&out[1], &script[3], 20), 0); // compare the 20 relevant chars of the CKeyId in the script +} + +BOOST_AUTO_TEST_CASE(compress_script_to_cscript_id) +{ + // case CScriptID + CScript script, redeemScript; + script << OP_HASH160 << ToByteVector(CScriptID(redeemScript)) << OP_EQUAL; + BOOST_CHECK_EQUAL(script.size(), 23); + + std::vector<unsigned char> out; + bool done = CompressScript(script, out); + BOOST_CHECK_EQUAL(done, true); + + // Check compressed script + BOOST_CHECK_EQUAL(out.size(), 21); + BOOST_CHECK_EQUAL(out[0], 0x01); + BOOST_CHECK_EQUAL(memcmp(&out[1], &script[2], 20), 0); // compare the 20 relevant chars of the CScriptId in the script +} + +BOOST_AUTO_TEST_CASE(compress_script_to_compressed_pubkey_id) +{ + CKey key; + key.MakeNewKey(true); // case compressed PubKeyID + + CScript script = CScript() << ToByteVector(key.GetPubKey()) << OP_CHECKSIG; // COMPRESSED_PUBLIC_KEY_SIZE (33) + BOOST_CHECK_EQUAL(script.size(), 35); + + std::vector<unsigned char> out; + bool done = CompressScript(script, out); + BOOST_CHECK_EQUAL(done, true); + + // Check compressed script + BOOST_CHECK_EQUAL(out.size(), 33); + BOOST_CHECK_EQUAL(memcmp(&out[0], &script[1], 1), 0); + BOOST_CHECK_EQUAL(memcmp(&out[1], &script[2], 32), 0); // compare the 32 chars of the compressed CPubKey +} + +BOOST_AUTO_TEST_CASE(compress_script_to_uncompressed_pubkey_id) +{ + CKey key; + key.MakeNewKey(false); // case uncompressed PubKeyID + CScript script = CScript() << ToByteVector(key.GetPubKey()) << OP_CHECKSIG; // PUBLIC_KEY_SIZE (65) + BOOST_CHECK_EQUAL(script.size(), 67); // 1 char code + 65 char pubkey + OP_CHECKSIG + + std::vector<unsigned char> out; + bool done = CompressScript(script, out); + BOOST_CHECK_EQUAL(done, true); + + // Check compressed script + BOOST_CHECK_EQUAL(out.size(), 33); + BOOST_CHECK_EQUAL(memcmp(&out[1], &script[2], 32), 0); // first 32 chars of CPubKey are copied into out[1:] + BOOST_CHECK_EQUAL(out[0], 0x04 | (script[65] & 0x01)); // least significant bit (lsb) of last char of pubkey is mapped into out[0] +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp index 96d7947b07..383d879040 100644 --- a/src/test/fuzz/transaction.cpp +++ b/src/test/fuzz/transaction.cpp @@ -43,12 +43,7 @@ void test_one_input(const std::vector<uint8_t>& buffer) } CValidationState state_with_dupe_check; - const bool valid_with_dupe_check = CheckTransaction(tx, state_with_dupe_check, /* fCheckDuplicateInputs= */ true); - CValidationState state_without_dupe_check; - const bool valid_without_dupe_check = CheckTransaction(tx, state_without_dupe_check, /* fCheckDuplicateInputs= */ false); - if (valid_with_dupe_check) { - assert(valid_without_dupe_check); - } + (void)CheckTransaction(tx, state_with_dupe_check); const CFeeRate dust_relay_fee{DUST_RELAY_TX_FEE}; std::string reason; diff --git a/src/validation.cpp b/src/validation.cpp index 70b847d3b0..9301066c6a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -392,7 +392,7 @@ static void UpdateMempoolForReorg(DisconnectedBlockTransactions& disconnectpool, // Used to avoid mempool polluting consensus critical paths if CCoinsViewMempool // were somehow broken and returning the wrong scriptPubKeys static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& view, const CTxMemPool& pool, - unsigned int flags, bool cacheSigStore, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { + unsigned int flags, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { AssertLockHeld(cs_main); // pool.cs should be locked already, but go ahead and re-take the lock here @@ -422,7 +422,8 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, CValidationSt } } - return CheckInputs(tx, state, view, flags, cacheSigStore, true, txdata); + // Call CheckInputs() to cache signature and script validity against current tip consensus rules. + return CheckInputs(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata); } namespace { @@ -959,7 +960,7 @@ bool MemPoolAccept::ConsensusScriptChecks(ATMPArgs& args, Workspace& ws, Precomp // invalid blocks (using TestBlockValidity), however allowing such // transactions into the mempool can be exploited as a DoS attack. unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(::ChainActive().Tip(), chainparams.GetConsensus()); - if (!CheckInputsFromMempoolAndCache(tx, state, m_view, m_pool, currentBlockScriptVerifyFlags, true, txdata)) { + if (!CheckInputsFromMempoolAndCache(tx, state, m_view, m_pool, currentBlockScriptVerifyFlags, txdata)) { return error("%s: BUG! PLEASE REPORT THIS! CheckInputs failed against latest-block but not STANDARD flags %s, %s", __func__, hash.ToString(), FormatStateMessage(state)); } @@ -3301,9 +3302,8 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "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)) + if (!CheckTransaction(*tx, state)) return state.Invalid(state.GetReason(), false, state.GetRejectReason(), strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage())); |