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

If users store a plain SHA256 hash of the password elsewhere, then KeyProtectedByPasswords can be decrypted using that hash. #392

Open
defuse opened this issue Apr 23, 2018 · 3 comments

Comments

@defuse
Copy link
Owner

defuse commented Apr 23, 2018

I made a poor choice when I was separating KeyProtectedByPassword's password-based encryption from the regular password-based encryption this library's public API exposes. I separated them by simply hashing the password once with SHA256. But now that I think about it, it's plausible that some user will store a plain SHA256 hash of the password somewhere else (e.g. as part of an insecure user authentication scheme). I should have added a domain separation string into that hash. We have three options:

  1. Ignore the problem, document it, and hope nobody does it.
  2. Fix it in a minor-version-compatible way (encryption always uses better-domain-separated hashes, but decryption will silently use whichever kind of hash it was encrypted with).
  3. In the next major version, make it refuse to decrypt the old kind, so that the only way to decrypt a KeyProtectedByPassword will be to change the password (which will create one with better domain separation) and then unlock the result of that.

(2) and (3) aren't mutually exclusive.

I bet most users aren't storing SHA256 hashes of the passwords, so doing (3) would add an unnecessary hurdle for for all those users to update to the next major version, which could be a bad thing for the ecosystem. If we don't do (3), everyone who happened to be storing a SHA256 of the password must find out on their own that they need to recreate all of their KeyProtectedByPasswords.

@defuse
Copy link
Owner Author

defuse commented Apr 23, 2018

BTW, it was @jedisct1's slides that led me to think this could be a problem: http://archive.hack.lu/2017/hacklu-crypto-api.pdf

@defuse defuse added the v2.2 label Apr 23, 2018
@defuse
Copy link
Owner Author

defuse commented Apr 23, 2018

Actually, option (2) would need to increase the major version, since, say we did that in 2.2, then someone with 2.2 should expect their KeyProtectedByPasswords to be decryptable with someone who has version 2.1, so it's not a backwards-compatible change. For 2.2, I'll add a warning to the docs, and we'll improve the domain separation in 3.0.

@defuse
Copy link
Owner Author

defuse commented Apr 9, 2021

I am not going to fix this issue but since it's a valid security issue I will leave the ticket open so people can see it instead of closing it as wontfix.

@defuse defuse added the v3.0 label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant