Page MenuHomePhabricator

CVE-2023-29137: GrowthExperiments: UserImpactHandler returns timezone preference data for arbitrary users
Closed, ResolvedPublicSecurity

Assigned To
Authored By
kostajh
Feb 2 2023, 11:40 AM
Referenced Files
F36817452: T328643-2.patch
Feb 9 2023, 11:31 AM
F36817245: T328643-2.patch
Feb 9 2023, 9:20 AM
F36704766: T328643-2.patch
Feb 3 2023, 7:09 PM
F36687906: T328643-2.patch
Feb 2 2023, 10:49 PM
F36582616: T328643.patch
Feb 2 2023, 12:12 PM
Restricted File
Feb 2 2023, 12:11 PM
F36582557: T328643.patch
Feb 2 2023, 11:44 AM

Description

The UserImpactHandler for GrowthExperiments inadvertently returns the timezone preference for arbitrary users, which can be used to de-anonymize users.

Visiting Special:Impact/{Username} and inspecting the server-side exported data from UserImpact.php or the REST request (w/rest.php/growthexperiments/v0/user-impact/%23{userId}) allows one to read the timezone preference for an arbitrary user.

We have a one-line patch to mitigate this by removing the timeZone field from the data returned by the UserImpactHandler.

We may follow up by further removing the timeZone field from the user impact cache entries that are stored in growthexperiments_user_impact, but let's do the quick fix first.

Affected wikis

This code is currently live on arwiki, bnwiki, cswiki, eswiki and testwiki.

Event Timeline

kostajh renamed this task from GrowthExperiments: UserImpactHandler returns time zone data for arbitrary users to GrowthExperiments: UserImpactHandler returns timezone preference data for arbitrary users.Feb 2 2023, 11:41 AM

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.

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

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

kostajh changed the task status from Open to In Progress.Feb 3 2023, 11:56 AM
kostajh triaged this task as High priority.

Good points, updated:

Good points, updated:

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

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

sbassett changed Author Affiliation from N/A to WMF Product.Feb 14 2023, 5:45 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from N/A to High.
sbassett moved this task from Incoming to Our Part Is Done on the Security-Team board.
sbassett added a project: SecTeam-Processed.
sbassett moved this task from Our Part Is Done to Watching on the Security-Team board.

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

mmartorana renamed this task from GrowthExperiments: UserImpactHandler returns timezone preference data for arbitrary users to CVE-2023-29137: GrowthExperiments: UserImpactHandler returns timezone preference data for arbitrary users.Apr 3 2023, 3:21 PM