[3.1.1] Implement more robust imagecrash protection for CSS width/height.

- Change API for HTMLPurifier_AttrDef_CSS_Length
- Implement HTMLPurifier_AttrDef_Switch class
- Implement HTMLPurifier_Length->compareTo, and make make() accept object instances

git-svn-id: http://htmlpurifier.org/svnroot/htmlpurifier/trunk@1754 48356398-32a2-884e-a903-53898d9a118a

NEWS

@@ -13,6 +13,10 @@ NEWS ( CHANGELOG and HISTORY )                                     HTMLPurifier
 . Added HTMLPurifier_UnitConverter and HTMLPurifier_Length for convenient
   handling of CSS-style lengths. HTMLPurifier_AttrDef_CSS_Length now uses
   this class.
+. API of HTMLPurifier_AttrDef_CSS_Length changed from __construct($disable_negative)
+  to __construct($min, $max). __construct(true) is equivalent to
+  __construct('0').
+. Added HTMLPurifier_AttrDef_Switch class
 
 3.1.0, released 2008-05-18
 # Unnecessary references to objects (vestiges of PHP4) removed from method

TODO

@@ -12,8 +12,7 @@ amount of effort to implement, it may get endlessly delayed. Do not be
 afraid to cast your vote for the next feature to be implemented!
 
 - Implement validation for query and for fragment
-- Allow imagecrash protection in CSS images to be turned off
+- Prevent percentages from being used in width/height attribute in images
-- Allow imagecrash protection in CSS to be configurable with a max value
 - Maintain old attribute data in tokens (configurable?)
 - Lazy update of token when validating attributes?
 - Investigate how early internal structures can be accessed; this would

configdoc/usage.xml

@@ -16,24 +16,29 @@
    <line>44</line>
   </file>
  </directive>
+ <directive id="CSS.MaxImgLength">
+  <file name="HTMLPurifier/CSSDefinition.php">
+   <line>157</line>
+  </file>
+ </directive>
  <directive id="CSS.Proprietary">
   <file name="HTMLPurifier/CSSDefinition.php">
-   <line>202</line>
+   <line>209</line>
   </file>
  </directive>
  <directive id="CSS.AllowTricky">
   <file name="HTMLPurifier/CSSDefinition.php">
-   <line>206</line>
+   <line>213</line>
   </file>
  </directive>
  <directive id="CSS.AllowImportant">
   <file name="HTMLPurifier/CSSDefinition.php">
-   <line>210</line>
+   <line>217</line>
   </file>
  </directive>
  <directive id="CSS.AllowedProperties">
   <file name="HTMLPurifier/CSSDefinition.php">
-   <line>262</line>
+   <line>269</line>
   </file>
  </directive>
  <directive id="Cache.DefinitionImpl">

library/HTMLPurifier.includes.php

@@ -74,6 +74,7 @@ require 'HTMLPurifier/AttrDef/CSS.php';
 require 'HTMLPurifier/AttrDef/Enum.php';
 require 'HTMLPurifier/AttrDef/Integer.php';
 require 'HTMLPurifier/AttrDef/Lang.php';
+require 'HTMLPurifier/AttrDef/Switch.php';
 require 'HTMLPurifier/AttrDef/Text.php';
 require 'HTMLPurifier/AttrDef/URI.php';
 require 'HTMLPurifier/AttrDef/CSS/Number.php';

library/HTMLPurifier.safe-includes.php

@@ -68,6 +68,7 @@ require_once $__dir . '/HTMLPurifier/AttrDef/CSS.php';
 require_once $__dir . '/HTMLPurifier/AttrDef/Enum.php';
 require_once $__dir . '/HTMLPurifier/AttrDef/Integer.php';
 require_once $__dir . '/HTMLPurifier/AttrDef/Lang.php';
+require_once $__dir . '/HTMLPurifier/AttrDef/Switch.php';
 require_once $__dir . '/HTMLPurifier/AttrDef/Text.php';
 require_once $__dir . '/HTMLPurifier/AttrDef/URI.php';
 require_once $__dir . '/HTMLPurifier/AttrDef/CSS/Number.php';

library/HTMLPurifier/AttrDef/CSS/Length.php

@@ -6,14 +6,15 @@
 class HTMLPurifier_AttrDef_CSS_Length extends HTMLPurifier_AttrDef
 {
     
-    protected $nonNegative;
+    protected $min, $max;
     
     /**
-     * @param $non_negative Bool indication whether or not negative values are
+     * @param HTMLPurifier_Length $max Minimum length, or null for no bound. String is also acceptable.
-     *                      allowed.
+     * @param HTMLPurifier_Length $max Maximum length, or null for no bound. String is also acceptable.
      */
-    public function __construct($non_negative = false) {
+    public function __construct($min = null, $max = null) {
-        $this->nonNegative = $non_negative;
+        $this->min = $min !== null ? HTMLPurifier_Length::make($min) : null;
+        $this->max = $max !== null ? HTMLPurifier_Length::make($max) : null;
     }
     
     public function validate($string, $config, $context) {
@@ -25,10 +26,18 @@ class HTMLPurifier_AttrDef_CSS_Length extends HTMLPurifier_AttrDef
         if (strlen($string) === 1) return false;
         
         $length = HTMLPurifier_Length::make($string);
-        if (!$length->isValid($this->nonNegative)) return false;
+        if (!$length->isValid()) return false;
         
-        $n = $length->getN();
+        if ($this->min) {
-        if ($this->nonNegative && $n < 0) return false;
+            $c = $length->compareTo($this->min);
+            if ($c === false) return false;
+            if ($c < 0) return false;
+        }
+        if ($this->max) {
+            $c = $length->compareTo($this->max);
+            if ($c === false) return false;
+            if ($c > 0) return false;
+        }
         
         return $length->toString();
     }

library/HTMLPurifier/AttrDef/Switch.php

@@ -0,0 +1,32 @@
+<?php
+
+/**
+ * Decorator that, depending on a token, switches between two definitions.
+ */
+class HTMLPurifier_AttrDef_Switch
+{
+    
+    protected $tag;
+    protected $withTag, $withoutTag;
+    
+    /**
+     * @param string $tag Tag name to switch upon
+     * @param HTMLPurifier_AttrDef $with_tag Call if token matches tag
+     * @param HTMLPurifier_AttrDef $without_tag Call if token doesn't match, or there is no token
+     */
+    public function __construct($tag, $with_tag, $without_tag) {
+        $this->tag = $tag;
+        $this->withTag = $with_tag;
+        $this->withoutTag = $without_tag;
+    }
+    
+    public function validate($string, $config, $context) {
+        $token = $context->get('CurrentToken', true);
+        if (!$token || $token->name !== $this->tag) {
+            return $this->withoutTag->validate($string, $config, $context);
+        } else {
+            return $this->withTag->validate($string, $config, $context);
+        }
+    }
+    
+}

library/HTMLPurifier/CSSDefinition.php

@@ -90,7 +90,7 @@ class HTMLPurifier_CSSDefinition extends HTMLPurifier_Definition
         $this->info['border-left-width'] = 
         $this->info['border-right-width'] = new HTMLPurifier_AttrDef_CSS_Composite(array(
             new HTMLPurifier_AttrDef_Enum(array('thin', 'medium', 'thick')),
-            new HTMLPurifier_AttrDef_CSS_Length(true) //disallow negative
+            new HTMLPurifier_AttrDef_CSS_Length('0') //disallow negative
         ));
         
         $this->info['border-width'] = new HTMLPurifier_AttrDef_CSS_Multiple($border_width);
@@ -116,7 +116,7 @@ class HTMLPurifier_CSSDefinition extends HTMLPurifier_Definition
         $this->info['line-height'] = new HTMLPurifier_AttrDef_CSS_Composite(array(
             new HTMLPurifier_AttrDef_Enum(array('normal')),
             new HTMLPurifier_AttrDef_CSS_Number(true), // no negatives
-            new HTMLPurifier_AttrDef_CSS_Length(true),
+            new HTMLPurifier_AttrDef_CSS_Length('0'),
             new HTMLPurifier_AttrDef_CSS_Percentage(true)
         ));
         
@@ -138,7 +138,7 @@ class HTMLPurifier_CSSDefinition extends HTMLPurifier_Definition
         $this->info['padding-bottom'] = 
         $this->info['padding-left'] = 
         $this->info['padding-right'] = new HTMLPurifier_AttrDef_CSS_Composite(array(
-            new HTMLPurifier_AttrDef_CSS_Length(true),
+            new HTMLPurifier_AttrDef_CSS_Length('0'),
             new HTMLPurifier_AttrDef_CSS_Percentage(true)
         ));
         
@@ -151,12 +151,19 @@ class HTMLPurifier_CSSDefinition extends HTMLPurifier_Definition
         
         $this->info['width'] =
         $this->info['height'] =
-        new HTMLPurifier_AttrDef_CSS_DenyElementDecorator(
+        new HTMLPurifier_AttrDef_Switch('img',
-        new HTMLPurifier_AttrDef_CSS_Composite(array(
+            // For img tags:
-            new HTMLPurifier_AttrDef_CSS_Length(true),
+            new HTMLPurifier_AttrDef_CSS_Composite(array(
-            new HTMLPurifier_AttrDef_CSS_Percentage(true),
+                new HTMLPurifier_AttrDef_CSS_Length('0', $config->get('CSS', 'MaxImgLength')),
-            new HTMLPurifier_AttrDef_Enum(array('auto'))
+                new HTMLPurifier_AttrDef_Enum(array('auto'))
-        )), 'img');
+            )),
+            // For everyone else:
+            new HTMLPurifier_AttrDef_CSS_Composite(array(
+                new HTMLPurifier_AttrDef_CSS_Length('0'),
+                new HTMLPurifier_AttrDef_CSS_Percentage(true),
+                new HTMLPurifier_AttrDef_Enum(array('auto'))
+            ))
+        );
         
         $this->info['text-decoration'] = new HTMLPurifier_AttrDef_CSS_TextDecoration();
         

library/HTMLPurifier/ConfigSchema/schema/CSS.MaxImgLength.txt

@@ -0,0 +1,11 @@
+CSS.MaxImgLength
+TYPE: string/null
+DEFAULT: '1200px'
+VERSION: 3.1.1
+--DESCRIPTION--
+<p>
+ This parameter sets the maximum allowed length on <code>img</code> tags,
+ effectively the <code>width</code> and <code>height</code> properties.
+ Only absolute units of measurement (in, pt, pc, mm, cm) and pixels (px) are allowed. This is
+ in place to prevent imagecrash attacks, disable with null at your own risk.
+</p>

library/HTMLPurifier/Length.php

@@ -44,6 +44,7 @@ class HTMLPurifier_Length
      * @warning Does not perform validation.
      */
     static public function make($s) {
+        if ($s instanceof HTMLPurifier_Length) return $s;
         $n_length = strspn($s, '1234567890.+-');
         $n = substr($s, 0, $n_length);
         $unit = substr($s, $n_length);
@@ -94,4 +95,19 @@ class HTMLPurifier_Length
         return $this->isValid;
     }
     
+    /**
+     * Compares two lengths, and returns 1 if greater, -1 if less and 0 if equal.
+     * @warning If both values are too large or small, this calculation will
+     *          not work properly
+     */
+    public function compareTo($l) {
+        if ($l === false) return false;
+        if ($l->unit !== $this->unit) {
+            $converter = new HTMLPurifier_UnitConverter();
+            $l = $converter->convert($l, $this->unit);
+            if ($l === false) return false;
+        }
+        return $this->n - $l->n;
+    }
+    
 }

tests/HTMLPurifier/AttrDef/CSS/LengthTest.php

@@ -28,12 +28,20 @@ class HTMLPurifier_AttrDef_CSS_LengthTest extends HTMLPurifier_AttrDefHarness
     
     function testNonNegative() {
         
-        $this->def = new HTMLPurifier_AttrDef_CSS_Length(true);
+        $this->def = new HTMLPurifier_AttrDef_CSS_Length('0');
         
         $this->assertDef('3cm');
         $this->assertDef('-3mm', false);
         
     }
     
+    function testBounding() {
+        $this->def = new HTMLPurifier_AttrDef_CSS_Length('-1in', '1in');
+        $this->assertDef('1cm');
+        $this->assertDef('-1cm');
+        $this->assertDef('0');
+        $this->assertDef('1em', false);
+    }
+    
 }
 

tests/HTMLPurifier/AttrDef/SwitchTest.php

@@ -0,0 +1,32 @@
+<?php
+
+class HTMLPurifier_AttrDef_SwitchTest extends HTMLPurifier_AttrDefHarness
+{
+    
+    protected $with, $without;
+    
+    function setUp() {
+        parent::setUp();
+        generate_mock_once('HTMLPurifier_AttrDef');
+        $this->with = new HTMLPurifier_AttrDefMock();
+        $this->without = new HTMLPurifier_AttrDefMock();
+        $this->def = new HTMLPurifier_AttrDef_Switch('tag', $this->with, $this->without);
+    }
+    
+    function testWith() {
+        $token = new HTMLPurifier_Token_Start('tag');
+        $this->context->register('CurrentToken', $token);
+        $this->with->expectOnce('validate');
+        $this->with->setReturnValue('validate', 'foo');
+        $this->assertDef('bar', 'foo');
+    }
+    
+    function testWithout() {
+        $token = new HTMLPurifier_Token_Start('other-tag');
+        $this->context->register('CurrentToken', $token);
+        $this->without->expectOnce('validate');
+        $this->without->setReturnValue('validate', 'foo');
+        $this->assertDef('bar', 'foo');
+    }
+    
+}

tests/HTMLPurifier/LengthTest.php

@@ -47,4 +47,25 @@ class HTMLPurifier_LengthTest extends HTMLPurifier_Harness
         $this->assertValidate('3miles', false);
     }
     
+    /**
+     * @param $s1 First string to compare
+     * @param $s2 Second string to compare
+     * @param $expect 0 for $s1 == $s2, 1 for $s1 > $s2 and -1 for $s1 < $s2
+     */
+    protected function assertComparison($s1, $s2, $expect = 0) {
+        $l1 = HTMLPurifier_Length::make($s1);
+        $l2 = HTMLPurifier_Length::make($s2);
+        $r1 = $l1->compareTo($l2);
+        $r2 = $l2->compareTo($l1);
+        $this->assertIdentical($r1 == 0 ? 0 : ($r1 > 0 ? 1 : -1), $expect);
+        $this->assertIdentical($r2 == 0 ? 0 : ($r2 > 0 ? 1 : -1), - $expect);
+    }
+    
+    function testCompareTo() {
+        $this->assertComparison('12in', '12in');
+        $this->assertComparison('12in', '12mm', 1);
+        $this->assertComparison('1px', '1mm', -1);
+        $this->assertComparison(str_repeat('2', 38) . 'in', '100px', 1);
+    }
+    
 }

tests/HTMLPurifier/Strategy/ValidateAttributesTest.php

@@ -177,9 +177,44 @@ class HTMLPurifier_Strategy_ValidateAttributesTest extends
         );
     }
     
-    function testRemoveCSSWidthAndHeightOnImg() {
+    function testKeepAbsoluteCSSWidthAndHeightOnImg() {
         $this->assertResult(
-            '<img src="" alt="" style="width:10px;height:10px;border:1px solid #000;" />',
+            '<img src="" alt="" style="width:10px;height:10px;border:1px solid #000;" />'
+        );
+    }
+    
+    function testRemoveLargeCSSWidthAndHeightOnImg() {
+        $this->assertResult(
+            '<img src="" alt="" style="width:10000000px;height:10000000px;border:1px solid #000;" />',
+            '<img src="" alt="" style="border:1px solid #000;" />'
+        );
+    }
+    
+    function testRemoveLargeCSSWidthAndHeightOnImgWithUserConf() {
+        $this->config->set('CSS', 'MaxImgLength', '1px');
+        $this->assertResult(
+            '<img src="" alt="" style="width:1mm;height:1mm;border:1px solid #000;" />',
+            '<img src="" alt="" style="border:1px solid #000;" />'
+        );
+    }
+    
+    function testKeepLargeCSSWidthAndHeightOnImgWhenToldTo() {
+        $this->config->set('CSS', 'MaxImgLength', null);
+        $this->assertResult(
+            '<img src="" alt="" style="width:10000000px;height:10000000px;border:1px solid #000;" />'
+        );
+    }
+    
+    function testRemoveRelativeCSSWidthAndHeightOnImg() {
+        $this->assertResult(
+            '<img src="" alt="" style="width:10em;height:10em;border:1px solid #000;" />',
+            '<img src="" alt="" style="border:1px solid #000;" />'
+        );
+    }
+    
+    function testRemovePercentCSSWidthAndHeightOnImg() {
+        $this->assertResult(
+            '<img src="" alt="" style="width:100%;height:100%;border:1px solid #000;" />',
             '<img src="" alt="" style="border:1px solid #000;" />'
         );
     }