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

Modify OTP generation to run in constant time #148

Merged
merged 1 commit into from Dec 14, 2022
Merged

Modify OTP generation to run in constant time #148

merged 1 commit into from Dec 14, 2022

Conversation

Changaco
Copy link

This commit fixes the OTP.generate_otp() method to run in constant time.

Copy link

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice idea! just a small nit.

Comment on lines +21 to +22
if digits > 10:
raise ValueError("digits must be no greater than 10")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces a breaking limitation. It is needed because of the hardcoded 10_000_000_000 below. See there for a suggestion of how to deal with this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limitation already existed. It comes from the fact that the HMAC is only 32 bits, so if you try to extract more than 10 digits you'll only get extra zeroes.

The proper limit is actually 9 digits, but the Steam subclass needs 10, so I allowed 10.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HMAC is as long as the hash digest (160 bits for SHA1) but @Changaco is correct that the output of the DT transform as specified in the HOTP RFC is shorter - it actually has 31 bits of information (the high order bit is masked out). That's still over 9 decimal digits worth of information, so 10 digits (not 9) is correct.

Comment on lines +44 to +45
str_code = str(10_000_000_000 + (code % 10**self.digits))
return str_code[-self.digits:]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dec_power = 10**self.digits
return str(dec_power + code % dec_power)[1:]

@kislyuk kislyuk merged commit 0b6319b into pyauth:develop Dec 14, 2022
@kislyuk
Copy link
Member

kislyuk commented Dec 14, 2022

Thanks. The original implementation was modeled after the Java reference implementation in the RFC, which did not incorporate any constant time considerations. While I don't think it's possible to leverage this into a successful attack if the implementer follows the security guidance in the RFC and in PyOTP documentation (in particular, implements throttling), if that's not followed it may be feasible to do better-than-brute-force (though I still can't imagine building an attack from this with TOTP... maybe with HOTP if you can increment the oracle's counter at will - which implies violating other parts of the security guidance as well).

@kislyuk
Copy link
Member

kislyuk commented Dec 14, 2022

Released in v2.8.0, please test

@Changaco
Copy link
Author

You're right that the problem comes from the reference implementation in the RFC. I've submitted an errata: https://www.rfc-editor.org/errata/eid7271

fikr4n added a commit to fikr4n/pyotp that referenced this pull request Mar 24, 2023
- Modify OTP generation to run in constant time (pyauth#148)

- Documentation improvements

- Drop Python 3.6 support; introduce Python 3.11 support
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 this pull request may close these issues.

None yet

3 participants