Fix XSS issue in style attribute handling (#1490227)

Conflicts: CHANGELOG
10 years ago · 04994005e7
parent b134f59eb1
commit 04994005e7
3 changed files with 26 additions and 3 deletions
--- a/1
+++ b/1
@ -5,6 +5,7 @@ CHANGELOG Roundcube Webmail
 - Fix wrong icon for download button in classic skin
 - Fix bug where sent message was saved in Sent folder even if disabled by user (#1490208)
 - Fix checks based on window.ActiveXObject in IE > 10
+- Fix XSS issue in style attribute handling (#1490227)

 RELEASE 1.0.4
 -------------
--- a/program/lib/Roundcube/rcube_washtml.php
+++ b/program/lib/Roundcube/rcube_washtml.php
@ -243,8 +243,8 @@ class rcube_washtml
                $t .= ' ' . $key . '="' . htmlspecialchars($value, ENT_QUOTES) . '"';
            }
            else if ($key == 'style' && ($style = $this->wash_style($value))) {
-                $quot = strpos($style, '"') !== false ? "'" : '"';
-                $t .= ' style=' . $quot . $style . $quot;
+                // replace double quotes to prevent syntax error and XSS issues (#1490227)
+                $t .= ' style="' . str_replace('"', '&quot;', $style) . '"';
            }
            else if ($key == 'background' || ($key == 'src' && strtolower($node->tagName) == 'img')) { //check tagName anyway
                if (($src = $this->config['cid_map'][$value])
--- a/tests/Framework/Washtml.php
+++ b/tests/Framework/Washtml.php
@ -159,7 +159,7 @@ class Framework_Washtml extends PHPUnit_Framework_TestCase
        $washer = new rcube_washtml;
        $washed = $washer->wash($html);

-        $this->assertRegExp('|style=\'font-family: "新細明體","serif"; color: red\'|', $washed, "Unicode chars in style attribute - quoted (#1489697)");
+        $this->assertRegExp('|style="font-family: \&quot;新細明體\&quot;,\&quot;serif\&quot;; color: red"|', $washed, "Unicode chars in style attribute - quoted (#1489697)");

        $html = "<html><meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\" />
            <body><span style='font-family:新細明體;color:red'>test</span></body></html>";
@ -183,4 +183,26 @@ class Framework_Washtml extends PHPUnit_Framework_TestCase
        $this->assertRegExp('|line-height: 1;|', $washed, "Untouched line-height (#1489917)");
        $this->assertRegExp('|; height: 10px|', $washed, "Fixed height units");
    }
+
+    /**
+     * Test invalid style cleanup - XSS prevention (#1490227)
+     */
+    function test_style_wash_xss()
+    {
+        $html = "<img style=aaa:'\"/onerror=alert(1)//'>";
+        $exp  = "<img style=\"aaa: '&quot;/onerror=alert(1)//'\" />";
+
+        $washer = new rcube_washtml;
+        $washed = $washer->wash($html);
+
+        $this->assertTrue(strpos($washed, $exp) !== false, "Style quotes XSS issue (#1490227)");
+
+        $html = "<img style=aaa:'&quot;/onerror=alert(1)//'>";
+        $exp  = "<img style=\"aaa: '&quot;/onerror=alert(1)//'\" />";
+
+        $washer = new rcube_washtml;
+        $washed = $washer->wash($html);
+
+        $this->assertTrue(strpos($washed, $exp) !== false, "Style quotes XSS issue (#1490227)");
+    }
 }