Signed integer overflow in CTxMemPool::PrioritiseTransaction(…) reachable via RPC call prioritisetransaction #20626
Comments
|
Huh, I haven't been able to reproduce this. Tried with the build steps you list, but didn't see any errors: $ src/bitcoin-cli prioritisetransaction cafebabecafebabecafebabecafebabecafebabecafebabecafebabecafebabe 0 -9123456789123456789
true
$ src/bitcoin-cli prioritisetransaction cafebabecafebabecafebabecafebabecafebabecafebabecafebabecafebabe 0 -9123456789123456789
trueAlso, should I be able to write a failing functional test? I've tried a few different things with no errors. ie: txid = wallet.send_self_transfer(from_node=self.nodes[0])['txid']
self.nodes[0].prioritisetransaction(txid=txid, fee_delta=-9123456789123456789)
self.nodes[0].prioritisetransaction(txid=txid, fee_delta=-9123456789123456789)or: txid = wallet.send_self_transfer(from_node=self.nodes[0])['txid']
self.nodes[0].prioritisetransaction(txid='cafebabecafebabecafebabecafebabecafebabecafebabecafebabecafebabe', fee_delta=-9123456789123456789)
self.nodes[0].prioritisetransaction(txid='cafebabecafebabecafebabecafebabecafebabecafebabecafebabecafebabe', fee_delta=-9123456789123456789)
|
|
@mjdietzx Oh, that's weird! I just re-tried against current |
|
Reproduced on current master @ 7b97563 debug log output
|
|
Fixed in #20383 |
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this issue
Apr 5, 2021
…ssion with function level suppression 585854a test: Replace blanket UBSan signed integer overflow suppression for txmempool.cpp with specific suppression (practicalswift) Pull request description: Replace file level (`txmempool.cpp`) signed integer overflow suppression with function level suppression (`CTxMemPool::PrioritiseTransaction`). The suppression was added yesterday in bitcoin#21586. Rationale: To avoid risk hiding other signed integer overflows in `txmempool.cpp`. Obviously it would be better if this signed integer overflow fixed instead of suppressed - see details bitcoin#20626. Any taker? :) To hit the issue via fuzzing: ``` $ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=validation_load_mempool src/test/fuzz/fuzz INFO: Seed: 1184244493 INFO: Loaded 1 modules (634418 inline 8-bit counters): 634418 [0x55a09fdfbf98, 0x55a09fe96dca), INFO: Loaded 1 PC tables (634418 PCs): 634418 [0x55a09fe96dd0,0x55a0a08450f0), INFO: 1264 files found in mempool/ INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1040698 bytes INFO: seed corpus: files: 1264 min: 1b max: 1040698b total: 15997133b rss: 197Mb txmempool.cpp:847:15: runtime error: signed integer overflow: -7211388903327006720 + -7211353718954917888 cannot be represented in type 'long' #0 0x55a09c3ce2d8 in CTxMemPool::PrioritiseTransaction(uint256 const&, long const&) /home/thomas/bitcoin/src/txmempool.cpp:847:15 ``` ACKs for top commit: JeremyRubin: utACK 585854a hebasto: ACK 585854a, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 5a343f028c1e1a1aba3b51a0eced605849184891ffafecb3cd2b424c6cfea01afd7c2672274936b0bac646075ec066408a570bf6b34bc9b87399a53ce20d8a23
|
Looks like fuzzing with |
maflcko
pushed a commit
that referenced
this issue
Jun 27, 2022
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: #20626 Fixes: #20383 Fixes: #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
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this issue
Jun 27, 2022
…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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
and others
While fuzzing the RPC interface I stumbled upon a signed integer overflow in
CTxMemPool::PrioritiseTransaction(…)which is reachable via the following twoprioritisetransactionRPC calls:Nothing high priority of course, but still worth fixing :)
The text was updated successfully, but these errors were encountered: