Closed lynn-stephenson closed 1 year ago
Thank you very much for the analysis. It's excellent. You are right in all issues. When the development of the app started it was more like an experimental and UI testing. We definitely have to fix these points as soon as possible. I hope I can work on it for the next few weeks.
Sorry, I was partially wrong about point 3. Only in certain cases would it be considered vulnerable to padding oracle attacks. You (the oracle) must decrypt a chosen ciphertext and leak padding errors for it to be effective.
Just because I was wrong about the attack doesn't mean it negates the need for authenticated encryption.
Issues
The salt is predictable, static, and not sufficiently random.
Predictable, static, and non-random IVs cripple the confidentially provided by AES in CBC mode.
The current implementation of AES in CBC mode is vulnerable to padding oracle attacks due to no authentication, or integrity via a MAC.
PBKDF2 with 73 iterations is not enough for key derivation from passwords or passphrases.
SHA1's output of 160 bits is not enough for a 256 bit key. The way PBKDF2 works, leads to issues where adversaries may optimize for a faster verification method if you ask PBKDF2 for more than a hash function will output. See 1Password's implementation flaws.
Solutions
Use a CPRNG for the salt.
Use a CPRNG for the IV.
Provide authenticity and integrity via HMAC, with SHA256. Encrypt then MAC!
Use 100,000+ iterations for passwords or passphrases. If you are deriving keys from keys, a few iterations is fine.
If you are deriving a single 256 bit key, use SHA256 instead of SHA1 with PBKDF2. If you are deriving two 256 bit keys, use SHA512. Just do not ask PBKDF2 for more bits or bytes than the hash function outputs.
You should take a look at Moxie's blog post on cryptographic doom principle.
Consider using Argon2, or Scrypt in the near future for key derivation instead of PBKDF2. Also consider an AEAD cipher, like ChaCha20-Poly1305.