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

maflcko · 2024-07-12T15:51:36Z

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

DrahtBot · 2024-07-12T15:51:39Z

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.

brunoerg · 2024-07-12T18:06:02Z

Concept ACK

brunoerg

utACK fab54db

tdb3

ACK fab54db

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

hodlinator

ACK fab54db

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);

hodlinator · 2024-07-13T21:39:59Z

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 https://github.com/bitcoin/bitcoin/pull/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: {

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 bitcoin#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) bitcoin#8 0x7f078e8cdc3b (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77

maflcko · 2024-07-17T11:48:00Z

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

ut-ACK fac932b

Nice that the added tests managed to trigger the sanitizer! 👍

tdb3

re ACK fac932b

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

danielabrozzoni

ACK fac932b

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

brunoerg

reACK fac932b

achow101 · 2024-07-18T20:19:54Z

ACK fac932b

Github-Pull: bitcoin#30444 Rebased-From: fab54db

…-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 Github-Pull: bitcoin#30444 Rebased-From: fac932b

rest: Reject negative outpoint index in getutxos parsing

fab54db

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

brunoerg approved these changes Jul 12, 2024

View reviewed changes

tdb3 approved these changes Jul 13, 2024

View reviewed changes

This was referenced Jul 13, 2024

fix: Make TxidFromString() respect string_view length #30436

Merged

refactor: Replace ParseHex with consteval ""_hex literals #30377

Merged

hodlinator approved these changes Jul 13, 2024

View reviewed changes

maflcko marked this pull request as draft July 16, 2024 08:03

maflcko marked this pull request as ready for review July 17, 2024 11:45

hodlinator approved these changes Jul 17, 2024

View reviewed changes

DrahtBot requested review from tdb3 and brunoerg July 17, 2024 12:05

tdb3 approved these changes Jul 18, 2024

View reviewed changes

danielabrozzoni reviewed Jul 18, 2024

View reviewed changes

brunoerg approved these changes Jul 18, 2024

View reviewed changes

achow101 merged commit 6144aa2 into bitcoin:master Jul 18, 2024
16 checks passed

maflcko deleted the 2407-rest-index branch July 19, 2024 08:42

hodlinator mentioned this pull request Jul 24, 2024

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

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 1, 2024

rest: Reject negative outpoint index in getutxos parsing

d21ecec

Github-Pull: bitcoin#30444 Rebased-From: fab54db

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maflcko commented Jul 12, 2024

DrahtBot commented Jul 12, 2024 •

edited

Loading

brunoerg commented Jul 12, 2024

brunoerg left a comment

tdb3 left a comment

hodlinator left a comment

hodlinator commented Jul 13, 2024

maflcko commented Jul 17, 2024

hodlinator left a comment

tdb3 left a comment

danielabrozzoni left a comment

brunoerg left a comment

achow101 commented Jul 18, 2024

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

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

Conversation

maflcko commented Jul 12, 2024

DrahtBot commented Jul 12, 2024 • edited Loading

Code Coverage

Reviews

Conflicts

brunoerg commented Jul 12, 2024

brunoerg left a comment

Choose a reason for hiding this comment

tdb3 left a comment

Choose a reason for hiding this comment

hodlinator left a comment

Choose a reason for hiding this comment

hodlinator commented Jul 13, 2024

maflcko commented Jul 17, 2024

hodlinator left a comment

Choose a reason for hiding this comment

tdb3 left a comment

Choose a reason for hiding this comment

danielabrozzoni left a comment

Choose a reason for hiding this comment

brunoerg left a comment

Choose a reason for hiding this comment

achow101 commented Jul 18, 2024

DrahtBot commented Jul 12, 2024 •

edited

Loading