Fix cross-site scripting (XSS) via HTML or Plain text messages with m…

…alicious content [CVE-2020-35730] Credits to Alex Birnberg <[email protected]>
roundcube · Dec 27, 2020 · 0bceba3 · 0bceba3
1 parent 0efb565
commit 0bceba3
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 10 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -5,6 +5,7 @@ CHANGELOG Roundcube Webmail
 - Fix folder list issue whan special folder is a subfolder (#7647)
 - Fix Elastic's folder subscription toggle in search result (#7653)
 - Fix state of subscription toggle on folders list after changing folder state from the search result (#7653)
+- Security: Fix cross-site scripting (XSS) via HTML or Plain text messages with malicious content [CVE-2020-35730]
 
 RELEASE 1.4.9
 -------------

diff --git a/program/lib/Roundcube/rcube_string_replacer.php b/program/lib/Roundcube/rcube_string_replacer.php
@@ -25,7 +25,7 @@
  */
 class rcube_string_replacer
 {
-    public static $pattern = '/##str_replacement_(\d+)##/';
+    public $pattern;
     public $mailto_pattern;
     public $link_pattern;
     public $linkref_index;
@@ -45,6 +45,10 @@ class rcube_string_replacer
      */
     function __construct($options = array())
     {
+        // Create hard-to-guess replacement string
+        $uniq_ident    = sprintf('%010d%010d', mt_rand(), mt_rand());
+        $this->pattern = '/##' . $uniq_ident . '##(\d+)##/';
+
         // Simplified domain expression for UTF8 characters handling
         // Support unicode/punycode in top-level domain part
         $utf_domain = '[^?&@"\'\\/()<>\s\r\t\n]+\\.?([^\\x00-\\x2f\\x3b-\\x40\\x5b-\\x60\\x7b-\\x7f]{2,}|xn--[a-zA-Z0-9]{2,})';
@@ -55,7 +59,7 @@ function __construct($options = array())
         $link_prefix = "([\w]+:\/\/|{$this->noword}[Ww][Ww][Ww]\.|^[Ww][Ww][Ww]\.)";
 
         $this->options         = $options;
-        $this->linkref_index   = '/\[([^\]#]+)\](:?\s*##str_replacement_(\d+)##)/';
+        $this->linkref_index   = '/\[([^\]#]+)\](:?\s*' . substr($this->pattern, 1, -1) . ')/';
         $this->linkref_pattern = '/\[([^\]#]+)\]/';
         $this->link_pattern    = "/$link_prefix($utf_domain([$url1]*[$url2]+)*)/";
         $this->mailto_pattern  = "/("
@@ -88,7 +92,7 @@ public function add($str)
      */
     public function get_replacement($i)
     {
-        return '##str_replacement_' . $i . '##';
+        return str_replace('(\d+)', $i, substr($this->pattern, 1, -1));
     }
 
     /**
@@ -135,7 +139,7 @@ public function link_callback($matches)
     public function linkref_addindex($matches)
     {
         $key = $matches[1];
-        $this->linkrefs[$key] = $this->urls[$matches[3]];
+        $this->linkrefs[$key] = isset($this->urls[$matches[3]]) ? $this->urls[$matches[3]] : null;
 
         return $this->get_replacement($this->add('['.$key.']')) . $matches[2];
     }
@@ -185,7 +189,7 @@ public function mailto_callback($matches)
      */
     public function replace_callback($matches)
     {
-        return $this->values[$matches[1]];
+        return isset($this->values[$matches[1]]) ? $this->values[$matches[1]] : null;
     }
 
     /**
@@ -216,7 +220,7 @@ public function replace($str)
      */
     public function resolve($str)
     {
-        return preg_replace_callback(self::$pattern, array($this, 'replace_callback'), $str);
+        return preg_replace_callback($this->pattern, array($this, 'replace_callback'), $str);
     }
 
     /**

diff --git a/program/lib/Roundcube/rcube_utils.php b/program/lib/Roundcube/rcube_utils.php
@@ -435,10 +435,10 @@ public static function mod_css_styles($source, $container_id, $allow_remote = fa
 
         // add #container to each tag selector and prefix to id/class identifiers
         if ($container_id || $prefix) {
-            // (?!##str) below is to not match with ##str_replacement_0##
-            // from rcube_string_replacer used above, this is needed for
-            // cases like @media { body { position: fixed; } } (#5811)
-            $regexp   = '/(^\s*|,\s*|\}\s*|\{\s*)((?!##str):?[a-z0-9\._#\*\[][a-z0-9\._:\(\)#=~ \[\]"\|\>\+\$\^-]*)/im';
+            // Exclude rcube_string_replacer pattern matches, this is needed
+            // for cases like @media { body { position: fixed; } } (#5811)
+            $excl     = '(?!' . substr($replacements->pattern, 1, -1) . ')';
+            $regexp   = '/(^\s*|,\s*|\}\s*|\{\s*)(' . $excl . ':?[a-z0-9\._#\*\[][a-z0-9\._:\(\)#=~ \[\]"\|\>\+\$\^-]*)/im';
             $callback = function($matches) use ($container_id, $prefix) {
                 $replace = $matches[2];
 

diff --git a/tests/Framework/Text2Html.php b/tests/Framework/Text2Html.php
@@ -120,4 +120,21 @@ function test_text2html($input, $output, $options)
 
         $this->assertEquals($output, $html);
     }
+
+    /**
+     * Test XSS issue
+     */
+    function test_text2html_xss()
+    {
+        $input = "\n[<script>evil</script>]:##str_replacement_0##\n";
+        $t2h = new rcube_text2html($input);
+
+        $html = $t2h->get_html();
+
+        $expected = "<div class=\"pre\"><br>\n"
+            . "[&lt;script&gt;evil&lt;/script&gt;]:##str_replacement_0##<br>\n"
+            . "</div>";
+
+        $this->assertEquals($expected, $html);
+    }
 }