Page MenuHomePhabricator

Linker::specialLink() does not escape the link text
Closed, ResolvedPublic

Description

	public static function specialLink( $name, $key = '' ) {
		if ( $key == '' ) {
			$key = strtolower( $name );
		}

		return self::linkKnown( SpecialPage::getTitleFor( $name ), wfMessage( $key )->text() );
	}

->text() should actually be ->escaped(). I don't think this is exploitable aside from making a text message into a raw HTML one.

The usage in the Nostalgia skin and WikimediaIncubator extension look fine to me,

(Noticed after csteipp pointed out that the new Linker rewrite should escape by default)

Event Timeline

dpatrick added a project: Vuln-XSS.
dpatrick added a subscriber: dpatrick.

Hey @Legoktm, we'd like to make this bug public. How soon will your rewrite of the linker land?

Thinking about this a little more.

There is no good reason that wfMessage() should ever have an unsafe output in the case of missing key. The fact that wfMessage( 'script>alert(1)<script' )->text() is evil, is totally unessary and weird. We should maybe make it strip angled brackets (They are already illegal in message names as not a valid title character), and maybe use square brackets as the delimiter instead.

Thinking about this a little more.

There is no good reason that wfMessage() should ever have an unsafe output in the case of missing key. The fact that wfMessage( 'script>alert(1)<script' )->text() is evil, is totally unessary and weird. We should maybe make it strip angled brackets (They are already illegal in message names as not a valid title character), and maybe use square brackets as the delimiter instead.

Which I actually did in https://gerrit.wikimedia.org/r/#/c/296665/

Could someone clarify which codebases / repositories this task is about? (@Legoktm, if you remember?) Wondering which tags to potentially add...

@Aklapper - the function within the description is from core, and the issue described seems to have been fixed on master years ago as part of the larger bug T85864. There have been/still are a few additional bugs which deal with similar message-escaping issues.

Thanks for the quick reply! Does that mean that this task is obsolete and should be closed, as additional bugs should be handled in other dedicated tickets?

sbassett closed this task as Resolved.EditedMay 7 2020, 3:14 PM
sbassett claimed this task.
sbassett added a subscriber: Nikerabbit.

@Aklapper - yes, we can resolve this for now as the primary issue within the description was fixed. We should likely also resolve/invalidate/decline T85864 per @Nikerabbit's recent suggestion there, as that task is incredibly stale and any new work, should it be picked up, should just become new sub-tasks of T2212.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".May 7 2020, 3:15 PM