Page MenuHomePhabricator

BotPassword can bypass CentralAuth's account lock (CVE-2018-0505)
Closed, ResolvedPublic

Description

Related with {T194204} (T194204#4203099)

Pre requirements

  • User having a change user rights privileges (*not* restrict to "userrights" permissions)

Steps to Reproduce

  1. Go to https://test.wikipedia.org with no logged in session (e.g. Open with Secret mode)
  2. Make sure you are not logged in
  3. logged in via https://test.wikipedia.org/wiki/Special:Login
  4. Create bot password from https://test.wikipedia.org/wiki/Special:BotPasswords/test20180514
  5. Make sure "Basic rights" is checked
  6. Write down the botpassword credentials
  7. Steward does lock the account ( Step 3 )
  8. Go to https://test.wikipedia.org/wiki/Special:ApiSandbox#action=query&format=json&meta=tokens&type=login
  9. Push [Make request] button
  10. copy "logintoken" value (without last '\')
  11. Go to https://test.wikipedia.org/wiki/Special:ApiSandbox#action=login&format=json
  12. input "lgname" form in botpassword's username
  13. input "lgpassword" form in botpassword's password
  14. input "lgtoken" form in that get step 10
  15. Push [Make request] button
  16. Go to https://test.wikipedia.org/wiki/Special:ApiSandbox#action=query&format=json&meta=tokens&type=userrights
  17. Push [Make request] button
  18. Write down the "userrightstoken" value
  19. Go to https://test.wikipedia.org/wiki/Special:ApiSandbox#action=userrights&format=json
  20. input "user" form or "userid" form
  21. choose add or remove rights
  22. input "token" form that get in step 17
  23. Push [Make request] button
  24. done

Problems:

  1. botpassword can be logged in when user have been locked (It seems not found an available "hook" for this)
  2. userrights can be changeable in "basic" grants.
  3. CentralAuth does not reject a request because CentralAuth does not use hook for "ChangeUserGroups"

Event Timeline

Rxy created this task.May 13 2018, 7:03 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 13 2018, 7:03 PM
Rxy triaged this task as Unbreak Now! priority.May 13 2018, 7:04 PM
Rxy added subscribers: Nemo_bis, acl*stewards.
alanajjar added subscribers: Bawolff, jrbs.

CentralAuth does not reject a request because CentralAuth does not use hook for "ChangeUserGroups"

Actually, its because central auth will block anything using that uses $title->userCan, but not things that use $wgUser->isAllowed()

+2

Rxy renamed this task from BotPassword (also maybe OAuth too) can be bypass CentralAuth's account lock to BotPassword can be bypass CentralAuth's account lock.May 13 2018, 8:10 PM

[20:09] bawolff !log Deployed patch for T194605

This should just be considered the emergency patch - Come monday, we need to take a hard look at everything botpasswords, and also how account locking works (Should also use hooks to kill all rights (except possibly read) from $wgUser->isAllowed()).

Teles added a subscriber: Teles.May 13 2018, 8:13 PM
Vituzzu renamed this task from BotPassword can be bypass CentralAuth's account lock to BotPassword can bypass CentralAuth's account lock.May 13 2018, 8:57 PM

So outstanding concerns:

  • Locking an account should probably remove all the rights (via $wgUser->isAllowed() )
  • Unclear if locking an account rotates gu_auth_token
    • Would that even be enough with redis caching, and what about local user_token session.
  • Locking does not appear to rotate bp_token, or otherwise cancel ongoing bot sessions.
Samtar added a subscriber: Samtar.May 14 2018, 8:47 AM

Given the recent abuse of botpasswords maybe T150526: BotPasswords: grant all rights is not a good idea?

revi added a subscriber: revi.May 14 2018, 9:51 AM
Tgr added a subscriber: Tgr.May 14 2018, 12:15 PM

Same considerations probably apply to owner-only OAuth consumers (non-owner-only too, but that's hard to exploit).

More generally, authentication methods like bot passwords and OAuth consumers are handled by SessionManager but their credentials are "self-contained" (unlike sessions which can be reset to force authentication to fall back to AuthManager-handled credentials), and thus SessionManager::invalidateSessionsForUser ignores them.

Also, ideally SessionManager would call User::isLocked but that method probably needs better caching first. (OTOH CheckBlocksSecondaryAuthenticationProvider not calling it either seems like an oversight that's trivial to fix.)

Anomie added a subscriber: Anomie.May 14 2018, 2:55 PM

and also how account locking works

That's probably the more useful thing to do. It seems rather ill-defined at the moment what "locking" even means and how it's different from "blocking" the user.

I note that CentralAuth locks don't necessarily try to prevent login to locked accounts, although it does on WMF wikis, at least if CentralAuthUser::canAuthenticate() is anything to go by. If $wgCentralAuthLockedCanEdit is non-empty it doesn't by itself prevent login.

So outstanding concerns:

  • Locking an account should probably remove all the rights (via $wgUser->isAllowed() )

See above. This does not seem to be what "locking" is currently intended to mean, at least for CentralAuth, since $wgCentralAuthLockedCanEdit exists.

  • Unclear if locking an account rotates gu_auth_token
    • Would that even be enough with redis caching, and what about local user_token session.

It appears that it does not.

  • Locking does not appear to rotate bp_token, or otherwise cancel ongoing bot sessions.

To cancel ongoing sessions, an extension should call SessionManager::invalidateSessionsForUser().

More generally, authentication methods like bot passwords and OAuth consumers are handled by SessionManager but their credentials are "self-contained" (unlike sessions which can be reset to force authentication to fall back to AuthManager-handled credentials), and thus SessionManager::invalidateSessionsForUser ignores them.

That "thus" shouldn't be true, and as far as I can tell it isn't since SessionManager itself resets the user's token which will cause SessionManager::loadSessionInfoFromStore() to be unable to load any stored session data.

However, invalidating the existing session does not prevent a new session from being created on the next request.

Also, ideally SessionManager would call User::isLocked but that method probably needs better caching first. (OTOH CheckBlocksSecondaryAuthenticationProvider not calling it either seems like an oversight that's trivial to fix.)

As I mentioned above, it's very unclear what "locking" a user is even supposed to do.

and also how account locking works

That's probably the more useful thing to do. It seems rather ill-defined at the moment what "locking" even means and how it's different from "blocking" the user.
I note that CentralAuth locks don't necessarily try to prevent login to locked accounts, although it does on WMF wikis, at least if CentralAuthUser::canAuthenticate() is anything to go by. If $wgCentralAuthLockedCanEdit is non-empty it doesn't by itself prevent login.

My understanding is that $wgCentralAuthLockedCanEdit was intended as an appeal feature. You'd be locked out of all other wikis, but you'd be allow to appeal your case on a specific page on e.g. Meta-Wiki. Given that the feature isn't used, I don't think it's worth keeping around...

AIUI the definition of a lock *should* be that it is impossible to log in to the account, and you're unable to perform any actions with it.

AIUI the definition of a lock *should* be that it is impossible to log in to the account, and you're unable to perform any actions with it.

yes, that's exactly what happens. If locked accounts can perform any action right now, this is not how it should work. Locked means account disabled, fully; not even able to log-in (and session terminated if locked while logged in fwiw).

Bawolff added a comment.EditedMay 14 2018, 5:19 PM

I think the fact that there is ambiguity around these concepts in our authentication flow is problematic in and of itself.

Im not super attached to lock preventing bot logins if we decide thats not the definition of "lock", but i think its a good immediate term solution well we figure this out.

It seems rather ill-defined at the moment what "locking" even means and how it's different from "blocking" the user.

Lock is supposedly a "temporary" solution until T17294 is fixed.

cc'ing @TBolliger as this arguably is somewhat related to Anti-Harassment team.

I ran into merge conflicts when applying this patch to 1.32.0-wmf.4. I think I resolved the conflict in a sane manner, however, I'd like someone else to take a look. @Bawolff: Can you look at the patch as applied on tin with commit hash of f4976d9e40496d584409e61516673b17e6f201ec and verify that I did't mess it up?

@Bawolff: Could you reply to mmodell's last comment, please? TIA!

@Bawolff: Could you reply to mmodell's last comment, please? TIA!

I replied to him on irc

@Bawolff the patch in /srv/patches/1.32.0-wmf.7/core/02-T194605.patch doesn't seem to apply cleanly to the newly cut /srv/mediawiki-staging/php-1.32.0-wmf.7 branch. It looks like there's a difference in the messages used between what's in core and what's in the patch. Could you take a look?

When I try to apply the patch, the differences I see are the patch has botpasswords-locked and wmf.7 has botpasswords-needs-reset. Did a patch for this merge into master recently?

demon added a subscriber: demon.Jun 5 2018, 8:08 PM

Locked means account disabled, fully; not even able to log-in (and session terminated if locked while logged in fwiw).

Why on earth do we even bother with such a distinction?

I got the patch applied like I think makes sense, but could still use verification/sign-off. Patch is on deploy1001 at /srv/patches/1.32.0-wmf.7/core/02-T194605-alternative.patch

greg added a subscriber: greg.Jun 5 2018, 8:19 PM

@Bawolff: Could you reply to mmodell's last comment, please? TIA!

I replied to him on irc

In the future please reply on task instead of breaking the communication across multiple mediums.

Locked means account disabled, fully; not even able to log-in (and session terminated if locked while logged in fwiw).

Why on earth do we even bother with such a distinction?

Mainly due to the lack of global blocking for accounts. Over the years, some elements of locking have grown to be more of a feature than a bug. For example, it's useful when an account is compromised to prevent the attacker from changing any personal settings once the account is locked. If global blocking was extended to accounts, it would significantly change the workflow and need for this feature.

Locked means account disabled, fully; not even able to log-in (and session terminated if locked while logged in fwiw).

Why on earth do we even bother with such a distinction?

I don't understand what do you mean. It is not a distinction but a CentralAuth feature, and the only way as of today to globally disable an account. As @Ajraddatz said, GlobalBlocking cannot be used for accounts.

demon removed a subscriber: demon.Jun 6 2018, 9:51 PM

(Which, again, is a known bug: T17294.)

Reedy lowered the priority of this task from Unbreak Now! to High.Jun 11 2018, 6:44 PM
Reedy added a comment.Jul 7 2018, 6:04 PM

There's current 2 patches 02-T194605.patch and 02-T194605-alternate.patch on deploy1001

Should we delete one of them?

Any updates? Thanks.

Also, there are two restricted tasks linked here. Could I be added to them if that's okay? (if not, I understand).

Tgr added a comment.Aug 2 2018, 12:49 PM

Also, there are two restricted tasks linked here. Could I be added to them if that's okay? (if not, I understand).

I only see one (T194204) and you are already added to that.

@Tgr I mean, this task has two parents. Both of them appear to me as Restricted Task.

hoo added a subscriber: hoo.Aug 3 2018, 10:27 AM

@MarcoAurelio These are both tracking tasks for what needs to be in future security releases, so nothing specifically relevant for this.

Legoktm closed this task as Resolved.Aug 29 2018, 7:20 AM
Legoktm assigned this task to Bawolff.

Marking as resolved since the fix for this was reviewed and deployed to Wikimedia sites. This ticket will be made public after the next MediaWiki security release.

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 20 2018, 9:34 PM
MoritzMuehlenhoff renamed this task from BotPassword can bypass CentralAuth's account lock to BotPassword can bypass CentralAuth's account lock (CVE-2018-0505).Sep 21 2018, 7:24 AM