Page MenuHomePhabricator

Allow different MFA factor types (SMS, TOTP, Duo, ...) to share "sync" tokens when enrolling new factors
ClosedPublic

Authored by epriestley on Jan 23 2019, 4:42 PM.

Details

Summary

Depends on D20019. Ref T13222. Currently, TOTP uses a temporary token to make sure you've set up the app on your phone properly and that you're providing an answer to a secret which we generated (not an attacker-generated secret).

However, most factor types need some kind of sync token. SMS needs to send you a code; Duo needs to store a transaction ID. Turn this "TOTP" token into an "MFA Sync" token and lift the implementation up to the base class.

Also, slightly simplify some of the HTTP form gymnastics.

Test Plan
  • Hit the TOTP enroll screen.
  • Reloaded it, got new secrets.
  • Reloaded it more than 10 times, got told to stop generating new challenges.
  • Answered a challenge properly, got a new TOTP factor.
  • Grepped for removed class name.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jan 23 2019, 4:42 PM
epriestley requested review of this revision.Jan 23 2019, 4:44 PM
epriestley added inline comments.Jan 23 2019, 4:50 PM
src/applications/auth/factor/PhabricatorAuthFactor.php
289–301

This is a new piece of behavior: we didn't previously have this limit. This is mostly so that we don't send you a trillion SMS messages if you reload the form a bunch of times.

(But: I think attackers can currently CSRF this endpoint. Not the end of the world, but I'll make a note and fix that.)

321

Now, the token code is always a random digest, and the factor adds any secret properties it wants to the token body.

Before, for TOTP, the code and the secret were the same, but this was just convenient since they happened to be reasonable substitutes for one another.

For SMS, the secret will be the code we texted you ("12345678"). For duo, it will be something like a transaction ID. These don't make good token codes to identify the challenge you're responding to since they aren't sufficiently random/unpredictable.

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
41

This is slightly new. Before, we put totp = true in the form so we could tell if you submitted it or not.

However, we can do this check in a slightly easier way by just testing if we generated or loaded a sync token. If we generated a new one, you can't have a response yet. If we loaded one, you definitely submitted the form so we expect a response.

107–108
  • "totp" (which meant "did the user submit a guess?") has been simplified into "is new token?", above.
  • "totpkey" (which identified the sync token) is now part of the sync token code.
src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
269–274

This is new. We didn't revoke the tokens before since it's mostly pointless, but at least does something now that we have a limit on outstanding, unexpired sync attempts.

amckinley accepted this revision.Jan 23 2019, 7:33 PM
This revision is now accepted and ready to land.Jan 23 2019, 7:33 PM
This revision was automatically updated to reflect the committed changes.