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.
Description
Description
Details
Details
Customize query in gerrit
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Reedy | T180272 Release MediaWiki 1.27.4/1.28.3/1.29.2 | |||
| Resolved | • demon | T169127 Release MediaWiki 1.30 | |||
| Resolved | Reedy | T168823 Tracking bug for 1.27.4/1.28.3/1.29.2 security releases | |||
| Resolved | Bawolff | T134100 On private wikis, login form shouldn't distinguish between login failure due to bad username and bad password |
Event Timeline
Comment Actions
Related:
T34716: $wgWhitelistRead leaks username data (Because it allows viewing old revisions and diffs)
{T60752}
We should eventually get all these fixed up.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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)
Comment Actions
@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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
I've just found a very old public duplicate of this: T13757: Don't say whether user exists on incorrect login info, for wikis with no anonymous viewing.
Comment Actions
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.
Comment Actions
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.
Comment Actions
Closes as resolved (due to patches existing, and backports having been made), for ease of tracking in parent bugs
Comment Actions
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.)
Comment Actions
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
Comment Actions
Change 391420 merged by Reedy:
[mediawiki/core@REL1_30] SECURITY: Do not reveal if user exists during login failure
Comment Actions
Change 391412 merged by Ejegg:
[mediawiki/core@fundraising/REL1_27] SECURITY: Do not reveal if user exists during login failure
Comment Actions
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
Comment Actions
Change 391450 merged by Reedy:
[mediawiki/core@master] SECURITY: Do not reveal if user exists during login failure