Remove user input from URI error message #280

1. 
   ghost commented at 9:36 pm on April 13, 2021: none

   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:

   0bitcoin: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):

   

2. 
   sipa commented at 9:42 pm on April 13, 2021: contributor

   Nice find. Presumably we could also just sanitize the address before showing?
3. 
   ghost commented at 9:57 pm on April 13, 2021: none

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

4. hebasto added the label Feature on Apr 14, 2021
5. 
   jarolrod commented at 6:16 am on April 14, 2021: member

   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?
6. 
   Bosch-0 commented at 0:54 am on April 15, 2021: none

   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?
7. 
   GBKS commented at 8:12 am on April 15, 2021: none

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

8. 
   hebasto commented at 1:00 pm on April 15, 2021: member

   My vote is for @Bosch-0’s and @GBKS’s vision.
9. 
   ghost commented at 4:59 am on April 16, 2021: none

   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 https://github.com/bitcoin-core/gui/pull/280/commits/9885de1178889479d77321956808e95f4299e591

   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:

   0bitcoin: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:

   https://github.com/bitcoin-core/gui/blob/585cbe22575306c05b156ea90faa9caa1add4c87/src/qt/paymentserver.cpp#L238

10. 
    GBKS commented at 6:53 am on April 16, 2021: none

    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.
11. Talkless approved
12. 
    Talkless commented at 3:39 pm on April 25, 2021: none

    tACK 9885de1178889479d77321956808e95f4299e591, tested on Debian Sid with Qt 5.15.2

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

    0#!/usr/bin/bash
    1
    2/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.

13. 
    ghost commented at 4:16 pm on April 26, 2021: none

    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 https://github.com/bitcoin/bitcoin/pull/21755 to improve error messages for invalid addresses.

14. 
    Bosch-0 commented at 2:43 am on April 27, 2021: none

    tACK https://github.com/bitcoin-core/gui/commit/a79ca55f3ee03fd6f8f01cf4434759bf9d02ffe6 on windows 10 - looks good to me!

    Before / After:

    

15. in src/qt/paymentserver.cpp:246 in a79ca55f3e outdated

    243                                "Due to widespread security flaws in BIP70 it's strongly recommended that any merchant instructions to switch wallets be ignored.\n"
    244                                "If you are receiving this error you should request the merchant provide a BIP21 compatible URI."),
    245                             CClientUIInterface::ICON_WARNING);
    246                     }
    247-                    Q_EMIT message(tr("URI handling"), tr("Invalid payment address %1").arg(recipient.address),
    248+                    Q_EMIT message(tr("URI handling"), tr(error_msg.c_str()),
    

    ---

    ---

    

    jarolrod commented at 4:21 am on April 30, 2021:

    how would tr(error_msg.c_str()) work for translations?

    The DecodeDestination function will return one of many messages depending on the error of the input address. I don’t think that QTranslator will input addresses into DecodeDestination to get all the error messages so that they can be translated.

    ---

    

    unknown commented at 5:15 am on April 30, 2021:

    Sorry I have no clue how translation works in Bitcoin Core.

    ---

    

    hebasto commented at 10:54 am on April 30, 2021:

    0                    Q_EMIT message(tr("URI handling"), QString::fromStdString(error_msg),
    

    will work here.

    If required, error translation could be added in follow up.

    ---

    

    hebasto commented at 10:56 am on April 30, 2021:

    Sorry I have no clue how translation works in Bitcoin Core.

    See #280 (review)

16. 
    jarolrod commented at 4:21 am on April 30, 2021: member

    concept ack
17. in src/qt/paymentserver.cpp:236 in a79ca55f3e outdated

    231@@ -232,15 +232,18 @@ void PaymentServer::handleURIOrFile(const QString& s)
    232             SendCoinsRecipient recipient;
    233             if (GUIUtil::parseBitcoinURI(s, &recipient))
    234             {
    235-                if (!IsValidDestinationString(recipient.address.toStdString())) {
    236+                std::string error_msg;
    237+                CTxDestination dest = DecodeDestination(recipient.address.toStdString(), error_msg);
    

    ---

    ---

    

    hebasto commented at 10:53 am on April 30, 2021:

    nit:

    0                const CTxDestination dest = DecodeDestination(recipient.address.toStdString(), error_msg);
    

18. hebasto changes_requested
19. 
    hebasto commented at 10:55 am on April 30, 2021: member

    Approach ACK a79ca55f3ee03fd6f8f01cf4434759bf9d02ffe6.
20. 
    Rspigler commented at 5:18 pm on May 1, 2021: contributor

    tested ACK commit a79ca55f3ee03fd6f8f01cf4434759bf9d02ffe6

    Great find!

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

21. Remove user input from URI error message

    + Detailed error messages for invalid address
    + Used `IsValidDestination` instead of `IsValidDestinationString`
    + Referred to https://github.com/bitcoin/bitcoin/pull/20832 for solution

    3bad0b3fad
22. 
    ghost commented at 7:47 am on May 2, 2021: none

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

    0bitcoin:tb1q2ansurk0fypm2aex605m7z5rxxfe2k4umkxa2u
    1
    2bitcoin:tb1q2ansurk0fypm2aex605m7z5rxxfe2k4umkxa2u?label=Luke-Jr
    3
    4bitcoin:tb1q2ansurk0fypm2aex605m7z5rxxfe2k4umkxa2u?amount=20.3&label=Luke-Jr
    5
    6bitcoin: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: https://github.com/bitcoin/bitcoin/pull/21819. Maybe I used mainnet address in testnet.

23. hebasto approved
24. 
    hebasto commented at 10:18 am on May 2, 2021: member

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

    Error message:

    

25. hebasto added this to the milestone 22.0 on May 8, 2021
26. 
    jarolrod commented at 2:36 am on May 10, 2021: member

    tACK 3bad0b3fada9ab7c5b03d31dc33d72654c1ba2be

    Tested on macOS 11.3, Qt 5.15.2

    Master	PR.

27. MarcoFalke added the label needs backport (0.21) on May 10, 2021
28. MarcoFalke removed this from the milestone 22.0 on May 10, 2021
29. MarcoFalke added this to the milestone 0.21.2 on May 10, 2021
30. 
    MarcoFalke commented at 6:26 am on May 10, 2021: contributor

    Concept ACK. Nice find. Added for backport.
31. hebasto merged this on May 10, 2021
32. hebasto closed this on May 10, 2021

    ---

33. sidhujag referenced this in commit 95f52b2b67 on May 11, 2021
34. MarcoFalke referenced this in commit c0b2ff7927 on May 22, 2021
35. 
    MarcoFalke commented at 8:01 am on May 22, 2021: contributor

    Backported in bitcoin/bitcoin#22022
36. MarcoFalke removed the label needs backport (0.21) on May 22, 2021
37. MarcoFalke referenced this in commit 46320ba72f on May 22, 2021
38. fanquake referenced this in commit 419f9b3b3b on May 31, 2021
39. ComputerCraftr referenced this in commit 4ea1145a06 on Jun 9, 2021
40. gwillen referenced this in commit 77bd932406 on Jun 1, 2022
41. bitcoin-core locked this on Aug 16, 2022

Contributors
ghost sipa jarolrod Bosch-0 GBKS hebasto Talkless Rspigler MarcoFalke

Labels
Feature

Milestone
0.21.2