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

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2407-rest-index changing 2 files +12 −6
  1. 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 -.
  2. rest: Reject negative outpoint index in getutxos parsing fab54db9f1
  3. 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.

  4. DrahtBot added the label RPC/REST/ZMQ on Jul 12, 2024
  5. brunoerg commented at 6:06 pm on July 12, 2024: contributor
    Concept ACK
  6. brunoerg approved
  7. brunoerg commented at 9:32 pm on July 12, 2024: contributor
    utACK fab54db9f1d0e634f4a697480dbb87b87940dc5c
  8. tdb3 approved
  9. 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
  10. hodlinator approved
  11. 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);
  12. 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: {
  13. maflcko marked this as a draft on Jul 16, 2024
  14. 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
  15. maflcko marked this as ready for review on Jul 17, 2024
  16. 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.
  17. hodlinator approved
  18. 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:

  19. DrahtBot requested review from tdb3 on Jul 17, 2024
  20. DrahtBot requested review from brunoerg on Jul 17, 2024
  21. tdb3 approved
  22. 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
  23. 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.

  24. brunoerg approved
  25. brunoerg commented at 2:59 pm on July 18, 2024: contributor
    reACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1
  26. achow101 commented at 8:19 pm on July 18, 2024: member
    ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1
  27. achow101 merged this on Jul 18, 2024
  28. achow101 closed this on Jul 18, 2024
  29. maflcko deleted the branch on Jul 19, 2024


maflcko DrahtBot brunoerg tdb3 hodlinator danielabrozzoni achow101

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me