Potential PSBT issues found by the PSBT fuzzer #17149

issue practicalswift openend this issue on October 15, 2019
  1. practicalswift commented at 10:37 am on October 15, 2019: contributor

    Potential PSBT issues found by the PSBT fuzzer (#17136):

     0$ src/test/fuzz/psbt
 1…
 2==8112==ERROR: AddressSanitizer: heap-use-after-free on address 0x6070001083f0 at pc 0x55a5d650f41b bp 0x7ffe03cc3c90 sp 0x7ffe03cc3c88
 3READ of size 8 at 0x6070001083f0 thread T0
 4    [#0](/bitcoin-bitcoin/0/) 0x55a5d650f41a in CTxOut::operator=(CTxOut const&) src/./primitives/transaction.h:133:7
 5    [#1](/bitcoin-bitcoin/1/) 0x55a5d8362e8d in PartiallySignedTransaction::GetInputUTXO(CTxOut&, int) const src/psbt.cpp:72:14
 6    [#2](/bitcoin-bitcoin/2/) 0x55a5d6dae419 in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:32:19
 7    [#3](/bitcoin-bitcoin/3/) 0x55a5d64b27d7 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
 8…
 9
10$ src/test/fuzz/psbt
11…
12AddressSanitizer:DEADLYSIGNAL
13=================================================================
14==22238==ERROR: AddressSanitizer: SEGV on unknown address 0x6098001079e0 (pc 0x55880d55c50f bp 0x7fff8d03d8f0 sp 0x7fff8d03d860 T0)
15==22238==The signal is caused by a READ memory access.
16    [#0](/bitcoin-bitcoin/0/) 0x55880d55c50e in CTxOut::operator=(CTxOut const&) src/./primitives/transaction.h:133:7
17    [#1](/bitcoin-bitcoin/1/) 0x55880f3b6a83 in SignPSBTInput(SigningProvider const&, PartiallySignedTransaction&, int, int, SignatureData*, bool) src/psbt.cpp:262:14
18    [#2](/bitcoin-bitcoin/2/) 0x55880f3b81ff in FinalizePSBT(PartiallySignedTransaction&) src/psbt.cpp:311:21
19    [#3](/bitcoin-bitcoin/3/) 0x55880d500ee9 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:62:11
20…
21
22$ src/test/fuzz/psbt
23…
24node/psbt.cpp:82:26: runtime error: signed integer overflow: 7857790368227518552 + 6582955728264977243 cannot be represented in type 'long'
25    [#0](/bitcoin-bitcoin/0/) 0x55a8b66b13ee in AnalyzePSBT(PartiallySignedTransaction)::$_0::operator()(long, CTxOut const&) const src/node/psbt.cpp:82:26
26    [#1](/bitcoin-bitcoin/1/) 0x55a8b66b1043 in long std::accumulate<__gnu_cxx::__normal_iterator<CTxOut*, std::vector<CTxOut, std::allocator<CTxOut> > >, long, AnalyzePSBT(PartiallySignedTransaction)::$_0>(__gnu_cxx::__normal_iterator<CTxOut*, std::vector<CTxOut, std::allocator<CTxOut> > >, __gnu_cxx::__normal_iterator<CTxOut*, std::vector<CTxOut, std::allocator<CTxOut> > >, long, AnalyzePSBT(PartiallySignedTransaction)::$_0) /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_numeric.h:154:11
27    [#2](/bitcoin-bitcoin/2/) 0x55a8b66aeecf in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:80:27
28    [#3](/bitcoin-bitcoin/3/) 0x55a8b5db1a17 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
29…
30
31$ src/test/fuzz/psbt
32…
33policy/feerate.cpp:18:34: runtime error: signed integer overflow: -7497025602362688435 * 1000 cannot be represented in type 'long'
34    [#0](/bitcoin-bitcoin/0/) 0x5587f480b863 in CFeeRate::CFeeRate(long const&, unsigned long) src/policy/feerate.cpp:18:34
35    [#1](/bitcoin-bitcoin/1/) 0x5587f327051f in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:116:22
36    [#2](/bitcoin-bitcoin/2/) 0x5587f2971ac7 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
37…
  2. practicalswift added the label Bug on Oct 15, 2019
  3. fanquake commented at 1:16 pm on October 15, 2019: member
    cc @achow101.
  4. MarcoFalke commented at 4:09 pm on October 15, 2019: member
    Would be nice to provide the seeds or a unit test, maybe even a fix?
  5. MarcoFalke added the label PSBT on Oct 15, 2019
  6. practicalswift commented at 11:14 pm on October 15, 2019: contributor

    @MarcoFalke Tentative fix for the two first issues was submitted in the linked PR #17136 (comment) before opening this issue.

    The only seed necessary to quickly reach these code paths is found in the PSBT unit test:

    https://github.com/bitcoin/bitcoin/blob/eb292af309aa57f3d7998b01307dd4cb91702908/src/wallet/test/psbt_wallet_tests.cpp#L65-L69

  7. practicalswift commented at 9:23 am on November 4, 2019: contributor
    It should be noted that these code paths are reachable from the RPC code AFAICT which means that an RPC user could deliberately trigger these conditions. Would be nice to get the fix merged :)
  8. practicalswift commented at 2:45 pm on November 8, 2019: contributor

    I found another PSBT issue that probably needs addressing.

    An assertion failure is reachable if a carefully constructed transaction is passed to AnalyzePSBT. I haven’t verified but this should be reachable via RPC.

    0psbt: consensus/tx_verify.cpp:130: unsigned int GetP2SHSigOpCount(const CTransaction &, 
1  const CCoinsViewCache &): Assertion `!coin.IsSpent()' failed.
2…
3    [#9](/bitcoin-bitcoin/9/) 0x5647d7830746 in GetP2SHSigOpCount(CTransaction const&, CCoinsViewCache const&) src/consensus/tx_verify.cpp:130:9
4    [#10](/bitcoin-bitcoin/10/) 0x5647d783087b in GetTransactionSigOpCost(CTransaction const&, CCoinsViewCache const&, int) src/consensus/tx_verify.cpp:146:20
5    [#11](/bitcoin-bitcoin/11/) 0x5647d6e719e1 in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:125:58

    The line numbers can be off slightly due to local modifications.

    Let me know if a minimised test case is needed and I’ll create that.

    Friendly ping @achow101 :)

  9. achow101 commented at 4:03 pm on November 8, 2019: member
    I don’t think that assertion is reachable. The coins are not fetched from the actual UTXO set. Rather they are created from the PSBT itself. At no point are the marked as spent.
  10. practicalswift commented at 3:01 pm on November 10, 2019: contributor

    @achow101 Yes, but you’re forgetting that coinEmpty.IsSpent() == true :)

    Note that the attacker controlled PartiallySignedTransaction psbtx can be created with psbtx.tx such that inputs.AccessCoin(tx.vin[i].prevout) returns the coinEmpty in CCoinsViewCache::AccessCoin . Note that coinEmpty.IsSpent() holds true which means that the assertion fails :)

    Proof of concept:

    0$ bitcoind &
1$ bitcoin-cli analyzepsbt "cHNidP8BAJoCAAAAAljoeiG1ba8MI76OcHBFbDNvfLqlyHV5JPVFiHuyq911AAAAAAD/////g40EJ9DsZQpoqka7CwmK6kQiwHGyyng1Kgd5WdB86h0BAAAAAP////8CcKrwCAAAAAAWAEHYXCtx0AYLCcmIauuBXlCZHdoSTQDh9QUAAAAAFv8/wADXYP/7//////8JxOh0LR2HAI8AAAAAAAEBIADC6wsAAAAAF2oUt/X69ELjeX2nTof+fZ10l+OyAokDAQcJAwEHEAABAACAAAEBIADC6wsAAAAAF2oUt/X69ELjeX2nTof+fZ10l+OyAokDAQcJAwEHENkMak8AAAAA"
2bitcoind: consensus/tx_verify.cpp:130: unsigned int 
3    GetP2SHSigOpCount(const CTransaction &, const CCoinsViewCache &): 
4    Assertion `!coin.IsSpent()' failed.

    Code:

    https://github.com/bitcoin/bitcoin/blob/73b26e38d7a174d5409dda8aa1fe9804e7779a1f/src/rpc/rawtransaction.cpp#L1683-L1690

    https://github.com/bitcoin/bitcoin/blob/3c40bc6726b6dc639c4ca2c00c720bccd4cd4dc7/src/consensus/tx_verify.cpp#L129-L130

    https://github.com/bitcoin/bitcoin/blob/848f245d04b604abcae84dcb9f0f6078325d418d/src/coins.cpp#L116-L125

  11. practicalswift commented at 10:14 am on November 19, 2019: contributor
    @achow101 Can you confirm the last issue reported? :)
  12. Sjors commented at 10:45 am on November 19, 2019: member
    I managed to reproduce that last issue.
  13. achow101 commented at 8:20 pm on November 19, 2019: member
    @practicalswift #17524 fixes it.
  14. practicalswift commented at 10:39 am on November 20, 2019: contributor
    @Sjors @achow101 PSBT experts: can you think of scenarios where untrusted PSBT can end up being processed by Bitcoin Core? I’m trying to gauge the potential impact of the issues described in this issue. Can we think of a scenario where any of these could be a vulnerability?
  15. Sjors commented at 10:51 am on November 20, 2019: member
    @practicalswift if that were the case, email would be a better place to discuss.
  16. fanquake closed this on Jan 29, 2020
  17. MarcoFalke commented at 6:44 pm on January 29, 2020: member
    @practicalswift Now that the issue is fixed, it would be nice if you could submit the seeds to our repo: https://github.com/bitcoin-core/qa-assets
  18. practicalswift commented at 11:21 pm on January 29, 2020: contributor
    @MarcoFalke Done! :)
  19. sidhujag referenced this in commit ef5e467032 on Feb 1, 2020
  20. sidhujag referenced this in commit d1e8291434 on Nov 10, 2020
  21. DrahtBot locked this on Feb 15, 2022
  22. knst referenced this in commit 89cb2be7e2 on Jan 3, 2023
  23. knst referenced this in commit d6419c1a68 on Jan 23, 2023
  24. knst referenced this in commit 1f9ddff17f on Jan 24, 2023
  25. knst referenced this in commit 92bdb9aa41 on Jan 25, 2023
  26. knst referenced this in commit e5d1646529 on Jan 30, 2023
  27. knst referenced this in commit c013fbbc79 on Jan 31, 2023
  28. PastaPastaPasta referenced this in commit 63ed912c73 on Feb 4, 2023


practicalswift fanquake MarcoFalke achow101 Sjors

Labels
Bug PSBT

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-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me