descriptor: check whitespace in pubkeys within fragments

brunoerg commented at 2:27 pm on January 4, 2025: contributor

Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public key within a fragment (e.g. pk( KEY), pk(KEY ) or pk( KEY )). I have noticed that one of the reasons is that the DecodeBase58 function simply ignore these whitespaces.

This PR changes the ParsePubkeyInner to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. importdescriptors), but an already imported descriptor won’t be affected by this check, especially because we store descriptors from ToString.

For context: https://github.com/brunoerg/bitcoinfuzz/issues/72

DrahtBot commented at 2:27 pm on January 4, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31603.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

brunoerg force-pushed on Jan 4, 2025

DrahtBot added the label CI failed on Jan 4, 2025

DrahtBot commented at 2:32 pm on January 4, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35146721643

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Jan 4, 2025

furszy commented at 3:57 pm on January 4, 2025: member

As you are forbidding something that was previously allowed, this breaks compatibility with earlier versions. Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly. Will either need a descriptor upgrade procedure or a compatibility + warning window.

brunoerg marked this as a draft on Jan 6, 2025

brunoerg commented at 4:02 pm on January 7, 2025: contributor

As you are forbidding something that was previously allowed, this breaks compatibility with earlier versions. Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly. Will either need a descriptor upgrade procedure or a compatibility + warning window.

Good point. Moved it to draft to work on these things.

brunoerg force-pushed on Jan 9, 2025

brunoerg renamed this:
~~descriptor: check whitespace in `ParsePubkeyInner`~~
descriptor: check whitespace in pubkeys within fragments
on Jan 9, 2025

brunoerg marked this as ready for review on Jan 9, 2025

brunoerg commented at 5:30 pm on January 9, 2025: contributor

Ready for review!

Thanks, @furszy for the comment. I’ve changed the approched and described it on description.

darosior commented at 7:09 pm on January 10, 2025: member

Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.

As far as i remember we only ever store in the wallet descriptor produced by ToString(), which would mean no such wallet can exist. This would merely be an interface break for RPC’s which parse descriptors, given the lack of usecase of using a space inside a pk() expression this would be pretty minor.

furszy commented at 7:36 pm on January 10, 2025: member

Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.

As far as i remember we only ever store in the wallet descriptor produced by ToString(), which would mean no such wallet can exist. This would merely be an interface break for RPC’s which parse descriptors, given the lack of usecase of using a space inside a pk() expression this would be pretty minor.

Was thinking about that a few days ago and started checking it, but discovered another bug along the way and, because it is related, wanted to fix that before commenting here again. Storing the ToString() output means we store the descriptor with a different checksum, which could be confusing for users calling listdescriptors. However, it’s the lesser of the evils, so the compatibility window might not be necessary. A test for this would be useful, verifying that we are actually storing the descriptor without whitespaces, which results in a different checksum from the one provided by the user.

Note about the test: this can’t be done for taproot descriptors that do not contain all key/script paths key material (this is the bug I discovered, wip branch).

brunoerg commented at 9:01 pm on January 10, 2025: contributor

As far as i remember we only ever store in the wallet descriptor produced by ToString(), which would mean no such wallet can exist. This would merely be an interface break for RPC’s which parse descriptors, given the lack of usecase of using a space inside a pk() expression this would be pretty minor.

It means I can go back to the previous approach (simpler one) and only change the ParsePubkeyInner, there is no need to control it in the descriptor Parse anymore.

brunoerg force-pushed on Jan 15, 2025

brunoerg commented at 8:20 pm on January 15, 2025: contributor

Force-pushed simplifying the approach. Now it just checks the whitespace in the ParsePubkeyInner function.

test: fix descriptors in `ismine_tests`

Some descriptors contain whitespace in public keys
within fragments. This fixes it.

3b92878966

brunoerg force-pushed on Jan 15, 2025

in src/script/descriptor.cpp:1521 in 66911a02e8 outdated

1514@@ -1515,6 +1515,9 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
1515     if (str.size() == 0) {
1516         error = "No key provided";
1517         return {};
1518+    } else if (IsSpace(str[0]) || IsSpace(str.back())) {
1519+        error = strprintf("Pubkey '%s' is invalid due to whitespace", str);
1520+        return {};

furszy commented at 3:31 pm on January 17, 2025:

nit: Because there is a return statement in the if block that is above, you could remove the else and use str.front() instead of str[0].

brunoerg commented at 5:38 pm on January 17, 2025:

Done.

brunoerg force-pushed on Jan 17, 2025

in doc/release-notes-31603.md:4 in 250692de3b outdated

0@@ -0,0 +1,6 @@
1+Updated RPCs
2+--------
3+
4+- Any RPC which one of the parameters are descriptors will throw an error

sipa commented at 5:33 pm on January 17, 2025:

Any RPC in which one of the parameters…

brunoerg commented at 5:37 pm on January 17, 2025:

Done

brunoerg force-pushed on Jan 17, 2025

brunoerg commented at 5:38 pm on January 17, 2025: contributor

Force-pushed addressing #31603 (review) and #31603 (review)

in src/script/descriptor.cpp:1522 in 4b87013931 outdated

1515@@ -1516,6 +1516,10 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
1516         error = "No key provided";
1517         return {};
1518     }
1519+    if (IsSpace(str.front()) || IsSpace(str.back())) {
1520+        error = strprintf("Pubkey '%s' is invalid due to whitespace", str);
1521+        return {};
1522+    }

furszy commented at 5:54 pm on January 17, 2025:

This could also be a WIP formatted private key right? Should use the general term “key” here. E.g.

0strprintf(key '%s is invalid due to an extra whitespace at %s of the string', key_str, (IsSpace(str.front())?"beginning" : "end"));`

brunoerg commented at 6:21 pm on January 17, 2025:

Yes, it could be a WIF private key. I will change it to use “key”.

brunoerg commented at 6:27 pm on January 17, 2025:

Done.

descriptor: check whitespace in ParsePubkeyInner

Due to Base58, pubkeys with whitespace at the beginning
end/or at the end are successfully parsed. This commit
add a parameter into `ParsePubkeyInner` to control whether
it should return an error if the first or last char of the
pubkey is a space.

9ce93791c4

test: descriptor: check whitespace into pubkeys 4ff27851e5

test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys 38423c7a1b

docs: add release notes for 31603 4b0e67a8a4

brunoerg force-pushed on Jan 17, 2025

brunoerg commented at 6:28 pm on January 17, 2025: contributor

Force-pushed addressing #31603 (review) (thanks, @furszy)

DrahtBot added the label CI failed on Jan 17, 2025

descriptor: check whitespace in pubkeys within fragments #31603

Code Coverage & Benchmarks

Reviews

Conflicts