Skip to content
This repository has been archived by the owner. It is now read-only.

Ask user to confirm passphrase #246

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@saleemrashid
Copy link
Contributor

commented Nov 18, 2017

Asks the user to confirm the passphrase before using it.

Perhaps we should not allow the user to cancel the passphrase as the UX with TREZOR Wallet is terrible. Rather than asking again for the passphrase, TREZOR Wallet will keep sending the same passphrase unless the device is reattached or the page reloaded.

Fixes #62

@prusnak prusnak added this to the 1.6.0 milestone Nov 19, 2017

@prusnak

This comment has been minimized.

Copy link
Member

commented Nov 19, 2017

Will discuss with the team on Monday. Thanks!

Two discuss points:

a) do we want to show the dialog if the provided passphrase is empty? (e.g., password manager always sends empty passphrase)
b) should the protectButton be cancellable?

@jhoenicke

This comment has been minimized.

Copy link
Collaborator

commented Nov 19, 2017

Can we add a field, e.g., confirmOnDevice to PassphraseAck message? This way the behaviour can be triggered by PC (if it supports it). I'm not sure if an extra byte or two fits into the "tiny" packet, though.

@saleemrashid

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2017

@jhoenicke NACK. We should keep it consistent to reduce confusion.

I think the least controversial choice would be to go with "no" for both of @prusnak's points.

Regarding tiny packets, we could rewrite them to use the same code as normal packets, essentially removing their limitations.

@jhoenicke

This comment has been minimized.

Copy link
Collaborator

commented Nov 19, 2017

We should keep it consistent to reduce the confusion.

But the wallet could still keep it consistent. The wallet could see that the Trezor supports this, only asks for the passphrase once and always enables confirmation on device. And an old wallet that doesn't support the feature would just work the same way it always worked, because it doesn't set the flag. This way you avoid the problem with cancelling and also the old wallet doesn't get an unexpected ButtonRequest.

Or are you worried that a malicious wallet may disable this feature to hide that it plays tricks with the passphrase?

@saleemrashid

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2017

only asks for the passphrase once

I disagree with this sentiment of #62. I think we should continue to ask the user to confirm on the computer.

the old wallet doesn't get an unexpected ButtonRequest.

Software should be designed to always expect ButtonRequest so I don't think this is an issue

Or are you worried that a malicious wallet may disable this feature to hide that it plays tricks with the passphrase?

Yes, this is part of the issue.

@prusnak prusnak removed this from the 1.6.0 milestone Nov 20, 2017

@saleemrashid saleemrashid force-pushed the saleemrashid:confirm-passphrase branch from 5c5e50b to 39b6ed4 Nov 20, 2017

@saleemrashid

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2017

Force-pushed least controversial option #246 (comment)

@prusnak prusnak added this to the 1.6.1 milestone Nov 20, 2017

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

I have not tested this, but. There might be a problem with current implementation in trezor web wallet

Right now, on start of every action, we call "init", which erases the passphrase, and then send the passphrase, which is remembered in browser in plaintext. If we added this (if it works how I think), the user will need to confirm passphrase before every action.

It might be usable, I am not sure. I would need to try it. :)

Also, does this correctly deals with whitespace? (for example spaces after the passphrase, or 2 spaces instead of 1, or tabs/space in passphrase)

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

I have tried it on our web wallet, it works fine. We should probably remove "double" passphrase with this

@saleemrashid

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2017

@karel-3d We could store a hash of the last non-empty passphrase in memory and compare the new passphrase to that.

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

We could, but now that I am thinking about this again and actually trying it, it's not that terrible, and we would introduce possible bugs with that.

@saleemrashid

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2017

@karel-3d I don't think we can really introduce bugs with that. Also, "not that terrible" != "great" 😄

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

There are still possible attacks on this, with whitespace. You can add spaces after the passphrase and it still looks like the original passphrase. (I am not sure how limited this attack is, I am not sure how long the passphrase can be.)

@saleemrashid

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2017

@karel-3d Well, a power user will know that there are at most 16 characters per line so the attack is somewhat limited 😄

More seriously, this is a good start but you raise a good point and it may be worth revisiting #51

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

I think that we should not wait with #51 for this; even with whitespace bugs, this is better than the current state

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2018

I made a pull request to this pull request :) here

saleemrashid#1

@saleemrashid saleemrashid force-pushed the saleemrashid:confirm-passphrase branch from e3b91d4 to 832ff23 Jan 6, 2018

sha256_Raw((const uint8_t *)(ppa->passphrase), length, hash);

bool sameAsLast = memcmp(hash, passphraseHash, 32) == 0;
bool sameAsLast = memcmp(hash, passphraseHash, SHA256_DIGEST_LENGTH) == 0;

This comment has been minimized.

Copy link
@saleemrashid

saleemrashid Jan 6, 2018

Author Contributor

Non-constant time check here shouldn't be a problem because you would need to do a pre-image attack to exploit it. And the user would notice all the passphrase confirmations!

@saleemrashid

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2018

Rebased on master and merged previous confirmed hash checks. Thanks @karel-3d!

@prusnak prusnak referenced this pull request Feb 4, 2018

Closed

show passphrase on Trezor #62

@karel-3d

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2018

With the device state logic here - 7fa8ae1 - saving and checking of passphrase hash is no longer needed, since that is what device state does. (At least that is my understanding of the feature.)

So the last two commits have to be rewritten somewhat

@prusnak prusnak added the feature label Apr 2, 2018

@prusnak prusnak removed this from the v1.7.0 milestone Oct 3, 2018

AlexITC pushed a commit to wiringbits/trezor-mcu that referenced this pull request Mar 9, 2019

@prusnak

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

We'll be redesigning passphrase in the new monorepo: trezor/trezor-firmware#1

@prusnak prusnak closed this Apr 15, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.