Perhaps the bug title should be "don't count initial login as valid for any operation that requires reauth".

Making this happen would probably involve

1. Removing the setting of AuthManager:lastAuthId and AuthManager:lastAuthTimestamp from AuthManager::setSessionDataForUser().
2. Communicating "this is a security reauth" somehow into the calls to AuthManager::beginAuthentication() from ApiClientLogin and AuthManagerSpecialPage.
   1. For the API, that'll probably involve a new flag parameter for the client to indicate its intention. I'm not sure what would be appropriate for Special:UserLogin.
3. Using that "this is a security reauth" flag to decide whether to set AuthManager:lastAuthId and AuthManager:lastAuthTimestamp after the call to AuthManager::setSessionDataForUser() here (but not at any other calls).

@alanajjar my apologies for the removal, we are restricting certain tasks to only users with 2factor authentication on their Phab accounts.

@Tgr and @Legoktm my understanding is @JBennett is in communication with you both about this task.

We have discussed two fairly similar options: don't count the initial login for the reauth timer, and have a per-security level timer (so reauthing for the OAuth security level would not give you access to the password change form). I think doing the latter makes more sense: it gives the first for free (since the normal login does not have any security level), it's something we'll want because some things are not that security-sensitive and need a longer timer for some reason, and also because some things that require reauth are publicly logged so an attacker could watch for that and use the grace period to attack and do a different action. Also, we should keep in mind where we want the reauth system to be in the longer term (IMO, T197153: Make some providers optional for reauthentication) and make changes that move towards that.

With that in mind, I think we should

1. add a new $options parameter to AuthManager::beginAuthentication() and AuthManager::continueAuthentication() with a securityLevel option (and probably also a securityLevelDescription so we can tell the user in a nice detailed and localizable way what they are reauthing for)
2. stash those things in authnState (and expose to providers with something similar to AuthManager::getAuthenticationSessionData() - that's not directly relevant to this task but enables T197153)
3. set AuthManager:lastAuthSecurityLevel in AuthManager::setSessionDataForUser() and check it in AuthManager::securitySensitiveOperationStatus()

Thinking a bit more about this, I'm not sure I understand the goal of this task (although exposing the security level to the auth framework is a good idea anyway). The task description refers to an incident where the attacker pulled of a stored XSS attack affecting all users, and then changed targeted users' passwords by logging them out and doing a password change in the five-minute grace period after they logged back in. This was presumably simpler to pull off than just phishing their passwords (pretending they got logged out and sending them to a fake login page), but since he was in full control of the site he could just as well have done that. So this task does not seem too useful as a mitigation, although it might increase the workload of the attacker a bit.

A more effective change might be to only ask for the second factor during reauth; that prevents disclosure of the user's password and makes phishing for the password more suspicious. Once we move to U2F the second factor won't be phishable at all.
Of course that assumes the user having 2FA in the first place...

In T207557#4704528, @Tgr wrote:

1. add a new $options parameter to AuthManager::beginAuthentication() and AuthManager::continueAuthentication() with a securityLevel option (and probably also a securityLevelDescription so we can tell the user in a nice detailed and localizable way what they are reauthing for)

Or derive the i18n message key from securityLevel.

3. set AuthManager:lastAuthSecurityLevel in AuthManager::setSessionDataForUser() and check it in AuthManager::securitySensitiveOperationStatus()

I note that "lastAuthSecurityLevel" would mean that if you reauth for feature A, then reauth for feature B, it will cancel that reauth for feature A. Is that what we actually want?

In T207557#4704528, @Tgr wrote:

add a new $options parameter to AuthManager::beginAuthentication() and AuthManager::continueAuthentication() with a securityLevel option (and probably also a securityLevelDescription so we can tell the user in a nice detailed and localizable way what they are reauthing for)

After playing around with it a bit I don't really like this option. I'd like to add support in the future for disabling of specific providers on reauth. (Specifically, only ask for the second factor if it is set up - if the reauth happens during an XSS attack, the attacker can sniff the the password + second factor and use it to disable 2FA and has full control over the account from there on. If we only ask for the second factor, and make the disabling of 2FA require a password, then XSS-based phishing attacks become much more limited on scope as second factors are one use only.) But that would require knowledge of the security level before beginAuthentication(), in getAuthenticationRequests().

So maybe a better option would be to add $options to getAuthenticationRequests() instead, have it return a ReauthenticationRequest which is empty but includes some kind of server-side validation, and then use that for modifying the behavior of getAuthenticationSessionData(). Seems a bit contrived, but there is no easy way to pass internal state from getAuthenticationRequests() to the auth process instead. (I'd love to go back in time and split AuthManager into a stateless service and an AuthSession object which handles the stateful parts and exists from getAuthenticationSessionData() to the end of authentication.)

(@Anomie: both fair points. There would have to be an array of timestamps per security level, I suppose.)

It sounds like you're getting to the level of complication where it might make more sense to create an ACTION_REAUTHENTICATE instead of trying to reuse ACTION_LOGIN.

In T207557#4715108, @Tgr wrote:

So maybe a better option would be to add $options to getAuthenticationRequests() instead, have it return a ReauthenticationRequest which is empty but includes some kind of server-side validation, and then use that for modifying the behavior of getAuthenticationSessionData(). Seems a bit contrived, but there is no easy way to pass internal state from getAuthenticationRequests() to the auth process instead.

getAuthenticationRequests() is all about passing state to the auth process, in the form of AuthenticationRequest objects.

Why would it need to modify the behavior of getAuthenticationSessionData() though? All getAuthenticationSessionData() is for is returning data that was previously passed to setAuthenticationSessionData() during the current authentication process.

(I'd love to go back in time and split AuthManager into a stateless service and an AuthSession object which handles the stateful parts and exists from getAuthenticationSessionData() to the end of authentication.)

How would would your "AuthSession" differ from the existing Session?

In T207557#4715893, @Anomie wrote:

It sounds like you're getting to the level of complication where it might make more sense to create an ACTION_REAUTHENTICATE instead of trying to reuse ACTION_LOGIN.

That does sounds nicer, but I can't think of a sane migration path. Providers not aware of the new action just doing nothing on reauthentication could be pretty bad.

getAuthenticationRequests() is all about passing state to the auth process, in the form of AuthenticationRequest objects.

Yeah but they are persisted on the client side; only in begin* are they stored in the session. That's a problem for e.g. challange-response type providers. It can be worked around (we already do for captchas) but it's a bit awkward.

Why would it need to modify the behavior of getAuthenticationSessionData() though?

Uh, sorry, I meant to say setSessionDataForUser().

How would would your "AuthSession" differ from the existing Session?

I'll file a task to describe that idea to avoid getting sidetracked here.

This is a quick&dirty hack that only does the minimal thing described in the task, so we can have something deployed while a better solution is being discussed.

Filed T208667: Tie reauthentication (login with elevated security) to a specific security level for the more complex goal discussed above. It's not a vulnerability, and the change will be way too complex to sanely review without gerrit anyway, so tracking it in secret would be contraproductive.

In T207557#4716564, @Tgr wrote:

In T207557#4715893, @Anomie wrote:

How would would your "AuthSession" differ from the existing Session?

I'll file a task to describe that idea to avoid getting sidetracked here.

T208707: AuthManager::getAuthenticationRequests() and begin/continue* should be in the same authentication session

In T207557#4716659, @Tgr wrote:

0001-SECURITY-Do-not-trigger-elevated-security-for-normal.patch1 KBDownload

This is a quick&dirty hack that only does the minimal thing described in the task, so we can have something deployed while a better solution is being discussed.

That looks like it should work for its intended purpose.

Any objections to making this task public? There is no real vulnerability here, and private patches tend to linger forever without getting review / getting deployed.

In T207557#4751677, @Tgr wrote:

Any objections to making this task public? There is no real vulnerability here, and private patches tend to linger forever without getting review / getting deployed.

I agree that this could be made public. (For now I'll just fix the unusual view policy on this task, so that we can add subscribers to it to let them view it.)

In T207557#11820337, @matmarex wrote:

I agree that this could be made public. (For now I'll just fix the unusual view policy on this task, so that we can add subscribers to it to let them view it.)

I just made it fully public.