Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trezor: it should be possible to use an empty passphrase #639

Closed
prusnak opened this issue Oct 30, 2022 · 11 comments · Fixed by #644
Closed

Trezor: it should be possible to use an empty passphrase #639

prusnak opened this issue Oct 30, 2022 · 11 comments · Fixed by #644

Comments

@prusnak
Copy link
Collaborator

prusnak commented Oct 30, 2022

In case a user enters empty passphrase on Trezor, it should work and a passphrase-less wallet should be open (the same as on a Trezor with passphrase protection off).

It seems that HWI disallows empty passphrases leading to confusions such as:

@moneymanolis
Copy link

@achow101 - any update / input on this from your side?

@achow101
Copy link
Member

It's not clear to me what the actual issue is, can you provide a reproduction for HWI?

I don't see anything in HWI that prevents using the empty passphrase. It's not explicitly disallowed; it should just be passing the empty passphrase straight through to the device.

@achow101
Copy link
Member

Ah, I think I see the issue now. enumerate is expecting non-empty strings for the passphrase and thus returning an error. However the actual client itself can deal with empty passphrases. If you ignore enumerate and just execute a command with a device with the empty passphrase, it should work.

@achow101
Copy link
Member

@moneymanolis Can you test #644

@moneymanolis
Copy link

Will do asap. Thanks for responding so quickly with a PR.

@moneymanolis
Copy link

moneymanolis commented Nov 17, 2022

Initial testing results @achow101:
On hwi master this does NOT work:
./hwi.py -t "trezor" -d "webusb:020:2:4" --password "" enumerate

Result is:
[{"type": "trezor", "path": "webusb:020:2:4", "label": "temp", "model": "trezor_1", "needs_pin_sent": false, "needs_passphrase_sent": true, "error": "Could not open client or get fingerprint information: Passphrase needs to be specified before the fingerprint information can be retrieved", "code": -12}]

On #644 it works, result is sth. like:
[{"type": "trezor", "path": "webusb:020:2:4", "label": "temp", "model": "trezor_1", "needs_pin_sent": false, "needs_passphrase_sent": false, "fingerprint": "59967cfd"}]

Update:
But if I run
./hwi.py -t "trezor" -d "webusb:020:2:4" enumerate

I get [{"type": "trezor", "path": "webusb:020:2:4", "label": "temp", "model": "trezor_1", "needs_pin_sent": false, "needs_passphrase_sent": true, "error": "Could not open client or get fingerprint information: Passphrase needs to be specified before the fingerprint information can be retrieved", "code": -12}]
on #644

Shouldn't that work?

Just as a sidenote if you send the pin and then go straight to signtx: master and #644 work. My expectation was, that I'd had to provide the (empty) passphrase. But okay.

I am working now on testing #644 in conjunction with Specter, too.

@achow101
Copy link
Member

But if I run ./hwi.py -t "trezor" -d "webusb:020:2:4" enumerate

I get [{"type": "trezor", "path": "webusb:020:2:4", "label": "temp", "model": "trezor_1", "needs_pin_sent": false, "needs_passphrase_sent": true, "error": "Could not open client or get fingerprint information: Passphrase needs to be specified before the fingerprint information can be retrieved", "code": -12}] on #644

Shouldn't that work?

Not necessarily. I think that's just an issue of what users expect. Do they expect it to work if passphrase protection is enabled but no passphrase is provided? Or should they expect to always need to provide a passphrase, even if it's the empty string?

I guess either way the behavior should be unified as it currently is not as you point out.

@prusnak
Copy link
Collaborator Author

prusnak commented Nov 17, 2022

@matejcik Can you please weigh in and tell us what is the behaviour that makes most sense (so it is aligned with how we do stuff in python-trezor/trezorctl/Suite)?

@moneymanolis
Copy link

Using those changes in Specter cryptoadvance/specter-desktop#1977 and the PR from hwi (#644) works for me!

@moneymanolis
Copy link

We have a bunch of Specter Desktop users who at some point had the passphrase feature enabled but didn't use it in the sense that they used an empty string only. Technically, and the Trezor teams knows this much better, the part of the PBKDF2 function (the salt part) from BIP39 where the passphrase goes is never really empty. It is always filled with the string "mnemonic" and if there is a passphrase this string is concatenated with the passphrase. So, I'd say the passphrase can only be an empty string or a non-empty string.

@matejcik
Copy link
Contributor

what is the behaviour that makes most sense (so it is aligned with how we do stuff in python-trezor/trezorctl/Suite)?

Right now there's actually a split between trezorctl and Suite.
trezorctl does the easy thing and if passphrase is enabled, always asks for it.
Suite enables passphrase by default even for people who never use the hidden wallet feature. By selecting the "default wallet", you auto-open the "" passphrase.

The Suite behavior is the more correct one UX-wise. So I believe that if the passphrase is not explicitly specified, HWI should default to the "" passphrase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants