Page MenuHomePhabricator

On private wikis, login form shouldn't distinguish between login failure due to bad username and bad password
Closed, ResolvedPublic

Description

Today I was logging into officewiki and mistyped my username. The wiki dutifully informed me that there's no such username registered. However, since as an anonymous user I can't see user pages or Special:ListUsers, it probably shouldn't tell me whether a username exists.

Event Timeline

matmarex created this task.May 1 2016, 10:35 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 1 2016, 10:35 AM
matmarex merged a task: Restricted Task.May 1 2016, 10:57 AM
csteipp triaged this task as Low priority.May 3 2016, 9:18 PM
csteipp added a subscriber: csteipp.

Related:
T34716: $wgWhitelistRead leaks username data (Because it allows viewing ?action=history)
{T60752}

We should eventually get all these fixed up.

Weird. Today I tried on enwikipedia - invalid username and invalid password gave same error message.

Then I tried on checkuser.wikimedia.org, where they did not give the same error.

Anomie added a subscriber: Anomie.Jul 10 2016, 7:04 PM

Weird. Today I tried on enwikipedia - invalid username and invalid password gave same error message.

Probably from CentralAuth. Since $wgCentralAuthStrict is set it doesn't abstain (fall back to other providers) when a PasswordAuthenticationRequest is submitted, and the error routine doesn't differentiate between missing account and wrong password in that case.

Then I tried on checkuser.wikimedia.org, where they did not give the same error.

Here LocalPasswordPrimaryAuthenticationProvider is abstaining (allowing fallback to other providers, if any) if a PasswordAuthenticationRequest was submitted and the user doesn't exist, which it shouldn't do when configured as authoritative (which is the default). The different error messages are "wrongpassword" when the user exists and "authmanager-authn-no-primary" (meaning all providers abstained) when it doesn't.

The fix is simple enough: change line 99 to return $this->failResponse( $req );. I don't know if you want to do that as a security patch or not.

BTW, even if the message thing is fixed, the code is still probably open to a timing attack for determining whether the password was actually tested or not.

Bawolff claimed this task.May 22 2017, 7:55 PM

We just got a msg to security@wikimedia.org about this. I want to aim to get a fix for this in the next security release (at least for the message part. Timing attack im less sure about)

@Anomie Would you be able to review this patch? It makes the LocalPassword thing be non-authoritative in the event the current user can't read Special:ListUsers. This seemed like the easiest way to fix the issue.

The fix is simple enough: change line 99 to return $this->failResponse( $req );. I don't know if you want to do that as a security patch or not.

That's the fix for the opposite problem right? (The error message isn't very descriptive for non-existent users in the public wiki case). It may make sense to change that, although I wouldn't consider it a security bug.

Anomie added a comment.Jun 7 2017, 2:25 PM


@Anomie Would you be able to review this patch? It makes the LocalPassword thing be non-authoritative in the event the current user can't read Special:ListUsers. This seemed like the easiest way to fix the issue.

Making password-using providers always non-authoritative on private wikis prevents things like $wgCentralAuthStrict from working. While CA won't be being used on a private wiki at WMF, the "don't fall back to other password-using providers" use case could still be a valid one.

The fix is simple enough: change line 99 to return $this->failResponse( $req );. I don't know if you want to do that as a security patch or not.

That's the fix for the opposite problem right? (The error message isn't very descriptive for non-existent users in the public wiki case). It may make sense to change that, although I wouldn't consider it a security bug.

Somewhat (a really accurate error for that case would be "unknown user" rather than "wrong password"), but it also fixes this problem.

The problem here is that the 'unknown user' case is returning a different error from the 'correct user with a wrong password' case. Both my proposed change and yours will fix this: mine by making both cases return "wrong password", yours by making both cases return "I don't know how to handle that submission".

The "opposite problem" is that the 'unknown user' case returns a very vague error message. My change would change that error message to "wrong password", which is slightly better even if still misleading. Your change would make every 'wrong password' case use that very vague error message on private wikis.

Roughly equivalent to your fix (but not breaking things like $wgCentralAuthStrict) would be to change the private wikis' configuration to set LocalPasswordPrimaryAuthenticationProvider as non-authoritative, with no code change necessary. That could be improved (avoiding the very vague error) by adding an "AlwaysFailPasswordPrimaryAuthenticationProvider" after LocalPasswordPrimaryAuthenticationProvider that unconditionally fails authentication with "wrong password" for every PasswordAuthenticationRequest.

Somewhat (a really accurate error for that case would be "unknown user" rather than "wrong password"), but it also fixes this problem.

Ah, my bad. I thought you were suggesting making that error with unknown user not wrong password. I see what you mean now, and I see that that would fix the problem. Although ideally I'd like something along the lines of $isPrivateWiki ? AuthenticationResponse::newFail( 'wrongpassword' ) : AuthenticationResponse::newFail( 'wronguser' ); to be less confusing in the common case.

Are there any cases where failing on the no user case could break something - e.g. if you had a primary auth thing that ran after local password auth that allowed you to log in with a username not currently in the db? I assume such cases are normally expected to have a sort earlier than the LocalPassword auth, but is that true in general?

Roughly equivalent to your fix (but not breaking things like $wgCentralAuthStrict) would be to change the private wikis' configuration to set LocalPasswordPrimaryAuthenticationProvider as non-authoritative, with no code change necessary.

I was thinking about doing that, however currently $wgAuthManagerAutoConfig is set up prior to whether or not the wiki is private. $wgAuthManagerAutoConfig could maybe be moved to Setup.php, but I'm unclear if that would break things.

Are there any cases where failing on the no user case could break something - e.g. if you had a primary auth thing that ran after local password auth that allowed you to log in with a username not currently in the db? I assume such cases are normally expected to have a sort earlier than the LocalPassword auth, but is that true in general?

Yes, if a wiki is configured with a PasswordAuthenticationRequest-using provider after LocalPassword, it would most likely break if LocalPassword returns fail rather than abstain for a missing user.

The answer there is to not configure things that way, that's why the "authoritative" flag exists.

I was thinking about doing that, however currently $wgAuthManagerAutoConfig is set up prior to whether or not the wiki is private. $wgAuthManagerAutoConfig could maybe be moved to Setup.php, but I'm unclear if that would break things.

$wgAuthManagerAutoConfig exists so extensions can auto-install themselves in the simple case. So it needs to be available for extensions to insert stuff into during registration.

You could always use 'factory' rather than 'class' in the provider-spec. ObjectFactory supports that. Or we could make LocalPasswordPrimaryAuthenticationProvider default to non-authoritative always and have AlwaysFailPrimaryAuthenticationProvider handle the failure case.

Although ideally I'd like something along the lines of $isPrivateWiki ? AuthenticationResponse::newFail( 'wrongpassword' ) : AuthenticationResponse::newFail( 'wronguser' ); to be less confusing in the common case.

The difficulty there is that you don't really know whether 'wronguser' is really correct. Say, for example, a CentralAuth-like provider abstains because it's in non-strict mode and a wrong password was supplied for the central user. Then LocalPassword doesn't find the user locally at all. "wronguser" isn't really correct, the user exists in the CentralAuth-like provider, but LocalPassword has no way to know that unless we add some data-passing. AuthManager does provide for that sort of data passing (that's how password reset happens, for example), but it might need every password-using module to be edited to set a "user exists" flag.

Reedy added a subscriber: Reedy.Nov 3 2017, 12:01 AM

This patch need some followup work?

The difficulty there is that you don't really know whether 'wronguser' is really correct. Say, for example, a CentralAuth-like provider abstains because it's in non-strict mode and a wrong password was supplied for the central user. Then LocalPassword doesn't find the user locally at all. "wronguser" isn't really correct, the user exists in the CentralAuth-like provider, but LocalPassword has no way to know that unless we add some data-passing. AuthManager does provide for that sort of data passing (that's how password reset happens, for example), but it might need every password-using module to be edited to set a "user exists" flag.

@Anomie So it seems to me that the issue is that there is a difference between being authoritative for the password, and authoritative for user existence. What are your thoughts on simply adding another flag to the LocalPasswordPrimaryAuthenticationProvider that specifies if the provider is also authoritative for user existence?

Then if an extension wants to add checks for users that aren't in local user db, they could modify $wgAuthManagerAutoConfig so the flag isn't set.

The difficulty there is that you don't really know whether 'wronguser' is really correct. Say, for example, a CentralAuth-like provider abstains because it's in non-strict mode and a wrong password was supplied for the central user. Then LocalPassword doesn't find the user locally at all. "wronguser" isn't really correct, the user exists in the CentralAuth-like provider, but LocalPassword has no way to know that unless we add some data-passing. AuthManager does provide for that sort of data passing (that's how password reset happens, for example), but it might need every password-using module to be edited to set a "user exists" flag.

@Anomie So it seems to me that the issue is that there is a difference between being authoritative for the password, and authoritative for user existence. What are your thoughts on simply adding another flag to the LocalPasswordPrimaryAuthenticationProvider that specifies if the provider is also authoritative for user existence?


Then if an extension wants to add checks for users that aren't in local user db, they could modify $wgAuthManagerAutoConfig so the flag isn't set.

On second thought, that doesn't seem right, and since the release is pending, lets just do what you suggested originally with doing failResponse() for username not existing.





Reedy closed this task as Resolved.Nov 13 2017, 5:09 PM

Closes as resolved (due to patches existing, and backports having been made), for ease of tracking in parent bugs

Tgr reopened this task as Open.Nov 13 2017, 10:37 PM

The error message says Incorrect password entered. If we want to use this for incorrect usernames as well, it should say so, to avoid frustrating users who misremember their user names. (Consider this a virtual -1; the rest of the comment is just thinking about possible follow-ups.)

Also, this is hard to solve in a generic way. If there are multiple providers (say you have a private wiki which uses both LDAP accounts and local DB accounts), a login attempt for a valid account will fail at the first provider; a login attempt for an invalid account will fail at the last provider (assuming it has similar logic to AbstractPasswordPrimaryAuthenticationProvider with the authoritative flag on; otherwise it will fail with authmanager-authn-no-primary). So not only does each provider have to make sure the "failed password" error and the "no such user" error is the same, they have to use the same message as other providers as well.

IMO the only way to handle these in a generic way is to provide some kind of utility method for providers to pass their error messages through, and on private wikis it would normalize the error message, and all providers would be expected to use it.

(This kind of problem comes up in other contexts as well, e.g. when trying to hide whether something was a password error or a captcha error.)

With the english language message adjusted:





Bawolff closed this task as Resolved.Nov 13 2017, 11:40 PM
Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Nov 15 2017, 12:02 AM

Change 391420 had a related patch set uploaded (by Reedy; owner: Brian Wolff):
[mediawiki/core@REL1_30] SECURITY: Do not reveal if user exists during login failure

https://gerrit.wikimedia.org/r/391420

Change 391420 merged by Reedy:
[mediawiki/core@REL1_30] SECURITY: Do not reveal if user exists during login failure

https://gerrit.wikimedia.org/r/391420

Change 391412 merged by Ejegg:
[mediawiki/core@fundraising/REL1_27] SECURITY: Do not reveal if user exists during login failure

https://gerrit.wikimedia.org/r/391412

Change 391450 had a related patch set uploaded (by Reedy; owner: Brian Wolff):
[mediawiki/core@master] SECURITY: Do not reveal if user exists during login failure

https://gerrit.wikimedia.org/r/391450

Change 391450 merged by Reedy:
[mediawiki/core@master] SECURITY: Do not reveal if user exists during login failure

https://gerrit.wikimedia.org/r/391450