Remove user input from URI error message

[ghost]

Removes the user input from error message to avoid it being used in attacks.

Its not really a vulnerability in Bitcoin Core because involves social engineering, dependency on user environment etc. But this PR improves security and by avoiding abuse of URI error in future.

Example of an attack:

1. User opens a link in firefox:

bitcoin:tb1qag2e6yhl52hr53vdxzaxvnjtueupvuftan4yfu%0A%0AWARNING%3A%20DO%20NOT%20CLOSE%20THIS%20WINDOW%20OR%20TURN%20OFF%20YOUR%20PC!%20IF%20YOU%20ABORT%20THIS%20PROCESS%2C%20YOU%20COULD%20DESTROY%20ALL%20OF%20YOU%20DATA!%20PLEASE%20ENSURE%20THAT%20YOUR%20POWER%20CABLE%20IS%20PLUGGED%20IN!%0A%0AYou%20became%20victim%20of%20the%20XYZ%20RANSOMWARE!%0A%0AThe%20hard%20disks%20of%20your%20computer%20have%20been%20encrypted%20with%20a%20military%20grade%20encryption%20algorithm.%20There%20is%20no%20way%20to%20restore%20your%20data%20without%20a%20special%20key.%20You%20can%20purchase%20this%20key%20on%20the%20darknet%20page%20shown%20in%20step%202.%0ATo%20purchase%20your%20key%20and%20restore%20your%20data%2C%20please%20follow%20these%20three%20easy%20steps%3A%0A%0A1.%20Download%20the%20Tor%20browser%20at%20%E2%80%9Chttps%3A%2F%2Fwww.torproject.org%2F%E2%80%9C.%0A2.%20Visit%20one%20of%20the%20following%20pages%20with%20the%20Tor%20Browser%3A%0Ahttp%3A%2F%2Frandomchars.onion%2Fabc123%0A3.%20Send%20BTC%20by%20following%20the%20instructions%20on%20the%20page

2. User selects Bitcoin Core to open the link:

3. User is asked to send BTC with some message convincing enough which can be different depending on the victim:

After this PR (No user input mentioned in the error):

[sipa]

Nice find. Presumably we could also just sanitize the address before showing?

[ghost]

Presumably we could also just sanitize the address before showing?

Yes that's possible. I thought a generic error message is good enough in this case for user and there is no need to know the wrong address or anything else which was entered.

For sanitization I will have to do more research if it's already done for something else in GUI or find best way to do it in C++.

[jarolrod]

I think it would be better to always sanitize the address before showing it. @Bosch-0 what do you think about this from a UX perspective. should the incorrect address be shown back to the user?

[Bosch-0]

The blank error that prayank presented in the OP would be my suggestion. The only information I would include (if you can) alongside the incorrect address is why the address is invalid. Say for example is opening a testnet address when running mainnet it could notify the user of this. @GBKS thoughts?

[GBKS]

Agree with Bosch. Giving a reason why it's invalid would be helpful for the user to address the problem. Alternatively, if the input should stay visible, it could be clearly separated from the UI copy (see crappy mock-up below).

[hebasto]

My vote is for @Bosch-0's and @GBKS's vision.

[ghost]

Thanks everyone for the review.

I think it would be better to always sanitize the address before showing it.

@jarolrod
IMO there is no need to show address in error message if the address fails validation.

The blank error that prayank presented in the OP would be my suggestion. The only information I would include (if you can) alongside the incorrect address is why the address is invalid.

@Bosch-0
Done. Made changes in 9885de1

Alternatively, if the input should stay visible, it could be clearly separated from the UI copy (see crappy mock-up below).

@GBKS Thanks for sharing the mockup. However, less is better in this case IMO.

My vote is for @Bosch-0's and @GBKS's vision.

@hebasto Few examples of different reasons mentioned in the error messages:

1. User input mentioned in the description of this PR:

bitcoin:tb1qag2e6yhl52hr53vdxzaxvnjtueupvuftan4yfu%0A%0AWARNING%3A%20DO%20NOT%20CLOSE%20THIS%20WINDOW%20OR%20TURN%20OFF%20YOUR%20PC!%20IF%20YOU%20ABORT%20THIS%20PROCESS%2C%20YOU%20COULD%20DESTROY%20ALL%20OF%20YOU%20DATA!%20PLEASE%20ENSURE%20THAT%20YOUR%20POWER%20CABLE%20IS%20PLUGGED%20IN!%0A%0AYou%20became%20victim%20of%20the%20XYZ%20RANSOMWARE!%0A%0AThe%20hard%20disks%20of%20your%20computer%20have%20been%20encrypted%20with%20a%20military%20grade%20encryption%20algorithm.%20There%20is%20no%20way%20to%20restore%20your%20data%20without%20a%20special%20key.%20You%20can%20purchase%20this%20key%20on%20the%20darknet%20page%20shown%20in%20step%202.%0ATo%20purchase%20your%20key%20and%20restore%20your%20data%2C%20please%20follow%20these%20three%20easy%20steps%3A%0A%0A1.%20Download%20the%20Tor%20browser%20at%20%E2%80%9Chttps%3A%2F%2Fwww.torproject.org%2F%E2%80%9C.%0A2.%20Visit%20one%20of%20the%20following%20pages%20with%20the%20Tor%20Browser%3A%0Ahttp%3A%2F%2Frandomchars.onion%2Fabc123%0A3.%20Send%20BTC%20by%20following%20the%20instructions%20on%20the%20page

Before this PR:

After this PR:

2. User input: bitcoin:bc1qjjrlgl28uqjtv0at2n3pfv3qtuvrm85vr0am5v Mainnet bech32 address on testnet. Prefix on testnet is tb1.

Before this PR:

After this PR:

3. User input: bitcoin:1GmHaHzNDfMC8TY3Qaw9xt9FeBnzBYtGXz Mainnet legacy address on testnet. Prefix on testnet is N or M

Before this PR:

After this PR:

There is one more case which was already mentioned and can be tested with bitcoin:?r=https://merchant.com/pay.php?h%3D2a8628fc2fbe:

gui/src/qt/paymentserver.cpp

Line 238 in 585cbe2

tr("Cannot process payment request because BIP70 is not supported.\n"

[GBKS]

Invalid prefix for Base58-encoded address requires technical understanding (What is Base58? Which part of the address is the prefix and what prefixes are valid?) to make sense of. Would it be possible to simplify the language? Something like Addresses should start with a 1.

[Talkless]

tACK 9885de1, tested on Debian Sid with Qt 5.15.2

To reproduce, I've created wrapper open.sh file with this content:

#!/usr/bin/bash

/home/vincas/code/bitcoin/280-bitcoin-core-gui.git/_build_pr/src/qt/bitcoin-qt -regtest $@

Then, in about:config Firefox page I've set network.protocol-handler.expose.bitcoin to boolean false value.

Finally, entered specially-crafted link into Firefox address bar to open it, selected open.sh I've prepared earlier.

Master: got cracker message, this PR: got vague (but safe) error message as expected.

I personally don't care much about "usefulness" of the error message, better just fix it ASAP and improve in later PR.

[ghost]

I personally don't care much about "usefulness" of the error message, better just fix it ASAP and improve in later PR.

I agree. I think this PR is good enough to be merged. I have opened another PR bitcoin/bitcoin#21755 to improve error messages for invalid addresses.

[Bosch-0]

tACK a79ca55 on windows 10 - looks good to me!

Before / After:

[jarolrod]

concept ack

[hebasto]

Approach ACK a79ca55.

[Rspigler]

tested ACK commit a79ca55

Great find!

A valid address still contains a label, but an invalid address will now only contain the error message (without extra info).

[ghost]

@Rspigler I tried the examples mentioned in BIP 21 with a testnet address in bitcoin-qt(testnet). They are all working.

bitcoin:tb1q2ansurk0fypm2aex605m7z5rxxfe2k4umkxa2u

bitcoin:tb1q2ansurk0fypm2aex605m7z5rxxfe2k4umkxa2u?label=Luke-Jr

bitcoin:tb1q2ansurk0fypm2aex605m7z5rxxfe2k4umkxa2u?amount=20.3&label=Luke-Jr

bitcoin:tb1q2ansurk0fypm2aex605m7z5rxxfe2k4umkxa2u?amount=50&label=Luke-Jr&message=Donation%20for%20project%20xyz

Not sure what did I test last time when I got error and opened this PR: bitcoin/bitcoin#21819. Maybe I used mainnet address in testnet.

[hebasto]

ACK 3bad0b3, tested on Linux Mint 20.1 (Qt 5.12.8).

Error message:

[jarolrod]

tACK 3bad0b3

Tested on macOS 11.3, Qt 5.15.2

Master	PR.

[maflcko]

Concept ACK. Nice find. Added for backport.

[maflcko]

Backported in bitcoin/bitcoin#22022