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

Security risk of seeing "secret key" after the seed is originally inserted #2628

Closed
Hero-Gamer opened this issue Aug 17, 2022 · 7 comments
Closed

Comments

@Hero-Gamer
Copy link

  • A originally simple question on the telegram: "Hi, just wanted to ask a couple questions... First, is there a way to see my private seed of Hiro Wallet? And, if I am already stacking, how can add more STX to the pool? Cheers"
    Link to chat convo: https://t.me/StacksChat/312656
    Then you can just scroll down to see the full convo between Gustavo and Jw.

  • This question turned into a legitimate question around critical security vulnerability of the current Hiro wallet feature.

  • I quote the chat: "I don't think one should be able to see his seed by using the wallet program/extension after the seed is originally inserted. Yet on Hiro you have the option to see your "secret key". I think this possibility should be removed altogether from the extension"

  • Pls verify can you verify if above is true? If it is the case, sounds like it might be worth addressing quickly in light of the recent Solana wallet hack, all users in crypto are very sensitive about wallet security, including Stackers I see many conversations around Stacks ecosystem.

CC: @sjwhitely
Shout out to Gustavo for bringing this to light.

@markmhendrickson
Copy link
Collaborator

The wallet requires your password to display the Secret Key upon selecting "View Secret Key" in the app nav:

image

Is the concern here that someone might get your password then use this feature to get the Secret Key?

@Hero-Gamer
Copy link
Author

Yes.

Although we do know that if someone knows your wallet password AND has access to your file system he will be able to decrypt the private keys using the wallet password this feature.

Although possibly useful for an uncaring user who forgot to take note of his seed phrase upon wallet creation (a HUGE mistake, obviously), does represent an additional layer of vulnerability to the wallet, for a hacker could get access to all of your addresses if he gets to know your seed phrase.

@wileyj
Copy link
Contributor

wileyj commented Aug 17, 2022

There is also the concern of a compromised machine with a screen reader or keylogger, not to mention how the seed phrase is stored on the device for later retrieval.
Passwords, particularly on a local machine, can be cracked using dictionary based attacks - and depending on how the seed phrase is retrieved/displayed, it could be possible to get the plain-text seed phrase from memory.

@markmhendrickson
Copy link
Collaborator

markmhendrickson commented Aug 18, 2022

Thanks for the extra input here. The particular attack scenarios of concern appear to be:

  1. Someone has your password, accesses your file system and uses it to decrypt your Secret Key
  2. Someone maliciously installs password-cracking software (e.g. dictionary attack) that decrypts your Secret Key (as encrypted and stored in your file system)
  3. Someone maliciously installs snooping software that waits for the wallet to decrypt the Secret Key in memory
  4. Someone maliciously installs a screen reader / key logger on your machine and snoops on your Secret Key upon viewed in the wallet after password entry

To resolve scenario # 1 or # 2, we'd have to remove passwords from the wallet in general and simply ask for users to re-enter their Secret Key every time they want to unlock the wallet. This is similar to how the Stacks Wallet for desktop used to work (in Stacks 1.0 days) and risks adding UX friction if users generally have an easier time reentering passwords rather than 12- or 24-word keys.

However, there's an argument to make here that many users simply copy and paste their passwords from password managers these days anyway, since our password criteria are very tough (and make memorization difficult). So the UX friction may not be substantially different.

That said, for scenario # 1 we'd need to believe that users are more likely to expose their passwords to others vs. their Secret Keys. And for scenario # 2, we'd need to believe that the encrypted key is indeed possible to get cracked in a usefully short amount of time.

To resolve # 3 or # 4, the user would have to use a hardware wallet (e.g. Ledger) even if passwords were removed. Since presumably the user would have to enter their Secret Key each time they want to sign a transaction or message in lieu of storing it in memory (encrypted with password or otherwise). Otherwise, the snooping software or screen reader is going to capture the Secret Key in software mode / sans hardware device whenever the wallet needs it at the moment of signing (either as typed or unencrypted for usage).

@kyranjamie may have some additional thoughts here, particularly as they concern our current password encryption scheme re: scenario # 2

@kyranjamie
Copy link
Collaborator

I don't think one should be able to see his seed by using the wallet program/extension after the seed is originally inserted. Yet on Hiro you have the option to see your "secret key". I think this possibility should be removed altogether from the extension

It's important to understand that, in order to sign transactions and actually use the wallet, the wallet needs the private key. Whether or not we allow the user to view it later, it's still there. Disabling this feature offers virtually no additional security, and puts users who've lost their seed at risk.

It is not a critical security vulnerability. Metamask, among many other wallets, support this, and it's an in demand feature for the desktop wallet.

if someone knows your wallet password AND has access to your file system he will be able to decrypt the private keys using the wallet password this feature

Absolutely true. If an attacker has these things I'd be extremely concerned. There's very little we can do to prevent loss at this point. But I see little relation between this and the issue in question.

Keylogger/screen reader
If a user's machine has either a keylogger or screen reader installed, it can be considered compromised. Is the concern that if a user has both of these, but not quite full disk access/remote code execution, then if they happened to view their key then, their key could be stolen? @wileyj

It seems far-fetched to me that by not allowing a user to see their seed phrase, there'd be any additional protection for a compromised machine.

Passwords on a local machine
This seems unrelated to the view seed phrase concern. But we use zxcvbn to prevent low entropy passwords, and Argon2 with strong settings to make brute force attacks infeasible on the most powerful hardware.

I'm going to close this as it's a non issue. There's a precedent for this feature.

Always open to chat about general wallet security. There's a discussion here with some ideas to keep the in memory key more isolated from other script contexts. If there are other security concerns, open a new issue to discuss there.

@kyranjamie kyranjamie closed this as not planned Won't fix, can't repro, duplicate, stale Aug 18, 2022
@wileyj
Copy link
Contributor

wileyj commented Aug 18, 2022

@kyranjamie @markmhx awesome responses, thanks!
the details really help clarify why this works, but also how it works - I agree that the scenarios i proposed are edge-cases, but this issue arose from some concern in the community that you could see the seed phrase in the wallet.

figured it was better to ask the source vs making assumptions as to why/how it's an option.

again, awesome response/explanation - thanks again

@Hero-Gamer
Copy link
Author

Hero-Gamer commented Aug 18, 2022

  • I relayed the messages to the community member who made the comments in TG originally. He said, "Keplr (you can do all seed-related actions offline)does not offer this option among others . I do think it's not ideal from a security point of view. But I guess they have to take into consideration the fact that some people lose their seeds or fail to take note of it upon wallet creation. So it's a trade-off I guess"

  • I just checked MetaMask, in MM you can view your seeds from using a password.

  • Separately, I did a quick search on Twitter on related subject, came across a tweet from a supposedly Cyber security PhD student at RHUL, https://twitter.com/simonb03jan2009/status/1442505315203829762?t=hvSABv7NEWMv6lY8cKG3lw&s=19
    He suggested 2FA as a method. Just putting this separate security option out there as a thought. (will add this comment on the general wallet security thread too)

Thanks guys.

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

No branches or pull requests

4 participants