trezor / trezor-suite

Trezor Suite Monorepo
https://trezor.io/trezor-suite
Other
695 stars 246 forks source link

Warn about non-ASCII characters in passphrase #4941

Open sime opened 2 years ago

sime commented 2 years ago

User story

As a Trezor Suite user I want to protect my privacy So that my funds are safe when my seed is compromised

As a Trezor Suite user I want to create a hidden wallet passphrase on suite And I want to confirm it on my device So that I can trust I have create a hidden wallet securely

As a Trezor Suite user I want to access my hidden wallet And I want to enter my passphrase on a device I trust (Model T) So that I can access my funds

Design: Figma

Spec: Notion

Acceptance criteria

Given I am on 'Select wallet type' When I type '€' Then I should see a 'Unable to create a wallet with this passphrase' And the 'Access Hidden wallet' button should be disabled And the 'Enter passphrase on Trezor' link should be disabled

Caveat: Backwards compatibility issue, potentially blocking users with UTF-8 characters in passphrases.

Mockup

Screenshot 2022-02-14 at 16 26 47

Screenshots

Passphrase 'Š'

IMG_20220214_155118

Passphrase '€'

IMG_20220214_160314

prusnak commented 2 years ago

Caveat: Backwards compatibility issue, potentially blocking users with UTF-8 characters in passphrases.

There should probably be some way how to override this and get back to the old behaviour?

sime commented 2 years ago

Caveat: Backwards compatibility issue, potentially blocking users with UTF-8 characters in passphrases.

There should probably be some way how to override this and get back to the old behaviour?

My current thinking is to inform (warn) the user when they enter a passhrase with non-sanctioned characters, on-device entry is then impossible.

This way there is no 'go back feature', and suite remains backwards compatible.

Hannsek commented 1 year ago

I would put there only warning that you won't be able to type the passphrase directly on device. That's it.

Hannsek commented 1 year ago

Let's put there this:

ℹ️ Special characters cannot be entered on the device.

"Learn more" button with the link: tps://trezor.io/learn/a/trezor-suite-settings#ASCII-chars

Hannsek commented 1 year ago

@Hermez-cz consider bumping a prio or put it into some of the upcoming releases.

AdamSchinzel commented 1 year ago

A bit related to #9044

andrewkozlik commented 4 weeks ago

FTR, although the BIP 39 spec allows any unicode characters in the passphrase, the SLIP 39 spec (Shamir backup) allows only printable ASCII, which is the character set that can be entered on Trezor. Unfortunately the firmware does not enforce this restriction for SLIP 39, which means that some users may have created passphrases that are incompatible with the spec.

andrewkozlik commented 4 weeks ago

After realizing the discrepancy between the SLIP-39 spec and Suite behavior, I think that we should prioritize this issue.

  1. If a user with a SLIP-39 backup enters a character into the passphrase field that is not printable ASCII, then Suite should warn them: "Non-standard passphrase. Passphrases should only contain ASCII characters. Using this passphrase may lead to compatibility issues in the future." The user should be allowed to continue though, so that those who are already using a non-standard passphrase can still access their wallet.
  2. If a user with a SLIP-39 backup enters a non-standard passphrase and Suite discovers that all wallets for the passphrase are empty and have no history, then instead of offering the user to open the wallet and continue to the "Confirm passphrase" dialog, Suite should force them to use a printable ASCII passphrase. For example: "This wallet is empty and hasn't been used before. To create a new wallet please use a passphrase with ASCII characters only."

FTR, these are printable ASCII: A - Z, a - z, 0 - 9, space, !, ", #, $, %, &, \, ', (, ), *, +, ,, -, ., /, :, ;, <, =, >, ?, @, [, \, ], ^, _, `, {, |, }, ~.

Hermez-cz commented 3 weeks ago

After realizing the discrepancy between the SLIP-39 spec and Suite behavior, I think that we should prioritize this issue.

  1. If a user with a SLIP-39 backup enters a character into the passphrase field that is not printable ASCII, then Suite should warn them: "Non-standard passphrase. Passphrases should only contain ASCII characters. Using this passphrase may lead to compatibility issues in the future." The user should be allowed to continue though, so that those who are already using a non-standard passphrase can still access their wallet.
  2. If a user with a SLIP-39 backup enters a non-standard passphrase and Suite discovers that all wallets for the passphrase are empty and have no history, then instead of offering the user to open the wallet and continue to the "Confirm passphrase" dialog, Suite should force them to use a printable ASCII passphrase. For example: "This wallet is empty and hasn't been used before. To create a new wallet please use a passphrase with ASCII characters only."

FTR, these are printable ASCII: A - Z, a - z, 0 - 9, space, !, ", #, $, %, &, \, ', (, ), *, +, ,, -, ., /, :, ;, <, =, >, ?, @, [, \, ], ^, _, `, {, |, }, ~.

Hi, the part of "Suite discovers that all wallets for the passphrase are empty and have no history" might become tricky if the user might have some funds on a coin that is currently not enabled. Otherwise all sounds like the right solution, thanks.

Hannsek commented 3 weeks ago

Therefore this should be done together with the idea of silent discovery of all coins. But we must think it through from the privacy pov.

Hermez-cz commented 3 days ago

Design and short spec

Since only the Printable ASCII character set is fully compatible with SLIP-39 backups, we’ve prepared two slightly different designs: a less strict one for BIP-39 and a more strict one for SLIP-39. Therefore, we’ll need to display the appropriate version based on the user’s current backup type.

Common for both versions If the user uses only Printable ASCII characters, don't bother the user with any extra instructions. Keep the current interface design please. Image

BIP-39 combination If the user writes any non-Printable ASCII character:

If the user deletes the non-recommended character(s):

In case of an error message shown together with the recommending element, it should look like this. The button should be inactive and carry the same copywriting as when active according to the situation. Image

SLIP-39 combination It's basically the same as for BIP-39, only the informational element and the non-recomended CTA are in a yellow warning state:

Note: Users won’t be blocked in any of the non-error states described above, as they may already have such a passphrase. We will enhance this in a future Passphrase UX, where we’ll consider the user’s mental model—whether they expect funds in the passphrase or if it’s new for them. Another improvement opportunity will come with the potential for automatic coin discovery and activation for wallets where funds are expected. (another issue related to it: https://github.com/trezor/trezor-suite/issues/14229)