Page MenuHomePhabricator

Checkuser stores users to cu_log with trailing spaces, allowing all CUs to turn off Special:CheckuserLog at will
Closed, ResolvedPublicSecurity

Description

Bug

After I made the following API query on testwiki:

{
	"action": "query",
	"format": "json",
	"list": "checkuser",
	"curequest": "userips",
	"cutarget": "Martin Urbanec (test) ",
	"cureason": "testing a bug in CU interface",
	"cutoken": "REDACTED"
}

https://test.wikipedia.org/wiki/Special:CheckUserLog broke, it started to show a stacktrace instead:

Stacktrace
[YDaao09kHU2l9rVxPFJzGAAAAA8] /wiki/Special:CheckUserLog Wikimedia\Assert\ParameterAssertionException: Bad value for parameter $title: invalid name 'Contributions/Martin_Urbanec_(test)_'

Backtrace:

from /srv/mediawiki/php-1.36.0-wmf.32/vendor/wikimedia/assert/src/Assert.php(72)
#0 /srv/mediawiki/php-1.36.0-wmf.32/includes/title/TitleValue.php(186): Wikimedia\Assert\Assert::parameter(boolean, string, string)
#1 /srv/mediawiki/php-1.36.0-wmf.32/includes/title/TitleValue.php(150): TitleValue::assertValidSpec(integer, string, string, string)
#2 /srv/mediawiki/php-1.36.0-wmf.32/includes/specialpage/SpecialPage.php(125): TitleValue->__construct(integer, string, string)
#3 /srv/mediawiki/php-1.36.0-wmf.32/includes/specialpage/SpecialPage.php(108): SpecialPage::getTitleValueFor(string, string, string)
#4 /srv/mediawiki/php-1.36.0-wmf.32/includes/Linker.php(977): SpecialPage::getTitleFor(string, string)
#5 /srv/mediawiki/php-1.36.0-wmf.32/extensions/CheckUser/src/LogPager.php(38): Linker::userToolLinks(string, string)
#6 /srv/mediawiki/php-1.36.0-wmf.32/includes/pager/IndexPager.php(611): MediaWiki\CheckUser\LogPager->formatRow(stdClass)
#7 /srv/mediawiki/php-1.36.0-wmf.32/extensions/CheckUser/src/Specials/SpecialCheckUserLog.php(93): IndexPager->getBody()
#8 /srv/mediawiki/php-1.36.0-wmf.32/includes/specialpage/SpecialPage.php(645): MediaWiki\CheckUser\Specials\SpecialCheckUserLog->execute(NULL)
#9 /srv/mediawiki/php-1.36.0-wmf.32/includes/specialpage/SpecialPageFactory.php(1405): SpecialPage->run(NULL)
#10 /srv/mediawiki/php-1.36.0-wmf.32/includes/MediaWiki.php(310): MediaWiki\SpecialPage\SpecialPageFactory->executePath(Title, RequestContext)
#11 /srv/mediawiki/php-1.36.0-wmf.32/includes/MediaWiki.php(944): MediaWiki->performRequest()
#12 /srv/mediawiki/php-1.36.0-wmf.32/includes/MediaWiki.php(548): MediaWiki->main()
#13 /srv/mediawiki/php-1.36.0-wmf.32/index.php(53): MediaWiki->run()
#14 /srv/mediawiki/php-1.36.0-wmf.32/index.php(46): wfIndexMain()
#15 /srv/mediawiki/w/index.php(3): require(string)
#16 {main}

This issue is caused by CU API storing username with a trailing space into cu_log:

mysql:research@dbstore1004.eqiad.wmnet [testwiki]> select count(*) from cu_log where cul_target_text like "Martin Urbanec (test)" and cul_timestamp like "20210224%";
+----------+
| count(*) |
+----------+
|        0 |
+----------+
1 row in set (0.001 sec)

mysql:research@dbstore1004.eqiad.wmnet [testwiki]> select count(*) from cu_log where cul_target_text like "Martin Urbanec (test) " and cul_timestamp like "20210224%";
+----------+
| count(*) |
+----------+
|        1 |
+----------+
1 row in set (0.001 sec)

mysql:research@dbstore1004.eqiad.wmnet [testwiki]>

Special:CheckUserLog apparently can't deal with such a corner case, and fails with Wikimedia\Assert\ParameterAssertionException.

Security impact

Any checkuser can turn Special:CheckUserLog off, making it impossible to track CU usage when this bug is in place.

Notes

Despite a lot of information in the task description, and even a row in cu_log table, this can be published once fixed IMO. I intentionally broke testwiki to prove my theory, and I'm fine with publishing information about that exact check. No other private info should be here as of task creation.

Event Timeline

Urbanecm changed Author Affiliation from N/A to Wikimedia Communities.Feb 24 2021, 6:35 PM
Urbanecm added a project: CheckUser.
Urbanecm triaged this task as High priority.

Those patches should fix this. The first one is to fix the user-facing issue, and should unbreak Special:CheckuserLog on affected wikis. The second one stops saving malformed data into cul_target_text. I'll fill a follow-up to create a maintenance script to fix old entries, to make our data consistent, once this is public.

+1 to the patches above. I assume Linker::userLink( $row->cul_target_id, $row->cul_target_text ) in LogPager doesn't need a trim because of https://gerrit.wikimedia.org/g/mediawiki/core/+/21ab535b83b97866cb9b79dcede95e8b7c32858f/includes/Linker.php#914. I guess feel free to deploy these unless you want @Reedy or I to do so instead.

The second patch has my virtual +2 (and blessings). The first one is good to be merged for now but I think it should be reverted later once the log is cleaned up somehow).

23:03 <Urbanecm> !log Deploy security patches for T275669
23:03 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log

Patch deployed, it works now.

@sbassett Can you do the final honours, please? (backport mainly, I guess; unsure whether CVE is necessary for this one)

sbassett lowered the priority of this task from High to Low.Feb 25 2021, 5:16 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".

Change 666963 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/CheckUser@master] SECURITY: Trim cul_target_text in LogPager

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

Change 666964 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/CheckUser@master] SECURITY: Trim target before storing it to cu_log

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

I kept these as separate patches for the backports so as to (hopefully) make reverting the first patch easier, if and when that's needed. These don't cleanly apply to REL1_35 and REL1_31, mainly due to directory/file name refactoring, but I can work on new patches for those, post them here and then push them up to gerrit for review/merge.

Change 666964 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] SECURITY: Trim target before storing it to cu_log

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

Change 666963 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] SECURITY: Trim cul_target_text in LogPager

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

Change 667023 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/CheckUser@REL1_31] SECURITY: Trim target before storing it to cu_log

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

Change 667024 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/CheckUser@REL1_31] SECURITY: Trim cul_target_text in LogPager

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

Change 667025 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/CheckUser@REL1_35] SECURITY: Trim target before storing it to cu_log

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

Change 667027 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/CheckUser@REL1_35] SECURITY: Trim cul_target_text in LogPager

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

I kept these as separate patches for the backports so as to (hopefully) make reverting the first patch easier, if and when that's needed. These don't cleanly apply to REL1_35 and REL1_31, mainly due to directory/file name refactoring, but I can work on new patches for those, post them here and then push them up to gerrit for review/merge.

Merged them.

Change 667023 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@REL1_31] SECURITY: Trim target before storing it to cu_log

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

Change 667024 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@REL1_31] SECURITY: Trim cul_target_text in LogPager

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

Change 667025 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@REL1_35] SECURITY: Trim target before storing it to cu_log

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

Change 667027 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@REL1_35] SECURITY: Trim cul_target_text in LogPager

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

Merged them.

Thanks! And here's the REL1_31 and REL1_35 patches, just in case anybody ever needs them: