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

muxator commented at 4:26 pm on October 6, 2022: none

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.

0for (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 commented at 4:32 pm on October 6, 2022: member

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 commented at 4:38 pm on October 6, 2022: member

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

maflcko commented at 4:41 pm on October 6, 2022: member

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

instagibbs commented at 4:45 pm on October 6, 2022: member

ah, my mistake, ignore my comments

achow101 commented at 4:53 pm on October 6, 2022: member

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.

rpc: fix crash in deriveaddresses when derivation index is 2147483647

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.

addf9d6502

rpc: add non-regression test about deriveaddresses crash when index is 2147483647

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

9153ff3e27

muxator commented at 8:42 pm on October 6, 2022: none

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 commented at 11:48 am on October 7, 2022: contributor

Concept ACK

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

DrahtBot commented at 12:22 pm on October 7, 2022: contributor

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 commented at 2:38 pm on October 20, 2022: contributor

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.

willcl-ark approved

achow101 commented at 11:05 pm on October 25, 2022: member

ACK 9153ff3e274953ea0d92d53ddab4c72deeace1b1

maflcko merged this on Oct 26, 2022

maflcko closed this on Oct 26, 2022

maflcko added the label RPC/REST/ZMQ on Oct 26, 2022

maflcko added the label Descriptors on Oct 26, 2022

maflcko added the label Needs backport (24.x) on Oct 26, 2022

maflcko added this to the milestone 24.0 on Oct 26, 2022

maflcko commented at 8:17 am on October 26, 2022: member

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

maflcko commented at 9:40 am on October 26, 2022: member

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.

maflcko added the label Needs backport (22.x) on Oct 26, 2022

maflcko added the label Needs backport (23.x) on Oct 26, 2022

muxator deleted the branch on Oct 26, 2022

sidhujag referenced this in commit bb9805936f on Oct 27, 2022

fanquake referenced this in commit bf2bf73bcb on Oct 28, 2022

fanquake referenced this in commit e4b8c9b2bf on Oct 28, 2022

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

fanquake commented at 10:10 am on October 28, 2022: member

Backported to 24.x in #26410.

fanquake referenced this in commit d9f1c89e49 on Oct 28, 2022

fanquake referenced this in commit f8ed34d1a9 on Oct 28, 2022

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

fanquake commented at 10:21 am on October 28, 2022: member

Backported to 23.x in #26411.

fanquake referenced this in commit db20d278e2 on Oct 28, 2022

fanquake referenced this in commit 403de22119 on Oct 28, 2022

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

fanquake commented at 10:55 am on October 28, 2022: member

Backported to 22.x in #26413.

fanquake referenced this in commit 2e8880abc0 on Oct 31, 2022

maflcko referenced this in commit 4ff9be5c33 on Oct 31, 2022

maflcko referenced this in commit 65c2f787c5 on Oct 31, 2022

bitcoin locked this on Oct 28, 2023

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

Conflicts