Page MenuHomePhabricator

MobileFrontend taint-check fails
Closed, ResolvedPublic

Description

While making unrelated changes (not touching those files)...

<?xml version="1.0" encoding="ISO-8859-15"?>
21:50:13 <checkstyle version="6.5">
21:50:13   <file name="includes/specials/SpecialMobileDiff.php">
21:50:13     <error line="357" severity="warning" message="Calling method \OutputPage::addHTML() in \SpecialMobileDiff::showFooter that outputs using tainted argument $[arg #1]. (Caused by: Builtin-\OutputPage::addHTML) (Caused by: ../../includes/page/ImagePage.php +892; includes/specials/SpecialMobileDiff.php +349; Builtin-\MediaWiki\Linker\LinkRenderer::makeLink; Builtin-\MediaWiki\Linker\LinkRenderer::makeLink; ../../includes/user/User.php +2191; ../../includes/user/User.php +388; ../../includes/user/User.php +383; ../../includes/user/User.php +388; ../../includes/user/User.php +383; ../../includes/user/User.php +388; ../../includes/user/User.php +383; includes/specials/SpecialMobileDiff.php +415; Builtin-\Message::parse; ../../includes/language/Message.php +940)" source="SecurityCheck-XSS"/>
21:50:13   </file>
21:50:13   <file name="includes/specials/SpecialMobileWatchlist.php">
21:50:13     <error line="271" severity="error" message="Calling method \Wikimedia\Rdbms\DBConnRef::select() in \SpecialMobileWatchlist::doFeedQuery that outputs using tainted argument $conds. (Caused by: Builtin-\Wikimedia\Rdbms\DBConnRef::select) (Caused by: ../../includes/logging/LogPager.php +367)" source="SecurityCheck-SQLInjection"/>
21:50:13     <error line="271" severity="error" message="Calling method \Wikimedia\Rdbms\DBConnRef::select() in \SpecialMobileWatchlist::doFeedQuery that outputs using tainted argument $fields. (Caused by: Builtin-\Wikimedia\Rdbms\DBConnRef::select) (Caused by: ../../includes/logging/LogPager.php +367; includes/specials/SpecialMobileWatchlist.php +271)" source="SecurityCheck-SQLInjection"/>
21:50:13     <error line="271" severity="error" message="Calling method \Wikimedia\Rdbms\DBConnRef::select() in \SpecialMobileWatchlist::doFeedQuery that outputs using tainted argument $tables. (Caused by: Builtin-\Wikimedia\Rdbms\DBConnRef::select) (Caused by: ../../includes/logging/LogPager.php +367; includes/specials/SpecialMobileWatchlist.php +271)" source="SecurityCheck-SQLInjection"/>
21:50:13   </file>
21:50:13 </checkstyle>

Event Timeline

Reedy created this task.Apr 19 2020, 9:06 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 19 2020, 9:06 PM
Daimona renamed this task from MobileFrontend phan fails to MobileFrontend taint-check fails.Apr 20 2020, 10:19 AM
Daimona edited projects, added phan-taint-check-plugin; removed phan.

While making unrelated changes (not touching those files)

Taint-check is often dark magic, so that doesn't really matters... Renaming other files will change the "environment" (e.g. order of analysis, and other things which could cause new issues to appear).

I've tried upgrading to the latest phan+taint-check but all errors are still there, so I analyzed them one by one.

SpecialMobileDiff: the caused-by lines are confusing, but the only faulty line is $this->listGroups( $user, $this->mobileContext ), which in turns calls UserGroupMembership::getLink, which leads us to T183174 once again. Taint-check just cannot understand how that method works, because of the custom escaping taking place. I recommend suppressing the issue, so you'll be able to remove the suppression once T183174 is done.

SpecialMobileWatchlist: This is a known issue with ChangeTags::modifyDisplayQuery. It was mitigated in taint-check 3.0.2, but apparently it still remains for extensions. Again, you can simply suppress the issue inline, until it's fixed upstream.

sbassett triaged this task as Medium priority.Apr 21 2020, 1:48 AM

Change 591382 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/MobileFrontend@master] build: Upgrade mediawiki-phan-config from 0.9.2 to 0.10.2 and make pass

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

Oh, hey, just found this.

Change 591382 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] build: Upgrade mediawiki-phan-config from 0.9.2 to 0.10.2 and make pass

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

Jdforrester-WMF closed this task as Resolved.Apr 21 2020, 7:28 PM
Jdforrester-WMF claimed this task.