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

In T260485#6406304, @daniel wrote:

[...]
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.

In T260485#6406304, @daniel wrote:

[...]
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!

In T260485#6407710, @Urbanecm wrote:

In T260485#6406304, @daniel wrote:

[...]
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.

In T260485#6407859, @Urbanecm wrote:

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

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...

In T260485#6415184, @daniel wrote:

T260485-defend.patch2 KBDownload

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:

In T260485#6421362, @Urbanecm wrote:

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?

@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.

In T260485#6423419, @Urbanecm wrote:

@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 :)

In T260485#6423412, @daniel wrote:

T260485-new-actor.patch5 KBDownload

@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.

In T260485#6423412, @daniel wrote:

T260485-new-actor.patch5 KBDownload

@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.

In T260485#6425566, @BPirkle wrote:

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.

Patch deployed. Untagging clinic duty again

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

In T260485#6480889, @Reedy wrote:

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

In T260485#6482147, @Reedy wrote:

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

In T260485#6491810, @Reedy wrote:

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

Resolved?

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

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