Page MenuHomePhabricator

Calling method OutputPage::prependHTML() in CollaborationKitHooks::onOutputPageBeforeHTML that outputs using tainted argument
Closed, ResolvedPublic

Description

https://integration.wikimedia.org/ci/job/mwext-php72-phan-seccheck-docker/54373/console

<?xml version="1.0" encoding="ISO-8859-15"?>
11:59:05 <checkstyle version="6.5">
11:59:05   <file name="includes/CollaborationKitHooks.php">
11:59:05     <error line="137" severity="warning" message="Calling method \OutputPage::prependHTML() in \CollaborationKitHooks::onOutputPageBeforeHTML that outputs using tainted argument $[arg #1]. (Caused by: Builtin-\OutputPage::prependHTML) (Caused by: includes/content/CollaborationHubTOC.php +169; includes/CollaborationKitHooks.php +122)" source="SecurityCheck-XSS"/>
11:59:05   </file>
11:59:05 </checkstyle>

Event Timeline

Daimona edited projects, added phan-taint-check-plugin; removed phan.
Daimona subscribed.

I'm investigating.

So, here is the path that I've reconstructed.

  • CollaborationKitHooks:137 - outputs $toc->renderSubpageToC( $parentHub ), let's see whether it's tainted.

In that method, we find:

public function renderSubpageToC( Title $title ) {
// ...
$itemLink = CollaborationKitImage::makeImage(
	$itemImage,
	16,
	[ 'link' => $itemTitle, 'label' => $itemDisplayTitle, 'colour' => $colour ]
);
$html .= Html::rawElement(
	'li',
	[ 'class' => 'mw-ck-toc-item' ],
	$itemLink
);
return $html;

The taintedness comes from ::makeImage, so let's go there.

public static function makeImage( $image, $width, $options = [] ) {
// ...
$imageObj = MediaWikiServices::getInstance()->getRepoGroup()->findFile( $image ); // Line 61
// ...
$imageCode = self::makeImageFromFile( $imageObj, $width, $link,
				$renderAsWikitext, $label, $squareAdjustmentAxis ); // Line 93
$imageBlock = Html::rawElement( 'div', $wrapperAttributes, $imageCode );
return $imageBlock;

and finally:

protected static function makeImageFromFile( $imageObj, $width, $link,
	$renderAsWikitext, $label, $squareAdjustmentAxis
) {
// ...
$imageTitle = $imageObj->getTitle();
$imageFullName = $imageTitle->getFullText();
// ...
$wikitext = "[[{$imageFullName}|{$widthText}px";
// ...
return $wikitext;

Title::getFullText() is always considered tainted, hence the issue. Note, I haven't checked whether the code above is passing something unsafe!

The problem is more that the function allows to create html or wikitext ($renderAsWikitext)

Escaping Title::getFullText in the wikitext case seems not helpful as that would produce an unparseable link in wikitext when the file name contains &'"

Ah, note that I didn't really analyze the code itself. I just investigated where the failure originates. I'm even unsure if this can be considered a false positive or not.

Having done some initial investigation, I'm removing myself as assignee.

Daimona claimed this task.

Fixed a long time ago with r602064 (and unused suppression removed with r605338).