Reject invalid coin height and output index when loading assumeutxo #22146

1. 
   MarcoFalke commented at 7:30 AM on June 4, 2021: member

   It should be impossible to have a coin at a height higher than the height of the snapshot block, so reject those early to avoid integer wraparounds and hash collisions later on.

   Same for the outpoint index.

   Both issues were found by fuzzing:

   • The height issue by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793
   • The outpoint issue by my fuzz server: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793#c2
2. DrahtBot added the label Validation on Jun 4, 2021
3. 
   practicalswift commented at 8:15 PM on June 5, 2021: contributor

   Concept ACK

   Feels great to see OSS-Fuzz find potential robustness issues in new code long long before any end-users risk hitting them.

4. 
   theStack commented at 12:27 PM on June 9, 2021: member

   Concept ACK

5. MarcoFalke force-pushed on Jun 9, 2021
6. MarcoFalke marked this as a draft on Jun 9, 2021
7. Reject invalid coin height and output index when loading assumeutxo fa9ebedec3
8. MarcoFalke force-pushed on Jun 9, 2021
9. MarcoFalke marked this as ready for review on Jun 9, 2021
10. 
    practicalswift commented at 7:53 PM on June 13, 2021: contributor

    cr ACK fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4: patch looks correct

11. theStack approved
12. 
    theStack commented at 3:09 PM on June 27, 2021: member

    Code review ACK fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4

13. MarcoFalke requested review from jamesob on Jun 27, 2021
14. in src/validation.cpp:4993 in fa9ebedec3

    4988 | @@ -4989,6 +4989,14 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
    4989 |                        coins_count - coins_left);
    4990 |              return false;
    4991 |          }
    4992 | +        if (coin.nHeight > base_height ||
    4993 | +            outpoint.n >= std::numeric_limits<decltype(outpoint.n)>::max() // Avoid integer wrap-around in coinstats.cpp:ApplyHash
    

    ---

    ---

    

    fanquake commented at 3:33 AM on June 28, 2021:

    nit: #include <limits>

    ---

    

    MarcoFalke commented at 6:55 AM on June 28, 2021:

    The file is already using std::numeric_limits, so it seem cleanup that can be done independent of this change. In fact there a quite a few other files which are missing the include, so I am leaving this for another pull:

    -src/bench/nanobench.h
    -src/bitcoin-util.cpp
    -src/blockencodings.cpp
    -src/blockencodings.h
    -src/bloom.cpp
    -src/chain.cpp
    -src/chainparams.cpp
    -src/fs.cpp
    -src/init.cpp
    -src/leveldb/util/env_windows.cc
    -src/miner.cpp
    -src/net_processing.cpp
    -src/policy/rbf.cpp
    -src/primitives/transaction.h
    -src/qt/intro.cpp
    -src/qt/optionsdialog.cpp
    -src/rpc/blockchain.cpp
    -src/rpc/mining.cpp
    -src/script/interpreter.cpp
    -src/support/lockedpool.cpp
    -src/test/fuzz/bloom_filter.cpp
    -src/test/fuzz/pow.cpp
    -src/test/fuzz/rolling_bloom_filter.cpp
    -src/test/fuzz/tx_pool.cpp
    -src/test/fuzz/util.cpp
    -src/test/fuzz/util.h
    -src/test/random_tests.cpp
    -src/test/rpc_tests.cpp
    -src/test/scriptnum_tests.cpp
    -src/test/serialize_tests.cpp
    -src/test/sighash_tests.cpp
    -src/test/skiplist_tests.cpp
    -src/test/util_tests.cpp
    -src/test/versionbits_tests.cpp
    -src/txmempool.cpp
    -src/txrequest.cpp
    -src/util/system.h
    -src/validation.cpp
    -src/wallet/interfaces.cpp
    -src/wallet/rpcwallet.cpp
    -src/wallet/scriptpubkeyman.cpp
    -src/wallet/spend.cpp
    -src/wallet/wallet.cpp
    

15. benthecarman approved
16. 
    benthecarman commented at 7:08 AM on June 28, 2021: contributor

    crACK fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4

17. 
    jamesob commented at 1:55 PM on June 28, 2021: member

    crACK https://github.com/bitcoin/bitcoin/pull/22146/commits/fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4

    Looks good; thanks for finding and fixing @MarcoFalke.

18. MarcoFalke merged this on Jun 28, 2021
19. MarcoFalke closed this on Jun 28, 2021

    ---

20. MarcoFalke deleted the branch on Jun 28, 2021
21. sidhujag referenced this in commit c1fad57263 on Jun 28, 2021
22. gwillen referenced this in commit 9fc1ba6008 on Jun 1, 2022
23. DrahtBot locked this on Aug 18, 2022

Contributors

MarcoFalkepracticalswifttheStackfanquakebenthecarmanjamesob

Review Requested
jamesob