rest: Reject negative outpoint index early in getutxos parsing #30444

In rest_getutxos outpoint indexes such as +N or -N are accepted. This should be harmless, because any index out of range should be treated as a non-existent utxo. However, a negative index can't exist ever, so it seems better to reject all signs, whether + or -.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage

For detailed information about the code coverage, see the test coverage report.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

Concept ACK

utACK fab54db9f1d0e634f4a697480dbb87b87940dc5c

ACK fab54db9f1d0e634f4a697480dbb87b87940dc5c

Looks good. Ran a few manual tests:

$ src/bitcoin-cli createwallet test
$ src/bitcoin-cli -generate 101
$ src/bitcoin-cli -named sendtoaddress address=bcrt1qg2yks0c0slr8nklg30ts48lcq7yslcvaak7n9k amount=39 fee_rate=3
(0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284)
$ src/bitcoin-cli -generate 1
$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284-0.json
{"chainHeight":102,"chaintipHash":"3ee95ffa58c763071fe4ec6605afd277fad506bf27ed54228636520ba8b3130a","bitmap":"1","utxos":[{"height":102,"value":39.00000000,"scriptPubKey":{"asm":"0 4289683f0f87c679dbe88bd70a9ff807890fe19d","desc":"addr(bcrt1qg2yks0c0slr8nklg30ts48lcq7yslcvaak7n9k)#sqy6t4hk","hex":"00144289683f0f87c679dbe88bd70a9ff807890fe19d","address":"bcrt1qg2yks0c0slr8nklg30ts48lcq7yslcvaak7n9k","type":"witness_v0_keyhash"}}]}
$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284-1.js
on
{"chainHeight":102,"chaintipHash":"3ee95ffa58c763071fe4ec6605afd277fad506bf27ed54228636520ba8b3130a","bitmap":"1","utxos":[{"height":102,"value":10.99999577,"scriptPubKey":{"asm":"0 9bd3377b8422f75cd642b18b44ac5481793d8682","desc":"addr(bcrt1qn0fnw7uyytm4e4jzkx95ftz5s9unmp5zndfzcp)#ks2s847w","hex":"00149bd3377b8422f75cd642b18b44ac5481793d8682","address":"bcrt1qn0fnw7uyytm4e4jzkx95ftz5s9unmp5zndfzcp","type":"witness_v0_keyhash"}}]}
$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284--1.j
son
Parse error
$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284-+1.j
son
Parse error
$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284--0.j
son
Parse error
$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284-+0.j
son
Parse error

ACK fab54db9f1d0e634f4a697480dbb87b87940dc5c

Good to see more improvements to the robustness of that part of rest.cpp. Ran the test-change without the C++ modifications and verified that signed (+/-) output indexes were previously supported. As that capability served no purpose and is unlikely to have been used, I support this narrowing of the API.

Also noticed what appears to be missing test coverage of Parse[U]Int*:

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 5654c8b0a8..7fff75c46c 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -627,6 +627,7 @@ BOOST_AUTO_TEST_CASE(test_ParseInt32)
     BOOST_CHECK(ParseInt32("1234", nullptr));
     BOOST_CHECK(ParseInt32("0", &n) && n == 0);
     BOOST_CHECK(ParseInt32("1234", &n) && n == 1234);
+    BOOST_CHECK(ParseInt32("+1234", &n) && n == 1234);
     BOOST_CHECK(ParseInt32("01234", &n) && n == 1234); // no octal
     BOOST_CHECK(ParseInt32("2147483647", &n) && n == 2147483647);
     BOOST_CHECK(ParseInt32("-2147483648", &n) && n == (-2147483647 - 1)); // (-2147483647 - 1) equals INT_MIN
@@ -848,6 +849,7 @@ BOOST_AUTO_TEST_CASE(test_ParseInt64)
     BOOST_CHECK(ParseInt64("1234", nullptr));
     BOOST_CHECK(ParseInt64("0", &n) && n == 0LL);
     BOOST_CHECK(ParseInt64("1234", &n) && n == 1234LL);
+    BOOST_CHECK(ParseInt64("+1234", &n) && n == 1234LL);
     BOOST_CHECK(ParseInt64("01234", &n) && n == 1234LL); // no octal
     BOOST_CHECK(ParseInt64("2147483647", &n) && n == 2147483647LL);
     BOOST_CHECK(ParseInt64("-2147483648", &n) && n == -2147483648LL);
@@ -983,6 +985,7 @@ BOOST_AUTO_TEST_CASE(test_ParseUInt64)
     BOOST_CHECK(ParseUInt64("1234", nullptr));
     BOOST_CHECK(ParseUInt64("0", &n) && n == 0LL);
     BOOST_CHECK(ParseUInt64("1234", &n) && n == 1234LL);
+    BOOST_CHECK(ParseUInt64("+1234", &n) && n == 1234LL);
     BOOST_CHECK(ParseUInt64("01234", &n) && n == 1234LL); // no octal
     BOOST_CHECK(ParseUInt64("2147483647", &n) && n == 2147483647LL);
     BOOST_CHECK(ParseUInt64("9223372036854775807", &n) && n == 9223372036854775807ULL);

Have you considered migrating away from the other ParseInt32() call in the same file? Gets rid of the Valgrind comment. This version keeps this one signed since it used in 2 places expecting signed:

diff --git a/src/rest.cpp b/src/rest.cpp
index 185508d2c9..e3e2dd96e6 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -954,8 +954,8 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
     std::string height_str;
     const RESTResponseFormat rf = ParseDataFormat(height_str, str_uri_part);
 
-    int32_t blockheight = -1; // Initialization done only to prevent valgrind false positive, see [#18785](/bitcoin-bitcoin/18785/)
-    if (!ParseInt32(height_str, &blockheight) || blockheight < 0) {
+    auto blockheight{ToIntegral<int32_t>(height_str)};
+    if (!blockheight || *blockheight < 0) {
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid height: " + SanitizeString(height_str));
     }
 
@@ -966,10 +966,10 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
         ChainstateManager& chainman = *maybe_chainman;
         LOCK(cs_main);
         const CChain& active_chain = chainman.ActiveChain();
-        if (blockheight > active_chain.Height()) {
+        if (*blockheight > active_chain.Height()) {
             return RESTERR(req, HTTP_NOT_FOUND, "Block height out of range");
         }
-        pblockindex = active_chain[blockheight];
+        pblockindex = active_chain[*blockheight];
     }
     switch (rf) {
     case RESTResponseFormat::BINARY: {

Pushed a commit to fix a harmless unsigned-integer-overflow warning. @hodlinator Happy to review your suggestions to other functions in a separate pull request, if you create one. But I'll try to keep this one focussed for now.

ut-ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

Nice that the added tests managed to trigger the sanitizer! :+1:

re ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

Re-ran manual tests:

$ src/bitcoind -regtest -rest=1
$ src/bitcoin-cli -regtest createwallet test
$ src/bitcoin-cli -regtest -generate 101
$ src/bitcoin-cli -named sendtoaddress address=bcrt1qg2yks0c0slr8nklg30ts48lcq7yslcvaak7n9k amount=39 fee_rate=3
65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438
$ src/bitcoin-cli -generate 1
{
  "address": "bcrt1q73g2xwe4ktc4zfyu8uaft9y8gj0skp7kxnuzvl",
  "blocks": [
    "5ce35b7ccf566226501c094663254e07c85f82d331f1c6441ca8b44028ee9407"
  ]
}
$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438-0.json
{"chainHeight":103,"chaintipHash":"5ce35b7ccf566226501c094663254e07c85f82d331f1c6441ca8b44028ee9407","bitmap":"1","utxos":[{"height":102,"value":39.00000000,"scriptPubKey":{"asm":"0 4289683f0f87c679dbe88bd70a9ff807890fe19d","desc":"addr(bcrt1qg2yks0c0slr8nklg30ts48lcq7yslcvaak7n9k)#sqy6t4hk","hex":"00144289683f0f87c679dbe88bd70a9ff807890fe19d","address":"bcrt1qg2yks0c0slr8nklg30ts48lcq7yslcvaak7n9k","type":"witness_v0_keyhash"}}]}
$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438-1.json
{"chainHeight":103,"chaintipHash":"5ce35b7ccf566226501c094663254e07c85f82d331f1c6441ca8b44028ee9407","bitmap":"1","utxos":[{"height":102,"value":10.99999577,"scriptPubKey":{"asm":"0 88e8f95a4e0b010acc8dced9132d47446a4dada5","desc":"addr(bcrt1q3r50jkjwpvqs4nydemv3xt28g34ymtd9ynxd9t)#0pnhxc0f","hex":"001488e8f95a4e0b010acc8dced9132d47446a4dada5","address":"bcrt1q3r50jkjwpvqs4nydemv3xt28g34ymtd9ynxd9t","type":"witness_v0_keyhash"}}]}
$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438--1.json
Parse error
$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438-+1.json
Parse error
$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438--0.json
Parse error
$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438-+0.json
Parse error

ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

I checked manually that + and - in getutxos calls were once accepted and are now rejected.

reACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1