rpc: Sanitize label name in various RPCs with tests #26186

aureleoules · 2022-09-27T10:06:48Z

The following RPCs did not sanitize the optional label name:

importprivkey
importaddress
importpubkey
importmulti
importdescriptors
listsinceblock

Thus is was possible to import an address with a label * which should not be possible.
The wildcard label is used for backwards compatibility in the listtransactions rpc.
I added test coverage for these RPCs.

DrahtBot · 2022-09-27T12:51:45Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, stickies-v, ajtowns, theStack, achow101
Concept ACK	MarcoFalke, 1440000bytes

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
#26174 (rpc, wallet: add ability to retrieve all address book entries by w0xlt)
#24897 ([Draft / POC] Silent Payments by w0xlt)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation 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.

stickies-v

Approach ACK 3012200

test/functional/wallet_labels.py

src/wallet/rpc/backup.cpp

aureleoules · 2022-09-27T14:55:47Z

Thanks for the review @stickies-v, I've addressed all your comments.

test/functional/wallet_labels.py

aureleoules · 2022-09-27T15:18:59Z

Pushed, thanks @stickies-v.

stickies-v

ACK d293585 - nice improvement, straightforward code changes and test.

Left a few nits, nothing blocking.

test/functional/wallet_labels.py

maflcko

Concept ACK. Left a follow-up idea

src/wallet/rpc/backup.cpp

test/functional/wallet_labels.py

src/wallet/rpc/backup.cpp

src/wallet/rpc/coins.cpp

src/wallet/rpc/addresses.cpp

stickies-v

ACK 8b71661

aureleoules · 2022-12-15T10:04:40Z

Please restore it.

Done @luke-jr.

src/wallet/rpc/util.h

src/wallet/rpc/util.cpp

ghost · 2022-12-18T19:28:51Z

Concept ACK

stickies-v

re-ACK 408e375

Reduced string copying compared to my previous ACK. One non-blocking nit.

src/wallet/rpc/transactions.cpp

src/wallet/rpc/util.h

furszy

ACK 893c288

ajtowns

Concept ACK.

src/wallet/rpc/util.h

- importprivkey - importaddress - importpubkey - listtransactions - listsinceblock - importmulti - importdescriptors

src/wallet/rpc/util.h

furszy

diff ACK 65e78bd

stickies-v

re-ACK 65e78bd

stickies-v · 2023-01-04T15:57:08Z

src/wallet/rpc/util.cpp

+    static const std::string empty_string;
+    if (value.isNull()) return empty_string;


nit: not needed anymore now that we're returning a string copy

Suggested change

static const std::string empty_string;

if (value.isNull()) return empty_string;

if (value.isNull()) return "";

Or return {};

ajtowns · 2023-01-06T04:03:15Z

ACK 65e78bd

theStack

re-ACK 65e78bd

achow101 · 2023-01-09T20:57:23Z

test/functional/wallet_labels.py

+            [node.getnewaddress],
+            [node.setlabel, address],
+            [node.getaddressesbylabel],
+            [node.importpubkey, pubkey],


In 65e78bd "test: Invalid label name coverage"

importpubkey is a legacy wallet only RPC. The reason this test still passes with --descriptors is because the test framework has this (and some of the other import RPCs) aliased to importdescriptors.

Good to know. But considering this PR has 4 ACKs, is it worth the change? The test is executed with both --legacy-wallet and --descriptors anyway.

achow101 · 2023-01-10T22:22:14Z

ACK 65e78bd

65e78bd test: Invalid label name coverage (Aurèle Oulès) 552b51e refactor: Add sanity checks in LabelFromValue (Aurèle Oulès) 67e7ba8 rpc: Sanitize label name in various RPCs (Aurèle Oulès) Pull request description: The following RPCs did not sanitize the optional label name: - importprivkey - importaddress - importpubkey - importmulti - importdescriptors - listsinceblock Thus is was possible to import an address with a label `*` which should not be possible. The wildcard label is used for backwards compatibility in the `listtransactions` rpc. I added test coverage for these RPCs. ACKs for top commit: ajtowns: ACK 65e78bd achow101: ACK 65e78bd furszy: diff ACK 65e78bd stickies-v: re-ACK 65e78bd theStack: re-ACK 65e78bd Tree-SHA512: ad99f2824d4cfae352166b76da4ca0069b7c2eccf81aaa0654be25bbb3c6e5d6b005d93960f3f4154155f80e12be2d0cebd5529922ae3d2a36ee4eed82440b31

fanquake added the RPC/REST/ZMQ label Sep 27, 2022

glozow added Wallet RPC/REST/ZMQ and removed RPC/REST/ZMQ labels Sep 27, 2022

stickies-v reviewed Sep 27, 2022

View reviewed changes

test/functional/wallet_labels.py Outdated Show resolved Hide resolved

test/functional/wallet_labels.py Outdated Show resolved Hide resolved

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-09-test-label-invalid branch from 3012200 to cef09dd Compare September 27, 2022 14:54

aureleoules force-pushed the 2022-09-test-label-invalid branch from cef09dd to b2d0d4b Compare September 27, 2022 14:58

stickies-v reviewed Sep 27, 2022

View reviewed changes

test/functional/wallet_labels.py Outdated Show resolved Hide resolved

test/functional/wallet_labels.py Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-09-test-label-invalid branch from b2d0d4b to d293585 Compare September 27, 2022 15:15

stickies-v approved these changes Sep 28, 2022

View reviewed changes

test/functional/wallet_labels.py Outdated Show resolved Hide resolved

test/functional/wallet_labels.py Outdated Show resolved Hide resolved

test/functional/wallet_labels.py Outdated Show resolved Hide resolved

maflcko reviewed Sep 28, 2022

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-09-test-label-invalid branch 3 times, most recently from 62535d8 to 8867b03 Compare September 28, 2022 14:33

DrahtBot mentioned this pull request Sep 28, 2022

rpc, wallet: add ability to retrieve all address book entries #26174

Closed

aureleoules mentioned this pull request Sep 28, 2022

wallet, rpc: add label to listsinceblock #25934

Merged

maflcko added this to the 24.0 milestone Sep 28, 2022

aureleoules force-pushed the 2022-09-test-label-invalid branch from 8867b03 to 5b98f5a Compare September 28, 2022 21:26

jonatack reviewed Sep 30, 2022

View reviewed changes

test/functional/wallet_labels.py Show resolved Hide resolved

stickies-v reviewed Oct 3, 2022

View reviewed changes

src/wallet/rpc/backup.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/coins.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/addresses.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-09-test-label-invalid branch from 5b98f5a to 8b71661 Compare October 3, 2022 12:33

stickies-v approved these changes Oct 3, 2022

View reviewed changes

maflcko requested a review from achow101 October 4, 2022 10:48

This was referenced Oct 5, 2022

[Draft / POC] Silent Payments #24897

Closed

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

Open

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

maflcko removed this from the 24.0 milestone Oct 17, 2022

DrahtBot mentioned this pull request Dec 15, 2022

rpc: Return block hash & height in getbalances, gettransaction and getwalletinfo #26094

Merged

furszy reviewed Dec 15, 2022

View reviewed changes

src/wallet/rpc/util.h Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-09-test-label-invalid branch 2 times, most recently from cce2e87 to b119bb8 Compare December 16, 2022 13:52

luke-jr reviewed Dec 18, 2022

View reviewed changes

src/wallet/rpc/util.cpp Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-09-test-label-invalid branch from b119bb8 to 408e375 Compare December 19, 2022 10:07

stickies-v approved these changes Dec 19, 2022

View reviewed changes

src/wallet/rpc/transactions.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/util.h Outdated Show resolved Hide resolved

furszy approved these changes Dec 19, 2022

View reviewed changes

ajtowns reviewed Jan 3, 2023

View reviewed changes

src/wallet/rpc/util.h Outdated Show resolved Hide resolved

aureleoules force-pushed the 2022-09-test-label-invalid branch from 408e375 to 6d70265 Compare January 4, 2023 11:29

rpc: Sanitize label name in various RPCs

67e7ba8

- importprivkey - importaddress - importpubkey - listtransactions - listsinceblock - importmulti - importdescriptors

aureleoules force-pushed the 2022-09-test-label-invalid branch from 6d70265 to 632202c Compare January 4, 2023 11:31

furszy reviewed Jan 4, 2023

View reviewed changes

src/wallet/rpc/util.h Outdated Show resolved Hide resolved

aureleoules added 2 commits January 4, 2023 13:45

refactor: Add sanity checks in LabelFromValue

552b51e

test: Invalid label name coverage

65e78bd

aureleoules force-pushed the 2022-09-test-label-invalid branch from 632202c to 65e78bd Compare January 4, 2023 12:45

furszy approved these changes Jan 4, 2023

View reviewed changes

stickies-v approved these changes Jan 4, 2023

View reviewed changes

DrahtBot mentioned this pull request Jan 7, 2023

refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc #26840

Closed

theStack approved these changes Jan 8, 2023

View reviewed changes

maflcko assigned achow101 Jan 9, 2023

achow101 reviewed Jan 9, 2023

View reviewed changes

achow101 merged commit 68f88bc into bitcoin:master Jan 10, 2023

aureleoules deleted the 2022-09-test-label-invalid branch January 12, 2023 11:51

bitcoin locked and limited conversation to collaborators Jan 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: Sanitize label name in various RPCs with tests #26186

rpc: Sanitize label name in various RPCs with tests #26186

aureleoules commented Sep 27, 2022 •

edited

DrahtBot commented Sep 27, 2022 •

edited

stickies-v left a comment

aureleoules commented Sep 27, 2022

aureleoules commented Sep 27, 2022

stickies-v left a comment

maflcko left a comment

stickies-v left a comment

aureleoules commented Dec 15, 2022

ghost commented Dec 18, 2022

stickies-v left a comment

furszy left a comment

ajtowns left a comment

furszy left a comment

stickies-v left a comment

stickies-v Jan 4, 2023

ajtowns Jan 6, 2023

ajtowns commented Jan 6, 2023

theStack left a comment

achow101 Jan 9, 2023

aureleoules Jan 10, 2023 •

edited

achow101 commented Jan 10, 2023

		static const std::string empty_string;
		if (value.isNull()) return empty_string;

	static const std::string empty_string;
	if (value.isNull()) return empty_string;
	if (value.isNull()) return "";

rpc: Sanitize label name in various RPCs with tests #26186

rpc: Sanitize label name in various RPCs with tests #26186

Conversation

aureleoules commented Sep 27, 2022 • edited

DrahtBot commented Sep 27, 2022 • edited

Reviews

Conflicts

stickies-v left a comment

Choose a reason for hiding this comment

aureleoules commented Sep 27, 2022

aureleoules commented Sep 27, 2022

stickies-v left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

aureleoules commented Dec 15, 2022

ghost commented Dec 18, 2022

stickies-v left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

ajtowns left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v Jan 4, 2023

Choose a reason for hiding this comment

ajtowns Jan 6, 2023

Choose a reason for hiding this comment

ajtowns commented Jan 6, 2023

theStack left a comment

Choose a reason for hiding this comment

achow101 Jan 9, 2023

Choose a reason for hiding this comment

aureleoules Jan 10, 2023 • edited

Choose a reason for hiding this comment

achow101 commented Jan 10, 2023

aureleoules commented Sep 27, 2022 •

edited

DrahtBot commented Sep 27, 2022 •

edited

aureleoules Jan 10, 2023 •

edited