GWicke has uploaded a new change for review.

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


Change subject: Fix css decoding in the sanitizer
......................................................................

Fix css decoding in the sanitizer

While investigating our handling of the non-IE6 part of bug 55332, I noticed
that our CSS char escape decoding was fairly broken. The regexp was not
adjusted properly for JS, the return values were checking for the empty string
instead of undefined and codepointToUtf8 did not look as if it ever worked
(and used the deprecated unescape method too).

The good news is that the original code does not seem to suffer from the
reported security vulnerability either. The url\b(..) escape results in
urlb(..) which is harmless.

The new code properly decodes the escape and detects the css string as
dangerous.

Bug 55332 also discusses IE6-specific vulnerabilities, but as our current main
user is VE which won't ever be enabled with IE6 we probably don't need to care
any more. I asked for browser stat data in support of a possible
no-session-for-IE6 policy in bug 56575.

Bug: 55332
Change-Id: Ia0230f2258a360749a6cbffff716ed426a207ce9
---
M js/lib/ext.core.Sanitizer.js
M js/lib/mediawiki.Util.js
M js/tests/parserTests-blacklist.js
3 files changed, 13 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Parsoid 
refs/changes/17/94517/1

diff --git a/js/lib/ext.core.Sanitizer.js b/js/lib/ext.core.Sanitizer.js
index 28d3a76..938d15a 100644
--- a/js/lib/ext.core.Sanitizer.js
+++ b/js/lib/ext.core.Sanitizer.js
@@ -380,10 +380,10 @@
                        var backslash = '\\\\';
                        return new RegExp(backslash +
                                "(?:" +
-                                       "(" + nl + ") |" + // 1. Line 
continuation
-                                       "([0-9A-Fa-f]{1,6})" + space+ "? |" + 
// 2. character number
-                                       "(.) |" + // 3. backslash cancelling 
special meaning
-                                       "() |" + // 4. backslash at end of 
string
+                                       "(" + nl + ")|" + // 1. Line 
continuation
+                                       "([0-9A-Fa-f]{1,6})" + space + "?|" + 
// 2. character number
+                                       "(.)|" + // 3. backslash cancelling 
special meaning
+                                       "()$" + // 4. backslash at end of string
                                ")");
                }
 
@@ -837,12 +837,12 @@
        text = this.decodeCharReferences(text);
        text = text.replace(this.constants.cssDecodeRE, function() {
                                var c;
-                               if (arguments[1] !== '' ) {
+                               if (arguments[1] !== undefined ) {
                                        // Line continuation
                                        return '';
-                               } else if (arguments[2] !== '' ) {
+                               } else if (arguments[2] !== undefined ) {
                                        c = 
Util.codepointToUtf8(parseInt(arguments[2], 16));
-                               } else if (arguments[3] !== '' ) {
+                               } else if (arguments[3] !== undefined ) {
                                        c = arguments[3];
                                } else {
                                        c = '\\';
diff --git a/js/lib/mediawiki.Util.js b/js/lib/mediawiki.Util.js
index 93f3bb3..e36b2d0 100644
--- a/js/lib/mediawiki.Util.js
+++ b/js/lib/mediawiki.Util.js
@@ -884,7 +884,12 @@
 
        // Returns the utf8 encoding of the code point
        codepointToUtf8: function(cp) {
-               return unescape(encodeURIComponent(cp));
+               try {
+                       return String.fromCharCode(cp);
+               } catch (e) {
+                       // Return a tofu?
+                       return cp.toString();
+               }
        },
 
        // Returns true if a given Unicode codepoint is a valid character in 
XML.
diff --git a/js/tests/parserTests-blacklist.js 
b/js/tests/parserTests-blacklist.js
index 85fc651..573bb2c 100644
--- a/js/tests/parserTests-blacklist.js
+++ b/js/tests/parserTests-blacklist.js
@@ -291,9 +291,6 @@
 add("wt2html", "Bug 2095: link with pipe and three closing brackets, version 
2");
 add("wt2html", "Bug 2304: HTML attribute safety (unsafe breakout parameter; 
2309)");
 add("wt2html", "Bug 2304: HTML attribute safety (unsafe breakout parameter 2; 
2309)");
-add("wt2html", "MSIE CSS safety test: hex code");
-add("wt2html", "CSS line continuation 1");
-add("wt2html", "CSS line continuation 2");
 add("wt2html", "Parser hook: empty input");
 add("wt2html", "Parser hook: empty input using terminated empty elements");
 add("wt2html", "Parser hook: empty input using terminated empty elements 
(space before)");

-- 
To view, visit https://gerrit.wikimedia.org/r/94517
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia0230f2258a360749a6cbffff716ed426a207ce9
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Parsoid
Gerrit-Branch: master
Gerrit-Owner: GWicke <[email protected]>

_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to