Page MenuHomePhabricator

As a hardening measure, MW's various comment based strip markers (e.g. <!--LINK 0:0-->) should include quotes to avoid being included in attributes
Closed, ResolvedPublic


The MediaWiki parser puts markers like <!--LINK 0:0--> as a placeholder for links which gets replaced with real links later on. (see includes/parser/Parser.php, especially Parser::replaceInternalLinks() and includes/parser/LinkHolderArray.php )

In principle, doing the following

<div data-foo="<!--LINK 0:0-->">

is perfectly valid html (comments don't count in attributes). Most people (including us) wouldn't allow users to create such html without escaping the < for sanity reasons, but technically its a perfectly valid (and safe) html string to output.

Given its perfectly safe (technically), its possible that someone might hook MW up to something that outputs something like that.

If someone used the InternalParseBeforeLinks hook to add <div data-foo="<!--LINK 0:0-->"> to the text, and there was a link like [[Main_Page#/onmouseover=alert(1);//]] on the page, than this would be an XSS. Admittedly this is super contrived, as its unlikely someone would allow such html, and even less likely they would just randomly use the InternalParseBeforeLinks instead of one of the more proper hooks.

Nevertheless, in the interest of hardening, I think it might make sense to change these markers to be <!--LINK'" 0:0--> to make 100% sure they can never ever appear in another attribute. (And similar for <!--IWLINK 0--> <!--MWTOC-->), since its impossible to have both ' and " in an attribute value without escaping one of them.

Event Timeline

Bawolff created this task.Nov 9 2017, 7:54 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 9 2017, 7:54 PM
Bawolff triaged this task as Low priority.Nov 11 2017, 8:24 PM
Bawolff updated the task description. (Show Details)
Bawolff edited projects, added MediaWiki-Parser, good first task; removed MediaWiki-General.
Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptNov 11 2017, 8:25 PM
nikitavbv claimed this task.Dec 6 2017, 8:43 PM
nikitavbv added a subscriber: nikitavbv.

I will work on this. I will contact you in case I have some questions to ask or some progress reports to show

Change 396392 had a related patch set uploaded (by Phantom42; owner: Phantom42):
[mediawiki/core@master] Add quotes to comment based strip markers

Bawolff closed this task as Resolved.Dec 8 2017, 4:44 PM

Change 396392 merged by jenkins-bot:
[mediawiki/core@master] Add quotes to comment based strip markers