rest: Reject negative outpoint index early in getutxos parsing

maflcko commented at 3:51 pm on July 12, 2024: member

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 -.

rest: Reject negative outpoint index in getutxos parsing fab54db9f1

DrahtBot commented at 3:51 pm on July 12, 2024: contributor

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

Code Coverage

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator, tdb3, danielabrozzoni, brunoerg, achow101

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

Conflicts

No conflicts as of last run.

DrahtBot added the label RPC/REST/ZMQ on Jul 12, 2024

brunoerg commented at 6:06 pm on July 12, 2024: contributor

Concept ACK

brunoerg approved

brunoerg commented at 9:32 pm on July 12, 2024: contributor

utACK fab54db9f1d0e634f4a697480dbb87b87940dc5c

tdb3 approved

tdb3 commented at 0:14 am on July 13, 2024: contributor

ACK fab54db9f1d0e634f4a697480dbb87b87940dc5c

Looks good. Ran a few manual tests:

 0$ src/bitcoin-cli createwallet test
 1$ src/bitcoin-cli -generate 101
 2$ src/bitcoin-cli -named sendtoaddress address=bcrt1qg2yks0c0slr8nklg30ts48lcq7yslcvaak7n9k amount=39 fee_rate=3
 3(0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284)
 4$ src/bitcoin-cli -generate 1
 5$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284-0.json
 6{"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"}}]}
 7$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284-1.js
 8on
 9{"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"}}]}
10$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284--1.j
11son
12Parse error
13$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284-+1.j
14son
15Parse error
16$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284--0.j
17son
18Parse error
19$ curl localhost:18443/rest/getutxos/0383cce20ad8c1f119f4f1e027bbb132c5e0500518c6adfd79098242ad733284-+0.j
20son
21Parse error

hodlinator approved

hodlinator commented at 11:57 am on July 13, 2024: contributor

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*:

 0diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
 1index 5654c8b0a8..7fff75c46c 100644
 2--- a/src/test/util_tests.cpp
 3+++ b/src/test/util_tests.cpp
 4@@ -627,6 +627,7 @@ BOOST_AUTO_TEST_CASE(test_ParseInt32)
 5     BOOST_CHECK(ParseInt32("1234", nullptr));
 6     BOOST_CHECK(ParseInt32("0", &n) && n == 0);
 7     BOOST_CHECK(ParseInt32("1234", &n) && n == 1234);
 8+    BOOST_CHECK(ParseInt32("+1234", &n) && n == 1234);
 9     BOOST_CHECK(ParseInt32("01234", &n) && n == 1234); // no octal
10     BOOST_CHECK(ParseInt32("2147483647", &n) && n == 2147483647);
11     BOOST_CHECK(ParseInt32("-2147483648", &n) && n == (-2147483647 - 1)); // (-2147483647 - 1) equals INT_MIN
12@@ -848,6 +849,7 @@ BOOST_AUTO_TEST_CASE(test_ParseInt64)
13     BOOST_CHECK(ParseInt64("1234", nullptr));
14     BOOST_CHECK(ParseInt64("0", &n) && n == 0LL);
15     BOOST_CHECK(ParseInt64("1234", &n) && n == 1234LL);
16+    BOOST_CHECK(ParseInt64("+1234", &n) && n == 1234LL);
17     BOOST_CHECK(ParseInt64("01234", &n) && n == 1234LL); // no octal
18     BOOST_CHECK(ParseInt64("2147483647", &n) && n == 2147483647LL);
19     BOOST_CHECK(ParseInt64("-2147483648", &n) && n == -2147483648LL);
20@@ -983,6 +985,7 @@ BOOST_AUTO_TEST_CASE(test_ParseUInt64)
21     BOOST_CHECK(ParseUInt64("1234", nullptr));
22     BOOST_CHECK(ParseUInt64("0", &n) && n == 0LL);
23     BOOST_CHECK(ParseUInt64("1234", &n) && n == 1234LL);
24+    BOOST_CHECK(ParseUInt64("+1234", &n) && n == 1234LL);
25     BOOST_CHECK(ParseUInt64("01234", &n) && n == 1234LL); // no octal
26     BOOST_CHECK(ParseUInt64("2147483647", &n) && n == 2147483647LL);
27     BOOST_CHECK(ParseUInt64("9223372036854775807", &n) && n == 9223372036854775807ULL);

hodlinator commented at 9:39 pm on July 13, 2024: contributor

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:

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

maflcko marked this as a draft on Jul 16, 2024

refactor: Use util::Split to avoid a harmless unsigned-integer-overflow

The previous commit added a test which would fail the
unsigned-integer-overflow sanitizer. The warning is harmless and can be
triggered on any commit, since the code was introduced.

For reference, the warning would happen when the separator `-` was not
present.

For example:

  GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json

would result in:

rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long')
    #0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77
    #1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
    #2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9
    #3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13
    #4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12
    #5 0x7f078ebcbbb3  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2)
    #6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o
    #7 0x7f078e840a93  (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
    #8 0x7f078e8cdc3b  (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77

fac932bf93

maflcko marked this as ready for review on Jul 17, 2024

maflcko commented at 11:48 am on July 17, 2024: member

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.

hodlinator approved

hodlinator commented at 12:05 pm on July 17, 2024: contributor

ut-ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

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

DrahtBot requested review from tdb3 on Jul 17, 2024

DrahtBot requested review from brunoerg on Jul 17, 2024

tdb3 approved

tdb3 commented at 0:02 am on July 18, 2024: contributor

re ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

Re-ran manual tests:

 0$ src/bitcoind -regtest -rest=1
 1$ src/bitcoin-cli -regtest createwallet test
 2$ src/bitcoin-cli -regtest -generate 101
 3$ src/bitcoin-cli -named sendtoaddress address=bcrt1qg2yks0c0slr8nklg30ts48lcq7yslcvaak7n9k amount=39 fee_rate=3
 465203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438
 5$ src/bitcoin-cli -generate 1
 6{
 7  "address": "bcrt1q73g2xwe4ktc4zfyu8uaft9y8gj0skp7kxnuzvl",
 8  "blocks": [
 9    "5ce35b7ccf566226501c094663254e07c85f82d331f1c6441ca8b44028ee9407"
10  ]
11}
12$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438-0.json
13{"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"}}]}
14$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438-1.json
15{"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"}}]}
16$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438--1.json
17Parse error
18$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438-+1.json
19Parse error
20$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438--0.json
21Parse error
22$ curl localhost:18443/rest/getutxos/65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438-+0.json
23Parse error

danielabrozzoni commented at 2:23 pm on July 18, 2024: contributor

ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

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

brunoerg approved

brunoerg commented at 2:59 pm on July 18, 2024: contributor

reACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

achow101 commented at 8:19 pm on July 18, 2024: member

ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

achow101 merged this on Jul 18, 2024

achow101 closed this on Jul 18, 2024

maflcko deleted the branch on Jul 19, 2024

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

Code Coverage

Reviews

Conflicts