+2

I deployed this to wmf.21 and wmf.20 just now, using python3 deploy_security.py --run T328643.patch extensions/GrowthExperiments

Is the next step to push this patch to Gerrit, merge into master, and make this task public?

I think the next step should be to make the timezone always be UTC, to prevent the user timezone to be discoverable in other ways (the patch stops it from being exported directly, but it's still used to aggregate edit data on the server side and that can be reverse-engineered). That is a small enough change that it can be conveniently handled via the security patch process without the risk of breaking something that expects a userinfo field to be present. Then we wait for the cache to expire (that's two days), make the security issue public, port the patches to gerrit, and then we can handle T323748: UserImpact: Adjust time zone correction for page view URLs in whatever way is best (per Slack discussion, probably by using the server offset and ignoring user timezones entirely), and potentially clean up data structures that became unnecessary.

In T328643#8583821, @Tgr wrote:

T328643-2.patch1 KBDownload

+2, with a minor feedback to use UserTimeCorrection::SYSTEM instead of 'System'.

In T328643#8584566, @kostajh wrote:

In T328643#8583821, @Tgr wrote:

T328643-2.patch1 KBDownload

+2, with a minor feedback to use UserTimeCorrection::SYSTEM instead of 'System'.

Do we need to also update $this->timeZone = new UserTimeCorrection( $json['timeZone'][0], $date, $json['timeZone'][1] ); which is in UserImpact.php line 244?

In theory, that would become 'System' once the cache entries are updated, but we could just hardcode 'System' if we know we always want to use that.

Good points, updated:

+1

In T328643#8586281, @Tgr wrote:

Good points, updated:

T328643-2.patch1 KBDownload

-		$this->timeZone = new UserTimeCorrection( $json['timeZone'][0], $date, $json['timeZone'][1] );
+		$this->timeZone = new UserTimeCorrection( UserTimeCorrection::SYSTEM, $date, $json['timeZone'][1] );

Do we want the third parameter for UserTimeCorrection?

More generally, could we just remove $this->timeZone entirely and bump the cache version? It doesn't seem like UserImpact::getTimezone() is called anywhere.

Hm... I was going to say that that field should be the same for everyone (it's supposed to just contain $wgLocalTZoffset) but apparently not:

wikiadmin2023@10.64.48.111(cswiki)> SELECT JSON_EXTRACT(geui_data, '$.timeZone[1]') tz, count(*) FROM growthexperiments_user_impact GROUP BY JSON_EXTRACT(geui_data, '$.timeZone[1]');
+------+----------+
| tz   | count(*) |
+------+----------+
| 0    |        5 |
| 60   |      573 |
+------+----------+
2 rows in set (0.034 sec)

Looks like it's just passing the user-specific offset there. Which works because when the user's timezone preference is SYSTEM it will be correct, and when it is something else the third parameter of UserTimezoneCorrection is ignored entirely, but it means this does contain PII.

More generally, could we just remove $this->timeZone entirely and bump the cache version?

Doing that.

In T328643#8600529, @Tgr wrote:

T328643-2.patch4 KBDownload

composer test says:

FILE: ...src/mediawiki/w/extensions/GrowthExperiments/includes/UserImpact/UserImpact.php
-------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement "DateTime"
   |         |     (MediaWiki.Classes.UnusedUseStatement.UnusedUse)
-------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------

Fixed in this version:

CR+2 to @Tgr's patch.

Deployed.

@sbassett I think this can be made public and then we can follow up on Gerrit. The vulnerability is in an experimental feature which is new in 1.40, so even if there are third-party users of GrowthExperiments, they aren't affected.

Impact: user timezone settings were visible via an (although obscure and undocumented but publicly accessible) API since mid-December on the pilot wikis used for T323526: New Impact Module: Start experiment for the new Impact module on Growth Pilot wikis (ar, bn, cs, es).

Change 889295 had a related patch set uploaded (by Gergő Tisza; author: Kosta Harlan):

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Exclude the timeZone property from user impact data export

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

Change 889296 had a related patch set uploaded (by Gergő Tisza; author: Kosta Harlan):

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Do not expose user timezones

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

Change 889295 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Exclude the timeZone property from user impact data export

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

Change 889296 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Do not expose user timezones

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