Page MenuHomePhabricator

CentralAuth uses wrong actor ID when locally suppressing the user (CVE-2020-25869)
Closed, ResolvedPublicSecurity

Description

While investigating T260455, I noticed that CentralAuth uses metawiki's actor ID when inserting a local ipblocks entry to suppress an account globally.

See my prod db queries below:

wikiadmin@10.64.32.77(enwiki)> select * from ipblocks where ipb_user=39840215\G
*************************** 1. row ***************************
              ipb_id: 9978832
         ipb_address: /REDACTED INFO/
            ipb_user: 39840215
       ipb_reason_id: 320564205
       ipb_timestamp: 20200729041051
            ipb_auto: 0
       ipb_anon_only: 0
  ipb_create_account: 1
          ipb_expiry: infinity
     ipb_range_start:
       ipb_range_end:
ipb_enable_autoblock: 1
         ipb_deleted: 1
     ipb_block_email: 1
        ipb_by_actor: 51123802
  ipb_allow_usertalk: 0                                                                                                                                                                                             ipb_parent_block_id: NULL                                                                                                                                                                                                 ipb_sitewide: 1                                                                                                                                                                                            1 row in set (0.00 sec)   

wikiadmin@10.64.48.150(enwiki)> select * from actor where actor_id=51123802;
Empty set (0.00 sec)

wikiadmin@10.64.0.91(metawiki)> select * from actor where actor_id=51123802\G
*************************** 1. row ***************************
  actor_id: 51123802
actor_user: NULL
actor_name: Meta>MusikAnimal
1 row in set (0.00 sec)

Hypothesis: MediaWiki doesn't show the block in the blocking interface, because the actor ID is wrong (non-existent).

Impact: Special:Block doesn't say anything about the block, the block doesn't take effect and it is not possible to change/alter/remove the block.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Putting this back into the PET inbox, so we can think about how to prioritize and schedule the work of getting the fix into master, backporting it to 1.35, and making improvements beyond the present minimal fix, e.g. T260933.

One thing that still needs doing is to confirm that this issue does not exist in the 1.34 release. If it does, this needs to remain private until a security release is out for 1.34.

Another thing to do is write a script to clean up broken rows in the ipblocks table. I'm actually not quite sure how to best fix them.

This comment was removed by daniel.

By the way, a big shout out to @Urbanecm, who wrote the patch, and helped me to test it! Thank you!

Urbanecm lowered the priority of this task from Unbreak Now! to Needs Triage.Aug 24 2020, 2:59 PM

Tested in production via Special:Centralauth with UrbanecmTest, and it works:

wikiadmin@10.64.32.76(enwiki)> select * from ipblocks where ipb_user=(select user_id from user where user_name="UrbanecmTest")\G
*************************** 1. row ***************************
              ipb_id: 10028120
         ipb_address: UrbanecmTest
            ipb_user: 28337545
       ipb_reason_id: 330596334
       ipb_timestamp: 20200824145403
            ipb_auto: 0
       ipb_anon_only: 0
  ipb_create_account: 1
          ipb_expiry: infinity
     ipb_range_start:
       ipb_range_end:
ipb_enable_autoblock: 1
         ipb_deleted: 1
     ipb_block_email: 1
        ipb_by_actor: 201972434
  ipb_allow_usertalk: 0
 ipb_parent_block_id: NULL
        ipb_sitewide: 1
1 row in set (0.00 sec)
wikiadmin@10.64.32.76(enwiki)> select * from actor where actor_id=201972434\G
*************************** 1. row ***************************
  actor_id: 201972434
actor_user: NULL
actor_name: Meta>Martin Urbanec
1 row in set (0.00 sec)

Manually lifting the CA-placed block works as expected, but lifting the block by unsuppressing the account doesn't. I'll search if that's known, and if not, I'll create a ticket. Anyway, since suppressing works, I've deleted temporary CSS/JS from meta that disabled the functionality:

 16:53, 24 August 2020 Martin Urbanec talk contribs block deleted page MediaWiki:Group-steward.js (T260485 should be resolved) (view/restore)
16:53, 24 August 2020 Martin Urbanec talk contribs block deleted page MediaWiki:Group-steward.css (T260485 should be resolved) (view/restore)

and sent a follow-up email to stewards announcing this is fixed. Thanks for the deployment, @daniel.

I'm also resetting the priority to Needs Triage, given this no longer is an UBN from my perspective.

[...]
Another thing to do is write a script to clean up broken rows in the ipblocks table. I'm actually not quite sure how to best fix them.

Created T261143 for that.

[...]
One thing that still needs doing is to confirm that this issue does not exist in the 1.34 release. If it does, this needs to remain private until a security release is out for 1.34.

I've checked that, and 1.34 is affected as well. @sbassett @Reedy Would it be possible to get it backported to REL1_34 as well? Thanks!

[...]
One thing that still needs doing is to confirm that this issue does not exist in the 1.34 release. If it does, this needs to remain private until a security release is out for 1.34.

I've checked that, and 1.34 is affected as well. @sbassett @Reedy Would it be possible to get it backported to REL1_34 as well? Thanks!

FWIW, the security team does not have to create the backport patches

I'm pinging you, because that was what I did at past security tasks I participated in, and it "worked" :-). I think it's better if the sec team does that, as it de facto publishes the vulnerability :-). However, I'm happy to abide by any SOP, if there is any on that matter.

I'm pinging you, because that was what I did at past security tasks I participated in, and it "worked" :-). I think it's better if the sec team does that, as it de facto publishes the vulnerability :-). However, I'm happy to abide by any SOP, if there is any on that matter.

It really makes no difference who backports or releases the fixes

We're accepting this for clinic duty in the platform team. We believe that the backporting and data corruption cleanup should be their own tasks, but we'll add separate tickets for each of those

Thanks @eprodromou . Here's a task for the data corruption: T261143

Backports for 1.34 and 1.35:

Untagging the Platform Engineering team. There's nothing left to do for us here. I have put the relevant subtasks on our boards, though, in particular one for fixing broken blocks in the database.

The patches by Urbanecm fix the problem for a known case.
Here is another patch, defending against other callers making the same mistake, causing User::getActorId() to fail when called with a DB connection that is not for the local wiki.

This patch should be deployed to a debug host first, to see if it makes anything explode.

I was originally thinking we could put this patch on gerrit, since by itself it's relatively inconspicuous. However, it will break CentralAuth in the absence of the security fix. So since it depends on a security fix, it has to be handled as a security fix itself...

The patches by Urbanecm fix the problem for a known case.
Here is another patch, defending against other callers making the same mistake, causing User::getActorId() to fail when called with a DB connection that is not for the local wiki.

This patch should be deployed to a debug host first, to see if it makes anything explode.

I was originally thinking we could put this patch on gerrit, since by itself it's relatively inconspicuous. However, it will break CentralAuth in the absence of the security fix. So since it depends on a security fix, it has to be handled as a security fix itself...

On line 34, is the comparison if ( $dbDomain !== $db->getDomainID() ) { meant to be if ( $dbDomain !== $localDomain ) {?

On line 34, is the comparison if ( $dbDomain !== $db->getDomainID() ) { meant to be if ( $dbDomain !== $localDomain ) {?

Of course. Silly me. Thanks!

Sadly, this is still not fixed :/. My patch apparently fixed this only for cases when the steward does have a local actor ID, but not for cases when the actor ID needs to be generated :/. You can query https://meta.wikimedia.org/w/index.php?title=Special:CentralAuth&target=T260485+test+cswiki vs https://meta.wikimedia.org/w/index.php?title=Special:CentralAuth&target=T260485+test+dewiki, I have actor ID for Meta>Martin Urbanec at cswiki, but not at dewiki.

The reason is that if my new sec function ActorMigration::getExistingActorId() doesn't get the ID, it calls load, and the story is the same. A possible workaround:

The reason is that if my new sec function ActorMigration::getExistingActorId() doesn't get the ID, it calls load, and the story is the same. A possible workaround:

Calling setItemLoaded() may not solve the issue, since the User object may already have a local actor ID loaded, which would be the wrong one again.

I think we wont get around moving the code for assigning a new actor ID into ActorMigration. I'll give that a go.

@daniel Good point, so...what about this?


@Urbanecm I moved the code for assigning an actor ID to ActorMigration. I think that's where it belongs, really. This should fix the issue.

@daniel I was thinking about a possible nicer solution (to make security patches light, as they should be), and this is my idea: what about skipping call to User::load for good, until this is properly fixed? Patch above.

@daniel I was thinking about a possible nicer solution (to make security patches light, as they should be), and this is my idea: what about skipping call to User::load for good, until this is properly fixed? Patch above.

Looks like we had the same idea :)


@Urbanecm I moved the code for assigning an actor ID to ActorMigration. I think that's where it belongs, really. This should fix the issue.

+1. Works as expected, see my following shell.php fiddling.

{P12423}

I tested both cases when the actor ID did not exist yet in target wiki, and when it did, both with User::getActorId and ActorMigration.


@Urbanecm I moved the code for assigning an actor ID to ActorMigration. I think that's where it belongs, really. This should fix the issue.

+2, approved to the quoted patch. For clarity, here are the first few lines from the approved .patch file:

From 84b37d0d9d588b8fd16e5dcb0007702fc255a28d Mon Sep 17 00:00:00 2001
From: daniel <dkinzler@wikimedia.org>
Date: Mon, 31 Aug 2020 16:03:36 +0200
Subject: [PATCH] SECURITY: ensure actor ID from correct wiki is used.

This builds on top of Urbanecm's patch, now also covering the case
where the actor ID does not exist in the target DB, but does exist in
the local DB.

Bug: T260485
Change-Id: I2336954c665366a99f9995df9b08071d4de6db79

Oh, and for the record, I ran phan and the full phpunit suite of tests locally against Daniel's patch.

Oh, and for the record, I ran phan and the full phpunit suite of tests locally against Daniel's patch.

Many thanks!

We can't deploy today, due to the DC switchover. But we can do it tomorrow.

Can someone clarify the state of these?

In WMF prod there is 07-T260485.patch and 08-T26048-5.patch (it's obviously fine there's two patches), but it's unclear from the comments which they actually refer to in/from this task, and as such, which were actually deployed

Especially as F32231898 (marked as -02 in the name) doesn't match 08-T260485-2.patch on deploy1001

I guess the backports in T260485#6413123 probably aren't useful now (ie superceded).

reedy@deploy1001:/srv/patches/1.36.0-wmf.9/core$ cat 07-T260485.patch 
From e95b7202e16f77c02edaaa58354d769badf4db67 Mon Sep 17 00:00:00 2001
From: Martin Urbanec <martin.urbanec@wikimedia.cz>
Date: Thu, 20 Aug 2020 21:51:51 +0200
Subject: [PATCH] SECURITY: ActorMigration: Load user from the correct database

In ActorMigration::getInsertValues, when creating a User object, calling
User::getActorId triggers a call to User::load, which ignores
the database passed to getInsertValues, meaning incorrect actor IDs
are returned.

To ensure that the correct (foreign) database is used, try
to get the actor ID from the correct database within ActorMigration
service, and if that fails, let User class handle the actor ID creation.

Todo notes are left in the patch to fix the issue properly,
by making User object wiki-aware.

Bug: T260485
Change-Id: Iaa886a1824e5a74f4501ca7e28917c780222aac0
---
reedy@deploy1001:/srv/patches/1.36.0-wmf.9/core$ cat 08-T260485-2.patch 
From 84b37d0d9d588b8fd16e5dcb0007702fc255a28d Mon Sep 17 00:00:00 2001
From: daniel <dkinzler@wikimedia.org>
Date: Mon, 31 Aug 2020 16:03:36 +0200
Subject: [PATCH] SECURITY: ensure actor ID from correct wiki is used.

This builds on top of Urbanecm's patch, now also covering the case
where the actor ID does not exist in the target DB, but does exist in
the local DB.

Bug: T260485
Change-Id: I2336954c665366a99f9995df9b08071d4de6db79
---

And does it affect REL1_31?

Also an update on T261325/T261143 would be useful (I see there is a patch).. And as such, whether the intention is to backport that too

Can someone clarify the state of these?

In WMF prod there is 07-T260485.patch and 08-T26048-5.patch (it's obviously fine there's two patches), but it's unclear from the comments which they actually refer to in/from this task, and as such, which were actually deployed

As well as I can reconstruct it, the patches in prod are from T260485#6406192 and T260485#6423760. I cited the sha1 if the file, Bill cited the whole commit message, to remove ambiguity. Should be possible to check against those.

For MW-1.31-release

$ git diff
diff --cc includes/ActorMigration.php
index 161c7a923b,192e0409c6..0000000000
--- a/includes/ActorMigration.php
+++ b/includes/ActorMigration.php
@@@ -201,12 -278,19 +218,26 @@@ class ActorMigration 
                        $ret[$key] = $user->getId();
                        $ret[$text] = $user->getName();
                }
++<<<<<<< HEAD
 +              if ( $this->stage >= MIGRATION_WRITE_BOTH ) {
 +                      // We need to be able to assign an actor ID if none exists
 +                      if ( !$user instanceof User && !$user->getActorId() ) {
 +                              $user = User::newFromAnyId( $user->getId(), $user->getName(), null );
++=======
+               if ( $this->stage & SCHEMA_COMPAT_WRITE_NEW ) {
+                       // UBN fix for T260485 - do not let User object to handle existing actor IDs
+                       // TODO: Make User object wiki-aware and let it handle all cases.
+                       $existingActorId = $this->getExistingActorId( $dbw, $user );
+                       if ( $existingActorId !== false ) {
+                               $ret[$actor] = $existingActorId;
+                       } else {
+                               // We need to be able to assign an actor ID if none exists
+                               if ( !$user instanceof User && !$user->getActorId() ) {
+                                       $user = User::newFromAnyId( $user->getId(), $user->getName(), null );
+                               }
+                               $ret[$actor] = $user->getActorId( $dbw );
++>>>>>>> SECURITY: ActorMigration: Load user from the correct database
                        }
-                       $ret[$actor] = $user->getActorId( $dbw );
                }
                return $ret;
        }

https://github.com/wikimedia/mediawiki/blob/REL1_31/includes/ActorMigration.php#L193-L212

The only conflict seems to be the change of the conditional. I presume we're good to leave the old version as is?

The version that changed it was rMW993baa3493f1: ActorMigration: Remove possibility of read-both - ActorMigration: Remove possibility of read-both so we should be good?

Taking the patch above forward as the base for the 2nd patch for this bug... Again on MW-1.31-release

diff --cc includes/ActorMigration.php
index b0aeeaf8ae,e6a6107ad4..0000000000
--- a/includes/ActorMigration.php
+++ b/includes/ActorMigration.php
@@@ -218,9 -333,9 +273,15 @@@ class ActorMigration 
                        $ret[$key] = $user->getId();
                        $ret[$text] = $user->getName();
                }
++<<<<<<< HEAD
 +              if ( $this->stage >= MIGRATION_WRITE_BOTH ) {
 +                      // UBN fix for T260485 - do not let User object to handle existing actor IDs
 +                      // TODO: Make User object wiki-aware and let it handle all cases.
++=======
+               if ( $this->stage & SCHEMA_COMPAT_WRITE_NEW ) {
+                       // NOTE: Don't use $user->getActorId(), since that may be for the wrong wiki (T260485)
+                       // TODO: Make User object wiki-aware and let it handle all cases (T260933)
++>>>>>>> SECURITY: ensure actor ID from correct wiki is used.
                        $existingActorId = $this->getExistingActorId( $dbw, $user );
                        if ( $existingActorId !== false ) {
                                $ret[$actor] = $existingActorId;
diff --cc includes/user/User.php
index c60d6cee77,511b493f92..0000000000
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@@ -2528,50 -1995,11 +2528,58 @@@ class User implements IDBAccessObject, 
                        $this->load();
                }
  
++<<<<<<< HEAD
 +              // Currently $this->mActorId might be null if $this was loaded from a
 +              // cache entry that was written when $wgActorTableSchemaMigrationStage
 +              // was MIGRATION_OLD. Once that is no longer a possibility (i.e. when
 +              // User::VERSION is incremented after $wgActorTableSchemaMigrationStage
 +              // has been removed), that condition may be removed.
 +              if ( $this->mActorId === null || !$this->mActorId && $dbw ) {
 +                      $q = [
 +                              'actor_user' => $this->getId() ?: null,
 +                              'actor_name' => (string)$this->getName(),
 +                      ];
 +                      if ( $dbw ) {
 +                              if ( $q['actor_user'] === null && self::isUsableName( $q['actor_name'] ) ) {
 +                                      throw new CannotCreateActorException(
 +                                              'Cannot create an actor for a usable name that is not an existing user'
 +                                      );
 +                              }
 +                              if ( $q['actor_name'] === '' ) {
 +                                      throw new CannotCreateActorException( 'Cannot create an actor for a user with no name' );
 +                              }
 +                              $dbw->insert( 'actor', $q, __METHOD__, [ 'IGNORE' ] );
 +                              if ( $dbw->affectedRows() ) {
 +                                      $this->mActorId = (int)$dbw->insertId();
 +                              } else {
 +                                      // Outdated cache?
 +                                      // Use LOCK IN SHARE MODE to bypass any MySQL REPEATABLE-READ snapshot.
 +                                      $this->mActorId = (int)$dbw->selectField(
 +                                              'actor',
 +                                              'actor_id',
 +                                              $q,
 +                                              __METHOD__,
 +                                              [ 'LOCK IN SHARE MODE' ]
 +                                      );
 +                                      if ( !$this->mActorId ) {
 +                                              throw new CannotCreateActorException(
 +                                                      "Cannot create actor ID for user_id={$this->getId()} user_name={$this->getName()}"
 +                                              );
 +                                      }
 +                              }
 +                              $this->invalidateCache();
 +                      } else {
 +                              list( $index, $options ) = DBAccessObjectUtils::getDBOptions( $this->queryFlagsUsed );
 +                              $db = wfGetDB( $index );
 +                              $this->mActorId = (int)$db->selectField( 'actor', 'actor_id', $q, __METHOD__, $options );
 +                      }
++=======
+               if ( !$this->mActorId && $dbw ) {
+                       $migration = MediaWikiServices::getInstance()->getActorMigration();
+                       $this->mActorId = $migration->getNewActorId( $dbw, $this );
+ 
+                       $this->invalidateCache();
++>>>>>>> SECURITY: ensure actor ID from correct wiki is used.
                        $this->setItemLoaded( 'actor' );
                }

First part is obviously trivially fixed, because the conflict is the same conditional...

But as code is being moved between files... Comparing is a bit harder, because it's changed *and* moved.

The User.php stuff.... Is conflicted by rMWc29909e59fd8: Mostly drop old pre-actor user schemas

Any thoughts?

Oh, and the ActorMigration::getNewActorId() needs some @since tags adding. I'll sort that when applying onto the various branches to be pushed out.

It looks like the code from User.php is moved basically verbatim and only really minor refactoring?

So can we just move the block of code in that conditional similarly across to ActorMigration, and then make similar updates replacing $this etc?

Ah, UserNameUtils is new in 1.35. So that's fun for 1.34 too

Proposed version of the patches...


Also

So can we just move the block of code in that conditional similarly across to ActorMigration, and then make similar updates replacing $this etc?

Should work.

@Reedy the security patch https://gerrit.wikimedia.org/r/c/mediawiki/core/+/629714 had phan failures that were not false positives:
User::isUsable should have been User::isUsableName
Fix at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/629522

Also https://gerrit.wikimedia.org/r/c/mediawiki/core/+/629704 - composer failure is not a false positive
ActorMigration::invalidateCache does not exist. I assume this was meant to be User::invalidateCache?
Fix at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/629523

I never said they were false positives

I never said they were false positives

I know, just wanted to let you know I sent patches to address the failures

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/629732 is failing on master

I think my PS on that should fix most of those failures, mostly caused by a lack of casting to an int for the return value...

Changes can be backported as appropriate

Legoktm renamed this task from CentralAuth uses wrong actor ID when locally suppressing the user to CentralAuth uses wrong actor ID when locally suppressing the user (CVE-2020-25689).Sep 27 2020, 11:56 AM
Reedy renamed this task from CentralAuth uses wrong actor ID when locally suppressing the user (CVE-2020-25689) to CentralAuth uses wrong actor ID when locally suppressing the user (CVE-2020-25869).Sep 28 2020, 1:13 PM
LSobanski added a subscriber: LSobanski.

Unsubscribing DBA as there is no action for us. Please add us back if you need any further assistance.

tstarling added a subscriber: tstarling.

Should be all cleaned up now.

tstarling changed the visibility from "Custom Policy" to "Public (No Login Required)".Nov 13 2020, 2:41 AM
tstarling changed the edit policy from "Custom Policy" to "All Users".

I removed the custom security policy on the basis that there is no need for it to be restricted now that it is fixed. I reviewed the comments above for PII and found none.

Change 640348 merged by jenkins-bot:
[mediawiki/extensions/WikimediaMaintenance@master] Scripts for fixing ipb_by_actor

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