Page MenuHomePhabricator

AdminLinks::makelink() doesn't HTML escape properly
Closed, ResolvedPublic

Description

	public static function makeLink( $title, $msg = null, $attrs = array(), $params = array() ) {
		if ( function_exists( 'MediaWiki\MediaWikiServices::getLinkRenderer' ) ) {
			// MW 1.28+
			$linkRenderer = MediaWikiServices::getInstance()->getLinkRenderer();
			return $linkRenderer->makeKnownLink( $title, $msg, $attrs, $params );
		} else {
			return Linker::linkKnown( $title, $msg, $attrs, $params );
		}

LinkRenderer will HTML escape $msg, but Linker will not. So on older MW versions this could potentially cause an XSS if developers assume that it will be escaped properly.

Probably the best fix would be to be consistent and not HTML escape in the LinkRenderer case by using HtmlArmor, and ensure that all callers do HTML escaping.

I noticed this while investigating a phan-taint-check-plugin error.

Event Timeline

This function seems to have been copied to Cargo and ApprovedRevs, though both of those are even more broken in different ways.

Cargo:


I think this closes an XSS with the |more results text= parameter on pre-1.28 versions of MediaWiki.

AdminLinks:


Note that I could not find a single caller of ALItem::newFromPage(), so I assumed that $desc was not escaped, like newFromEditLink and newFromExternalLink. Worst case some text will be double escaped instead of being under escaped.

ApprovedRevs:

@Yaron_Koren: Any comments on the patches attached here?

@Aklapper - sorry for not responding here before. I believe I took care of the Cargo patch with the commits 9b8d3a0a2d10 and 3407fb767040. I still need to take care of the Approved Revs and Admin Links patches.

Yaron_Koren claimed this task.

Okay, I believe all three patches are now merged in. In the case of Approved Revs, the code checked in was simpler than the original patch because support for MW < 1.28 was removed, soon after the original patches were created.

I forgot to say - thank you, @Legoktm, for your help with improving the security of all three extensions.

@Yaron_Koren - we can make this task public now, no? Given that all of the outstanding security issues have been patched?

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".