Skip to content

Commit

Permalink
Fix information leak (access to remote content) via insufficient CSS …
Browse files Browse the repository at this point in the history 
…filtering [CVE-2024-42010]

Credits to Oskar Zeino-Mahmalat (Sonar) https://www.sonarsource.com
  • Loading branch information
@alecpl
alecpl committed Aug 4, 2024
1 parent 89c8fe9 commit 602d0f5
Show file tree
Hide file tree
Showing 5 changed files with 167 additions and 125 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
157 changes: 120 additions & 37 deletions program/lib/Roundcube/rcube_utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
*
Expand Down
80 changes: 8 additions & 72 deletions program/lib/Roundcube/rcube_washtml.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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), ';');
}

/**
Expand Down Expand Up @@ -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;
}
}
52 changes: 37 additions & 15 deletions tests/Framework/Utils.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(&#039;xss3&#039;) )", '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(&#039;xss&#039;) )", '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(&#039;xss3&#039;) ); }', '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');
Expand Down Expand Up @@ -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(&#039;URL!&#039;) ) }";
$style = "body { background:url(alert(&#039;URL!&#039;)); }";
$mod = rcube_utils::mod_css_styles($style, 'rcmbody', true);
$this->assertSame("#rcmbody { background: /* evil! */; }", $mod);
$this->assertSame("#rcmbody {}", $mod);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion tests/Framework/Washtml.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}

/**
Expand Down

0 comments on commit 602d0f5

Please sign in to comment.