Skip to content
This repository has been archived by the owner on Jun 14, 2022. It is now read-only.

Secure PBKDF2 use #975

Open
py0xc3 opened this issue Mar 24, 2022 · 1 comment
Open

Secure PBKDF2 use #975

py0xc3 opened this issue Mar 24, 2022 · 1 comment

Comments

@py0xc3
Copy link

py0xc3 commented Mar 24, 2022

First, thank you for maintaining this wonderful tool!

Is your feature request related to a problem? Please describe.
PBKDF2-HMAC-SHA1 is no longer recommended, and may become a problem if there are too few iterations (SHA1 is the issue, especially in conjunction with the small number of iterations), although a 16byte/128bit salt remains fine. This only offers security if used in conjunction with very strong passwords (which most users are generally unlikely to use, especially when they have to often input it on their smartphone pads; on phones, it is often just short passwords, with limited numbers of characters, etc.).

This is not related to any collission attack or so. HMAC-SHA1 is not generally broken but the issue is that it is possible to try a lot of passwords with low computational power (and without much RAM; eventually further accelerated by, e.g., GPUs) in a short amount of time, especially if the number of iterations is low. PBKDF2 also does not solve this weakness, unlike Argon (thus, the hash algorithm and the iterations have to make the difference). The issue is facilitated by the Intel SHA extensions, which are currently introduced to new architectures. Therefore, as it is at the moment in andOTP, the user password has to make a big difference on itself.

This issue is related to password storages, and not related to the hashes in the implementations of the HOTP and TOTP RFCs.

For the external readers: this is not a critical vulnerability or anything like that (just to avoid misunderstandings). So keep using your andOTP ;)

I got the information with git grep --text PBKDF2. Unfortunately, I have no Java capabilities.

Describe the solution you'd like

Initially, increase the iterations (e.g., OWASP recommends 720 000 if SHA1 cannot be avoided, but generally recommends to replace it with SHA256, 310 000 iterations, or SHA512, 120 000 iterations).

Then, PBKDF2 with 128bit salt is fine when used with a strong hash. E.g., SHA256 or SHA512. Iterations, e.g., as suggested by OWASP. This should already create a much higher level of security without much code adjustments. It may be noted that the Intel SHA extensions will only support SHA1 and SHA256. So, if their use should be avoided, SHA512 would be the choice (however, sha256 with sufficient iterations remains absolutely fine).

On the long term, it makes sense to implement Argon2. Given the use case, the performance disadvantages seem not relevant. However, adjusted PBKDF2 as described above should already make a big difference and keeps the coding efforts limited.

There are several recommendations from experienced organizations that could be used as reference, for both Argon2 and PBKDF2, not just from OWASP. Feel free to investigate :)

Describe alternatives you've considered
None.

Additional context
The issue is only relevant if someone has access to andOTP or to the storage. But this is what PBKDF2 is trying to protect in this case anyway (but currently only with strong passwords).

@flocke
Copy link
Member

flocke commented Mar 26, 2022

Thank you for the detailed description. I fully agree with your assessment of the problem.

However I am currently in the process of rewriting andOTP from scratch. In that rewrite I am going to use Argon2 for key derivation.

But I have no timeline for the rewrite, as I don't have a lot of time to work on it at the moment. So in the meantime increasing the iterations should be a decent short-term solution.

@flocke flocke added this to the v0.9.1 milestone Mar 26, 2022
ironhaven added a commit to ironhaven/andOTP that referenced this issue Apr 21, 2022
This solves issue andOTP#975 by rasing the number of iterations to be more
than 720,000. Also for the sake of simplicity all new backups will
use the same number of iterations insead of a random number in a
small range. This change is backwards compatable with the old backups
that had a random iteration count.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants