>>> $cache->get( $user->getCacheKey( $cache ) )['mGroupExpirations']
=> [
     "abusefilter-helper" => "20220220113921",
     "global-rollbacker" => null,
   ]

Something's going very wrong here, since even the original TTL is set to a day (and the logic I mentioned above can only shorten it), as if something was just ignoring it.

Do we need to patch CentralAuth to ignore any cache entries that contain user groups that expire in the past?

I confirm that weird behavior referenced at T302248#7726529. CentralAuthUser::getInstance($u)->getGlobalRights() "sometimes" return the wrong set of permissions for Superpes. getClosestGlobalUserGroupExpiry() returns non-null.

[urbanecm@mwmaint1002 ~]$ mwscript shell.php dewiki
Psy Shell v0.11.1 (PHP 7.2.34-18+0~20210223.60+debian10~1.gbpb21322+wmf5 — cli) by Justin Hileman
>>> $u = User::newFromName('Superpes15')
=> User {#3306
     +mId: null,
     +mName: "Superpes15",
     +mActorId: null,
     +mRealName: null,
     +mEmail: null,
     +mTouched: null,
     +mEmailAuthenticated: null,
     +mFrom: "name",
     +mBlockedby: -1,
     +mHideName: null,
     +mBlock: null,
   }
>>> $u->isAllowed('abusefilter-view-private')
=> true
>>> use MediaWiki\Extension\CentralAuth\User\CentralAuthUser
>>> $cu = CentralAuthUser::getInstance($u)
=> MediaWiki\Extension\CentralAuth\User\CentralAuthUser {#3365
     +mStateDirty: false,
     +mHomeWiki: "itwiki",
   }
>>> $cu->getGlobalRights()
=> [
     "abusefilter-log",
     "abusefilter-log-detail",
     "abusefilter-log-private",
     "abusefilter-view",
     "abusefilter-view-private",
     "spamblacklistlog",
     "abusefilter-log-detail",
     "autoconfirmed",
     "autopatrol",
     "autoreviewrestore",
     "editsemiprotected",
     "markbotedits",
     "move",
     "movestable",
     "noratelimit",
     "patrolmarks",
     "rollback",
     "skipcaptcha",
     "suppressredirect",
   ]
>>> sudo $cu->getClosestGlobalUserGroupExpiry()
=> 1645357161
>>>

re-running the same snippet in a new shell.php session works as expected (w/o the additional rights being allowed). So...apparently, cache gets updated, but after it is consumed?

In T302248#7726551, @Majavah wrote:

>>> $cache->get( $user->getCacheKey( $cache ) )['mGroupExpirations']
=> [
     "abusefilter-helper" => "20220220113921",
     "global-rollbacker" => null,
   ]

Something's going very wrong here, since even the original TTL is set to a day (and the logic I mentioned above can only shorten it), as if something was just ignoring it.

CentralAuthUser::loadFromCache uses the following code to set the TTL:

$closestGugExpiry = $this->getClosestGlobalUserGroupExpiry();
if ( $closestGugExpiry ) {
	$ttl = min( time() - $closestGugExpiry, $ttl );
}

AFAICS time() - $closestGugExpiry is going to be negative when getClosestGlobalUserGroupExpiry returns a timestamp that's in the future. If I understand the path correctly, it ends up setting a negative TTL since $closestGugExpiry is expected to be in the future (and I'm not sure how are negative TTLs treated by core).

In T302248#7726667, @Urbanecm wrote:

In T302248#7726551, @Majavah wrote:

>>> $cache->get( $user->getCacheKey( $cache ) )['mGroupExpirations']
=> [
     "abusefilter-helper" => "20220220113921",
     "global-rollbacker" => null,
   ]

Something's going very wrong here, since even the original TTL is set to a day (and the logic I mentioned above can only shorten it), as if something was just ignoring it.

CentralAuthUser::loadFromCache uses the following code to set the TTL:

$closestGugExpiry = $this->getClosestGlobalUserGroupExpiry();
if ( $closestGugExpiry ) {
	$ttl = min( time() - $closestGugExpiry, $ttl );
}

AFAICS time() - $closestGugExpiry is going to be negative when getClosestGlobalUserGroupExpiry returns a timestamp that's in the future. If I understand the path correctly, it ends up setting a negative TTL since $closestGugExpiry is expected to be in the future (and I'm not sure how are negative TTLs treated by core).

Yeah, that definetly doesn't seem correct. It also is the other way around in core: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/aeea3a582e1df47d1aa9cf2febf3c20d0ba1ca9f/includes/user/User.php#545

Not sure if that is the only issue, but it definetly seems worth fixing.

Proposed patch:

In T302248#7726840, @Zabe wrote:

0001-SECURITY-Fix-ttl-for-groups-expiring-in-the-future.patch1 KBDownload

Deployed.

In T302248#7728138, @Majavah wrote:

Deployed.

Thanks. SAL. Also tracking at T276237 and T297839.

This problem seems to have fixed itself in production in the meantime:

>>> $users = CentralAuthServices::getDatabaseManager()->getCentralDB( DB_REPLICA )->selectFieldValues(  'global_user_groups', 'gug_user', [ 'gug_expiry < now()', 'gug_group' => 'abusefilter-helper' ] );
=> [
     "2645",
     "6677077",
     "44717363",
     "55412109",
     "56429570",
     "59866209",
   ]
>>> foreach ( $users as $id ) { var_dump( in_array( 'abusefilter-view-private', \MediaWiki\Extension\CentralAuth\User\CentralAuthUser::newFromId( (int)$id )->getGlobalRights() ) ); }
bool(false)
bool(false)
bool(false)
bool(false)
bool(false)
bool(false)
>>> User::newFromName( 'Superpes15' )->isAllowed( 'abusefilter-view-private' )
=> false

With the above fix deployed, is there anything left to do here?

In T302248#7728975, @sbassett wrote:

Thanks. SAL. Also tracking at T276237 and T297839.

Note that this feature didn't make it into a release branch yet.

In T302248#7730003, @Majavah wrote:

This problem seems to have fixed itself in production in the meantime:

>>> $users = CentralAuthServices::getDatabaseManager()->getCentralDB( DB_REPLICA )->selectFieldValues(  'global_user_groups', 'gug_user', [ 'gug_expiry < now()', 'gug_group' => 'abusefilter-helper' ] );
=> [
     "2645",
     "6677077",
     "44717363",
     "55412109",
     "56429570",
     "59866209",
   ]
>>> foreach ( $users as $id ) { var_dump( in_array( 'abusefilter-view-private', \MediaWiki\Extension\CentralAuth\User\CentralAuthUser::newFromId( (int)$id )->getGlobalRights() ) ); }
bool(false)
bool(false)
bool(false)
bool(false)
bool(false)
bool(false)
>>> User::newFromName( 'Superpes15' )->isAllowed( 'abusefilter-view-private' )
=> false

With the above fix deployed, is there anything left to do here?

I think ensure it was the cause. We should probably discuss the hardening you described at T302248#7726551 (I'm not sure about it; on one hand, it decreases the probability of this happening again, but it also makes it harder to fix the actual error in the code).

In T302248#7730106, @Urbanecm wrote:

With the above fix deployed, is there anything left to do here?

I think ensure it was the cause. We should probably discuss the hardening you described at T302248#7726551 (I'm not sure about it; on one hand, it decreases the probability of this happening again, but it also makes it harder to fix the actual error in the code).

Yes, that. But assuming things have been fixed well enough for now, there's no reason why this task couldn't be made public (I'm not seeing any obvious PII or sensitive info) and the backports pushed through gerrit so that the patch lands on a proper release branch next week, or whenever. Otherwise that will definitely happen once the next supplemental security release is sent out towards the end of March 2022.

In T302248#7726551, @Majavah wrote:

>>> $cache->get( $user->getCacheKey( $cache ) )['mGroupExpirations']
=> [
     "abusefilter-helper" => "20220220113921",
     "global-rollbacker" => null,
   ]

Something's going very wrong here, since even the original TTL is set to a day (and the logic I mentioned above can only shorten it), as if something was just ignoring it.

Do we need to patch CentralAuth to ignore any cache entries that contain user groups that expire in the past?

This seems like a good hardening measure to me, rather than relying on how well cache expiry works.

In T302248#7730117, @sbassett wrote:

[...]
Yes, that. But assuming things have been fixed well enough for now, there's no reason why this task couldn't be made public (I'm not seeing any obvious PII or sensitive info) and the backports pushed through gerrit so that the patch lands on a proper release branch next week, or whenever. Otherwise that will definitely happen once the next supplemental security release is sent out towards the end of March 2022.

I added a temporary global group to my alt account (https://meta.wikimedia.org/w/index.php?title=Special:Log&logid=46210369). I plan to check tomorrow the group disappeared. We don't have exact ways to reproduce unfortunately, but it's at least something :).

In T302248#7730649, @Legoktm wrote:

In T302248#7726551, @Majavah wrote:

Do we need to patch CentralAuth to ignore any cache entries that contain user groups that expire in the past?

This seems like a good hardening measure to me, rather than relying on how well cache expiry works.

Proposed patch:

In T302248#7731921, @Majavah wrote:

Proposed patch:

0001-SECURITY-Ignore-cached-CentralAuthUser-entries-with-.patch2 KBDownload

-1, getClosestGlobalUserGroupExpiry returns null for users without temporary groups, resulting in their groups being reloaded in every request.

In T302248#7731942, @Zabe wrote:

-1, getClosestGlobalUserGroupExpiry returns null for users without temporary groups, resulting in their groups being reloaded in every request.

Thanks, fixed.

In T302248#7731951, @Majavah wrote:

In T302248#7731942, @Zabe wrote:

-1, getClosestGlobalUserGroupExpiry returns null for users without temporary groups, resulting in their groups being reloaded in every request.

Thanks, fixed.

0001-SECURITY-Ignore-cached-CentralAuthUser-entries-with-.patch2 KBDownload

+2

There is a typo in the @param statement, but that doesn't matter too much, since it is a sec patch.

In T302248#7731979, @Zabe wrote:

In T302248#7731951, @Majavah wrote:

Thanks, fixed.

0001-SECURITY-Ignore-cached-CentralAuthUser-entries-with-.patch2 KBDownload

+2

Deployed too. I'll fix that typo when pushing these to Gerrit.

In T302248#7730117, @sbassett wrote:

Yes, that. But assuming things have been fixed well enough for now, there's no reason why this task couldn't be made public (I'm not seeing any obvious PII or sensitive info) and the backports pushed through gerrit so that the patch lands on a proper release branch next week, or whenever. Otherwise that will definitely happen once the next supplemental security release is sent out towards the end of March 2022.

Sounds good. The hardening patch also added log message that should warn us if this is happening again (at least the only known case was a caching issue, I really hope there aren't any other logic bugs here). I don't think there's much exploitable if this is made public, as an attacker would need to temporarily get added to global group by a steward in the first place.

In T302248#7732141, @Majavah wrote:

Deployed too. I'll fix that typo when pushing these to Gerrit.

Thanks.

Sounds good. The hardening patch also added log message that should warn us if this is happening again (at least the only known case was a caching issue, I really hope there aren't any other logic bugs here). I don't think there's much exploitable if this is made public, as an attacker would need to temporarily get added to global group by a steward in the first place.

This is now public, so we can get the backports going in gerrit.

Change 765335 had a related patch set uploaded (by SBassett; author: Zabe):

[mediawiki/extensions/CentralAuth@master] SECURITY: Fix ttl for groups expiring in the future

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

Change 765336 had a related patch set uploaded (by Zabe; author: Majavah):

[mediawiki/extensions/CentralAuth@master] SECURITY: Ignore cached CentralAuthUser entries with expired groups

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

Change 765335 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] SECURITY: Fix ttl for groups expiring in the future

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

Change 765336 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] SECURITY: Ignore cached CentralAuthUser entries with expired groups

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