Page MenuHomePhabricator

id attribute on headlines allow raw > [Possible issue in combination with language converter] (CVE-2017-8812)
Closed, ResolvedPublic

Assigned To
Authored By
Bawolff
Jan 28 2016, 11:10 PM
Referenced Files
F10767949: T125163-REL1_28.patch
Nov 13 2017, 5:57 PM
F10767948: T125163-REL1_27.patch
Nov 13 2017, 5:57 PM
F10767950: T125163-REL1_29.patch
Nov 13 2017, 5:57 PM
F4326064: T125163-master
Aug 1 2016, 12:21 PM
F4326062: T125163-REL1_27
Aug 1 2016, 12:21 PM
F4326058: T125163-REL1_23
Aug 1 2016, 12:21 PM
F4326060: T125163-REL1_26
Aug 1 2016, 12:21 PM
F3278917: Use more complex regex for html in autoConvert
Jan 28 2016, 11:10 PM

Description

With

(See T119158 and T73394) this will no longer really be an issue once that patch is deployed, but given all the trouble with lang converter, might make sense as a hardening step.

wikitext like:

==>foo==

makes

<div id=".3Efoo"></div>
<h2>
<span class="mw-headline" id=">foo">&gt;foo</span><span class="mw-editsection">
<span class="mw-editsection-bracket">[</span><a href="/w/git/index.php?title=Special:ExpandTemplates&amp;action=edit&amp;section=1" title="Edit section: &gt;foo">edit</a>
<span class="mw-editsection-bracket">]</span></span>
</h2>

Note in particular: <span class="mw-headline" id=">foo">

Making that > be &gt; might be better as a hardening step against lang converter (or anything that uses regexes in a similar manner) mucking about.

Patch:


As an aside, this issue was noticed with the help of afl

Backports:

(Master as of last July 2016)

Event Timeline

Bawolff raised the priority of this task from to Needs Triage.
Bawolff updated the task description. (Show Details)
Bawolff changed the visibility from "Public (No Login Required)" to "Custom Policy".
Bawolff changed the edit policy from "All Users" to "Custom Policy".
Bawolff changed Security from None to Software security bug.
Bawolff subscribed.

Turns out, I inadvertently fixed this in https://gerrit.wikimedia.org/r/#/c/362326/ not even knowing about this bug. This change is present in 1.30. Do we need to backport to older versions?

Turns out, I inadvertently fixed this in https://gerrit.wikimedia.org/r/#/c/362326/ not even knowing about this bug. This change is present in 1.30. Do we need to backport to older versions?

It'd be certainly useful to do so... Can we tease out the security related parts? As otherwise backporting this through 1.29 back to 1.27 isn't going to be fun for anyone... And obviously, we don't generally want to be trying to backport "features" unless they're supporting to actually making a security fix

Removed patches

Ok, so comparing @Bawolff's patches to https://gerrit.wikimedia.org/r/#/c/362326/ and specifically in Linker.php https://gerrit.wikimedia.org/r/#/c/362326/28/includes/Linker.php we see the patches are mostly identical.

MaxSem just updates some documentation, and uses slightly different variable names

Am I ok to just cherry pick this file change in the open?

From fd6e9ef2d481209b01fa6e1bb1c863b8257f0272 Mon Sep 17 00:00:00 2001
From: Max Semenik <maxsem.wiki@gmail.com>
Date: Thu, 29 Jun 2017 17:13:12 -0700
Subject: [PATCH] Human-readable section ID support

It adds the ability to replace the current section ID escaping
schema (.C0.DE) with a HTML5-compliant escaping schema that is
displayed as Unicode in many modern browsers.

See the linked bug for discussion of various options that were
considered before the implementation. A few remarks:
* Because Sanitizer::escapeId() is used in a bunch of places without
  escaping, I'm deprecating it without altering its behavior.
* The bug described in comments for Parser::guessLegacySectionNameFromWikiText()
  is still there in some Edge versions that display mojibake.

Bug: T152540
Change-Id: Id304010a0342efbb7ef2d56c5b8b244f2e4fb2c5
---

diff --git a/includes/Linker.php b/includes/Linker.php
index 4aae3ba..2ca851c 100644
--- a/includes/Linker.php
+++ b/includes/Linker.php
@@ -1608,22 +1608,24 @@
 	 *   a space and ending with '>'
 	 *   This *must* be at least '>' for no attribs
 	 * @param string $anchor The anchor to give the headline (the bit after the #)
-	 * @param string $html Html for the text of the header
+	 * @param string $html HTML for the text of the header
 	 * @param string $link HTML to add for the section edit link
-	 * @param bool|string $legacyAnchor A second, optional anchor to give for
+	 * @param string|bool $fallbackAnchor A second, optional anchor to give for
 	 *   backward compatibility (false to omit)
 	 *
 	 * @return string HTML headline
 	 */
 	public static function makeHeadline( $level, $attribs, $anchor, $html,
-		$link, $legacyAnchor = false
+		$link, $fallbackAnchor = false
 	) {
+		$anchorEscaped = htmlspecialchars( $anchor );
 		$ret = "<h$level$attribs"
-			. "<span class=\"mw-headline\" id=\"$anchor\">$html</span>"
+			. "<span class=\"mw-headline\" id=\"$anchorEscaped\">$html</span>"
 			. $link
 			. "</h$level>";
-		if ( $legacyAnchor !== false ) {
-			$ret = "<div id=\"$legacyAnchor\"></div>$ret";
+		if ( $fallbackAnchor !== false && $fallbackAnchor !== $anchor ) {
+			$fallbackAnchor = htmlspecialchars( $fallbackAnchor );
+			$ret = "<div id=\"$fallbackAnchor\"></div>$ret";
 		}
 		return $ret;
 	}

^ For 1.27-1.29.. Just needs a new commit summary at least

Reedy assigned this task to MaxSem.

This comment was removed by Bawolff.
MoritzMuehlenhoff renamed this task from id attribute on headlines allow raw > [Possible issue in combination with language converter] to id attribute on headlines allow raw > [Possible issue in combination with language converter] (CVE-2017-8812).Nov 14 2017, 9:29 AM
Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Nov 15 2017, 12:05 AM
Reedy changed the edit policy from "Custom Policy" to "All Users".

Change 391410 merged by jenkins-bot:
[mediawiki/core@fundraising/REL1_27] Make anchor for headlines escape > and <

https://gerrit.wikimedia.org/r/391410