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

Bump 2FA secret bit length from 80 bits to 160bits as recommended by RFC4226 #5933

Closed
evilaliv3 opened this issue May 7, 2021 · 2 comments · Fixed by #5958
Closed

Bump 2FA secret bit length from 80 bits to 160bits as recommended by RFC4226 #5933

evilaliv3 opened this issue May 7, 2021 · 2 comments · Fixed by #5958

Comments

@evilaliv3
Copy link
Contributor

While evaluating a patch for #5613 i noticed that pyotp seems to be still generating secrets of 80 bits length that is actually discouraged by RFC4226:

R6 - The algorithm MUST use a strong shared secret. The length of the shared secret MUST be at least 128 bits. This document RECOMMENDs a shared secret length of 160 bits.

The easily improve this I suggest we could just replace any call to pyotp.random_base32() in https://github.com/freedomofpress/securedrop/blob/fd2f5f86432005bdfc3c59119da98e442b7e4475/securedrop/models.py with a call to a simple function:

def generate_otp_secret():
    return base64.b32encode(os.urandom(20))
@zenmonkeykstop
Copy link
Contributor

This works with existing data (with 16-char OTP secrets) because the underlying db ignores the string lengths imposed by models.py, but it would probably be better to:

  • use pyotp >= 2.6.0, which enforces a 32-char minimum
  • update the db model to explicitly set that field to 32 chars (more for consistency's sake than anything else)

With either changes, existing 16-char secrets will still work. Any new secrets, either new accounts or otp refreshes, will be 32 chars long.

I'm not aware of any potential attacks that would require an enforced update for existing users, so haven't really looked at what would be required for that. It would be straightforward to detect a short secret and throw up a banner or something prompting the user to update. Bumping them straight to the account screen on login until they do is also a (less-friendly) option. I also haven't looked closely at implications for HOTP auth, but the secret key there is 80 bits and it's not immediately apparent to me how that can be changed.

@eloquence
Copy link
Member

See also #5613, which proposes removal of the pyotp dependency.

@zenmonkeykstop zenmonkeykstop modified the milestones: 2.0.0, 2.1.0 Jun 8, 2021
@zenmonkeykstop zenmonkeykstop removed this from the 2.1.0 milestone Sep 7, 2021
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

Successfully merging a pull request may close this issue.

3 participants