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

Fix second usage of 2FA code #399

Merged
merged 2 commits into from
Aug 9, 2022
Merged

Fix second usage of 2FA code #399

merged 2 commits into from
Aug 9, 2022

Conversation

xwillq
Copy link
Contributor

@xwillq xwillq commented Aug 8, 2022

Method verifyKeyNewer returns true when parameter $oldTimestamp is empty i.e. when there is no such code in cache. This allowed second usage of 2FA code:

First login with 2FA code:

verifyKeyNewer gets called with null as old timestamp, so it returns true. true is saved to cache as old timestamp.

Second login with same 2FA code:

verifyKeyNewer gets called with true as old timestamp, which later down the line gets converted to 1. verifyKeyNewer uses current time and window value for creating starting timestamp and discards old timestamp, because it is smaller than created timestamp. So, if 2FA code is within the window, it gets accepted.

This pull request fixes this bug by creating new timestamp if verifyKeyNewer returned true and writing this new timestamp to cache.

@driesvints
Copy link
Member

Doesn't this defeats the purpose of the "one" in OTP?

@xwillq
Copy link
Contributor Author

xwillq commented Aug 9, 2022

Maybe description isn't clear enough (I'm not a native speaker, sorry), but current implementation allows using code twice. This pull request fixes the bug.

@xwillq
Copy link
Contributor Author

xwillq commented Aug 9, 2022

To reproduce the bug you just need to use code second time while it is active, i.e 30 * $window seconds from code generation.

@driesvints
Copy link
Member

Got it 👍

@taylorotwell taylorotwell merged commit fc248d3 into laravel:1.x Aug 9, 2022
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