Page MenuHomePhabricator

SpecialLanguageStats.php fails phan tests due to possible XSS
Closed, ResolvedPublicSecurity

Description

See e.g. https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Translate/+/597989/

07:28:55 <checkstyle version="6.5">
07:28:55   <file name="specials/SpecialLanguageStats.php">
07:28:55     <error line="191" severity="warning" message="Calling method \OutputPage::addHTML() in \SpecialLanguageStats::execute that outputs using tainted argument $output. (Caused by: Builtin-\OutputPage::addHTML) (Caused by: specials/SpecialLanguageStats.php +157)" source="SecurityCheck-XSS"/>
07:28:55   </file>
07:28:55 </checkstyle>

All new patches for MediaWiki-extensions-Translate now failing due to that.

Details

Author Affiliation
Wikimedia Communities

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 22 2020, 1:24 PM
abi_ added a comment.May 22 2020, 1:44 PM

As far as I can tell, this code has been unchanged for a while. Did we add update phan or add any new rules to it?

Daimona added a subscriber: Daimona.

Might have been caused by https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/596807/, I would expect this kind of fallback, however I can't see an obvious connection. I'm trying to see whether bumping phan & taint-check solves the issue.

Might have been caused by https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/596807/, I would expect this kind of fallback, however I can't see an obvious connection. I'm trying to see whether bumping phan & taint-check solves the issue.

It does solve this specific issue, but other issues appeared: https://integration.wikimedia.org/ci/job/mwext-php72-phan-docker/53648/console they should be checked locally to see whether they're false positives or not, but I can't do that right now.

Perhaps we can assume that it was a false positive and suppress it.

Krinkle triaged this task as Unbreak Now! priority.May 22 2020, 9:51 PM
Krinkle added a subscriber: Krinkle.

Can't merge anything in Translate repo.

I'm sorry for not following up yesterday. I've created the patch below which just suppresses the issue for now.

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Translate/+/598158/

Daimona claimed this task.May 23 2020, 1:10 PM
sbassett added a subscriber: sbassett.EditedMay 23 2020, 8:17 PM

I'm sorry for not following up yesterday. I've created the patch below which just suppresses the issue for now.

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Translate/+/598158/

A bit of information disclosure here since this may be a legitimate XSS in certain contexts, though I'm personally fine with this for now to get things working again within the repo.

sbassett lowered the priority of this task from Unbreak Now! to High.May 23 2020, 8:49 PM

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Translate/+/598158/ is merged. Translate repo should be mergeable again - patch in the task description rechecked fine. Re-triaging to high and keeping private for the further investigation of the XSS.

I'm sorry for not following up yesterday. I've created the patch below which just suppresses the issue for now.

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Translate/+/598158/

A bit of information disclosure here since this may be a legitimate XSS in certain contexts, though I'm personally fine with this for now to get things working again within the repo.

Yeah well, the build was failing with a XSS warning, so this whole task here doesn't really need to be hidden...

Anyway, I've investigated the issue using taint-check 2.0.1. For some reason, it considers the first parameter of Linker::link (the target) as tainted, whereas it has no effect on the taintedness of the return value. Also, this is probably the root cause of several other taint-check failures that I've spotted recently in different repos.

The MWE is:

echo Linker::link( $_GET['x'], 'foo' );

going deeper, this is caused by the call to LinkRenderer::makeBrokenLink (which, BTW, isn't special-cased in taint-check like makeKnownLink, makeLink, and makePreloadedLink are). Deeper inside, the cause is LinkRender::runBeginHook, specifically ::runLegacyBeginHook. There, the LinkTarget is cast to Title, and ::makeBrokenLink is called again. This "loop" confuses taint-check a lot.

Solution: the quickest solution is to annotate $target as not tainted. Then we might special-case it in taint-check itself.

Solution: the quickest solution is to annotate $target as not tainted.

--> https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/598228/

Then we might special-case it in taint-check itself.

--> https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/598229/

Can we now make this task public, since the original issue was just a false positive?

MarcoAurelio added a comment.EditedMay 24 2020, 4:44 PM

Can we now make this task public, since the original issue was just a false positive?

If @sbassett is okay with that (cfr. T253383#6160573) then yes I guess.

Edit: I spoke to @Daimona and confirmed it was a false positive so I went ahead and made this task public.

MarcoAurelio changed the visibility from "Custom Policy" to "Public (No Login Required)".May 24 2020, 4:46 PM
MarcoAurelio changed the edit policy from "Custom Policy" to "All Users".

Change 598228 had a related patch set uploaded (by Krinkle; owner: Daimona Eaytoy):
[mediawiki/core@master] linker: Annotate first param of makeBrokenLink as not tainted

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

sbassett moved this task from Incoming to Watching on the Security-Team board.Jun 1 2020, 3:18 PM

Leaving this open until https://gerrit.wikimedia.org/r/598228 gets merged, otherwise I think it can be resolved.

Change 598228 merged by jenkins-bot:
[mediawiki/core@master] linker: Annotate first param of makeBrokenLink as not tainted

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

sbassett closed this task as Resolved.Jun 5 2020, 8:00 PM
sbassett moved this task from Watching to Our Part Is Done on the Security-Team board.
sbassett removed a project: Patch-For-Review.