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

TOTP: Enforce single-use of TOTP one-time passwords. #517

Merged
merged 7 commits into from
Feb 13, 2023

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Feb 10, 2023

Per the TOTP spec, section 5.2:

Note that a prover may send the same OTP inside a given time-step
window multiple times to a verifier. The verifier MUST NOT accept
the second attempt of the OTP after the successful validation has
been issued for the first OTP, which ensures one-time only use of an
OTP.

Once a TOTP token is entered, it should not be able to be re-used, it's one-time.
This is a hardening ticket, making the TOTP provider comply with the spec.

For example:

  • have two unique browser sessions open
  • generate a TOTP code, CODE1
  • Login with user/password/CODE1 on window 1
  • Login with user/password/CODE1 on window 2

Per spec, both windows logging in successfully shouldn't be allowed, window 2 should fail the TOTP code and be forced to wait for the next generated one.

For a an example of why this protection exists, If someone were to be looking over your shoulder as you type in your password and authcode, they should not be able to repeat the same on their own device and succeed at logging in, because you've already used your one-time code.

This PR takes it a step further, mostly out of convenience, by making any TOTP codes for previous-timestamp-ticks be invalid once you use the future code.

In this next scenario, both windows are logging in with unique TOTP codes, but with this change window 2 will be rejected as window 1 has already logged in with a newer one-time password.

  • have two unique browser sessions open
  • generate a TOTP code, note it down as CODE1
  • generate the next TOTP code, note it down as CODE2
  • In window 1: Login with user/password/CODE2
  • In window 2: Login with user/password/CODE1

That means if someone were standing beside you, saw the first TOTP code (with 5s remaining on it), and you waited
and entered the next code generated, they should still not be able to login during the grace period with that now "old" code.

In this next scenario, both logins will proceed and login:

  • have two unique browser sessions open
  • generate a TOTP code, note it down as CODE1
  • generate the next TOTP code, note it down as CODE2
  • In window 1: Login with user/pass/CODE1
  • In window 2: Login with user/pass/CODE2

There is no minimum wait between successful logins, only that the next login must be using a later-generated code.

There are edge-cases which this does break, but I don't believe is a concern. For example, if there is a shared user account, with multiple TOTP generators, and their clocks are out of sync, and multiple people try to login at the same time, some will fail.

Note: The login nonce prevents you having two windows open on the 2FA screen, you must start each test of the flow from the user/pass login screen.

See WordPress/wporg-two-factor#3

…ed token being valid if a newer token is used.
@dd32 dd32 added this to the 0.8.0 milestone Feb 10, 2023
@dd32 dd32 requested a review from iandunn February 10, 2023 06:07
…matches that in `calc_totp()`.

This ensures that the timestamp returned is the tick time, rather than the time during the tick that it passed.
@dd32 dd32 added Bug TOTP Time-based One-time Passwords labels Feb 10, 2023
Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

Code looks good and all of the scenarios tested well for me 👍🏻

providers/class-two-factor-totp.php Outdated Show resolved Hide resolved
tests/providers/class-two-factor-totp.php Outdated Show resolved Hide resolved
dd32 and others added 4 commits February 13, 2023 14:05
Trailing commas are not allowed in function calls in PHP 7.2 or earlier
@dd32 dd32 merged commit 249e50d into WordPress:master Feb 13, 2023
@dd32 dd32 deleted the fix/TOTP-time-validation branch February 13, 2023 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug TOTP Time-based One-time Passwords
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants