From 0716d499bc591f43a7eabc19da672bbbee41b976 Mon Sep 17 00:00:00 2001 From: Aleksander Machniak Date: Sat, 5 May 2018 17:12:18 +0200 Subject: [PATCH] Fix bug where some escape sequences in html styles could bypass security checks --- CHANGELOG | 3 ++- program/lib/Roundcube/rcube_utils.php | 3 ++- program/lib/Roundcube/rcube_washtml.php | 5 ++++- tests/Framework/Utils.php | 8 +++++++- tests/Framework/Washtml.php | 1 + tests/MailFunc.php | 2 +- 6 files changed, 17 insertions(+), 5 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 92ee97ca6..0f40a12b0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -82,7 +82,8 @@ CHANGELOG Roundcube Webmail - Fix so links over images are not removed in plain text signatures converted from HTML (#4473) - Fix various issues when downloading files with names containing non-ascii chars, use RFC 2231 (#5772) - Fix PHP Warning: Use of undefined constant IDNA_DEFAULT on systems without php-intl (#6244) -- Fix bug where some parts of quota information could be ignored (#6280) +- Fix bug where some parts of quota information could have been ignored (#6280) +- Fix bug where some escape sequences in html styles could bypass security checks RELEASE 1.3.6 ------------- diff --git a/program/lib/Roundcube/rcube_utils.php b/program/lib/Roundcube/rcube_utils.php index 5fcb4476e..f010ff117 100644 --- a/program/lib/Roundcube/rcube_utils.php +++ b/program/lib/Roundcube/rcube_utils.php @@ -518,7 +518,8 @@ class rcube_utils $out = html_entity_decode(html_entity_decode($content)); $out = trim(preg_replace('/(^$)/', '', trim($out))); - $out = preg_replace_callback('/\\\([0-9a-f]{2,4})\s*/i', $callback, $out); + $out = preg_replace_callback('/\\\([0-9a-f]{2,6})\s*/i', $callback, $out); + $out = preg_replace('/\\\([^0-9a-f])/i', '\\1', $out); $out = preg_replace('#/\*.*\*/#Ums', '', $out); $out = strip_tags($out); diff --git a/program/lib/Roundcube/rcube_washtml.php b/program/lib/Roundcube/rcube_washtml.php index d8b12f53e..fff1f44e8 100644 --- a/program/lib/Roundcube/rcube_washtml.php +++ b/program/lib/Roundcube/rcube_washtml.php @@ -246,8 +246,11 @@ class rcube_washtml // 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 (explode(';', $style) as $declaration) { - if (preg_match('/^\s*([a-z\-]+)\s*:\s*(.*)\s*$/i', $declaration, $match)) { + if (preg_match('/^\s*([a-z\\\-]+)\s*:\s*(.*)\s*$/i', $declaration, $match)) { $cssid = $match[1]; $str = $match[2]; $value = ''; diff --git a/tests/Framework/Utils.php b/tests/Framework/Utils.php index 205874caa..9c3857e1f 100644 --- a/tests/Framework/Utils.php +++ b/tests/Framework/Utils.php @@ -203,12 +203,15 @@ class Framework_Utils extends PHPUnit_Framework_TestCase $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("background:\\0075\\0072\\006c( javascript:alert('xss') )", 'rcmbody'); + $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("background: \\75 \\72 \\6C ('/images/img.png')", 'rcmbody'); $this->assertEquals("/* evil! */", $mod, "Don't allow encoding quirks (3)"); + $mod = rcube_utils::mod_css_styles("background: u\\r\\l('/images/img.png')", 'rcmbody'); + $this->assertEquals("/* evil! */", $mod, "Don't allow encoding quirks (4)"); + // position: fixed (#5264) $mod = rcube_utils::mod_css_styles(".test { position: fixed; }", 'rcmbody'); $this->assertEquals("#rcmbody .test { position: absolute; }", $mod, "Replace position:fixed with position:absolute (0)"); @@ -265,6 +268,9 @@ class Framework_Utils extends PHPUnit_Framework_TestCase $mod = rcube_utils::xss_entity_decode('#foo:after{content:"\003Cimg/src=x onerror=alert(2)>";}'); $this->assertNotContains('assertContains("url(", $mod, "Escape sequences resolving"); + // #5747 $mod = rcube_utils::xss_entity_decode(''); $this->assertContains('#foo', $mod, "Strip HTML comments from content, but not the content"); diff --git a/tests/Framework/Washtml.php b/tests/Framework/Washtml.php index ed523d23a..397eb718b 100644 --- a/tests/Framework/Washtml.php +++ b/tests/Framework/Washtml.php @@ -377,6 +377,7 @@ class Framework_Washtml extends PHPUnit_Framework_TestCase array("", false), array("", true), array("", false), + array('

', true), ); foreach ($html as $item) { diff --git a/tests/MailFunc.php b/tests/MailFunc.php index 8f951863a..2689001e2 100644 --- a/tests/MailFunc.php +++ b/tests/MailFunc.php @@ -216,7 +216,7 @@ class MailFunc extends PHPUnit_Framework_TestCase $body = rcmail_print_body($html, $this->get_html_part(), array('safe' => false, 'plain' => false)); $this->assertNotContains('onerror=alert(1)//">test', $body); - $this->assertContains('assertContains('