Created attachment 13472
minimal patch

Patch fixes usage of preg_match() in the specific location I identified (which I don't believe affects the WMF cluster). Of course I haven't audited/fixed every use of the preg_* functions in MediaWiki.

It is, however, slightly concerning that U+FDD0 becomes a plain question mark (U+003F) in at least IE 6.

Attached:

Why on earth would we leave quotes out in the first place????

That option should simply be removed; we should always output quotes.

I agree with Brion. The stated reason that, "If set to false, output will be a few bytes shorter, and the HTML will arguably be more readable" but I would rather we err on the side of caution and always quote them, which we do by default since Tim's patch in 2010.

We can either add Kevin's patch as the security fix, and then do a public change to always quote, or we could make that a security fix.

Tim, do you have a preference?

Added Krinkle too, in case he knows the history, or sees any potential issues with always quoting.

In addition to fixing the quoting, should all 66 noncharacters be blacklisted in
UtfNormal, despite http://www.unicode.org/L2/L2013/13006-nonchar-clarif.pdf,
which says "It is good practice to support all characters, including private use and noncharacter code points, except when those are used with a different
meaning internally"?

Created attachment 13811
patch to block noncharacters in UtfNormal

While this patch doesn't fix the actual quoting bug, it would have prevented me from exploiting it in the manner described (by filtering the problematic character out of the input).

To move this bug along a bit, I'm going to create and upload a patch to make Html unconditionally quote attribute values. Merely using a *blacklist* of characters that require quotes was not a good idea from a security point of view.

Attached:

Created attachment 13814
patch to always quote attributes

Attached:

In T57548#611040, @PleaseStand wrote:

Created attachment 13811
patch to block noncharacters in UtfNormal

While this patch doesn't fix the actual quoting bug, it would have prevented me from exploiting it in the manner described (by filtering the problematic character out of the input).

Attached:

0001-SECURITY-UtfNormal-add-remaining-noncharacters.patch4 KBDownload

While this is probably a good idea just generally, from a security perspective I don't think its sufficient, since the output of lua modules don't go through utf-8 normalization.

Lets kill $wgWellFormedXml=false entirely

• Extra options add extra complexity and maintenance burden
• Options which are used by very few people tend to get tested less
  • In particular, I think translatewiki is the only big wiki using this (?)
• The benefits of this feature are pretty low, even lower if you take gzip compression into account
• Escaping is an area of code where we should be very conservative
• Having escaping rules depend on making assumptions about which characters various browsers consider "whitespace", and assuming every browser in existence will follow it for now and forever, is scary.
  • We do not escape any of % * + , - ; ^ | which OWASP claims is unsafe in unquoted attributes, although it doesn't seem to have any info on which browsers or how to exploit using those characters (source: https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#RULE_.232_-_Attribute_Escape_Before_Inserting_Untrusted_Data_into_HTML_Common_Attributes )
• $wgWellFormedXml=false has had a negative security impact in the past (Usually not directly its fault, but has made other bugs more exploitable). Including:
  • This bug about non-characters [Probably not as big a deal now, since 1.27 doesn't support the versions of php required, but our legacy release is still potentially vulnerable. Not to mention that people might have php custom compiled with weird versions of PCRE]
  • T73394: XSS in language converter when used with Html class's tricky escaping (Not the current version of this feature)
• Its also the cause of these other non-security bugs:
  • T51232: $wgWellFormedXml = false; breaks our EditPage broken bot protection in edittoken
• It should also be noted that a proposal to enable wgWellFormedXml=false by default was previously rejected: T52040: Set $wgWellFormedXml = false by default

If any of the parts of $wgWellFormedXml = false turn out to be useful (Perhaps no / on self-closing elements - T110616), I would suggest that people could submit patches later to change it for all of MW, and those patches can be argued on their merits. Having two versions of how we serialize html is nuts.

To get things moving, I'd like to state as a (possibly lofty) goal, to have this feature removed in time for 1.27

@Nikerabbit: translatewiki.net seems to be the main user of this feature - Do you think they would have any objections to its removal from MediaWiki

Parsing-Team--ARCHIVED: Given that parsing team is the closest team to having responsibility for this area of code, do you guys have any objections to removing this feature?

Assuming that there's no objection from translatewiki.net or Parsing-Team--ARCHIVED, what I'd propose doing next:

1. Send an email to wikitech-l proposing the feature be removed, citing basically the reasons above but without referencing any specific security issue
2. If there's no objections, get F3900760 merged as a normal gerrit patch, so its on master, and anyone with an interest can object
3. When T133070: MediaWiki 1.27.1 security release comes around, have it backported in a security release, and at that point release info about this security issue

I think that would serve as a compromise between giving people an opportunity to object if they really want to keep this feature, and maintaining confidentiality about the security issue. I wouldn't propose this course of action of the security bug had wide applicability, but this bug only applies to really old versions of php and MW in an unusual configuration.

Also, here's the proposed patch:

Thoughts?

Note that https://gerrit.wikimedia.org/r/#/c/286495/ which removes $wgWellFormedXml has been merged into master. (Bug still open because we also still have supported branches)

Can this be made public now?

In T57548#2783841, @Legoktm wrote:

Can this be made public now?

Whoops.

Public now. Better years late than never I suppose.