Fix crash on deriveaddresses when index is 2147483647 (2^31-1) #26275

muxator · 2022-10-06T16:26:01Z

This PR is a proposal for fixing #26274 (better described there).

The problem is due to a signed int wrapping when the index parameter of the deriveaddresses RPC call has the value 2^31-1.

for (int i = range_begin; i <= range_end; ++i) {

the first commit adds a "temporary" test case (test/functional/rpc_deriveaddresses_crash.py) that shows the crash, and can be used to generate a core dump;
the second commit fixes the problem giving an explicit size to the i variable in a for loop, from int to int64_t. The same commit also removes the ephemeral test case and adds a passing test to test/functional/rpc_deriveaddresses.py, in order to prevent future regressions.

This is my first submission to this project and I do not know its conventions. Please advise if something needs to be changed.

instagibbs · 2022-10-06T16:32:52Z

Initial NACK on approach, as I believe that's not BIP32 compatible?

e.g. https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#private-parent-key--private-child-key

"ser32(i)" is implicitly stating that i must be 32 bits long maximum. Throwing an RPC error on detection of potential overflow seems best.

instagibbs · 2022-10-06T16:38:29Z

@achow101 should descriptors barf when asked to Expand something invalid?

maflcko · 2022-10-06T16:41:52Z

I don't think this changes behaviour. It only fixes a sanitizer warning

instagibbs · 2022-10-06T16:45:57Z

ah, my mistake, ignore my comments

achow101 · 2022-10-06T16:53:13Z

Concept ACK

Your first commit introduces a test file that ends up being removed in the second commit. I think it is better to just have the fix in a first commit, and the test in a second. Reviewers can then cherry-pick the test commit onto master to verify that it fails without the first commit. We generally don't want to be introducing stuff in a commit that is going to be removed immediately afterwards. It makes searching through history harder.

2147483647 is the maximum positive value of a signed int32, and - currently - the maximum value that the deriveaddresses bitcoin RPC call accepts as derivation index due to its input validation routines. Before this change, when the derivation index (and thus range_end) reached std::numeric_limits<int_32_t>::max(), the "i" variable in the for cycle (which is declared as int, and as such 32 bits in size on most platforms) would be incremented at the end of the first iteration and then warp back to -2147483648. This caused SIGABRT in bitcoind and a core dump. This change assigns "i" an explicit size of 64 bits on every platform, sidestepping the problem. Fixes #26274.

…s 2147483647 This test would cause a crash in bitcoind (see #26274) if the fix given in the previous commit was not applied.

muxator · 2022-10-06T20:42:07Z

I think it is better to just have the fix in a first commit, and the test in a second.

@achow101, I've rewritten the history organizing it as you proposed, thanks for the advice.

yancyribbens · 2022-10-07T11:48:12Z

Concept ACK

i should be the same size data type as range begin and range end to prevent an overflow.

DrahtBot · 2022-10-07T12:22:04Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

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.

willcl-ark · 2022-10-20T14:38:22Z

tACK both commits cherry-picked onto v24.0rc2.

I was able to reproduce the crash before the patch with ./src/bitcoin-cli deriveaddresses "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)#cjjspncu" "[2147483647, 2147483647]".

Afterwards this call succeeded deriving address bc1qpmmpntm7hjmfulr8tuqtyzv9s5ahn9lk857qy4.

achow101 · 2022-10-25T23:05:29Z

ACK 9153ff3

maflcko · 2022-10-26T08:17:05Z

Marked for backport, so that it is easier to run previous branches with sanitizers enabled

maflcko · 2022-10-26T09:40:53Z

Actually it is unclear whether this is a behaviour change. It is UB and my compilers happened to optimize the bug away, but there is no guarantee that other compilers or compiler options will do so as well.

…83647 (2^31-1) 9153ff3 rpc: add non-regression test about deriveaddresses crash when index is 2147483647 (muxator) addf9d6 rpc: fix crash in deriveaddresses when derivation index is 2147483647 (muxator) Pull request description: This PR is a proposal for fixing bitcoin#26274 (better described there). The problem is due to a signed int wrapping when the `index` parameter of the `deriveaddresses` RPC call has the value `2^31-1`. ```C++ for (int i = range_begin; i <= range_end; ++i) { ``` * the first commit adds a "temporary" test case (`test/functional/rpc_deriveaddresses_crash.py`) that shows the crash, and can be used to generate a core dump; * the second commit fixes the problem giving an explicit size to the `i` variable in a for loop, from `int` to `int64_t`. The same commit also removes the ephemeral test case and adds a passing test to `test/functional/rpc_deriveaddresses.py`, in order to prevent future regressions. This is my first submission to this project and I do not know its conventions. Please advise if something needs to be changed. ACKs for top commit: achow101: ACK 9153ff3 Tree-SHA512: 0477b57b15dc2c682cf539d6002f100d44a8c7e668041aa3340c39dcdbd40e083c75dec6896b6c076b044a01c2e5254272ae6696d8a1467539391926f270940a

2147483647 is the maximum positive value of a signed int32, and - currently - the maximum value that the deriveaddresses bitcoin RPC call accepts as derivation index due to its input validation routines. Before this change, when the derivation index (and thus range_end) reached std::numeric_limits<int_32_t>::max(), the "i" variable in the for cycle (which is declared as int, and as such 32 bits in size on most platforms) would be incremented at the end of the first iteration and then warp back to -2147483648. This caused SIGABRT in bitcoind and a core dump. This change assigns "i" an explicit size of 64 bits on every platform, sidestepping the problem. Fixes bitcoin#26274. Github-Pull: bitcoin#26275 Rebased-From: addf9d6

…s 2147483647 This test would cause a crash in bitcoind (see bitcoin#26274) if the fix given in the previous commit was not applied. Github-Pull: bitcoin#26275 Rebased-From: 9153ff3

fanquake · 2022-10-28T10:10:28Z

Backported to 24.x in #26410.

2147483647 is the maximum positive value of a signed int32, and - currently - the maximum value that the deriveaddresses bitcoin RPC call accepts as derivation index due to its input validation routines. Before this change, when the derivation index (and thus range_end) reached std::numeric_limits<int_32_t>::max(), the "i" variable in the for cycle (which is declared as int, and as such 32 bits in size on most platforms) would be incremented at the end of the first iteration and then warp back to -2147483648. This caused SIGABRT in bitcoind and a core dump. This change assigns "i" an explicit size of 64 bits on every platform, sidestepping the problem. Fixes bitcoin#26274. Github-Pull: bitcoin#26275 Rebased-From: addf9d6

…s 2147483647 This test would cause a crash in bitcoind (see bitcoin#26274) if the fix given in the previous commit was not applied. Github-Pull: bitcoin#26275 Rebased-From: 9153ff3

fanquake · 2022-10-28T10:21:53Z

Backported to 23.x in #26411.

2147483647 is the maximum positive value of a signed int32, and - currently - the maximum value that the deriveaddresses bitcoin RPC call accepts as derivation index due to its input validation routines. Before this change, when the derivation index (and thus range_end) reached std::numeric_limits<int_32_t>::max(), the "i" variable in the for cycle (which is declared as int, and as such 32 bits in size on most platforms) would be incremented at the end of the first iteration and then warp back to -2147483648. This caused SIGABRT in bitcoind and a core dump. This change assigns "i" an explicit size of 64 bits on every platform, sidestepping the problem. Fixes bitcoin#26274. Github-Pull: bitcoin#26275 Rebased-From: addf9d6

…s 2147483647 This test would cause a crash in bitcoind (see bitcoin#26274) if the fix given in the previous commit was not applied. Github-Pull: bitcoin#26275 Rebased-From: 9153ff3

fanquake · 2022-10-28T10:55:01Z

Backported to 22.x in #26413.

d570190 rpc: make `address` field optional (w0xlt) e4b8c9b rpc: add non-regression test about deriveaddresses crash when index is 2147483647 (muxator) bf2bf73 rpc: fix crash in deriveaddresses when derivation index is 2147483647 (muxator) b04f5f9 test: Test for out of bounds vout in sendall (Andrew Chow) dedee6a wallet: Check utxo prevout index out of bounds in sendall (Andrew Chow) 931db78 test: Test that sendall works with watchonly spending specific utxos (Andrew Chow) bbe864a wallet: Correctly check ismine for sendall (Andrew Chow) 4b7d30d Adjust `.tx/config` for new Transifex CLI (Hennadii Stepanov) Pull request description: Backports: * #26321 * #26344 * #26275 * #26349 ACKs for top commit: instagibbs: ACK d570190 hebasto: ACK d570190, I've cherry-picked commits manually and got zero diff with this PR branch. Tree-SHA512: dad64f4074b4f06d666c0f2d804eda92df241bcce0a49c28486311a151f2e9d46b75e1bce02de570dcc85957c9ce936debb2a4faa045800c9757c6c495115d7c

403de22 rpc: add non-regression test about deriveaddresses crash when index is 2147483647 (muxator) db20d27 rpc: fix crash in deriveaddresses when derivation index is 2147483647 (muxator) d174db0 Adjust `.tx/config` for new Transifex CLI (Hennadii Stepanov) Pull request description: Currently backports: * #26275 * #26321 Will leave open to collect backports for 22.1, ACKs for top commit: MarcoFalke: cherry-pick ACK 403de22 🏔 Tree-SHA512: f9095a5cad52ecb9580fcaf173a05148dce382ac773a6116e2aed47009402bdfa6cbce62e488fe96120f7a0a81a623eb3e0e4539fa88670adb8c14cf5e334fa5

f8ed34d rpc: add non-regression test about deriveaddresses crash when index is 2147483647 (muxator) d9f1c89 rpc: fix crash in deriveaddresses when derivation index is 2147483647 (muxator) bbea830 Adjust `.tx/config` for new Transifex CLI (Hennadii Stepanov) Pull request description: Backports: * #26275 * #26321 Will leave open to collect remaining backports before a 23.1. ACKs for top commit: MarcoFalke: cherry-pick ACK f8ed34d 🚝 Tree-SHA512: 2a96f66b0d81457a7017b0f4b041a7742008374d00a22d828502bfe170bfebb413e4e63382b10b9e2e250cb9e1be1a41030b8b6c5be42ffb23964808b12ee343

muxator mentioned this pull request Oct 6, 2022

bitcoind dumps core when deriveaddresses is called with index 2147483647 (2^31-1) #26274

Closed

muxator added 2 commits October 6, 2022 22:17

rpc: add non-regression test about deriveaddresses crash when index i…

9153ff3

…s 2147483647 This test would cause a crash in bitcoind (see #26274) if the fix given in the previous commit was not applied.

DrahtBot mentioned this pull request Oct 12, 2022

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Merged

willcl-ark approved these changes Oct 20, 2022

View reviewed changes

maflcko merged commit cf28837 into bitcoin:master Oct 26, 2022

maflcko added RPC/REST/ZMQ Descriptors Needs backport (24.x) labels Oct 26, 2022

maflcko added this to the 24.0 milestone Oct 26, 2022

maflcko added Needs backport (22.x) labels Oct 26, 2022

muxator deleted the fix-deriveaddresses-crash branch October 26, 2022 12:47

fanquake mentioned this pull request Oct 28, 2022

[24.x] rc3 backports #26410

Merged

fanquake removed the Needs backport (24.x) label Oct 28, 2022

fanquake mentioned this pull request Oct 28, 2022

[23.1] Backports #26411

Merged

fanquake removed the Needs backport (23.x) label Oct 28, 2022

fanquake mentioned this pull request Oct 28, 2022

[22.1] Backports #26413

Merged

fanquake removed the Needs backport (22.x) label Oct 28, 2022

bitcoin locked and limited conversation to collaborators Oct 28, 2023

Fix crash on deriveaddresses when index is 2147483647 (2^31-1) #26275

Fix crash on deriveaddresses when index is 2147483647 (2^31-1) #26275

Uh oh!

Conversation

muxator commented Oct 6, 2022

Uh oh!

instagibbs commented Oct 6, 2022

Uh oh!

instagibbs commented Oct 6, 2022

Uh oh!

maflcko commented Oct 6, 2022

Uh oh!

instagibbs commented Oct 6, 2022

Uh oh!

achow101 commented Oct 6, 2022

Uh oh!

muxator commented Oct 6, 2022

Uh oh!

yancyribbens commented Oct 7, 2022

Uh oh!

DrahtBot commented Oct 7, 2022

Conflicts

Uh oh!

willcl-ark commented Oct 20, 2022

Uh oh!

achow101 commented Oct 25, 2022

Uh oh!

maflcko commented Oct 26, 2022

Uh oh!

maflcko commented Oct 26, 2022

Uh oh!

fanquake commented Oct 28, 2022

Uh oh!

fanquake commented Oct 28, 2022

Uh oh!

fanquake commented Oct 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants