trezor: try empty passphrase first, make trezor usage easier for users #7793

ph4r05 · 2021-07-14T15:16:58Z

@matejcik in trezor/trezor-firmware#1686 (comment) proposed interesting idea for opening wallets with passphrases:

An improvement that can be done in Monero GUI immediately and even without the standard/hidden thing:
when you're opening the wallet, query the empty-passphrase address. If it matches, do not even show the passphrase dialog and proceed straight to wallet.

This PR implements the proposal. The flow is:

If device being used is Trezor, try loading address of the wallet with empty passphrase (no user prompt)
If address matches wallet address, open without passphrase prompt
If address does not match, try once again to load address, without passphrase prompt suppression. If there is a passphrase used, user can enter it.

Benefits:

Easier access for Trezors with passphrase enabled (soon, all devices will have this by default, as Trezor Suite enables passphrase by default)
Simplifies wallet opening for empty passphrases

Cons:

Opening Monero wallet with non-empty passphrase takes a bit longer, as empty passphrase is tried first. This is just one extra call to Trezor, so I think costs are justifiable.

This is just a PoC for discussion. Thanks for ideas.

ph4r05 · 2021-07-14T15:19:57Z

src/wallet/wallet2.cpp

+        fetch_device_address = false;
+        dev_cold->set_use_empty_passphrase(true);
+      } else {
+        fetch_device_address = true;


I wanted to use dev_cold->seen_passphrase_entry_prompt() and to try again with passphrase only if device obviously asked for any. But to avoid potential problems we can just ask anyway. The worst case is that address is loaded twice on error.

ph4r05 · 2021-07-15T08:01:59Z

@selsta what is pls your opinion about this?

selsta · 2021-07-15T08:44:29Z

will take a look

selsta · 2021-07-15T15:15:10Z

src/device_trezor/device_trezor_base.cpp

+        on_device = false;
+        passphrase = epee::wipeable_string("");
+      } else if (m_passphrase){
+        MWARNING("Answering passphrase prompt with a stored passphrase (do not use; passphrase can be seen by a potential malware / attacker)");


Can you explain "stored passphrase" ?

Sure, it was part of the old API, you could set m_passphrase to the device object, the passphrase will be then used instead of prompting user. I've added this warning as in general, we don't want users entering their passhprases on computers, but this may have useful use-cases when you know what are you doing. E.g., in tests, automation, etc. It was part of the API so I don't feel like removing it now.

selsta · 2021-07-15T15:16:12Z

I tested it and it works well, I think this is something we can add.

rating89us · 2021-07-15T18:31:45Z

I tested this PR with my six Monero GUI wallet files that I created when testing (see trezor/trezor-firmware#1686 (comment)).

If device being used is Trezor, try loading address of the wallet with empty passphrase (no user prompt)
If address matches wallet address, open without passphrase prompt

When opening the six Monero GUI wallet files with PF-enabled Trezor: this PR fixes the UX problem observed when I try to open Monero GUI wallet files with no PF (Wallets E & F), since it doesn't require anymore the user to enter a blank PF. You just have to type the password and the wallet opens without asking a PF.

If address does not match, try once again to load address, without passphrase prompt suppression. If there is a passphrase used, user can enter it.

When opening the six Monero GUI wallet files with PF-disabled Trezor: this PR doesn't fix the "ERROR: couldn't open wallet. Device wallet (46...) does not match wallet address (48...)" observed when I try to open Monero GUI wallet files with non-blank PF (wallets C & D). It seems that Trezor is still returning a blank PF to Monero GUI. No PF prompt appeared on Monero GUI, neither an option to enable PF protection on Trezor device.

selsta · 2021-07-15T18:33:55Z

First I tried opening the six Monero GUI wallet files with PF-disabled Trezor. This PR doesn't fix the "ERROR: couldn't open wallet. Device wallet (46...) does not match wallet address (48...)" observed when I try to open Monero GUI wallet files with PF (wallets C & D). It seems that Trezor is still returning a blank PF to Monero GUI.

This isn't meant to be fixed here but Trezor will in the future always enable passphrase so this shouldn't be an issue anymore.

rating89us · 2021-07-15T18:42:33Z

If Monero GUI could detect whether the Trezor device has PF protection enabled or not, we could display this message (instead of displaying 'address doesn't match' error): "You are opening a wallet file with PF, but you are using a PF-disabled Trezor device. Please open Trezor Suite and enable PF protection for your device in order to open this wallet file."

Or maybe it's possible to offer directly on Monero GUI the option to enable PF protection for the Trezor device.

Will it be possible for users to disable PF protection for the device in the future?

ph4r05 · 2021-07-15T20:10:46Z

Interesting points. Thanks for feedback!

This PR indeed does not address disabled passphrase Trezor. We could detect if passphrase is disabled on Trezor (and eventually show this in error message), but currently we cannot say if wallet was created with or without passphrase. I also don't see way to detect this. If user is entering passphrase on Trezor, we have no information about it. We could ask if user wants to use a "hidden wallet" when creating it and mark it to the wallet file. But other than that we have only very buggy heuristic. Same for existing wallets. So for sake of clarity and consistency I would maybe refrain from asking users whether to use passphrase or not during wallet creation.

Also, enabling passphrase in Monero wallet is risky as it may be confusing for users having also other coins.

What are your thoughts?

rating89us · 2021-07-15T20:41:28Z

Considering that it's not possible to know if the Monero GUI wallet file is protected or not by a PF, I have the following ideas for 'device address doesn't match error' in a PF disabled Trezor:

Idea 1: detect if Trezor device has PF protection disabled and when device address doesn't match, display the following error message: "ERROR: couldn't open wallet. Device wallet does not match wallet address. If this Monero GUI wallet is protected by a passphrase, first you need to enable passphrase protection for your Trezor device on Trezor Suite."

Idea 2: temporarily enable passphrase protection for Trezor device on Monero GUI when device address doesn't match. User clicks to open the wallet -> address doesn't match -> Monero GUI temporarily enables PF protection on Trezor device -> PF dialog opens in Monero GUI -> when PF dialog is closed, Monero GUI disables PF protection on Trezor device.

Idea 3: every time a Trezor wallet is opened on Monero GUI, the user informs Monero GUI whether he wants to open a Standard or Hidden wallet (display a dialog with "Standard wallet" and "Hidden wallet" buttons, the same UI of Trezor Suite). If the user clicks on Standard wallet, try to open it with blank PF (if address doesn't match, display: "error: this wallet is a hidden wallet or you are using a Trezor device with a different seed"). If the user clicks on Hidden wallet button and PF protection is enabled on Trezor device, display PF dialog (if address doesn't match, display: "error: wrong passphrase or you are using a Trezor device with a different seed"). If the user clicks on Hidden wallet button and PF protection is disabled on Trezor device, display the following error message: "Your Trezor device has PF protection disabled. In order to open this hidden wallet, first you need to enable passphrase protection on Trezor Suite."

ph4r05 · 2021-07-16T06:04:33Z

#1 I am not convinced this is the way to go. Other contributors would have to overrule me :)

#2 this won't work. I am strictly against manipulating passphrase settings in Trezor. It's up to user to do this, we won't change Trezor settings. And nothig is atomic here, if app crashes in the middle of the process, passphrase stays enabled, this would be very confusing and can lead to fund loss and even more support tickets.

#3 this was also proposed by @matejcik in different threads. But is quite a lot of work and I don't feel like implementing it now. Gains are too low to justify the API overhaul costs. It is definitely not for this PR.

ph4r05 · 2021-07-28T08:19:11Z

@moneromooo-monero pls also tis one, if you find some spare minutes 🙏 , thanks!

- Try empty passphrase first when opening a wallet, as all Trezors will have passphrase enabled by default by Trezor Suite by default. This feature enables easier access to all users using disabled passphrase (or empty passhprase) - If wallet address differs from device address with empty passphrase, another opening attempt is made, without passphrase suppression, so user can enter his passhprase if using some. In this scenario, nothing changes to user, wallet opening just consumes one more call to Trezor (get wallet address with empty passphrase) - also change how m_passphrase is used. Previous version did not work well with recent passphrase entry mechanism change (made in Trezor), thus this commit fixes the behaviour).

ph4r05 · 2021-09-13T07:53:43Z

ping :)

ph4r05 commented Jul 14, 2021

View reviewed changes

ph4r05 force-pushed the pr/010-trezor-try-empty-passphrase-first branch 3 times, most recently from 60deed9 to 82279df Compare July 15, 2021 09:47

ph4r05 changed the title ~~trezor: try empty passphrase first [WIP]~~ trezor: try empty passphrase first Jul 15, 2021

selsta reviewed Jul 15, 2021

View reviewed changes

rating89us mentioned this pull request Jul 16, 2021

wallet: rephrase error message on invalid device address #7790

Merged

andrewkozlik mentioned this pull request Jul 21, 2021

Investigate invalid Monero address after Trezor firmware update with Shamir backup trezor/trezor-firmware#1686

Closed

ph4r05 changed the title ~~trezor: try empty passphrase first~~ trezor: try empty passphrase first, make trezor usage easier for users Jul 21, 2021

ph4r05 force-pushed the pr/010-trezor-try-empty-passphrase-first branch from 82279df to 13a8a57 Compare August 3, 2021 11:54

selsta approved these changes Sep 25, 2021

View reviewed changes

luigi1111 merged commit 7f477c1 into monero-project:master Oct 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trezor: try empty passphrase first, make trezor usage easier for users #7793

trezor: try empty passphrase first, make trezor usage easier for users #7793

ph4r05 commented Jul 14, 2021 •

edited

Loading

ph4r05 Jul 14, 2021 •

edited

Loading

ph4r05 commented Jul 15, 2021

selsta commented Jul 15, 2021

selsta Jul 15, 2021

ph4r05 Jul 15, 2021

selsta commented Jul 15, 2021

rating89us commented Jul 15, 2021 •

edited

Loading

selsta commented Jul 15, 2021 •

edited

Loading

rating89us commented Jul 15, 2021 •

edited

Loading

ph4r05 commented Jul 15, 2021 •

edited

Loading

rating89us commented Jul 15, 2021 •

edited

Loading

ph4r05 commented Jul 16, 2021 •

edited

Loading

ph4r05 commented Jul 28, 2021

ph4r05 commented Sep 13, 2021

trezor: try empty passphrase first, make trezor usage easier for users #7793

trezor: try empty passphrase first, make trezor usage easier for users #7793

Conversation

ph4r05 commented Jul 14, 2021 • edited Loading

ph4r05 Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

ph4r05 commented Jul 15, 2021

selsta commented Jul 15, 2021

selsta Jul 15, 2021

Choose a reason for hiding this comment

ph4r05 Jul 15, 2021

Choose a reason for hiding this comment

selsta commented Jul 15, 2021

rating89us commented Jul 15, 2021 • edited Loading

selsta commented Jul 15, 2021 • edited Loading

rating89us commented Jul 15, 2021 • edited Loading

ph4r05 commented Jul 15, 2021 • edited Loading

rating89us commented Jul 15, 2021 • edited Loading

ph4r05 commented Jul 16, 2021 • edited Loading

ph4r05 commented Jul 28, 2021

ph4r05 commented Sep 13, 2021

ph4r05 commented Jul 14, 2021 •

edited

Loading

ph4r05 Jul 14, 2021 •

edited

Loading

rating89us commented Jul 15, 2021 •

edited

Loading

selsta commented Jul 15, 2021 •

edited

Loading

rating89us commented Jul 15, 2021 •

edited

Loading

ph4r05 commented Jul 15, 2021 •

edited

Loading

rating89us commented Jul 15, 2021 •

edited

Loading

ph4r05 commented Jul 16, 2021 •

edited

Loading