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

Possible Vulnerabilities #4

Open
NotAFile opened this issue Jun 3, 2018 · 2 comments
Open

Possible Vulnerabilities #4

NotAFile opened this issue Jun 3, 2018 · 2 comments

Comments

@NotAFile
Copy link

NotAFile commented Jun 3, 2018

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.
@JanmanX
Copy link
Owner

JanmanX commented Jun 20, 2018

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.

@NotAFile
Copy link
Author

NotAFile commented Jun 20, 2018

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.

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

2 participants