Fix CSS injection vulnerability reported by CERT Polska

CHANGELOG.md

@@ -3,7 +3,8 @@
 ## Unreleased
 
 - Managesieve: Fix handling of string-list format values for date tests in Out of Office (#10075)
-- Fix remote image blocking bypass via SVG content reported by nullcathedral.
+- Fix remote image blocking bypass via SVG content reported by nullcathedral
+- Fix CSS injection vulnerability reported by CERT Polska
 
 ## Release 1.6.12
 

program/lib/Roundcube/rcube_utils.php

@@ -445,6 +445,10 @@ class rcube_utils
             return '/* invalid! */';
         }
 
+        // remove html and css comments
+        $source = preg_replace('/(^\s*<\!--)|(-->\s*$)/m', '', $source);
+        $source = self::remove_css_comments($source);
+
         // To prevent from a double-escaping tricks we consider a script with
         // any escape sequences (after de-escaping them above) an evil script.
         // This probably catches many valid scripts, but we\'re on the safe side.
@@ -452,8 +456,10 @@ class rcube_utils
             return '/* evil! */';
         }
 
-        // remove html comments
-        $source = preg_replace('/(^\s*<\!--)|(-->\s*$)/m', '', $source);
+        // If after removing comments there are still comments it's most likely a hack
+        if (strpos('/*', $source) !== false || strpos('<!--', $source) !== false) {
+            return '/* evil! */';
+        }
 
         $url_callback = static function ($url) use ($allow_remote) {
             if (strpos($url, 'data:image') === 0) {
@@ -468,8 +474,14 @@ class rcube_utils
         $replacements = new rcube_string_replacer();
 
         // cut out all contents between { and }
-        while (($pos = strpos($source, '{', $last_pos)) && ($pos2 = strpos($source, '}', $pos))) {
-            $nested = strpos($source, '{', $pos+1);
+        while (($pos = strpos($source, '{', $last_pos)) && ($pos2 = strpos($source, '}', $pos) ?: (strlen($source) - 1))) {
+            // In case there was no closing brace add one
+            if ($source[$pos2] != '}') {
+                $pos2++;
+                $source .= '}';
+            }
+
+            $nested = strpos($source, '{', $pos + 1);
             if ($nested && $nested < $pos2) { // when dealing with nested blocks (e.g. @media), take the inner one
                 $pos = $nested;
             }
@@ -595,19 +607,8 @@ class rcube_utils
      */
     public static function parse_css_block($style)
     {
-        $pos = 0;
-
-        // first remove comments
-        while (($pos = strpos($style, '/*', $pos)) !== false) {
-            $end = strpos($style, '*/', $pos+2);
-
-            if ($end === false) {
-                $style = substr($style, 0, $pos);
-            }
-            else {
-                $style = substr_replace($style, '', $pos, $end - $pos + 2);
-            }
-        }
+        // Remove comments
+        $style = self::remove_css_comments($style);
 
         // Replace new lines with spaces
         $style = preg_replace('/[\r\n]+/', ' ', $style);
@@ -659,6 +660,30 @@ class rcube_utils
         return $result;
     }
 
+    /**
+     * Remove CSS comments from styles.
+     *
+     * @param string $style CSS style
+     *
+     * @return string CSS style
+     */
+    public static function remove_css_comments($style)
+    {
+        $pos = 0;
+
+        while (($pos = strpos($style, '/*', $pos)) !== false) {
+            $end = strpos($style, '*/', $pos + 2);
+
+            if ($end === false) {
+                $style = substr($style, 0, $pos);
+            } else {
+                $style = substr_replace($style, '', $pos, $end - $pos + 2);
+            }
+        }
+
+        return $style;
+    }
+
     /**
      * Explode css style value
      *

tests/Framework/Utils.php

@@ -276,6 +276,10 @@ class Framework_Utils extends PHPUnit\Framework\TestCase
         $mod = rcube_utils::mod_css_styles(".test { position\n: fixed; top: 0; }", 'rcmbody');
         $this->assertEquals("#rcmbody .test { position: absolute; top: 0; }", $mod, "Replace position:fixed with position:absolute (5)");
 
+        // missing closing brace
+        $mod = \rcube_utils::mod_css_styles('.test { position: fixed; top: 0;', 'rcmbody');
+        $this->assertSame('#rcmbody .test { position: absolute; top: 0; }', $mod, 'Replace position:fixed with position:absolute (6)');
+
         // allow data URIs with images (#5580)
         $mod = rcube_utils::mod_css_styles("body { background-image: url(data:image/png;base64,123); }", 'rcmbody');
         $this->assertStringContainsString("#rcmbody { background-image: url(data:image/png;base64,123);", $mod, "Data URIs in url() allowed [1]");
@@ -300,9 +304,15 @@ class Framework_Utils extends PHPUnit\Framework\TestCase
         $mod = rcube_utils::mod_css_styles($style, 'rcmbody', true);
         $this->assertSame("#rcmbody { color: red; }", $mod);
 
-        $style = "body { background:url(alert('URL!')); }";
-        $mod = rcube_utils::mod_css_styles($style, 'rcmbody', true);
-        $this->assertSame("#rcmbody {}", $mod);
+        $style = 'body { background:url(alert('URL!')); }';
+        $mod = \rcube_utils::mod_css_styles($style, 'rcmbody', true);
+        $this->assertSame('#rcmbody {}', $mod);
+
+        // CSS comments and nesting
+        $mod = \rcube_utils::mod_css_styles('/* b { content: "*/* {background-color: silver;}', 'rcmbody', true);
+        $this->assertSame('#rcmbody * { background-color: silver; }', $mod);
+        $mod = \rcube_utils::mod_css_styles('//* test */* {background-color: silver;}', 'rcmbody', true);
+        $this->assertSame('/* evil! */', $mod);
     }
 
     /**