EncryptTextApp icon indicating copy to clipboard operation
EncryptTextApp copied to clipboard

Possible Vulnerabilities

Open NotAFile opened this issue 7 years ago • 2 comments

I saw this in f-droid and felt like checking your crypto code.

I'm not familiar with the Java crypto libraries, but from what I can tell, it has a number of crypto issues:

  • it appears you are using SHA256 for key derivation. This is very fast to compute (and brute force) and unsuitable for deriving an AES key from. Consider using, say, PBKDF2 instead.
  • You are using CBC, however it appears you are not using a Message Authentication Code to verify the integrity of the message. This means your code is, at least theoretically, vulnerable to a padding oracle, and message modification. You can verify the integrity of the ciphertext with e.g. a sha256 HMAC to prevent this.
  • it appears you are using a hardcoded IV. This is un-ideal in general, but with some modes, like CBC, it is potentially catastrophical, in this case if the same message is encrypted with multiple keys.

NotAFile avatar Jun 03 '18 10:06 NotAFile

Hi! First off, thanks for looking through the code. I really appreciate someone taking the time to reading the code and giving feedback, so that the application can be improved.

I should also mention that I am no cryptographer, so most of my knowledge about encryption is from Wikipedia and other encryption articles on the internet. Please correct me if any of my statements are wrong!

  1. Hashing algorithm for key derivation

Good catch, I was not aware that there was a difference in this aspect. From what I can see, SHA256 is a great hashing function. The trouble is, it is too fast compared to PBKDF2, which is designed to be slow.

While I do not believe SHA256 in itself is a weakness, too short passwords might be open to bruteforce attacks.

I will investigate this further. The change is quite small, but I am afraid this will prevent users from deciphering text encrypted by the previous version of the application.

  1. No Message Authentication Code

I am aware that the application cannot detect if the encrypted message has been tempered with. Adding a checksum would be trivial, but this would increase the message length by at least 32 bytes for SHA256. This application is meant to be very lightweight, and adding additional 32 bytes to, for example, SMS messages would be inconvenient to the user. Highlighting large chunks of text is also tedious.

I think the simplicity outweighs the burden of having to occasionally resend an encrypted message.

  1. Hardcoded IV

This is absolutely my fault, I misunderstood how the IV actually worked. While the IV should be different every time, it does not have to be kept secret, and can thus be transported with the cipher-text.

Although this appends more bytes to the message, this actually improves the security of the application. I will try to implement this ASAP.

JanmanX avatar Jun 20 '18 12:06 JanmanX

Thank your for your response! I'll add a few thoughts:

I am afraid this will prevent users from deciphering text encrypted by the previous version of the application

This is true, it might be wise to add a "version" byte to the front for the future. It might also be possible to add some kind of fallback mode that retries with the old key derivation method when decryption with the new one fails.

While I do not believe SHA256 in itself is a weakness, too short passwords might be open to bruteforce attacks.

It is true that SHA256 is not broken itself, but it is still a weakness in the way you are using it. Average passwords with SHA2556 are very much in reach of being cracked with cheap hardware these days. Using an computationally expensive key derivation function to slow down attackers by, say, 100 000x will increase protection from password cracking for all users, even the ones with short passwords. The length of "too short" passwords will also only rise over time, as hardware advances.

Adding a checksum would be trivial, but this would increase the message length by at least 32 bytes for SHA256

This is true. You could truncate the HMAC as a workaround, it would still be better than nothing. I think being able to definitively say a message is valid or not valid is also valuable. The cryptographer in me hates the idea of using CBC without very strong message authentication, but for human-to-human communication, it's probably tolerable.

NotAFile avatar Jun 20 '18 19:06 NotAFile