From 602d0f566eb39b6dcb739ad78323ec434a3b92ce Mon Sep 17 00:00:00 2001 From: Aleksander Machniak Date: Sat, 3 Aug 2024 09:24:19 +0200 Subject: [PATCH] Fix information leak (access to remote content) via insufficient CSS filtering [CVE-2024-42010] Credits to Oskar Zeino-Mahmalat (Sonar) https://www.sonarsource.com --- CHANGELOG.md | 1 + program/lib/Roundcube/rcube_utils.php | 157 ++++++++++++++++++------ program/lib/Roundcube/rcube_washtml.php | 80 ++---------- tests/Framework/Utils.php | 52 +++++--- tests/Framework/Washtml.php | 2 +- 5 files changed, 167 insertions(+), 125 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c9fd474418..b34a0a370be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - Fix bug where imap_conn_option's 'socket' was ignored (#9566) - Fix XSS vulnerability in post-processing of sanitized HTML content [CVE-2024-42009] - Fix XSS vulnerability in serving of attachments other than HTML or SVG [CVE-2024-42008] +- Fix information leak (access to remote content) via insufficient CSS filtering [CVE-2024-42010] ## Release 1.6.7 diff --git a/program/lib/Roundcube/rcube_utils.php b/program/lib/Roundcube/rcube_utils.php index ba058eee3b0..b9db11f0a3a 100644 --- a/program/lib/Roundcube/rcube_utils.php +++ b/program/lib/Roundcube/rcube_utils.php @@ -416,23 +416,41 @@ public static function html_identifier($str, $encode = false) */ public static function mod_css_styles($source, $container_id, $allow_remote = false, $prefix = '') { - $last_pos = 0; - $replacements = new rcube_string_replacer; - - // ignore the whole block if evil styles are detected $source = self::xss_entity_decode($source); - $stripped = preg_replace('/[^a-z\(:;]/i', '', $source); - $evilexpr = 'expression|behavior|javascript:|import[^a]' . (!$allow_remote ? '|url\((?!data:image)' : ''); - if (preg_match("/$evilexpr/i", $stripped)) { + // No @import allowed + // TODO: We should just remove it, not invalidate the whole content + if (stripos($source, '@import') !== false) { return '/* evil! */'; } - $strict_url_regexp = '!url\s*\(\s*["\']?(https?:)//[a-z0-9/._+-]+["\']?\s*\)!Uims'; + // Incomplete style expression + if (strpos($source, '{') === false) { + return '/* invalid! */'; + } + + // 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. + if (preg_match('/\\\[0-9a-fA-F]{2}/', $source)) { + return '/* evil! */'; + } // remove html comments $source = preg_replace('/(^\s*<\!--)|(-->\s*$)/m', '', $source); + $url_callback = static function ($url) use ($allow_remote) { + if (strpos($url, 'data:image') === 0) { + return $url; + } + if ($allow_remote && preg_match('|^https?://[a-z0-9/._+-]+$|i', $url)) { + return $url; + } + }; + + $last_pos = 0; + $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); @@ -441,36 +459,9 @@ public static function mod_css_styles($source, $container_id, $allow_remote = fa } $length = $pos2 - $pos - 1; $styles = substr($source, $pos+1, $length); - $output = ''; - - // check every css rule in the style block... - foreach (self::parse_css_block($styles) as $rule) { - // Remove 'page' attributes (#7604) - if ($rule[0] == 'page') { - continue; - } - - // Convert position:fixed to position:absolute (#5264) - if ($rule[0] == 'position' && strcasecmp($rule[1], 'fixed') === 0) { - $rule[1] = 'absolute'; - } - else if ($allow_remote) { - $stripped = preg_replace('/[^a-z\(:;]/i', '', $rule[1]); - - // allow data:image and strict url() values only - if ( - stripos($stripped, 'url(') !== false - && stripos($stripped, 'url(data:image') === false - && !preg_match($strict_url_regexp, $rule[1]) - ) { - $rule[1] = '/* evil! */'; - } - } - - $output .= sprintf(" %s: %s;", $rule[0] , $rule[1]); - } + $styles = self::sanitize_css_block($styles, $url_callback); - $key = $replacements->add($output . ' '); + $key = $replacements->add(strlen($styles) ? " {$styles} " : ''); $repl = $replacements->get_replacement($key); $source = substr_replace($source, $repl, $pos+1, $length); $last_pos = $pos2 - ($length - strlen($repl)); @@ -518,6 +509,63 @@ public static function mod_css_styles($source, $container_id, $allow_remote = fa return $source; } + /** + * Parse and sanitize single CSS block + * + * @param string $styles CSS styles block + * @param ?callable $url_callback URL validator callback + * + * @return string + */ + public static function sanitize_css_block($styles, $url_callback = null) + { + $output = []; + + // check every css rule in the style block... + foreach (self::parse_css_block($styles) as $rule) { + $property = $rule[0]; + $value = $rule[1]; + + if ($property == 'page') { + // Remove 'page' attributes (#7604) + continue; + } elseif ($property == 'position' && strcasecmp($value, 'fixed') === 0) { + // Convert position:fixed to position:absolute (#5264) + $value = 'absolute'; + } elseif (preg_match('/expression|image-set/i', $value)) { + continue; + } else { + $value = ''; + foreach (self::explode_css_property_block($rule[1]) as $val) { + if ($url_callback && preg_match('/^url\s*\(/i', $val)) { + if (preg_match('/^url\s*\(\s*[\'"]?([^\'"\)]*)[\'"]?\s*\)/iu', $val, $match)) { + if ($url = $url_callback($match[1])) { + $value .= ' url(' . $url . ')'; + } + } + } else { + // whitelist ? + $value .= ' ' . $val; + + // #1488535: Fix size units, so width:800 would be changed to width:800px + if ($val + && preg_match('/^(left|right|top|bottom|width|height)/i', $property) + && preg_match('/^[0-9]+$/', $val) + ) { + $value .= 'px'; + } + } + } + } + + if (strlen($value)) { + $output[] = $property . ': ' . trim($value); + } + } + + return count($output) > 0 ? implode('; ', $output) . ';' : ''; + } + /** * Explode css style. Property names will be lower-cased and trimmed. * Values will be trimmed. Invalid entries will be skipped. @@ -590,6 +638,41 @@ public static function parse_css_block($style) return $result; } + /** + * Explode css style value + * + * @param string $style CSS style + * + * @return array List of CSS values + */ + public static function explode_css_property_block($style) + { + $style = preg_replace('/\s+/', ' ', $style); + $result = []; + $strlen = strlen($style); + $q = false; + + // explode value + for ($p = $i = 0; $i < $strlen; $i++) { + if (($style[$i] == '"' || $style[$i] == "'") && ($i == 0 || $style[$i - 1] != '\\')) { + if ($q == $style[$i]) { + $q = false; + } elseif (!$q) { + $q = $style[$i]; + } + } + + if (!$q && $style[$i] == ' ' && ($i == 0 || !preg_match('/[,\(]/', $style[$i - 1]))) { + $result[] = substr($style, $p, $i - $p); + $p = $i + 1; + } + } + + $result[] = (string) substr($style, $p); + + return $result; + } + /** * Generate CSS classes from mimetype and filename extension * diff --git a/program/lib/Roundcube/rcube_washtml.php b/program/lib/Roundcube/rcube_washtml.php index bf488368654..e682b29142d 100644 --- a/program/lib/Roundcube/rcube_washtml.php +++ b/program/lib/Roundcube/rcube_washtml.php @@ -257,48 +257,19 @@ private function wash_style($style) { $result = []; - // Remove unwanted white-space characters so regular expressions below work better - $style = preg_replace('/[\n\r\s\t]+/', ' ', $style); - // Decode insecure character sequences $style = rcube_utils::xss_entity_decode($style); - foreach (rcube_utils::parse_css_block($style) as $rule) { - $cssid = $rule[0]; - $value = ''; - - foreach ($this->explode_style($rule[1]) as $val) { - if (preg_match('/^url\(/i', $val)) { - if (preg_match('/^url\(\s*[\'"]?([^\'"\)]*)[\'"]?\s*\)/iu', $val, $match)) { - if ($url = $this->wash_uri($match[1])) { - $value .= ' url(' . htmlspecialchars($url, ENT_QUOTES, $this->config['charset']) . ')'; - } - } - } - else if (!preg_match('/^(behavior|expression)/i', $val)) { - // Set position:fixed to position:absolute for security (#5264) - if (!strcasecmp($cssid, 'position') && !strcasecmp($val, 'fixed')) { - $val = 'absolute'; - } - - // whitelist ? - $value .= ' ' . $val; - - // #1488535: Fix size units, so width:800 would be changed to width:800px - if (preg_match('/^(left|right|top|bottom|width|height)/i', $cssid) - && preg_match('/^[0-9]+$/', $val) - ) { - $value .= 'px'; - } - } - } + // Remove unwanted white-space characters + $style = preg_replace('/[\n\r\t]+/', ' ', $style); - if (isset($value[0])) { - $result[] = $cssid . ': ' . trim($value); + $uri_callback = function ($uri) { + if ($uri = $this->wash_uri($uri)) { + return htmlspecialchars($uri, \ENT_QUOTES, $this->config['charset']); } - } - - return implode('; ', $result); + }; + + return rtrim(rcube_utils::sanitize_css_block($style, $uri_callback), ';'); } /** @@ -990,39 +961,4 @@ protected function fix_html5($html) return $html; } - - /** - * Explode css style value - * - * @param string $style CSS style - * - * @return array List of CSS rules - */ - protected function explode_style($style) - { - $result = []; - $strlen = strlen($style); - $q = false; - - // explode value - for ($p = $i = 0; $i < $strlen; $i++) { - if (($style[$i] == "\"" || $style[$i] == "'") && ($i == 0 || $style[$i-1] != "\\")) { - if ($q == $style[$i]) { - $q = false; - } - else if (!$q) { - $q = $style[$i]; - } - } - - if (!$q && $style[$i] == ' ' && ($i == 0 || !preg_match('/[,\(]/', $style[$i-1]))) { - $result[] = substr($style, $p, $i - $p); - $p = $i + 1; - } - } - - $result[] = (string) substr($style, $p); - - return $result; - } } diff --git a/tests/Framework/Utils.php b/tests/Framework/Utils.php index 7ea6ffa0bbf..4cd2750771a 100644 --- a/tests/Framework/Utils.php +++ b/tests/Framework/Utils.php @@ -217,26 +217,48 @@ function test_mod_css_styles() */ function test_mod_css_styles_xss() { - $mod = rcube_utils::mod_css_styles("body.main2cols { background-image: url('../images/leftcol.png'); }", 'rcmbody'); - $this->assertEquals("/* evil! */", $mod, "No url() values allowed"); + $mod = \rcube_utils::mod_css_styles('font-size: 1em;', 'rcmbody'); + $this->assertSame('/* invalid! */', $mod); $mod = rcube_utils::mod_css_styles("@import url('http://localhost/somestuff/css/master.css');", 'rcmbody'); - $this->assertEquals("/* evil! */", $mod, "No import statements"); + $this->assertSame('/* evil! */', $mod); - $mod = rcube_utils::mod_css_styles("left:expression(document.body.offsetWidth-20)", 'rcmbody'); - $this->assertEquals("/* evil! */", $mod, "No expression properties"); + $mod = \rcube_utils::mod_css_styles("@\\69mport url('http://localhost/somestuff/css/master.css');", 'rcmbody'); + $this->assertSame('/* evil! */', $mod); - $mod = rcube_utils::mod_css_styles("left:exp/* */ression( alert('xss3') )", 'rcmbody'); - $this->assertEquals("/* evil! */", $mod, "Don't allow encoding quirks"); + $mod = \rcube_utils::mod_css_styles("@\\5C 69mport url('http://localhost/somestuff/css/master.css'); a { color: red; }", ''); + $this->assertSame('/* evil! */', $mod); - $mod = rcube_utils::mod_css_styles("background:\\0075\\0072\\00006c( javascript:alert('xss') )", 'rcmbody'); - $this->assertEquals("/* evil! */", $mod, "Don't allow encoding quirks (2)"); + $mod = \rcube_utils::mod_css_styles("body.main2cols { background-image: url('../images/leftcol.png'); }", 'rcmbody'); + $this->assertSame('#rcmbody.main2cols {}', $mod); - $mod = rcube_utils::mod_css_styles("background: \\75 \\72 \\6C ('/images/img.png')", 'rcmbody'); - $this->assertEquals("/* evil! */", $mod, "Don't allow encoding quirks (3)"); + $mod = \rcube_utils::mod_css_styles('p { left:expression(document.body.offsetWidth-20); }', 'rcmbody'); + $this->assertSame('#rcmbody p {}', $mod); - $mod = rcube_utils::mod_css_styles("background: u\\r\\l('/images/img.png')", 'rcmbody'); - $this->assertEquals("/* evil! */", $mod, "Don't allow encoding quirks (4)"); + $mod = \rcube_utils::mod_css_styles('p { left:exp/* */ression( alert('xss3') ); }', 'rcmbody'); + $this->assertSame('#rcmbody p {}', $mod); + + $mod = \rcube_utils::mod_css_styles("p { background:\\0075\\0072\\00006c('//evil.com/test'); }", 'rcmbody'); + $this->assertSame('#rcmbody p {}', $mod); + + $mod = \rcube_utils::mod_css_styles("p { background: \\75 \\72 \\6C ('/images/img.png'); }", 'rcmbody'); + $this->assertSame('#rcmbody p {}', $mod); + + $mod = \rcube_utils::mod_css_styles("p { background: u\\r\\l('/images/img2.png'); }", 'rcmbody'); + $this->assertSame('#rcmbody p {}', $mod); + + $mod = \rcube_utils::mod_css_styles("p { background: url('//żą.ść?data:image&leak') }", 'rcmbody'); + $this->assertSame('#rcmbody p {}', $mod); + + $mod = \rcube_utils::mod_css_styles("p { background: url('//data:image&leak'); }", 'rcmbody'); + $this->assertSame('#rcmbody p {}', $mod); + + // Note: This looks to me like a bug in browsers, for now we don't allow image-set at all + $mod = \rcube_utils::mod_css_styles("p { background: image-set('//evil.com/img.png' 1x); }", 'rcmbody'); + $this->assertSame('#rcmbody p {}', $mod); + + $mod = \rcube_utils::mod_css_styles('p { background: none !important; }', 'rcmbody'); + $this->assertSame('#rcmbody p { background: none !important; }', $mod); // position: fixed (#5264) $mod = rcube_utils::mod_css_styles(".test { position: fixed; }", 'rcmbody'); @@ -278,9 +300,9 @@ function test_mod_css_styles_xss() $mod = rcube_utils::mod_css_styles($style, 'rcmbody', true); $this->assertSame("#rcmbody { color: red; }", $mod); - $style = "body { background:url(alert('URL!') ) }"; + $style = "body { background:url(alert('URL!')); }"; $mod = rcube_utils::mod_css_styles($style, 'rcmbody', true); - $this->assertSame("#rcmbody { background: /* evil! */; }", $mod); + $this->assertSame("#rcmbody {}", $mod); } /** diff --git a/tests/Framework/Washtml.php b/tests/Framework/Washtml.php index 6bfbc14c84c..4fdae1adec7 100644 --- a/tests/Framework/Washtml.php +++ b/tests/Framework/Washtml.php @@ -289,7 +289,7 @@ function test_style_wash() $washer = new rcube_washtml; $washed = $washer->wash($html); - $this->assertTrue(strpos($washed, $expected) !== false, "White-space and new-line characters handling"); + $this->assertSame($this->cleanupResult($washed), $expected, 'White-space and new-line characters handling'); } /**