Remove user input from URI error message #280

ghost · 2021-04-13T21:36:53Z

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:

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

User selects Bitcoin Core to open the link:

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 · 2021-04-13T21:42:22Z

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

ghost · 2021-04-13T21:57:27Z

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 · 2021-04-14T06:16:40Z

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 · 2021-04-15T00:54:45Z

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 · 2021-04-15T08:12:07Z

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 · 2021-04-15T13:00:22Z

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

ghost · 2021-04-16T04:59:21Z

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:

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:

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

Before this PR:

After this PR:

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 · 2021-04-16T06:53:17Z

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 · 2021-04-26T16:16:05Z

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 · 2021-04-27T02:43:50Z

tACK a79ca55 on windows 10 - looks good to me!

Before / After:

jarolrod

concept ack

src/qt/paymentserver.cpp

hebasto

Approach ACK a79ca55.

src/qt/paymentserver.cpp

Rspigler · 2021-05-01T17:18:12Z

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).

+ Detailed error messages for invalid address + Used `IsValidDestination` instead of `IsValidDestinationString` + Referred to bitcoin/bitcoin#20832 for solution

ghost · 2021-05-02T07:47:43Z

@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 · 2021-05-10T06:26:49Z

Concept ACK. Nice find. Added for backport.

3bad0b3 Remove user input from URI error message (unknown) Pull request description: 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: ![image](https://user-images.githubusercontent.com/13405205/114619801-8ee9a080-9cc8-11eb-9fad-23a2b831e8df.png) 3. User is asked to send BTC with some message convincing enough which can be different depending on the victim: ![image](https://user-images.githubusercontent.com/13405205/114620061-d3753c00-9cc8-11eb-8314-e3362ebb90ac.png) **After this PR** (_No user input mentioned in the error_): ![image](https://user-images.githubusercontent.com/13405205/114624342-2b627180-9cce-11eb-93a8-0b2438d71571.png) ACKs for top commit: hebasto: ACK 3bad0b3, tested on Linux Mint 20.1 (Qt 5.12.8). jarolrod: tACK 3bad0b3 Tree-SHA512: aac2fdfcaa7a9cd6582750c1960682554795640f5aacb78bdae121724e1151da3cbb62b8f8b1e0bc37347afe78b3e9a446277cab8e009d2a1050c0e971f001b3

+ Detailed error messages for invalid address + Used `IsValidDestination` instead of `IsValidDestinationString` + Referred to bitcoin#20832 for solution Github-Pull: bitcoin-core/gui#280 Rebased-From: 3bad0b3

maflcko · 2021-05-22T08:01:43Z

Backported in bitcoin/bitcoin#22022

+ Detailed error messages for invalid address + Used `IsValidDestination` instead of `IsValidDestinationString` + Referred to bitcoin#20832 for solution Github-Pull: bitcoin-core/gui#280 Rebased-From: 3bad0b3

09620b8 Update Windows code signing certificate (Andrew Chow) 46320ba Remove user input from URI error message (unknown) f2a8898 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack) Pull request description: ACKs for top commit: achow101: ACK 09620b8 Diffs match. hebasto: ACK 09620b8, tested bitcoin-core/gui#280 behavior. fanquake: ACK 09620b8 Tree-SHA512: 1c4aaec42ea047261b5d30851bca605540eccf572708403335b38016127d3230b5380b3f5ef03921ed62192239b0d3da9787d51f557ed7911bf6bb2a7c172753

09620b89f5f78434080196162c75e3e65cc8d47f Update Windows code signing certificate (Andrew Chow) 46320ba72f80dd865b05bbfe2654cde124859881 Remove user input from URI error message (unknown) f2a88986a18f197f6a4690f5dcca248f6b6170e2 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack) Pull request description: ACKs for top commit: achow101: ACK 09620b89f5f78434080196162c75e3e65cc8d47f Diffs match. hebasto: ACK 09620b89f5f78434080196162c75e3e65cc8d47f, tested bitcoin-core/gui#280 behavior. fanquake: ACK 09620b89f5f78434080196162c75e3e65cc8d47f Tree-SHA512: 1c4aaec42ea047261b5d30851bca605540eccf572708403335b38016127d3230b5380b3f5ef03921ed62192239b0d3da9787d51f557ed7911bf6bb2a7c172753

hebasto added the Feature label Apr 14, 2021

ghost mentioned this pull request Apr 21, 2021

Error messages for invalid address bitcoin/bitcoin#21741

Closed

Talkless approved these changes Apr 25, 2021

View reviewed changes

jarolrod reviewed Apr 30, 2021

View reviewed changes

src/qt/paymentserver.cpp Outdated Show resolved Hide resolved

hebasto requested changes Apr 30, 2021

View reviewed changes

src/qt/paymentserver.cpp Outdated Show resolved Hide resolved

src/qt/paymentserver.cpp Outdated Show resolved Hide resolved

ghost mentioned this pull request Apr 30, 2021

doc: Add info about supported format for URI bitcoin/bitcoin#21819

Closed

Remove user input from URI error message

3bad0b3

+ Detailed error messages for invalid address + Used `IsValidDestination` instead of `IsValidDestinationString` + Referred to bitcoin/bitcoin#20832 for solution

hebasto approved these changes May 2, 2021

View reviewed changes

hebasto added this to the 22.0 milestone May 8, 2021

jarolrod reviewed May 10, 2021

View reviewed changes

maflcko added the needs backport (0.21) label May 10, 2021

maflcko modified the milestones: 22.0, 0.21.2 May 10, 2021

hebasto merged commit b49fe0a into bitcoin-core:master May 10, 2021

hebasto mentioned this pull request May 10, 2021

Fix PaymentServer::handleURIOrFile warning message #225

Closed

maflcko removed the needs backport (0.21) label May 22, 2021

hebasto mentioned this pull request May 24, 2021

Final backports for 0.21.2rc1 bitcoin/bitcoin#22022

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge b49fe0a into merged_master (Bitcoin PR bitcoin-core/gui#280)

77bd932

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove user input from URI error message #280

Remove user input from URI error message #280

ghost commented Apr 13, 2021 •

edited by ghost

Loading

sipa commented Apr 13, 2021

ghost commented Apr 13, 2021

jarolrod commented Apr 14, 2021

Bosch-0 commented Apr 15, 2021 •

edited

Loading

GBKS commented Apr 15, 2021

hebasto commented Apr 15, 2021

ghost commented Apr 16, 2021

GBKS commented Apr 16, 2021

Talkless left a comment •

edited

Loading

ghost commented Apr 26, 2021 •

edited by ghost

Loading

Bosch-0 commented Apr 27, 2021

jarolrod left a comment

hebasto left a comment

Rspigler commented May 1, 2021

ghost commented May 2, 2021

hebasto left a comment

jarolrod left a comment

maflcko commented May 10, 2021

maflcko commented May 22, 2021

Remove user input from URI error message #280

Remove user input from URI error message #280

Conversation

ghost commented Apr 13, 2021 • edited by ghost Loading

sipa commented Apr 13, 2021

ghost commented Apr 13, 2021

jarolrod commented Apr 14, 2021

Bosch-0 commented Apr 15, 2021 • edited Loading

GBKS commented Apr 15, 2021

hebasto commented Apr 15, 2021

ghost commented Apr 16, 2021

GBKS commented Apr 16, 2021

Talkless left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Apr 26, 2021 • edited by ghost Loading

Bosch-0 commented Apr 27, 2021

jarolrod left a comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

Rspigler commented May 1, 2021

ghost commented May 2, 2021

hebasto left a comment

Choose a reason for hiding this comment

jarolrod left a comment

Choose a reason for hiding this comment

maflcko commented May 10, 2021

maflcko commented May 22, 2021

ghost commented Apr 13, 2021 •

edited by ghost

Loading

Bosch-0 commented Apr 15, 2021 •

edited

Loading

Talkless left a comment •

edited

Loading

ghost commented Apr 26, 2021 •

edited by ghost

Loading