Page MenuHomePhabricator

Do not ask for password on reauthentication when 2FA is enabled
Open, Needs TriagePublic

Description

Currently reauthentication requires the same data as a normal login - typically, for a sensitive account, password and second factor. That's suboptimal for multiple reasons:

  • Asking for the password + second factor regularly kind of conditions users to be easily phished. Other authentication services like Google's account system defend against this by never asking the password and second factor together (except on normal login), only one of them. That way, a phishing page that fakes the reauth process accurately does not get enough information to steal the account, and one that asks for all data becomes more suspicious.
  • A second factor can only be used once, and for a limited time, so someone who steals it can only do limited damage. A password + second factor together can be used to disable 2FA and have arbitrary control over the account from there on. So when reauthentication happens in a context where the attacker can steal the credentials (temporary XSS, malware, vulnerable network in case of a HTTP-accessible site), not asking for the password makes credentials theft much more dangerous.
  • If reauthentication happens relatively often enough to be mildly inconvenient to the user, having to do both steps means twice the inconvenience.

Event Timeline

Tgr created this task.Nov 4 2018, 6:04 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 4 2018, 6:04 AM

Depending on the specifics of "reauthentication", this may be a duplicate of T203256.

Depending on the specifics of "reauthentication", this may be a duplicate of T203256.

This is a different type of reauthentication. In this context we mean things like change password or email.

Tgr added a subscriber: Anomie.Dec 5 2018, 8:00 PM

@Anomie I wonder what is the best way to do this? Making non-primary providers optional using the information provided in 471664 is easy but making primary providers optional comes with a bunch of problems I failed to consider in advance:

  • primary providers add a bunch of fields to the initial set of request (such as username/password), which shouldn't be needed.
  • the auth framework itself adds the username field, which is also not needed (although this would be easy to fix).
  • conceptually, primary authentication should not happen at all (the goal of that is to identify the user but we already know who he is).

I can only think of terrible ways of dealing with that:

  1. Make a primary reauthentication provider for OATHAuth which shows a 2FA input field, and use ChangeAuthFormFields to get rid of the other fields. This duplicates code, it's a terrible hack, and it does not really work with the API.
  2. Same but instead of using the hook move the "does the user have 2FA set up?" check to core and make every primary provider opt out (not add any requests and always ABSTAIN) when that check passes. Saner and should work fine with the API but every single primary provider must be in on it.
  3. Same but instead of the OATHAuth primary adding a TOTPAuthenticationRequest, have it do an unconditional PASS, and use the usual secondary for the 2FA check. Less code duplication and web can use the autosubmit mechanism added in T141471 to hide the primary step from the user; there is no API equivalent though.
  4. Bite the bullet and make reauth a separate auth action type which can skip the primary step entirely. Makes the already very complex auth system even more complex, needs interface changes and needs to be backwards-compatible security-wise which seems tricky (given that users with no 2FA and sites with no 2FA extension still need the primary provider for reauthentication).
Anomie added a comment.Dec 5 2018, 9:33 PM

#1, #2, and #3 are all pretty bad. Of those three #3 is the least bad, but still very hacky.

#4 is better. Let's look more closely into that one.

We don't necessarily need a new action, it could work like you did in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/471664 by using ACTION_LOGIN with $options['securityLevel'].

The trick is knowing when "can skip the primary step entirely" applies. The obvious way would be to add a new method to SecondaryAuthenticationProvider to ask whether it can reauth a user, although as you point out that interface change would break any existing implementations that aren't using AbstractSecondaryAuthenticationProvider. If we want to avoid that we could make a new interface for that method, and any SecondaryAuthenticationProvider not implementing the interface would be considered to return "no" from the method.

If we go with reusing ACTION_LOGIN, then

  • When it applies getAuthenticationRequests() would return just the requests from the relevant secondaries (plus ElevatedSecurityAuthenticationRequest and whatever else turns out to be needed) and continueAuthentication() would skip running primaries and would only call the relevant secondaries. We'd need to make sure at least one of those secondaries actually returns PASS rather than ABSTAIN.
  • When it doesn't apply, we'd use exactly the code you have in gerrit:471664.
  • The web UI and API would automatically follow along with the changes you made in gerrit:471664. The worst that might happen is that meta=authmanagerinfo might result in API clients displaying just a submit button for the first step, if they don't duplicate the auto-submit logic we have in the web UI.

If we go with an ACTION_REAUTH instead, then

  • When it doesn't apply, getAuthenticationRequests() would fall back to ACTION_LOGIN (plus ElevatedSecurityAuthenticationRequest).
  • We'd probably still need the code from gerrit:471664 in continueAuthentication() to prevent the reauth switching users.
  • Whether we have beginReauthentication() and continueReauthentication() that maybe fall back to beginAuthentication() and continueAuthentication(), or just let the existing methods handle both actions, is up for grabs.
  • We might wind up making new special pages or API modules to match the new action. OTOH, neither SpecialUserLogin nor ApiClientLogin are that complex that the duplication would be much of a problem, the main downside would be existing clients trying to reauthenticate by using Special:UserLogin rather than the new special page (API clients would already be broken with gerrit:471664).

Yes, either one does make the system more complex, but it seems to me it wouldn't be that bad. We could add the complexity to continueAuthentication() having it branch based on the "can skip the primary step" flag, or we could have continueReauthentication() that sometimes calls continueAuthentication() and sometimes duplicates part of the logic.

Backwards compatibility seems straightforward enough, as old MediaWiki would continue using re-login and wikis with no 2FA extension would naturally fall back to gerrit:471664-style re-login too.