Skip to content

Commit

Permalink
partial bitcoin#21330: Deal with missing data in signature hashes mor…
Browse files Browse the repository at this point in the history 
…e consistently

excludes:
- 497718b (SegWit)
- 725d7ae (signet)
  • Loading branch information
@kwvg
kwvg committed Dec 21, 2024
1 parent ad7a373 commit 4a08920
Show file tree
Hide file tree
Showing 12 changed files with 59 additions and 37 deletions.
2 changes: 1 addition & 1 deletion src/coinjoin/server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ bool CCoinJoinServer::IsInputScriptSigValid(const CTxIn& txin) const
txNew.vin[nTxInIndex].scriptSig = txin.scriptSig;
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::IsInputScriptSigValid -- verifying scriptSig %s\n", ScriptToAsmStr(txin.scriptSig).substr(0, 24));
// TODO we're using amount=0 here but we should use the correct amount. This works because Dash ignores the amount while signing/verifying (only used in Bitcoin/Segwit)
if (!VerifyScript(txNew.vin[nTxInIndex].scriptSig, sigPubKey, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, MutableTransactionSignatureChecker(&txNew, nTxInIndex, 0))) {
if (!VerifyScript(txNew.vin[nTxInIndex].scriptSig, sigPubKey, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, MutableTransactionSignatureChecker(&txNew, nTxInIndex, 0, MissingDataBehavior::ASSERT_FAIL))) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::IsInputScriptSigValid -- VerifyScript() failed on input %d\n", nTxInIndex);
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion src/script/bitcoinconsensus.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ int dashconsensus_verify_script(const unsigned char *scriptPubKey, unsigned int

PrecomputedTransactionData txdata(tx);
CAmount am(0);
return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), flags, TransactionSignatureChecker(&tx, nIn, am, txdata), nullptr);
return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), flags, TransactionSignatureChecker(&tx, nIn, am, txdata, MissingDataBehavior::FAIL), nullptr);
} catch (const std::exception&) {
return set_error(err, dashconsensus_ERR_TX_DESERIALIZE); // Error deserializing
}
Expand Down
16 changes: 14 additions & 2 deletions src/script/interpreter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1516,10 +1516,22 @@ PrecomputedTransactionData::PrecomputedTransactionData(const T& txTo)
}

// explicit instantiation
template PrecomputedTransactionData::PrecomputedTransactionData(const CTransaction& txTo);
template PrecomputedTransactionData::PrecomputedTransactionData(const CMutableTransaction& txTo);
template void PrecomputedTransactionData::Init(const CTransaction& txTo, std::vector<CTxOut>&& spent_outputs);
template void PrecomputedTransactionData::Init(const CMutableTransaction& txTo, std::vector<CTxOut>&& spent_outputs);
template PrecomputedTransactionData::PrecomputedTransactionData(const CTransaction& txTo);
template PrecomputedTransactionData::PrecomputedTransactionData(const CMutableTransaction& txTo);

static bool HandleMissingData(MissingDataBehavior mdb)
{
switch (mdb) {
case MissingDataBehavior::ASSERT_FAIL:
assert(!"Missing data");
break;
case MissingDataBehavior::FAIL:
return false;
}
assert(!"Unknown MissingDataBehavior value");
}

template <class T>
uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn, int nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache)
Expand Down
14 changes: 12 additions & 2 deletions src/script/interpreter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,21 @@ class BaseSignatureChecker
virtual ~BaseSignatureChecker() {}
};

/** Enum to specify what *TransactionSignatureChecker's behavior should be
* when dealing with missing transaction data.
*/
enum class MissingDataBehavior
{
ASSERT_FAIL, //!< Abort execution through assertion failure (for consensus code)
FAIL, //!< Just act as if the signature was invalid
};

template <class T>
class GenericTransactionSignatureChecker : public BaseSignatureChecker
{
private:
const T* txTo;
const MissingDataBehavior m_mdb;
unsigned int nIn;
const CAmount amount;
const PrecomputedTransactionData* txdata;
Expand All @@ -169,8 +179,8 @@ class GenericTransactionSignatureChecker : public BaseSignatureChecker
virtual bool VerifySignature(const std::vector<unsigned char>& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const;

public:
GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn) : txTo(txToIn), nIn(nInIn), amount(amountIn), txdata(nullptr) {}
GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn) : txTo(txToIn), nIn(nInIn), amount(amountIn), txdata(&txdataIn) {}
GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, MissingDataBehavior mdb) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(nullptr) {}
GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn, MissingDataBehavior mdb) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(&txdataIn) {}
bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override;
bool CheckLockTime(const CScriptNum& nLockTime) const override;
bool CheckSequence(const CScriptNum& nSequence) const override;
Expand Down
2 changes: 1 addition & 1 deletion src/script/sigcache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class CachingTransactionSignatureChecker : public TransactionSignatureChecker
bool store;

public:
CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amount, PrecomputedTransactionData& txdataIn, bool storeIn=true) : TransactionSignatureChecker(txToIn, nInIn, amount, txdataIn), store(storeIn) {}
CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amount, PrecomputedTransactionData& txdataIn, bool storeIn=true) : TransactionSignatureChecker(txToIn, nInIn, amount, txdataIn, MissingDataBehavior::ASSERT_FAIL), store(storeIn) {}

bool VerifySignature(const std::vector<unsigned char>& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const override;
};
Expand Down
6 changes: 3 additions & 3 deletions src/script/sign.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

typedef std::vector<unsigned char> valtype;

MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn) : txTo(txToIn), nIn(nInIn), nHashType(nHashTypeIn), amount(amountIn), checker(txTo, nIn, amountIn) {}
MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn) : txTo(txToIn), nIn(nInIn), nHashType(nHashTypeIn), amount(amountIn), checker(txTo, nIn, amountIn, MissingDataBehavior::FAIL) {}

bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& address, const CScript& scriptCode, SigVersion sigversion) const
{
Expand Down Expand Up @@ -249,7 +249,7 @@ SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nI
Stacks stack(data);

// Get signatures
MutableTransactionSignatureChecker tx_checker(&tx, nIn, txout.nValue);
MutableTransactionSignatureChecker tx_checker(&tx, nIn, txout.nValue, MissingDataBehavior::FAIL);
SignatureExtractorChecker extractor_checker(data, tx_checker);
if (VerifyScript(data.scriptSig, txout.scriptPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, extractor_checker)) {
data.complete = true;
Expand Down Expand Up @@ -414,7 +414,7 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
UpdateInput(txin, sigdata);

ScriptError serror = SCRIPT_ERR_OK;
if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount), &serror)) {
if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount, MissingDataBehavior::FAIL), &serror)) {
if (serror == SCRIPT_ERR_INVALID_STACK_OPERATION) {
// Unable to sign input and verification failed (possible attempt to partially sign).
input_errors[i] = Untranslated("Unable to sign input, invalid stack size (possibly missing key)");
Expand Down
2 changes: 1 addition & 1 deletion src/test/evo_deterministicmns_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ static bool CheckTransactionSignature(const CTxMemPool& mempool, const CMutableT
BOOST_REQUIRE(txFrom);

CAmount amount = txFrom->vout[txin.prevout.n].nValue;
if (!VerifyScript(txin.scriptSig, txFrom->vout[txin.prevout.n].scriptPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker(&tx, i, amount))) {
if (!VerifyScript(txin.scriptSig, txFrom->vout[txin.prevout.n].scriptPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker(&tx, i, amount, MissingDataBehavior::ASSERT_FAIL))) {
return false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/fuzz/script_flags.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ FUZZ_TARGET(script_flags)
prevout.nValue = 1;
}

const TransactionSignatureChecker checker{&tx, i, prevout.nValue, txdata};
const TransactionSignatureChecker checker{&tx, i, prevout.nValue, txdata, MissingDataBehavior::ASSERT_FAIL};

ScriptError serror;
const bool ret = VerifyScript(tx.vin.at(i).scriptSig, prevout.scriptPubKey, verify_flags, checker, &serror);
Expand Down
16 changes: 8 additions & 8 deletions src/test/multisig_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,20 +77,20 @@ BOOST_AUTO_TEST_CASE(multisig_verify)
keys.assign(1,key[0]);
keys.push_back(key[1]);
s = sign_multisig(a_and_b, keys, CTransaction(txTo[0]), 0);
BOOST_CHECK(VerifyScript(s, a_and_b, flags, MutableTransactionSignatureChecker(&txTo[0], 0, amount), &err));
BOOST_CHECK(VerifyScript(s, a_and_b, flags, MutableTransactionSignatureChecker(&txTo[0], 0, amount, MissingDataBehavior::ASSERT_FAIL), &err));
BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_OK, ScriptErrorString(err));

for (int i = 0; i < 4; i++)
{
keys.assign(1,key[i]);
s = sign_multisig(a_and_b, keys, CTransaction(txTo[0]), 0);
BOOST_CHECK_MESSAGE(!VerifyScript(s, a_and_b, flags, MutableTransactionSignatureChecker(&txTo[0], 0, amount), &err), strprintf("a&b 1: %d", i));
BOOST_CHECK_MESSAGE(!VerifyScript(s, a_and_b, flags, MutableTransactionSignatureChecker(&txTo[0], 0, amount, MissingDataBehavior::ASSERT_FAIL), &err), strprintf("a&b 1: %d", i));
BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_INVALID_STACK_OPERATION, ScriptErrorString(err));

keys.assign(1,key[1]);
keys.push_back(key[i]);
s = sign_multisig(a_and_b, keys, CTransaction(txTo[0]), 0);
BOOST_CHECK_MESSAGE(!VerifyScript(s, a_and_b, flags, MutableTransactionSignatureChecker(&txTo[0], 0, amount), &err), strprintf("a&b 2: %d", i));
BOOST_CHECK_MESSAGE(!VerifyScript(s, a_and_b, flags, MutableTransactionSignatureChecker(&txTo[0], 0, amount, MissingDataBehavior::ASSERT_FAIL), &err), strprintf("a&b 2: %d", i));
BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_EVAL_FALSE, ScriptErrorString(err));
}

Expand All @@ -101,18 +101,18 @@ BOOST_AUTO_TEST_CASE(multisig_verify)
s = sign_multisig(a_or_b, keys, CTransaction(txTo[1]), 0);
if (i == 0 || i == 1)
{
BOOST_CHECK_MESSAGE(VerifyScript(s, a_or_b, flags, MutableTransactionSignatureChecker(&txTo[1], 0, amount), &err), strprintf("a|b: %d", i));
BOOST_CHECK_MESSAGE(VerifyScript(s, a_or_b, flags, MutableTransactionSignatureChecker(&txTo[1], 0, amount, MissingDataBehavior::ASSERT_FAIL), &err), strprintf("a|b: %d", i));
BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_OK, ScriptErrorString(err));
}
else
{
BOOST_CHECK_MESSAGE(!VerifyScript(s, a_or_b, flags, MutableTransactionSignatureChecker(&txTo[1], 0, amount), &err), strprintf("a|b: %d", i));
BOOST_CHECK_MESSAGE(!VerifyScript(s, a_or_b, flags, MutableTransactionSignatureChecker(&txTo[1], 0, amount, MissingDataBehavior::ASSERT_FAIL), &err), strprintf("a|b: %d", i));
BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_EVAL_FALSE, ScriptErrorString(err));
}
}
s.clear();
s << OP_0 << OP_1;
BOOST_CHECK(!VerifyScript(s, a_or_b, flags, MutableTransactionSignatureChecker(&txTo[1], 0, amount), &err));
BOOST_CHECK(!VerifyScript(s, a_or_b, flags, MutableTransactionSignatureChecker(&txTo[1], 0, amount, MissingDataBehavior::ASSERT_FAIL), &err));
BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_SIG_DER, ScriptErrorString(err));


Expand All @@ -124,12 +124,12 @@ BOOST_AUTO_TEST_CASE(multisig_verify)
s = sign_multisig(escrow, keys, CTransaction(txTo[2]), 0);
if (i < j && i < 3 && j < 3)
{
BOOST_CHECK_MESSAGE(VerifyScript(s, escrow, flags, MutableTransactionSignatureChecker(&txTo[2], 0, amount), &err), strprintf("escrow 1: %d %d", i, j));
BOOST_CHECK_MESSAGE(VerifyScript(s, escrow, flags, MutableTransactionSignatureChecker(&txTo[2], 0, amount, MissingDataBehavior::ASSERT_FAIL), &err), strprintf("escrow 1: %d %d", i, j));
BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_OK, ScriptErrorString(err));
}
else
{
BOOST_CHECK_MESSAGE(!VerifyScript(s, escrow, flags, MutableTransactionSignatureChecker(&txTo[2], 0, amount), &err), strprintf("escrow 2: %d %d", i, j));
BOOST_CHECK_MESSAGE(!VerifyScript(s, escrow, flags, MutableTransactionSignatureChecker(&txTo[2], 0, amount, MissingDataBehavior::ASSERT_FAIL), &err), strprintf("escrow 2: %d %d", i, j));
BOOST_CHECK_MESSAGE(err == SCRIPT_ERR_EVAL_FALSE, ScriptErrorString(err));
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/script_p2sh_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ Verify(const CScript& scriptSig, const CScript& scriptPubKey, bool fStrict, Scri
txTo.vin[0].scriptSig = scriptSig;
txTo.vout[0].nValue = 1;

return VerifyScript(scriptSig, scriptPubKey, fStrict ? SCRIPT_VERIFY_P2SH : SCRIPT_VERIFY_NONE, MutableTransactionSignatureChecker(&txTo, 0, txFrom.vout[0].nValue), &err);
return VerifyScript(scriptSig, scriptPubKey, fStrict ? SCRIPT_VERIFY_P2SH : SCRIPT_VERIFY_NONE, MutableTransactionSignatureChecker(&txTo, 0, txFrom.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err);
}


Expand Down
Loading

0 comments on commit 4a08920

Please sign in to comment.