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

Passphrase Verification for Seed Backup Flow #291

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

EverydayBitcoiner
Copy link
Contributor

I added a section to the end of the seed backup flow that will offer the opportunity for a user to verify their passphrase if the seed they are backing up has a passphrase. I made this up in response to issue #90 as a way for users to verify the passphrase in the backup process while still not showing the passphrase to the user again. This should work for both the seed word backup and the QR backup flows.

One thing that I did not implement in this flow that I think may be useful is a max number of attempts at passphrase verification (like 10 or something) before the seed gets wiped from memory.

Thoughts?

@EverydayBitcoiner
Copy link
Contributor Author

Also wondering if people would want this option in the SeedOptionsView.

@jdlcdl
Copy link

jdlcdl commented Dec 31, 2022

Still on the fence after giving this some consideration... but leaning towards "I like!"

I do like the choice of having seedsigner test me on important key material... but the passphrase is already visible as I'm entering it, and it's visible once again during finalization where I also get a chance to see my new fingerprint. That fingerprint, as long as recognized, is good verification all but once in hundreds of millions... less good if only a fraction of it is recognized... personally, I want my attention on the fingerprint but others can have their preference too. Since you propose the choice to skip passphrase verification, I cannot see how it hurts, while I do recognize how it can be helpful.

I do see a bug... Not in your implementation @EverydayBitcoiner, but rather in the current flow of adding a passphrase. I'll define it as "User should be able to enter an empty passphrase." which means they effectively are NOT adding a passphrase at all. This minor bug is not harmful in any way, it's just clunky. If you go to add a passphrase and then edit the passphrase to delete all chars, you cannot get back to the seed finalization step without a passphrase... whereas it's my opinion that user should be able to back all the way out and finalize the seed without it... it feels like being stuck in a loop. The work around is trivial, finalize whatever, discard and start over; it's no big deal at all... worth looking at (boyscout rule) the next time someone checks out this code and has the attention of peers' eyes.

@jdlcdl
Copy link

jdlcdl commented Dec 31, 2022

One thing that I did not implement in this flow that I think may be useful is a max number of attempts at passphrase verification (like 10 or something) before the seed gets wiped from memory.

I'm not sure I understand this one. I think that for the rightful owner, there is no harm for them to test themselves as much as they want... until they get it right. For an attacker, they'll choose to step around this and go straight to automated brute-force attack, assuming the wrench was persuasive in getting seedqr/words AND that the fool swinging didn't realize that they might need a passphrase too.

Also wondering if people would want this option in the SeedOptionsView.

Since I'm still on the fence, but only because of my own personal preferences, I can imagine others who want this would want this:

  1. primarily when creating a new seed.
  2. otherwise only during seed-backup... but then again, why the need to test the passphrase after you already loaded the seed?

I've already expressed my concerns (in my naturally redundant nature) that seedsigner is dangerously "leaky" in regards to seed-backup being available everytime we load a seed... and no way to disable it; but this concern is already solved for this particular user. I'll continue to rehash my concern about this anytime it rears it's ugly head, and others can continue to ignore it at their risk and at their own peril.

@EverydayBitcoiner
Copy link
Contributor Author

I do like the choice of having seedsigner test me on important key material... but the passphrase is already visible as I'm entering it, and it's visible once again during finalization where I also get a chance to see my new fingerprint. That fingerprint, as long as recognized, is good verification all but once in hundreds of millions... less good if only a fraction of it is recognized... personally, I want my attention on the fingerprint but others can have their preference too.

Yeah while SeedSigner does give you options to see and view the passphrase during the input process I generaly like to double check important passwords/passphrases by reentering them from my backup (whatever form that may be). I agree that the fingerprint is extremely useful for identification, but that doesn't help me know if my backed up passphrase is right until the next time I load my seed and then I'm in trouble if I wrote it down wrong.

I do see a bug... Not in your implementation @EverydayBitcoiner, but rather in the current flow of adding a passphrase.

I didn't notice that. I'll have to take a look.

I'm not sure I understand this one. I think that for the rightful owner, there is no harm for them to test themselves as much as they want... until they get it right. For an attacker, they'll choose to step around this and go straight to automated brute-force attack, assuming the wrench was persuasive in getting seedqr/words AND that the fool swinging didn't realize that they might need a passphrase too.

You may be right, I was just trying to think through precautions and this may not be it (see below for some additional thoughts you evoked about precautions).

I've already expressed my concerns (in my naturally redundant nature) that seedsigner is dangerously "leaky" in regards to seed-backup being available everytime we load a seed... and no way to disable it; but this concern is already solved for this particular user. I'll continue to rehash my concern about this anytime it rears it's ugly head, and others can continue to ignore it at their risk and at their own peril

This is an interesting point. I'm guessing you are partially referring to your Prudently Paranoid issue #273, which I just went back and reread the comments on. My current thoughts as I've mulled over this is that it may be good to have the backup option only available right when you load/create a seed. You could have an option in the SeedOptionsMenu to verify backup (pick correct words, enter passphrase).

@kdmukai makes an good point about not relying on the UI Settings for security (#273 (comment)) and that the seed backup used to load the seed is an equivalent or greater risk, which I will concede. That said, security "leak" points could be reduced with UI Settings. A UI layer of security may not be ironclad, but it could it could certainly thwart or prevent more basic attacks or security leaks.

I am guessing that @kdmukai does not want to lead users to believe that the in-memory seeds are secure simply because they cannot be accessed via the GUI which is a valid concern, but I'm currently inclined towards shifting how we treat seed words to be similar to how we treat passphrases (#90 (comment)) where you get to see them and make sure you backed them up (i.e. go through the backup process) at the very start, but are then unavailable to view after that.

@jdlcdl
Copy link

jdlcdl commented Dec 31, 2022

This is an interesting point. I'm guessing you are partially referring to your Prudently Paranoid issue #273, which I just went back and reread the comments on. My current thoughts as I've mulled over this is that it may be good to have the backup option only available right when you load/create a seed. You could have an option in the SeedOptionsMenu to verify backup (pick correct words, enter passphrase).

Right! I get that it could motivate bad habits to rely on a "settings" parameter... but from another perspective, ability to get to seed backup, or even to access utxos via address explorer or xpub is an exceptional one-time feature that may not be wanted under normal circumstances... AND there is no way to turn these features off, even if these concerns are primary to a user's threat model. My prudently paranoid settings use-case example was a hyperbolic "what if" scenario that is likely bad practice... but I still think it could be deemed sound in some cases... and who is anyone to define another's security/threat model?

I can be more redundant, this time with questions.

Does seedsigner support single-sig?
Does seedsigner support m of n multisig where m is 1?
Does there exist seedsigner dads who might have untrust-worthy wives? or teenagers who might let the wrong friend into the house?
If I've scanned my seedqr then locked it up in a hidden safe, must I strap my seedsigner to my arm so that power is pulled as my corpse hits the ground... or can I simply lock it up and relax, and be distracted when my wife calls me to the other room with gun to her head, or when the phone rings and I have to step away from my desk? Can I have hope that the special agent in charge must consider the possibility of uncertainty that they'll get my seedsigner with loaded seed+passphrase even if they never find the right combination of xor seeds among the many they'll find... so that when they sit me down they are motivated to negotiate with me instead of getting to see all my cards for free thanks to seedsigner? Can I have some hope to motivate my attacker to keep me alive and conscious... or even that I might entice them to become part of my new security detail? The answer to all of this is NO... seedsigner will leak whatever info it knows as long as it's still powered on. It's an invite to attack me via surprise... I'd rather they consider the possibility that I'm taking it all to my grave, so they keep me alive, and so that I can take them on a tour of people and places that will make their attack much more costly.

All of this is a non issue if I just use an m of n multisig where m is greater than 1 AND none of those seeds are also used for singlesig... but seedsigner allows less than this best practice, and even allows me to load multiple seeds... but not to protect it's own interface from leaking them if surprised.

@EverydayBitcoiner
Copy link
Contributor Author

but from another perspective, ability to get to seed backup, or even to access utxos via address explorer or xpub is an exceptional one-time feature that may not be wanted under normal circumstances

I think this all depends on the user so it's hard to say what is exceptional and what is normal.

AND there is no way to turn these features off, even if these concerns are primary to a user's threat model.

I'm on board with you here though I do understand @kdmukai's concern about not giving users a false sense of security using UI settings. I do think however that UI settings, while not perfect, can improve security incrementally.

If I've scanned my seedqr then locked it up in a hidden safe, must I strap my seedsigner to my arm so that power is pulled as my corpse hits the ground... or can I simply lock it up and relax

While your seedsigner may be "locked" it is not immune to compromise, but if something cannot be access via the GUI it is inherently more secure than a raw seed. That said, I think that as a best practice users should generally treat and protect their loaded seedsigner as if it was their raw seed.

seedsigner will leak whatever info it knows as long as it's still powered on.

Not totally true as the passphrase in unavailable through the UI.

@jdlcdl
Copy link

jdlcdl commented Jan 2, 2023

Not totally true as the passphrase in unavailable through the UI.

I agree with you on this. But since the master hdkey is on the running seedsigner, we can export xpub for new coordinator and sign for those utxos.

@kdmukai
Copy link
Contributor

kdmukai commented Jan 4, 2023

I do see a bug... Not in your implementation @EverydayBitcoiner, but rather in the current flow of adding a passphrase. I'll define it as "User should be able to enter an empty passphrase." which means they effectively are NOT adding a passphrase at all. This minor bug is not harmful in any way, it's just clunky. If you go to add a passphrase and then edit the passphrase to delete all chars, you cannot get back to the seed finalization step without a passphrase... whereas it's my opinion that user should be able to back all the way out and finalize the seed without it... it feels like being stuck in a loop.

I think the best bugfix here would be to allow an empty passphrase. However, some minor stupid danger in distinguishing an empty passphrase from one that just consists of invisible space(s).

Maybe in the empty case, the onscreen messaging can be different. Literally: "No passphrase specified" or something like that. And maybe below that: "Fingerprint remains: abcd1234".

This is probably all too wordy, but you get the idea.

@jdlcdl, can you log this as a separate issue and maybe tackle the fix?

@kdmukai
Copy link
Contributor

kdmukai commented Jan 4, 2023

@EverydayBitcoiner I'm not convinced that we need another opportunity to verify the passphrase.

However, I do think it's awkward to go through the backup process for a seed that we know was loaded with a passphrase. At the very least a warning at the start of the backup process should point out that the current fingerprint (result of seed + passphrase) is different from the "naked" seed that's about to be backed up.

A whole side discussion can be had about the wisdom of writing down just the "naked" seed's fingerprint on your backup or if it should reference instead/also the seed+passphrase's fingerprint.

@kdmukai
Copy link
Contributor

kdmukai commented Jan 5, 2023

but I'm currently inclined towards shifting how we treat seed words to be similar to how we treat passphrases (#90 (comment)) where you get to see them and make sure you backed them up (i.e. go through the backup process) at the very start, but are then unavailable to view after that.

That seems the most reasonable/consistent approach. The only thing really giving me pause is that the UI just doesn't have room. We'd be adding "Back up seed" alongside "Finalize seed". Having three menu options is somehow exponentially more confusing than simply choosing between two options. And adding an additional step ("Backup? Or continue?") is a big no; loading a seed needs to remain as streamlined as possible.

@EverydayBitcoiner
Copy link
Contributor Author

EverydayBitcoiner commented Jan 5, 2023

@EverydayBitcoiner I'm not convinced that we need another opportunity to verify the passphrase.

I did this mainly because there is some additional verification that comes from actually reentering the passphrase from wherever it is stored (paper, password manager, etc.). This is more thorough and IMO better than simply visually comparing my stored passphrase with the passphrase on the SeedSigner screen. I know that I can often make simple mistakes visually comparing things and the additional step of optionally entering the passphrase from the backup to ensure it was not accidentally written down wrong ensures the integrity of the passphrase better than visually comparing.

However, I do think it's awkward to go through the backup process for a seed that we know was loaded with a passphrase. At the very least a warning at the start of the backup process should point out that the current fingerprint (result of seed + passphrase) is different from the "naked" seed that's about to be backed up.

Yes, this one of the things that I thought about. Currently I could see someone thinking they backed up the whole thing and not know or forget they are missing the passphrase.

A whole side discussion can be had about the wisdom of writing down just the "naked" seed's fingerprint on your backup or if it should reference instead/also the seed+passphrase's fingerprint.

I was thinking about this and trying to figure out what the best option is. If the fingerprint stored with the backup is not the naked fingerprint it would give a potential adversary the information that there is a passphrase while the naked fingerprint would not. As you say, this is a whole different discussion.

That seems the most reasonable/consistent approach. The only thing really giving me pause is that the UI just doesn't have room. We'd be adding "Back up seed" alongside "Finalize seed". Having three menu options is somehow exponentially more confusing than simply choosing between two options. And adding an additional step ("Backup? Or continue?") is a big no; loading a seed needs to remain as streamlined as possible.

This could just be a differentiation between loading a seed vs. creating a seed. Most of my concerns here would generally be during the creation and backup of a new seed. When you load a seed (enter words, scan QR code) I don't know if there should be a backup option available (only scenario I can think of where that could be useful is if you only have a QR and want the words or have the words and want the QR -> this could maybe be an option in tools). I think maybe the creation and backup of a seed should be a single flow (gonna have to be careful here though to not confuse users given that the status quo is that you can backup a key anytime).

@jdlcdl
Copy link

jdlcdl commented Jan 5, 2023

@jdlcdl, can you log this as a separate issue and maybe tackle the fix?

Yes @kdmukai. I will give this a look with fresh eyes in the morning.

@kdmukai
Copy link
Contributor

kdmukai commented Mar 2, 2024

Lots of great discussion and possible next steps above, but specifically for the change offered here, concept NACK.

Backing up your naked seed / mnemonic is just a conceptually different activity from entering and verifying potentially many different bip39 passphrases for that one mnemonic. This PR's flow suggests / potentially misleads the user into thinking that the backup represents or only works for the currently-loaded mnemonic+passphrase combo.

Recommend this PR be closed, BUT very interested in pursuing some of the side changes discussed along the way.

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

Successfully merging this pull request may close these issues.

None yet

3 participants