Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files #20383

practicalswift · 2020-11-13T15:11:37Z

Closes #19278.

Avoid signed integer overflow when loading malformed mempool.dat files.

Avoid invalid integer negation when loading malformed mempool.dat files (or when processing prioritisetransaction RPC calls).

Add note about the valid range of inputs for FormatMoney(...).

Add test.

Before this patch:

$ xxd -p -r > mempool-signed-integer-overflow.dat << "EOF"
01000000000000003f2d3f3f21f800000000000000000000000000000000
6d697464657363656e64616e00000001000000ec000000003d6a6c000000
000000000000ec9bf601000000000000000000ec9b0001000000000001ff
fffef900000001000000ec0000000000ec9b000001000000000101000100
00000001000000ec000000003d6a6a000000000000000020ec9b000000fa
00
EOF
$ cp mempool-signed-integer-overflow.dat ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
…
txmempool.cpp:839:15: runtime error: signed integer overflow: -7211388903327006720 + -7211353718954917888 cannot be represented in type 'long'
…

$ xxd -p -r > mempool-invalid-negation.dat << "EOF"
0100000000000000002e000000005d2d000d020000000000000000000000
200000000000000000000080fc0000002d
EOF
$ cp mempool-invalid-negation.dat ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
…
util/moneystr.cpp:16:34: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount' (aka 'long'); cast to an unsigned type to negate this value to itself
…

After this patch:

$ xxd -p -r > mempool-signed-integer-overflow.dat << "EOF"
01000000000000003f2d3f3f21f800000000000000000000000000000000
6d697464657363656e64616e00000001000000ec000000003d6a6c000000
000000000000ec9bf601000000000000000000ec9b0001000000000001ff
fffef900000001000000ec0000000000ec9b000001000000000101000100
00000001000000ec000000003d6a6a000000000000000020ec9b000000fa
00
EOF
$ cp mempool-signed-integer-overflow.dat ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
…
2020-11-13T12:34:56Z PrioritiseTransaction(...) failed. Invalid fee delta?
…

$ xxd -p -r > mempool-invalid-negation.dat << "EOF"
0100000000000000002e000000005d2d000d020000000000000000000000
200000000000000000000080fc0000002d
EOF
$ cp mempool-invalid-negation.dat ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
…
2020-11-13T12:34:56Z PrioritiseTransaction(...) failed. Invalid fee delta?
…

src/txmempool.cpp

DrahtBot · 2020-11-13T20:11:47Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

bitcoind: Add -daemonwait option to wait for initialization #21007 (bitcoind: Add -daemonwait option to wait for initialization by laanwj)
util: Avoid invalid integer negation in FormatMoney and ValueFromAmount #20406 (util: Avoid invalid integer negation in FormatMoney and ValueFromAmount by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/test/fuzz/util.h

src/rpc/mining.cpp

src/validation.cpp

Crypt-iQ

crACK 2c61f90f047097fe372beb8bc8b4d779c66cd505

Will run my seeds against this patch.

src/validation.cpp

promag

Concept ACK, change looks good to me.

src/validation.cpp

practicalswift · 2020-11-16T07:13:35Z

@promag A fuzz test is added in #19259 ("tests: Add fuzzing harness for LoadMempool(...) and DumpMempool(...)"). Review welcome! :)

maflcko

If you want to eliminate this issue completely, the diff would be quite invasive. So I am not sure if we want this. Maybe a note "pls don't fuzz your rpc interface in production" is good enough?

maflcko · 2021-01-14T09:22:45Z

src/txmempool.cpp

This check isn't complete. You'd have to check all ancestors and descandants as well. Also, I don't understand why this can't be FeeDeltaRange https://github.com/bitcoin/bitcoin/pull/20089/files#r557249710

Implemented an alternative that also checks all ancestors and descendants here: #23418

src/rpc/mining.cpp

practicalswift · 2021-01-15T14:06:19Z

Maybe a note "pls don't fuzz your rpc interface in production" is good enough?

I think we should try to make all our interfaces robust against crazy inputs. That includes our RPC interface :)

Where "robustness" includes being able to handle crazy inputs without triggering ASAN, MSAN, TSAN or UBSAN warnings.

In other words I think we should strive for handling crazy user input (UI robustness), crazy programmatic input (RPC robustness) and crazy network input (P2P robustness) without triggering sanitizer warnings.

Note that even if we didn't care at all about robustness against crazy inputs in our RPC interface it is still makes perfect sense to fuzz the RPC interface since it is the broadest possible general "entry point" in to our application. By fuzzing the RPC interface we can reach large parts of our code base and uncover also non-RPC specific bugs.

maflcko · 2021-01-15T14:16:18Z

Sure, when it is possible to add a simple check if the input is invalid, but here it requires changing the whole mempool package tracking code

felipsoarez

Concept ACK

…action(...)

luke-jr · 2021-02-28T16:56:26Z

src/amount.h

 * */
 static const CAmount MAX_MONEY = 21000000 * COIN;
 inline bool MoneyRange(const CAmount& nValue) { return (nValue >= 0 && nValue <= MAX_MONEY); }
+inline bool FeeDeltaRange(const CAmount& fee_delta) { return -MAX_MONEY <= fee_delta && fee_delta <= MAX_MONEY; }


Fee deltas have no such range limit.

Context: #20089 (comment) + #20383 (comment)

…tion RPC fa07f84 Fix signed integer overflow in prioritisetransaction RPC (MarcoFalke) fa52cf8 refactor: Replace feeDelta by m_modified_fee (MarcoFalke) Pull request description: Signed integer overflow is UB in theory, but not in practice. Still, it would be nice to avoid this UB to allow Bitcoin Core to be compiled with sanitizers such as `-ftrapv` or ubsan. It is impossible to predict when and if an overflow occurs, since the overflow caused by a prioritisetransaction RPC might only be later hit when descendant txs are added to the mempool. Since it is impossible to predict reliably, leave it up to the user to use the RPC endpoint responsibly, considering their mempool limits and usage patterns. Fixes: bitcoin#20626 Fixes: bitcoin#20383 Fixes: bitcoin#19278 Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34146 / https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47132 ## Steps to reproduce Build the code without the changes in this pull. Make sure to pass the sanitizer flag: ``` ./autogen.sh && ./configure --with-sanitizers=signed-integer-overflow && make clean && make -j $(nproc) ``` ### Reproduce on RPC ``` ./src/bitcoind -chain=regtest -noprinttoconsole & ./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789 ./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789 |> txmempool.cpp:920:15: runtime error: signed integer overflow: 9123456789123456789 + 9123456789123456789 cannot be represented in type 'long int' ./src/bitcoin-cli -chain=regtest stop ``` ### By fuzzing ``` wget https://github.com/bitcoin/bitcoin/files/8921302/clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt FUZZ=validation_load_mempool ./src/test/fuzz/fuzz ./clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt |> txmempool.cpp:920:15: runtime error: signed integer overflow: 7214801925397553184 + 2314885530818453536 cannot be represented in type 'long int' |> validation_load_mempool: succeeded against 1 files in 0s. ACKs for top commit: vasild: ACK fa07f84 dunxen: ACK fa07f84 LarryRuane: ACK fa07f84 Tree-SHA512: 4a357950af55a49c9113da0a50c2e743c5b752f0514dd8d16cd92bfde2f77dd0ef56aa98452626df6f7f7a5b51d1227021f6bc94091201a179f0d488ee32a0df

DrahtBot added Build system Mempool Mining RPC/REST/ZMQ Utils/log/libs Validation labels Nov 13, 2020

luke-jr reviewed Nov 13, 2020

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

luke-jr reviewed Nov 13, 2020

View reviewed changes

src/test/fuzz/util.h Outdated Show resolved Hide resolved

luke-jr reviewed Nov 13, 2020

View reviewed changes

src/rpc/mining.cpp Outdated Show resolved Hide resolved

luke-jr reviewed Nov 13, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the signed-integer-malformed-mempool-dat-and-rpc branch 2 times, most recently from 2ea1249 to 2c61f90 Compare November 13, 2020 22:22

This was referenced Nov 14, 2020

Fix UBSan warnings triggered when loading corrupt mempool.dat files #19381

Closed

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

Crypt-iQ reviewed Nov 14, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

promag reviewed Nov 15, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

This was referenced Nov 16, 2020

Invalid integer negation in FormatMoney(CAmount n) and ValueFromAmount (CAmount n) when n = std::numeric_limits<CAmount>::min() #20402

Closed

util: Avoid invalid integer negation in FormatMoney and ValueFromAmount #20406

Merged

DrahtBot mentioned this pull request Nov 26, 2020

Remove obsolete NODISCARD ifdef forest. Use [[nodiscard]] (C++17). #20499

Merged

DrahtBot added the Needs rebase label Nov 30, 2020

practicalswift force-pushed the signed-integer-malformed-mempool-dat-and-rpc branch from 2c61f90 to 1977f9d Compare November 30, 2020 21:58

DrahtBot removed the Needs rebase label Nov 30, 2020

maflcko reviewed Jan 14, 2021

View reviewed changes

maflcko mentioned this pull request Jan 14, 2021

Signed integer overflow in CTxMemPool::PrioritiseTransaction(…) reachable via RPC call prioritisetransaction #20626

Closed

practicalswift force-pushed the signed-integer-malformed-mempool-dat-and-rpc branch from 1977f9d to 7dddffb Compare January 14, 2021 16:33

felipsoarez reviewed Jan 15, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 16, 2021

rpc: Return total fee in getmempoolinfo #20944

Merged

This was referenced Jan 27, 2021

bitcoind: Add -daemonwait option to wait for initialization #21007

Merged

rpc/validation: enable packages through testmempoolaccept #20833

Merged

DrahtBot added the Needs rebase label Feb 8, 2021

practicalswift added 5 commits February 11, 2021 08:43

util: Move AdditionOverflow(...) to new file src/util/overflow.h

bbd7918

mempool: Avoid signed integer overflow in CTxMemPool::PrioritiseTrans…

c425848

…action(...)

util: Add note about the valid range of inputs for FormatMoney(...)

7df8dbf

tests: Test invalid fee delta check

1db76e1

mempool: Avoid signed integer overflow in CTxMemPool::PrioritiseTrans…

7244d85

…action(...)

practicalswift force-pushed the signed-integer-malformed-mempool-dat-and-rpc branch from 7dddffb to 7244d85 Compare February 11, 2021 08:44

DrahtBot removed the Needs rebase label Feb 11, 2021

DrahtBot mentioned this pull request Feb 17, 2021

refactor: Make CWalletTx sync state type-safe #21206

Merged

luke-jr reviewed Feb 28, 2021

View reviewed changes

practicalswift closed this Feb 28, 2021

practicalswift deleted the signed-integer-malformed-mempool-dat-and-rpc branch April 10, 2021 19:44

maflcko mentioned this pull request Nov 2, 2021

Fix signed integer overflow in prioritisetransaction RPC #23418

Merged

bitcoin locked and limited conversation to collaborators Nov 2, 2022

Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files #20383

Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files #20383

Uh oh!

Conversation

practicalswift commented Nov 13, 2020

Uh oh!

Uh oh!

DrahtBot commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Crypt-iQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Nov 16, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 15, 2021

Uh oh!

felipsoarez left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

practicalswift Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Nov 13, 2020 •

edited

Loading

practicalswift commented Jan 15, 2021 •

edited

Loading