I think it would be sensible to import ExtParserFunctions::killMarkers() into CoreParserFunctions and to run it on pretty much everything. It's surprising that there aren't a hundred bugs reporting "strip marker exposed in CoreParserFunction X". That might be disruptive, so maybe it can wait until 1.20 if an audit of popular tag hook extensions comes up clear.

Obviously CharInsert should be patched along the lines suggested by Bawolff. We don't usually announce extension vulnerabilities, but since it's a popular extension, maybe we could include a note about it in the mail to mediawiki-announce for 1.18.2.

Created attachment 10269
Patch for CharInsert

Issue reproduced, fix tested. I'm not sure whether to commit this now with a coy commit message or to wait until closer to 1.18.2.

Attached:

Created attachment 10270
Unstrip in CPF::tagObj()

I think part of the blame lies with #tag, since before #tag was introduced, the arguments to tag hook functions could not include strip markers. Calling unstrip on the #tag inputs could be said to preserve interface backwards compatibility.

Patch proposed for 1.18.2. Ran parser tests.

attachment bug35315-tag-func.patch ignored as obsolete

(In reply to comment #3)

Created attachment 10270 [details]
Unstrip in CPF::tagObj()

I think part of the blame lies with #tag, since before #tag was introduced, the
arguments to tag hook functions could not include strip markers. Calling
unstrip on the #tag inputs could be said to preserve interface backwards
compatibility.

Patch proposed for 1.18.2. Ran parser tests.

Hmm, that would break Wikipedia's fancy <ref> inside a <ref> template (when they do things like {{#tag:ref|Some note<ref>citation for note</ref>}}). Otherwise it seems like that would fix a lot of things.

Maybe have some way tag extensions could specify that they want to not have things unstripped, in order to make having strip tags be opt in (Kind of reminds me of the /have a method to denote certain tag extensions should have PST be run on them/ bug).

Maybe removing U+007F in things like Xml::escapeTagsOnly and
MediaWiki's various other escaping functions might be a good idea. It
wouldn't fix the charinsert issue, but could prevent future problems
of a similar nature (I haven't actually looked to see if there's any
code that assumes strip markers can get through the escaping functions
safely, so that might not be that good an idea). Intuitively though, If I
pass something into an escaping function, usually that's the absolute
last step before output, and I would expect at that point that all
strip markers have already been unstriped, and that there would be no way for unescaped text to slip back in.

Also of interest, the following causes the parser to go in an indef loop:

<pre>test</pre>

{{#tag:pre|{{padleft:|21|<nowiki/>}}-pre-00000004-QINU}}

attachment bug35315-tag-func.patch ignored as obsolete

(In reply to comment #4)

Hmm, that would break Wikipedia's fancy <ref> inside a <ref> template (when
they do things like {{#tag:ref|Some note<ref>citation for note</ref>}}).
Otherwise it seems like that would fix a lot of things.

Good point. And there already is a parser test that checks for that, I'm not sure how I missed it.

Committed killMarkers/markerStripCallback patch in r114231.

Suggested release announcement text:

A long-standing bug in the wikitext parser (bug 22555) was discovered to have security implications. In the presence of the popular CharInsert extension, it leads to cross-site scripting (XSS). XSS may be possible with other extensions or perhaps even the MediaWiki core alone, although this is not confirmed at this time. A denial-of-service attack (infinite loop) is also possible regardless of configuration.

Created attachment 10295
Limit the number of iterations of the unstrip loop to the total number of items to unstrip

Sounds good.

Also might I suggest limiting the unstrip loop so that it is impossible to cause an indef loop (Hopefully the other change would prevent people from mucking with strip markers, but just in case, not having the potential for indef loop would be good to)

Attached:

(In reply to comment #8)

Created attachment 10295 [details]
Limit the number of iterations of the unstrip loop to the total number of items
to unstrip

Sounds good.

Also might I suggest limiting the unstrip loop so that it is impossible to
cause an indef loop (Hopefully the other change would prevent people from
mucking with strip markers, but just in case, not having the potential for
indef loop would be good to)

When I asked Tim about this, he didn't think it was necessary to be backported, but it's fine to go into trunk after the other commits have been made

Attached:

For XSS, this test case seems to work just as well:

{{#tag:charinsert|<nowiki>','',''); alert("XSS",')</nowiki>}}

It doesn't need forged strip markers. But the infinite loop in comment 4 does need forged strip markers.

(In reply to comment #8)

Created attachment 10295 [details]
Limit the number of iterations of the unstrip loop to the total number of items
to unstrip

This still left quite a bit of scope for algorithmic DoS. The iterative algorithm that you're hacking was an inefficient hack to start with, since it scans the entire expanded text at every recursion depth, giving a running time proportional to the depth multiplied by the size. I had a go at doing a proper job of it at https://gerrit.wikimedia.org/r/#change,5596

Attached: