HomePhabricator

When accepting a TOTP response, require it respond explicitly to a specific…

Authored by epriestley on Dec 17 2018, 4:40 PM.

Description

When accepting a TOTP response, require it respond explicitly to a specific challenge

Summary:
Depends on D19890. Ref T13222. See PHI873. Currently, we only validate TOTP responses against the current (realtime) timestep. Instead, also validate them against a specific challenge.

This mostly just moves us toward more specifically preventing responses from being reused, and supporting flows which must look more like this (SMS/push).

One rough edge here is that during the T+3 and T+4 windows (you request a prompt, then wait 60-120 seconds to respond) only past responses actually work (the current code on your device won't). For example:

  • At T+0, you request MFA. We issue a T+0 challenge that accepts codes T-2, T-1, T+0, T+1, and T+2. The challenge locks out T+3 and T+4 to prevent the window from overlapping with the next challenge we may issue (see D19890).
  • If you wait 60 seconds until T+3 to actually submit a code, the realtime valid responses are T+1, T+2, T+3, T+4, T+5. The challenge valid responses are T-2, T-1, T+0, T+1, and T+2. Only T+1 and T+2 are in the intersection. Your device is showing T+3 if the clock is right, so if you type in what's shown on your device it won't be accepted.
  • This may get refined in future changes, but, in the worst case, it's probably fine if it doesn't. Beyond 120s you'll get a new challenge and a full [-2, ..., +2] window to respond, so this lockout is temporary even if you manage to hit it.
  • If this doesn't get refined, I'll change the UI to say "This factor recently issued a challenge which has expired, wait N seconds." to smooth this over a bit.

Test Plan:

  • Went through MFA.
  • Added a new TOTP factor.
  • Hit some error cases on purpose.
  • Tried to use an old code a moment after it expired, got rejected.
  • Waited 60+ seconds, tried to use the current displayed factor, got rejected (this isn't great, but currently expected).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19893