search

defuse / php-encryption

Simple Encryption in PHP.
MIT License
3.77k stars 309 forks source link

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

Open defuse opened 6 years ago

defuse commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 3 years ago

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.