Protect against font family innerHTML/cssText attacks.

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

NEWS

@@ -19,6 +19,9 @@ NEWS ( CHANGELOG and HISTORY )                                     HTMLPurifier
   spaces.  This constitutes a slight semantic change, which can be
   reverted using %Output.FixInnerHTML.  Reported by Neike Taika-Tessaro
   and Mario Heiderich.
+# Protect against cssText/innerHTML by restricting allowed characters
+  used in fonts further than mandated by the specification.  Reported
+  by Neike Taika-Tessaro and Mario Heiderich.
 ! Added %HTML.Nofollow to add rel="nofollow" to external links.
 ! More types of SPL autoloaders allowed on later versions of PHP.
 ! Implementations for position, top, left, right, bottom, z-index

configdoc/usage.xml

@@ -96,27 +96,32 @@
  </directive>
  <directive id="Output.CommentScriptContents">
   <file name="HTMLPurifier/Generator.php">
-   <line>56</line>
+   <line>61</line>
+  </file>
+ </directive>
+ <directive id="Output.FixInnerHTML">
+  <file name="HTMLPurifier/Generator.php">
+   <line>62</line>
   </file>
  </directive>
  <directive id="Output.SortAttr">
   <file name="HTMLPurifier/Generator.php">
-   <line>57</line>
+   <line>63</line>
   </file>
  </directive>
  <directive id="Output.FlashCompat">
   <file name="HTMLPurifier/Generator.php">
-   <line>58</line>
+   <line>64</line>
   </file>
  </directive>
  <directive id="Output.TidyFormat">
   <file name="HTMLPurifier/Generator.php">
-   <line>87</line>
+   <line>93</line>
   </file>
  </directive>
  <directive id="Core.NormalizeNewlines">
   <file name="HTMLPurifier/Generator.php">
-   <line>101</line>
+   <line>107</line>
   </file>
   <file name="HTMLPurifier/Lexer.php">
    <line>266</line>
@@ -124,7 +129,7 @@
  </directive>
  <directive id="Output.Newline">
   <file name="HTMLPurifier/Generator.php">
-   <line>102</line>
+   <line>108</line>
   </file>
  </directive>
  <directive id="HTML.BlockWrapper">
@@ -254,6 +259,9 @@
   <file name="HTMLPurifier/URIDefinition.php">
    <line>64</line>
   </file>
+  <file name="HTMLPurifier/URIScheme.php">
+   <line>75</line>
+  </file>
  </directive>
  <directive id="URI.Base">
   <file name="HTMLPurifier/URIDefinition.php">
@@ -288,6 +296,11 @@
    <line>12</line>
   </file>
  </directive>
+ <directive id="CSS.AllowedFonts">
+  <file name="HTMLPurifier/AttrDef/CSS/FontFamily.php">
+   <line>50</line>
+  </file>
+ </directive>
  <directive id="Attr.AllowedClasses">
   <file name="HTMLPurifier/AttrDef/HTML/Class.php">
    <line>18</line>
@@ -454,7 +467,7 @@
  </directive>
  <directive id="Core.EscapeInvalidTags">
   <file name="HTMLPurifier/Strategy/MakeWellFormed.php">
-   <line>45</line>
+   <line>53</line>
   </file>
   <file name="HTMLPurifier/Strategy/RemoveForeignElements.php">
    <line>19</line>

library/HTMLPurifier/AttrDef/CSS/FontFamily.php

@@ -6,6 +6,39 @@
 class HTMLPurifier_AttrDef_CSS_FontFamily extends HTMLPurifier_AttrDef
 {
 
+    protected $mask = null;
+
+    public function __construct() {
+        $this->mask = '- ';
+        for ($c = 'a'; $c <= 'z'; $c++) $this->mask .= $c;
+        for ($c = 'A'; $c <= 'Z'; $c++) $this->mask .= $c;
+        for ($c = '0'; $c <= '9'; $c++) $this->mask .= $c; // cast-y, but should be fine
+        // special bytes used by UTF-8
+        for ($i = 0x80; $i <= 0xFF; $i++) {
+            // We don't bother excluding invalid bytes in this range,
+            // because the our restriction of well-formed UTF-8 will
+            // prevent these from ever occurring.
+            $this->mask .= chr($i);
+        }
+
+        /*
+            PHP's internal strcspn implementation is
+            O(length of string * length of mask), making it inefficient
+            for large masks.  However, it's still faster than
+            preg_match 8)
+          for (p = s1;;) {
+            spanp = s2;
+            do {
+              if (*spanp == c || p == s1_end) {
+                return p - s1;
+              }
+            } while (spanp++ < (s2_end - 1));
+            c = *++p;
+          }
+         */
+        // possible optimization: invert the mask.
+    }
+
     public function validate($string, $config, $context) {
         static $generic_names = array(
             'serif' => true,
@@ -56,17 +89,103 @@ class HTMLPurifier_AttrDef_CSS_FontFamily extends HTMLPurifier_AttrDef
             // shouldn't show up regardless
             $font = str_replace(array("\n", "\t", "\r", "\x0C"), ' ', $font);
 
-            // These ugly transforms don't pose a security
+            // Here, there are various classes of characters which need
-            // risk (as \\ and \" might).  We could try to be clever and
+            // to be treated differently:
-            // use single-quote wrapping when there is a double quote
+            //  - Alphanumeric characters are essentially safe.  We
-            // present, but I have choosen not to implement that.
+            //    handled these above.
-            // (warning: this code relies on the selection of quotation
+            //  - Spaces require quoting, though most parsers will do
-            // mark below)
+            //    the right thing if there aren't any characters that
-            $font = str_replace('\\', '\\5C ', $font);
+            //    can be misinterpreted
-            $font = str_replace('"',  '\\22 ', $font);
+            //  - Dashes rarely occur, but they fairly unproblematic
+            //    for parsing/rendering purposes.
+            //  The above characters cover the majority of Western font
+            //  names.
+            //  - Arbitrary Unicode characters not in ASCII.  Because
+            //    most parsers give little thought to Unicode, treatment
+            //    of these codepoints is basically uniform, even for
+            //    punctuation-like codepoints.  These characters can
+            //    show up in non-Western pages and are supported by most
+            //    major browsers, for example: "ＭＳ 明朝" is a
+            //    legitimate font-name
+            //    <http://ja.wikipedia.org/wiki/MS_明朝>.  See
+            //    the CSS3 spec for more examples:
+            //    <http://www.w3.org/TR/2011/WD-css3-fonts-20110324/localizedfamilynames.png>
+            //    You can see live samples of these on the Internet:
+            //    <http://www.google.co.jp/search?q=font-family+ＭＳ+明朝|ゴシック>
+            //    However, most of these fonts have ASCII equivalents:
+            //    for example, 'MS Mincho', and it's considered
+            //    professional to use ASCII font names instead of
+            //    Unicode font names.  Thanks Takeshi Terada for
+            //    providing this information.
+            //  The following characters, to my knowledge, have not been
+            //  used to name font names.
+            //  - Single quote.  While theoretically you might find a
+            //    font name that has a single quote in its name (serving
+            //    as an apostrophe, e.g. Dave's Scribble), I haven't
+            //    been able to find any actual examples of this.
+            //    Internet Explorer's cssText translation (which I
+            //    believe is invoked by innerHTML) normalizes any
+            //    quoting to single quotes, and fails to escape single
+            //    quotes.  (Note that this is not IE's behavior for all
+            //    CSS properties, just some sort of special casing for
+            //    font-family).  So a single quote *cannot* be used
+            //    safely in the font-family context if there will be an
+            //    innerHTML/cssText translation.  Note that Firefox 3.x
+            //    does this too.
+            //  - Double quote.  In IE, these get normalized to
+            //    single-quotes, no matter what the encoding.  (Fun
+            //    fact, in IE8, the 'content' CSS property gained
+            //    support, where they special cased to preserve encoded
+            //    double quotes, but still translate unadorned double
+            //    quotes into single quotes.)  So, because their
+            //    fixpoint behavior is identical to single quotes, they
+            //    cannot be allowed either.  Firefox 3.x displays
+            //    single-quote style behavior.
+            //  - Backslashes are reduced by one (so \\ -> \) every
+            //    iteration, so they cannot be used safely.  This shows
+            //    up in IE7, IE8 and FF3
+            //  - Semicolons, commas and backticks are handled properly.
+            //  - The rest of the ASCII punctuation is handled properly.
+            // We haven't checked what browsers do to unadorned
+            // versions, but this is not important as long as the
+            // browser doesn't /remove/ surrounding quotes (as IE does
+            // for HTML).
+            //
+            // With these results in hand, we conclude that there are
+            // various levels of safety:
+            //  - Paranoid: alphanumeric, spaces and dashes(?)
+            //  - International: Paranoid + non-ASCII Unicode
+            //  - Edgy: Everything except quotes, backslashes
+            //  - NoJS: Standards compliance, e.g. sod IE. Note that
+            //    with some judicious character escaping (since certain
+            //    types of escaping doesn't work) this is theoretically
+            //    OK as long as innerHTML/cssText is not called.
+            // We believe that international is a reasonable default
+            // (that we will implement now), and once we do more
+            // extensive research, we may feel comfortable with dropping
+            // it down to edgy.
 
-            // complicated font, requires quoting
+            // Edgy: alphanumeric, spaces, dashes and Unicode.  Use of
-            $final .= "\"$font\", "; // note that this will later get turned into &quot;
+            // str(c)spn assumes that the string was already well formed
+            // Unicode (which of course it is).
+            if (strspn($font, $this->mask) !== strlen($font)) {
+                continue;
+            }
+
+            // Historical:
+            // In the absence of innerHTML/cssText, these ugly
+            // transforms don't pose a security risk (as \\ and \"
+            // might--these escapes are not supported by most browsers).
+            // We could try to be clever and use single-quote wrapping
+            // when there is a double quote present, but I have choosen
+            // not to implement that.  (NOTE: you can reduce the amount
+            // of escapes by one depending on what quoting style you use)
+            // $font = str_replace('\\', '\\5C ', $font);
+            // $font = str_replace('"',  '\\22 ', $font);
+            // $font = str_replace("'",  '\\27 ', $font);
+
+            // font possibly with spaces, requires quoting
+            $final .= "'$font', ";
         }
         $final = rtrim($final, ', ');
         if ($final === '') return false;

library/HTMLPurifier/URI.php

@@ -185,7 +185,7 @@ class HTMLPurifier_URI
 
         // Reconstruct the result
         // One might wonder about parsing quirks from browsers after
-        // this reconstruction.  Unfortunately, parsing behaviro depends
+        // this reconstruction.  Unfortunately, parsing behavior depends
         // on what *scheme* was employed (file:///foo is handled *very*
         // differently than http:///foo), so unfortunately we have to
         // defer to the schemes to do the right thing.

tests/HTMLPurifier/AttrDef/CSS/FontFamilyTest.php

@@ -8,30 +8,32 @@ class HTMLPurifier_AttrDef_CSS_FontFamilyTest extends HTMLPurifier_AttrDefHarnes
         $this->def = new HTMLPurifier_AttrDef_CSS_FontFamily();
 
         $this->assertDef('Gill, Helvetica, sans-serif');
-        $this->assertDef('"Times New Roman", serif');
+        $this->assertDef("'Times New Roman', serif");
-        $this->assertDef('\'Times New Roman\'', '"Times New Roman"');
+        $this->assertDef("\"Times New Roman\"", "'Times New Roman'");
         $this->assertDef('01234');
         $this->assertDef(',', false);
-        $this->assertDef('Times New Roman, serif', '"Times New Roman", serif');
+        $this->assertDef('Times New Roman, serif', "'Times New Roman', serif");
-        $this->assertDef($d = '"John\'s Font"');
+        $this->assertDef($d = "'\xE5\xAE\x8B\xE4\xBD\x93'");
-        $this->assertDef("John's Font", $d);
-        $this->assertDef($d = "\"\xE5\xAE\x8B\xE4\xBD\x93\"");
         $this->assertDef("\xE5\xAE\x8B\xE4\xBD\x93", $d);
-        $this->assertDef("'\\','f'", "\"\\5C \", f");
+        $this->assertDef("'\\01'", "''");
-        $this->assertDef("'\\01'", "\"\"");
+        $this->assertDef("'\\20'", "' '");
-        $this->assertDef("'\\20'", "\" \"");
+        $this->assertDef("\\0020", "' '");
-        $this->assertDef("\\0020", "\" \"");
         $this->assertDef("'\\000045'", "E");
         $this->assertDef("','", false);
-        $this->assertDef("',' foobar','", "\" foobar\"");
+        $this->assertDef("',' foobar','", "' foobar'");
-        $this->assertDef("'\\27'", "\"'\"");
-        $this->assertDef('"\\22"', "\"\\22 \"");
-        $this->assertDef('"\\""', "\"\\22 \"");
-        $this->assertDef('"\'"', "\"'\"");
         $this->assertDef("'\\000045a'", "Ea");
         $this->assertDef("'\\00045 a'", "Ea");
-        $this->assertDef("'\\00045  a'", "\"E a\"");
+        $this->assertDef("'\\00045  a'", "'E a'");
         $this->assertDef("'\\\nf'", "f");
+        // No longer supported, except maybe in NoJS mode (see source
+        // file for more explanation)
+        //$this->assertDef($d = '"John\'s Font"');
+        //$this->assertDef("John's Font", $d);
+        //$this->assertDef("'\\','f'", "\"\\5C \", f");
+        //$this->assertDef("'\\27'", "\"'\"");
+        //$this->assertDef('"\\22"', "\"\\22 \"");
+        //$this->assertDef('"\\""', "\"\\22 \"");
+        //$this->assertDef('"\'"', "\"'\"");
     }
 
     function testAllowed() {
@@ -40,8 +42,8 @@ class HTMLPurifier_AttrDef_CSS_FontFamilyTest extends HTMLPurifier_AttrDefHarnes
         $this->assertDef('serif');
         $this->assertDef('sans-serif', false);
         $this->assertDef('serif, sans-serif', 'serif');
-        $this->assertDef('Times New Roman', '"Times New Roman"');
+        $this->assertDef('Times New Roman', "'Times New Roman'");
-        $this->assertDef('"Times New Roman"');
+        $this->assertDef("'Times New Roman'");
         $this->assertDef('foo', false);
     }
 

tests/HTMLPurifier/AttrDef/CSS/FontTest.php

@@ -11,10 +11,10 @@ class HTMLPurifier_AttrDef_CSS_FontTest extends HTMLPurifier_AttrDefHarness
         // hodgepodge of usage cases from W3C spec, but " -> '
         $this->assertDef('12px/14px sans-serif');
         $this->assertDef('80% sans-serif');
-        $this->assertDef('x-large/110% "New Century Schoolbook", serif');
+        $this->assertDef("x-large/110% 'New Century Schoolbook', serif");
         $this->assertDef('bold italic large Palatino, serif');
         $this->assertDef('normal small-caps 120%/120% fantasy');
-        $this->assertDef('300 italic 1.3em/1.7em "FB Armada", sans-serif');
+        $this->assertDef("300 italic 1.3em/1.7em 'FB Armada', sans-serif");
         $this->assertDef('600 9px Charcoal');
         $this->assertDef('600 9px/ 12px Charcoal', '600 9px/12px Charcoal');
 

tests/HTMLPurifier/AttrDef/CSSTest.php

@@ -62,7 +62,7 @@ class HTMLPurifier_AttrDef_CSSTest extends HTMLPurifier_AttrDefHarness
         $this->assertDef('width:-50px;', false);
         $this->assertDef('text-decoration:underline;');
         $this->assertDef('font-family:sans-serif;');
-        $this->assertDef('font-family:Gill, "Times New Roman", sans-serif;');
+        $this->assertDef("font-family:Gill, 'Times New Roman', sans-serif;");
         $this->assertDef('font:12px serif;');
         $this->assertDef('border:1px solid #000;');
         $this->assertDef('border-bottom:2em double #FF00FA;');

tests/HTMLPurifier/Filter/ExtractStyleBlocksTest.php

@@ -79,10 +79,13 @@ class HTMLPurifier_Filter_ExtractStyleBlocksTest extends HTMLPurifier_Harness
     }
 
     function test_cleanCSS_angledBrackets() {
-        $this->assertCleanCSS(
+        // [Content] No longer can smuggle in angled brackets using
-            ".class {\nfont-family:'</style>';\n}",
+        // font-family; when we add support for 'content', reinstate
-            ".class {\nfont-family:\"\\3C /style\\3E \";\n}"
+        // this test.
-        );
+        //$this->assertCleanCSS(
+        //    ".class {\nfont-family:'</style>';\n}",
+        //    ".class {\nfont-family:\"\\3C /style\\3E \";\n}"
+        //);
     }
 
     function test_cleanCSS_angledBrackets2() {
@@ -97,6 +100,7 @@ class HTMLPurifier_Filter_ExtractStyleBlocksTest extends HTMLPurifier_Harness
         $this->assertCleanCSS("div {bogus:tree;}", "div {\n}");
     }
 
+    /* [CONTENT]
     function test_cleanCSS_escapeCodes() {
         $this->assertCleanCSS(
             ".class {\nfont-family:\"\\3C /style\\3E \";\n}"
@@ -109,6 +113,7 @@ class HTMLPurifier_Filter_ExtractStyleBlocksTest extends HTMLPurifier_Harness
             ".class {\nfont-family:\"</style>\";\n}"
         );
     }
+     */
 
     function test_cleanCSS_scope() {
         $this->config->set('Filter.ExtractStyleBlocks.Scope', '#foo');

tests/HTMLPurifier/HTMLT/shift-jis-preserve-yen.htmlt

@@ -4,5 +4,5 @@ if (!function_exists('iconv')) return true;
 Core.Encoding = "Shift_JIS"
 Core.EscapeNonASCIICharacters = true
 --HTML--
-<b style="font-family:&quot;&#165;&quot;;">111</b>
+<b style="font-family:'&#165;';">111</b>
 --# vim: et sw=4 sts=4

tests/HTMLPurifier/HTMLT/shift-jis-remove-yen.htmlt

@@ -3,7 +3,7 @@ if (!function_exists('iconv')) return true;
 --INI--
 Core.Encoding = Shift_JIS
 --HTML--
-<b style="font-family:&quot;&#165;&quot;;">111</b>
+<b style="font-family:'&#165;';">111</b>
 --EXPECT--
-<b style="font-family:&quot;&quot;;">111</b>
+<b style="font-family:'';">111</b>
 --# vim: et sw=4 sts=4