Merge bitcoin/bitcoin#30482: rest: Reject truncated hex txid early in getutxos parsing

fanquake · fanquake · commit 30e8a79aefd4 · 2024-07-25T13:49:21.000+01:00
fac0c3d doc: Add release notes for two pull requests (MarcoFalke) fa7b57e refactor: Replace ParseHashStr with FromHex (MarcoFalke) fa90777 rest: Reject truncated hex txid early in getutxos parsing (MarcoFalke) fab6ddb refactor: Expose FromHex in transaction_identifier (MarcoFalke) fad2991 refactor: Implement strict uint256::FromHex() (MarcoFalke) fa103db scripted-diff: Rename SetHex to SetHexDeprecated (MarcoFalke) fafe4b8 test: refactor: Replace SetHex with uint256 constructor directly (MarcoFalke) Pull request description: In `rest_getutxos` truncated txids such as `aa` or `ff` are accepted. This is brittle at best. Fix it by rejecting any truncated (or overlarge) input. ---- Review note: This also starts a major refactor to rework hex parsing in Bitcoin Core, meaning that a few refactor commits are included as well. They are explained individually in the commit message and the work will be continued in the future. ACKs for top commit: stickies-v: re-ACK fac0c3d - only doc and test updates to address review comments, thanks! hodlinator: ACK fac0c3d Tree-SHA512: 473feb3fcf6118443435d1dd321006135b0b54689bfbbcb1697bb5811a449bef51f475c715de6911ff3c4ea3bdb75f601861ff93347bc4414d6b9e5298105dd7
diff --git a/doc/release-notes-30482.md b/doc/release-notes-30482.md
@@ -0,0 +1,6 @@
+Updated REST APIs
+-----------------
+- Parameter validation for `/rest/getutxos` has been improved by rejecting
+  truncated or overly large txids and malformed outpoint indices by raising an
+  HTTP_BAD_REQUEST "Parse error". Previously, these malformed requests would be
+  silently handled. (#30482, #30444)
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
@@ -264,8 +264,8 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
         throw std::runtime_error("TX input missing separator");
 
     // extract and validate TXID
-    uint256 txid;
-    if (!ParseHashStr(vStrInputParts[0], txid)) {
+    auto txid{Txid::FromHex(vStrInputParts[0])};
+    if (!txid) {
         throw std::runtime_error("invalid TX input txid");
     }
 
@@ -285,7 +285,7 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
     }
 
     // append to transaction input list
-    CTxIn txin(Txid::FromUint256(txid), vout, CScript(), nSequenceIn);
+    CTxIn txin(*txid, vout, CScript(), nSequenceIn);
     tx.vin.push_back(txin);
 }
 
@@ -625,16 +625,16 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
             if (!prevOut.checkObject(types))
                 throw std::runtime_error("prevtxs internal object typecheck fail");
 
-            uint256 txid;
-            if (!ParseHashStr(prevOut["txid"].get_str(), txid)) {
+            auto txid{Txid::FromHex(prevOut["txid"].get_str())};
+            if (!txid) {
                 throw std::runtime_error("txid must be hexadecimal string (not '" + prevOut["txid"].get_str() + "')");
             }
 
             const int nOut = prevOut["vout"].getInt<int>();
             if (nOut < 0)
                 throw std::runtime_error("vout cannot be negative");
 
-            COutPoint out(Txid::FromUint256(txid), nOut);
+            COutPoint out(*txid, nOut);
             std::vector<unsigned char> pkData(ParseHexUV(prevOut["scriptPubKey"], "scriptPubKey"));
             CScript scriptPubKey(pkData.begin(), pkData.end());
 
diff --git a/src/core_io.h b/src/core_io.h
@@ -37,15 +37,6 @@ std::string ScriptToAsmStr(const CScript& script, const bool fAttemptSighashDeco
 [[nodiscard]] bool DecodeHexBlk(CBlock&, const std::string& strHexBlk);
 bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
 
-/**
- * Parse a hex string into 256 bits
- * @param[in] strHex a hex-formatted, 64-character string
- * @param[out] result the result of the parsing
- * @returns true if successful, false if not
- *
- * @see ParseHashV for an RPC-oriented version of this
- */
-bool ParseHashStr(const std::string& strHex, uint256& result);
 [[nodiscard]] util::Result<int> SighashFromStr(const std::string& sighash);
 
 // core_write.cpp
diff --git a/src/core_read.cpp b/src/core_read.cpp
@@ -234,15 +234,6 @@ bool DecodeHexBlk(CBlock& block, const std::string& strHexBlk)
     return true;
 }
 
-bool ParseHashStr(const std::string& strHex, uint256& result)
-{
-    if ((strHex.size() != 64) || !IsHex(strHex))
-        return false;
-
-    result.SetHex(strHex);
-    return true;
-}
-
 util::Result<int> SighashFromStr(const std::string& sighash)
 {
     static const std::map<std::string, int> map_sighash_values = {
diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
@@ -277,7 +277,7 @@ void TransactionTableModel::updateAmountColumnTitle()
 void TransactionTableModel::updateTransaction(const QString &hash, int status, bool showTransaction)
 {
     uint256 updated;
-    updated.SetHex(hash.toStdString());
+    updated.SetHexDeprecated(hash.toStdString());
 
     priv->updateWallet(walletModel->wallet(), updated, status, showTransaction);
 }
diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
@@ -396,7 +396,7 @@ void TransactionView::contextualMenu(const QPoint &point)
 
     // check if transaction can be abandoned, disable context menu action in case it doesn't
     uint256 hash;
-    hash.SetHex(selection.at(0).data(TransactionTableModel::TxHashRole).toString().toStdString());
+    hash.SetHexDeprecated(selection.at(0).data(TransactionTableModel::TxHashRole).toString().toStdString());
     abandonAction->setEnabled(model->wallet().transactionCanBeAbandoned(hash));
     bumpFeeAction->setEnabled(model->wallet().transactionCanBeBumped(hash));
     copyAddressAction->setEnabled(GUIUtil::hasEntryData(transactionView, 0, TransactionTableModel::AddressRole));
@@ -416,7 +416,7 @@ void TransactionView::abandonTx()
     // get the hash from the TxHashRole (QVariant / QString)
     uint256 hash;
     QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString();
-    hash.SetHex(hashQStr.toStdString());
+    hash.SetHexDeprecated(hashQStr.toStdString());
 
     // Abandon the wallet transaction over the walletModel
     model->wallet().abandonTransaction(hash);
@@ -431,7 +431,7 @@ void TransactionView::bumpFee([[maybe_unused]] bool checked)
     // get the hash from the TxHashRole (QVariant / QString)
     uint256 hash;
     QString hashQStr = selection.at(0).data(TransactionTableModel::TxHashRole).toString();
-    hash.SetHex(hashQStr.toStdString());
+    hash.SetHexDeprecated(hashQStr.toStdString());
 
     // Bump tx fee over the walletModel
     uint256 newHash;
diff --git a/src/rest.cpp b/src/rest.cpp
@@ -217,9 +217,10 @@ static bool rest_headers(const std::any& context,
         return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count is invalid or out of acceptable range (1-%u): %s", MAX_REST_HEADERS_RESULTS, raw_count));
     }
 
-    uint256 hash;
-    if (!ParseHashStr(hashStr, hash))
+    auto hash{uint256::FromHex(hashStr)};
+    if (!hash) {
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+    }
 
     const CBlockIndex* tip = nullptr;
     std::vector<const CBlockIndex*> headers;
@@ -231,7 +232,7 @@ static bool rest_headers(const std::any& context,
         LOCK(cs_main);
         CChain& active_chain = chainman.ActiveChain();
         tip = active_chain.Tip();
-        const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
+        const CBlockIndex* pindex{chainman.m_blockman.LookupBlockIndex(*hash)};
         while (pindex != nullptr && active_chain.Contains(pindex)) {
             headers.push_back(pindex);
             if (headers.size() == *parsed_count) {
@@ -290,9 +291,10 @@ static bool rest_block(const std::any& context,
     std::string hashStr;
     const RESTResponseFormat rf = ParseDataFormat(hashStr, strURIPart);
 
-    uint256 hash;
-    if (!ParseHashStr(hashStr, hash))
+    auto hash{uint256::FromHex(hashStr)};
+    if (!hash) {
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+    }
 
     FlatFilePos pos{};
     const CBlockIndex* pblockindex = nullptr;
@@ -303,7 +305,7 @@ static bool rest_block(const std::any& context,
     {
         LOCK(cs_main);
         tip = chainman.ActiveChain().Tip();
-        pblockindex = chainman.m_blockman.LookupBlockIndex(hash);
+        pblockindex = chainman.m_blockman.LookupBlockIndex(*hash);
         if (!pblockindex) {
             return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
         }
@@ -390,8 +392,8 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const
         return RESTERR(req, HTTP_BAD_REQUEST, strprintf("Header count is invalid or out of acceptable range (1-%u): %s", MAX_REST_HEADERS_RESULTS, raw_count));
     }
 
-    uint256 block_hash;
-    if (!ParseHashStr(raw_blockhash, block_hash)) {
+    auto block_hash{uint256::FromHex(raw_blockhash)};
+    if (!block_hash) {
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + raw_blockhash);
     }
 
@@ -413,7 +415,7 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const
         ChainstateManager& chainman = *maybe_chainman;
         LOCK(cs_main);
         CChain& active_chain = chainman.ActiveChain();
-        const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(block_hash);
+        const CBlockIndex* pindex{chainman.m_blockman.LookupBlockIndex(*block_hash)};
         while (pindex != nullptr && active_chain.Contains(pindex)) {
             headers.push_back(pindex);
             if (headers.size() == *parsed_count)
@@ -494,8 +496,8 @@ static bool rest_block_filter(const std::any& context, HTTPRequest* req, const s
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/blockfilter/<filtertype>/<blockhash>");
     }
 
-    uint256 block_hash;
-    if (!ParseHashStr(uri_parts[1], block_hash)) {
+    auto block_hash{uint256::FromHex(uri_parts[1])};
+    if (!block_hash) {
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + uri_parts[1]);
     }
 
@@ -516,7 +518,7 @@ static bool rest_block_filter(const std::any& context, HTTPRequest* req, const s
         if (!maybe_chainman) return false;
         ChainstateManager& chainman = *maybe_chainman;
         LOCK(cs_main);
-        block_index = chainman.m_blockman.LookupBlockIndex(block_hash);
+        block_index = chainman.m_blockman.LookupBlockIndex(*block_hash);
         if (!block_index) {
             return RESTERR(req, HTTP_NOT_FOUND, uri_parts[1] + " not found");
         }
@@ -616,14 +618,14 @@ static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const
         jsonRequest.params = UniValue(UniValue::VARR);
 
         if (!hash_str.empty()) {
-            uint256 hash;
-            if (!ParseHashStr(hash_str, hash)) {
+            auto hash{uint256::FromHex(hash_str)};
+            if (!hash) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hash_str);
             }
 
             const ChainstateManager* chainman = GetChainman(context, req);
             if (!chainman) return false;
-            if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(ParseHashV(hash_str, "blockhash")))) {
+            if (!WITH_LOCK(::cs_main, return chainman->m_blockman.LookupBlockIndex(*hash))) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Block not found");
             }
 
@@ -704,9 +706,10 @@ static bool rest_tx(const std::any& context, HTTPRequest* req, const std::string
     std::string hashStr;
     const RESTResponseFormat rf = ParseDataFormat(hashStr, strURIPart);
 
-    uint256 hash;
-    if (!ParseHashStr(hashStr, hash))
+    auto hash{uint256::FromHex(hashStr)};
+    if (!hash) {
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
+    }
 
     if (g_txindex) {
         g_txindex->BlockUntilSyncedToCurrentChain();
@@ -715,7 +718,7 @@ static bool rest_tx(const std::any& context, HTTPRequest* req, const std::string
     const NodeContext* const node = GetNodeContext(context, req);
     if (!node) return false;
     uint256 hashBlock = uint256();
-    const CTransactionRef tx = GetTransaction(/*block_index=*/nullptr, node->mempool.get(), hash, hashBlock, node->chainman->m_blockman);
+    const CTransactionRef tx{GetTransaction(/*block_index=*/nullptr, node->mempool.get(), *hash, hashBlock, node->chainman->m_blockman)};
     if (!tx) {
         return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
     }
@@ -792,13 +795,14 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
             if (txid_out.size() != 2) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
             }
+            auto txid{Txid::FromHex(txid_out.at(0))};
             auto output{ToIntegral<uint32_t>(txid_out.at(1))};
 
-            if (!output || !IsHex(txid_out.at(0))) {
+            if (!txid || !output) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
             }
 
-            vOutPoints.emplace_back(TxidFromString(txid_out.at(0)), *output);
+            vOutPoints.emplace_back(*txid, *output);
         }
 
         if (vOutPoints.size() > 0)
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
@@ -345,13 +345,11 @@ static RPCHelpMan generateblock()
     std::vector<CTransactionRef> txs;
     const auto raw_txs_or_txids = request.params[1].get_array();
     for (size_t i = 0; i < raw_txs_or_txids.size(); i++) {
-        const auto str(raw_txs_or_txids[i].get_str());
+        const auto& str{raw_txs_or_txids[i].get_str()};
 
-        uint256 hash;
         CMutableTransaction mtx;
-        if (ParseHashStr(str, hash)) {
-
-            const auto tx = mempool.get(hash);
+        if (auto hash{uint256::FromHex(str)}) {
+            const auto tx{mempool.get(*hash)};
             if (!tx) {
                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Transaction %s not in mempool.", str));
             }
diff --git a/src/test/blockfilter_tests.cpp b/src/test/blockfilter_tests.cpp
@@ -149,8 +149,7 @@ BOOST_AUTO_TEST_CASE(blockfilters_json_test)
 
         unsigned int pos = 0;
         /*int block_height =*/ test[pos++].getInt<int>();
-        uint256 block_hash;
-        BOOST_CHECK(ParseHashStr(test[pos++].get_str(), block_hash));
+        BOOST_CHECK(uint256::FromHex(test[pos++].get_str()));
 
         CBlock block;
         BOOST_REQUIRE(DecodeHexBlk(block, test[pos++].get_str()));
@@ -165,11 +164,9 @@ BOOST_AUTO_TEST_CASE(blockfilters_json_test)
             tx_undo.vprevout.emplace_back(txout, 0, false);
         }
 
-        uint256 prev_filter_header_basic;
-        BOOST_CHECK(ParseHashStr(test[pos++].get_str(), prev_filter_header_basic));
+        uint256 prev_filter_header_basic{*Assert(uint256::FromHex(test[pos++].get_str()))};
         std::vector<unsigned char> filter_basic = ParseHex(test[pos++].get_str());
-        uint256 filter_header_basic;
-        BOOST_CHECK(ParseHashStr(test[pos++].get_str(), filter_header_basic));
+        uint256 filter_header_basic{*Assert(uint256::FromHex(test[pos++].get_str()))};
 
         BlockFilter computed_filter_basic(BlockFilterType::BASIC, block, block_undo);
         BOOST_CHECK(computed_filter_basic.GetFilter().GetEncoded() == filter_basic);
diff --git a/src/test/fuzz/hex.cpp b/src/test/fuzz/hex.cpp
@@ -27,8 +27,7 @@ FUZZ_TARGET(hex)
         assert(ToLower(random_hex_string) == hex_data);
     }
     (void)IsHexNumber(random_hex_string);
-    uint256 result;
-    (void)ParseHashStr(random_hex_string, result);
+    (void)uint256::FromHex(random_hex_string);
     (void)uint256S(random_hex_string);
     try {
         (void)HexToPubKey(random_hex_string);
diff --git a/src/test/pow_tests.cpp b/src/test/pow_tests.cpp
@@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_negative_target)
     uint256 hash;
     unsigned int nBits;
     nBits = UintToArith256(consensus.powLimit).GetCompact(true);
-    hash.SetHex("0x1");
+    hash = uint256{1};
     BOOST_CHECK(!CheckProofOfWork(hash, nBits, consensus));
 }
 
@@ -95,7 +95,7 @@ BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_overflow_target)
     const auto consensus = CreateChainParams(*m_node.args, ChainType::MAIN)->GetConsensus();
     uint256 hash;
     unsigned int nBits{~0x00800000U};
-    hash.SetHex("0x1");
+    hash = uint256{1};
     BOOST_CHECK(!CheckProofOfWork(hash, nBits, consensus));
 }
 
@@ -107,7 +107,7 @@ BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_too_easy_target)
     arith_uint256 nBits_arith = UintToArith256(consensus.powLimit);
     nBits_arith *= 2;
     nBits = nBits_arith.GetCompact();
-    hash.SetHex("0x1");
+    hash = uint256{1};
     BOOST_CHECK(!CheckProofOfWork(hash, nBits, consensus));
 }
 
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
@@ -62,7 +62,7 @@ static std::string ArrayToString(const unsigned char A[], unsigned int width)
 inline uint160 uint160S(std::string_view str)
 {
     uint160 rv;
-    rv.SetHex(str);
+    rv.SetHexDeprecated(str);
     return rv;
 }
 
@@ -157,7 +157,7 @@ BOOST_AUTO_TEST_CASE( comparison ) // <= >= < >
                    uint256S("1000000000000000000000000000000000000000000000000000000000000002"));
 }
 
-BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize
+BOOST_AUTO_TEST_CASE(methods) // GetHex SetHexDeprecated FromHex begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize
 {
     BOOST_CHECK_EQUAL(R1L.GetHex(), R1L.ToString());
     BOOST_CHECK_EQUAL(R2L.GetHex(), R2L.ToString());
@@ -166,12 +166,12 @@ BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex begin() end() size() GetLow64 G
     uint256 TmpL(R1L);
     BOOST_CHECK_EQUAL(TmpL, R1L);
     // Verify previous values don't persist when setting to truncated string.
-    TmpL.SetHex("21");
+    TmpL.SetHexDeprecated("21");
     BOOST_CHECK_EQUAL(TmpL.ToString(), "0000000000000000000000000000000000000000000000000000000000000021");
-    TmpL.SetHex(R2L.ToString());   BOOST_CHECK_EQUAL(TmpL, R2L);
-    TmpL.SetHex(ZeroL.ToString()); BOOST_CHECK_EQUAL(TmpL, uint256());
+    BOOST_CHECK_EQUAL(uint256::FromHex(R2L.ToString()).value(), R2L);
+    BOOST_CHECK_EQUAL(uint256::FromHex(ZeroL.ToString()).value(), uint256());
 
-    TmpL.SetHex(R1L.ToString());
+    TmpL = uint256::FromHex(R1L.ToString()).value();
     BOOST_CHECK_EQUAL_COLLECTIONS(R1L.begin(), R1L.end(), R1Array, R1Array + R1L.size());
     BOOST_CHECK_EQUAL_COLLECTIONS(TmpL.begin(), TmpL.end(), R1Array, R1Array + TmpL.size());
     BOOST_CHECK_EQUAL_COLLECTIONS(R2L.begin(), R2L.end(), R2Array, R2Array + R2L.size());
@@ -214,10 +214,10 @@ BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex begin() end() size() GetLow64 G
     BOOST_CHECK_EQUAL(MaxS.GetHex(), MaxS.ToString());
     uint160 TmpS(R1S);
     BOOST_CHECK_EQUAL(TmpS, R1S);
-    TmpS.SetHex(R2S.ToString());   BOOST_CHECK_EQUAL(TmpS, R2S);
-    TmpS.SetHex(ZeroS.ToString()); BOOST_CHECK_EQUAL(TmpS, uint160());
+    BOOST_CHECK_EQUAL(uint160::FromHex(R2S.ToString()).value(), R2S);
+    BOOST_CHECK_EQUAL(uint160::FromHex(ZeroS.ToString()).value(), uint160());
 
-    TmpS.SetHex(R1S.ToString());
+    TmpS = uint160::FromHex(R1S.ToString()).value();
     BOOST_CHECK_EQUAL_COLLECTIONS(R1S.begin(), R1S.end(), R1Array, R1Array + R1S.size());
     BOOST_CHECK_EQUAL_COLLECTIONS(TmpS.begin(), TmpS.end(), R1Array, R1Array + TmpS.size());
     BOOST_CHECK_EQUAL_COLLECTIONS(R2S.begin(), R2S.end(), R2Array, R2Array + R2S.size());
diff --git a/src/uint256.cpp b/src/uint256.cpp
diff --git a/src/uint256.h b/src/uint256.h
diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py

Original file line number	Diff line number	Diff line change
`@@ -277,7 +277,7 @@ void TransactionTableModel::updateAmountColumnTitle()`
`277`	`277`	`void TransactionTableModel::updateTransaction(const QString &hash, int status, bool showTransaction)`
`278`	`278`	`{`
`279`	`279`	`uint256 updated;`
`280`		`- updated.SetHex(hash.toStdString());`
	`280`	`+ updated.SetHexDeprecated(hash.toStdString());`
`281`	`281`
`282`	`282`	`priv->updateWallet(walletModel->wallet(), updated, status, showTransaction);`
`283`	`283`	`}`