Fix css styles leak from replied/forwarded message to the rest of the composed text (#6831)

Generally do the same with styles what we do on message preview.

This also fixes small bugs in handling styles:
- fix so <style> tag on the start of the HTML content is not ignored
- fix so body's background/bgcolor attributes are applied to the container (regression)

CHANGELOG

@@ -45,6 +45,7 @@ CHANGELOG Roundcube Webmail
 - Enigma: Fix bug where revoked users/keys were not greyed out in key info
 - Enigma: Fix error message when trying to encrypt with a revoked key (#6607)
 - Enigma: Fix "decryption oracle" bug [CVE-2019-10740] (#6638)
+- Fix css styles leak from replied/forwarded message to the rest of the composed text (#6831)
 - Fix invalid path to "add contact" icon when using assets_path setting
 - Fix invalid path to blocked.gif when using assets_path setting (#6752)
 - Fix so advanced search dialog is not automatically displayed on searchonly addressbooks (#6679)

program/steps/mail/compose.inc

@@ -335,10 +335,8 @@ function rcmail_process_compose_params(&$COMPOSE)
     // clean HTML message body which can be submitted by URL
     if (!empty($COMPOSE['param']['body'])) {
         if ($COMPOSE['param']['html'] = strpos($COMPOSE['param']['body'], '<') !== false) {
-            $wash_params = array('safe' => false, 'inline_html' => true);
-            $COMPOSE['param']['body'] = rcmail_wash_html($COMPOSE['param']['body'], $wash_params, array());
-            $COMPOSE['param']['body'] = preg_replace('/<!--[^>\n]+>/', '', $COMPOSE['param']['body']);
-            $COMPOSE['param']['body'] = preg_replace('/<\/?body>/', '', $COMPOSE['param']['body']);
+            $wash_params              = array('safe' => false, 'inline_html' => true);
+            $COMPOSE['param']['body'] = rcmail_prepare_html_body($COMPOSE['param']['body'], $wash_params);
         }
     }
 
@@ -497,7 +495,9 @@ function rcmail_spellchecker_init()
 
 function rcmail_prepare_message_body()
 {
-    global $RCMAIL, $MESSAGE, $COMPOSE, $HTML_MODE;
+    global $RCMAIL, $MESSAGE, $COMPOSE, $HTML_MODE, $CID_MAP;
+
+    $CID_MAP = array();
 
     // use posted message body
     if (!empty($_POST['_message'])) {
@@ -520,6 +520,23 @@ function rcmail_prepare_message_body()
         $isHtml   = rcmail_compose_editor_mode();
         $messages = array();
 
+        // save inline images to files (before HTML body washing)
+        if ($COMPOSE['mode'] == rcmail_sendmail::MODE_REPLY) {
+            rcmail_write_inline_attachments($MESSAGE);
+        }
+        // save attachments to files (before HTML body washing)
+        else {
+            rcmail_write_compose_attachments($MESSAGE, $isHtml);
+        }
+
+        // set is_safe flag (before HTML body washing)
+        if ($COMPOSE['mode'] == rcmail_sendmail::MODE_DRAFT) {
+            $MESSAGE->is_safe = true;
+        }
+        else {
+            rcmail_check_safe($MESSAGE);
+        }
+
         if (!empty($MESSAGE->parts)) {
             // collect IDs of message/rfc822 parts
             foreach ($MESSAGE->mime_parts() as $part) {
@@ -579,7 +596,8 @@ function rcmail_prepare_message_body()
             $body = rcmail_create_draft_body($body, $isHtml);
         }
     }
-    else { // new message
+    // new message
+    else {
         $isHtml = rcmail_compose_editor_mode();
     }
 
@@ -635,6 +653,7 @@ function rcmail_compose_part_body($part, $isHtml = false)
 
     if ($isHtml) {
         if ($part->ctype_secondary == 'html') {
+            $body = rcmail_prepare_html_body($body);
         }
         else if ($part->ctype_secondary == 'enriched') {
             $body = rcube_enriched::to_html($body);
@@ -751,13 +770,6 @@ function rcmail_create_reply_body($body, $bodyIsHtml)
         }
     }
     else {
-        // save inline images to files
-        $cid_map = rcmail_write_inline_attachments($MESSAGE);
-        // set is_safe flag (we need this for html body washing)
-        rcmail_check_safe($MESSAGE);
-        // clean up html tags
-        $body = rcmail_wash_html($body, array('safe' => $MESSAGE->is_safe), $cid_map);
-
         $suffix = '';
 
         if ($reply_indent) {
@@ -796,25 +808,9 @@ function rcmail_get_reply_header($message)
 
 function rcmail_create_forward_body($body, $bodyIsHtml)
 {
-    global $RCMAIL, $MESSAGE, $COMPOSE;
+    global $MESSAGE;
 
-    // add attachments
-    if (!isset($COMPOSE['forward_attachments']) && is_array($MESSAGE->mime_parts)) {
-        $cid_map = rcmail_write_compose_attachments($MESSAGE, $bodyIsHtml);
-    }
-
-    if (!$bodyIsHtml) {
-        $body = trim($body, "\r\n");
-    }
-    else {
-        // set is_safe flag (we need this for html body washing)
-        rcmail_check_safe($MESSAGE);
-
-        // clean up html tags
-        $body = rcmail_wash_html($body, array('safe' => $MESSAGE->is_safe), $cid_map);
-    }
-
-    return rcmail_get_forward_header($MESSAGE, $bodyIsHtml) . $body;
+    return rcmail_get_forward_header($MESSAGE, $bodyIsHtml) . trim($body, "\r\n");
 }
 
 function rcmail_get_forward_header($message, $bodyIsHtml = false, $extended = true)
@@ -871,44 +867,48 @@ function rcmail_get_forward_header($message, $bodyIsHtml = false, $extended = tr
 
 function rcmail_create_draft_body($body, $bodyIsHtml)
 {
-    global $MESSAGE, $COMPOSE;
-
-    // add attachments
-    // count($MESSAGE->mime_parts) can be 1 - e.g. attachment, but no text!
-    if (empty($COMPOSE['forward_attachments'])
-        && is_array($MESSAGE->mime_parts)
-        && count($MESSAGE->mime_parts) > 0
-    ) {
-        $cid_map = rcmail_write_compose_attachments($MESSAGE, $bodyIsHtml);
-    }
-
-    // clean up HTML tags - XSS prevention (#1489251)
-    if ($bodyIsHtml) {
-        $body = rcmail_wash_html($body, array('safe' => 1), $cid_map);
-
-        // cleanup
-        $body = preg_replace(array(
-                // remove comments (produced by washtml)
-                '/<!--[^>]+-->/',
-                // remove <body> tags
-                '/<body([^>]*)>/i',
-                '/<\/body>/i',
-                // convert TinyMCE's empty-line sequence (#1490463)
-                '/<p>\xC2\xA0<\/p>/',
-            ),
-            array(
-                '',
-                '',
-                '',
-                '<p><br /></p>',
-            ),
-            $body
-        );
-
-        // replace cid with href in inline images links
-        if (!empty($cid_map)) {
-            $body = str_replace(array_keys($cid_map), array_values($cid_map), $body);
-        }
+    // Return the draft body as-is
+    return $body;
+}
+
+// Clean up HTML content of Draft/Reply/Forward (part of the message)
+function rcmail_prepare_html_body($body, $wash_params = array())
+{
+    global $CID_MAP, $MESSAGE, $COMPOSE;
+    static $part_no;
+
+    // Set attributes of the part container
+    $container_id     = $COMPOSE['mode'] . 'body' . (++$part_no);
+    $container_attrib = array('id' => $container_id);
+
+    $body_args = array(
+        'safe'             => $MESSAGE->is_safe,
+        'plain'            => false,
+        'css_prefix'       => 'v' . $part_no,
+    );
+
+    // remove comments (produced by washtml)
+    $replace = array('/<!--[^>]+-->/' => '');
+
+    if ($COMPOSE['mode'] == rcmail_sendmail::MODE_DRAFT) {
+        // convert TinyMCE's empty-line sequence (#1490463)
+        $replace['/<p>\xC2\xA0<\/p>/'] = '<p><br /></p>';
+        // remove <body> tags
+        $replace['/<body([^>]*)>/i'] = '';
+        $replace['/<\/body>/i']      = '';
+    }
+    else {
+        $body_args['container_id']     = $container_id;
+        $body_args['container_attrib'] = $container_attrib;
+    }
+
+    // Make the HTML content safe and clean
+    $body = rcmail_wash_html($body, $wash_params + $body_args, $CID_MAP);
+    $body = preg_replace(array_keys($replace), array_values($replace), $body);
+    $body = rcmail_html4inline($body, $body_args);
+
+    if ($COMPOSE['mode'] != rcmail_sendmail::MODE_DRAFT) {
+        $body =  html::div($container_attrib, $body);
     }
 
     return $body;
@@ -938,18 +938,17 @@ function rcmail_remove_signature($body)
 
 function rcmail_write_compose_attachments(&$message, $bodyIsHtml)
 {
-    global $RCMAIL, $COMPOSE;
+    global $RCMAIL, $COMPOSE, $CID_MAP;
 
-    $loaded_attachments = array();
-    foreach ((array)$COMPOSE['attachments'] as $attachment) {
-        $loaded_attachments[$attachment['name'] . $attachment['mimetype']] = $attachment;
+    if ($message->pgp_mime || !empty($COMPOSE['forward_attachments'])) {
+        return $CID_MAP;
     }
 
-    $cid_map  = array();
-    $messages = array();
+    $messages           = array();
+    $loaded_attachments = array();
 
-    if ($message->pgp_mime) {
-        return $cid_map;
+    foreach ((array)$COMPOSE['attachments'] as $attachment) {
+        $loaded_attachments[$attachment['name'] . $attachment['mimetype']] = $attachment;
     }
 
     foreach ((array) $message->mime_parts() as $pid => $part) {
@@ -993,9 +992,9 @@ function rcmail_write_compose_attachments(&$message, $bodyIsHtml)
                         $RCMAIL->comm_path, $COMPOSE['id'], $attachment['id']);
 
                     if ($part->content_id)
-                        $cid_map['cid:'.$part->content_id] = $url;
+                        $CID_MAP['cid:'.$part->content_id] = $url;
                     else
-                        $cid_map[$part->content_location] = $url;
+                        $CID_MAP[$part->content_location] = $url;
                 }
             }
         }
@@ -1003,20 +1002,19 @@ function rcmail_write_compose_attachments(&$message, $bodyIsHtml)
 
     $COMPOSE['forward_attachments'] = true;
 
-    return $cid_map;
+    return $CID_MAP;
 }
 
 function rcmail_write_inline_attachments(&$message)
 {
-    global $RCMAIL, $COMPOSE;
-
-    $cid_map  = array();
-    $messages = array();
+    global $RCMAIL, $COMPOSE, $CID_MAP;
 
     if ($message->pgp_mime) {
-        return $cid_map;
+        return $CID_MAP;
     }
 
+    $messages = array();
+
     foreach ((array) $message->mime_parts() as $pid => $part) {
         if ($part->mimetype == 'message/rfc822') {
             $messages[] = $part->mime_id;
@@ -1035,14 +1033,14 @@ function rcmail_write_inline_attachments(&$message)
                     $RCMAIL->comm_path, $COMPOSE['id'], $attachment['id']);
 
                 if ($part->content_id)
-                    $cid_map['cid:'.$part->content_id] = $url;
+                    $CID_MAP['cid:'.$part->content_id] = $url;
                 else
-                    $cid_map[$part->content_location] = $url;
+                    $CID_MAP[$part->content_location] = $url;
             }
         }
     }
 
-    return $cid_map;
+    return $CID_MAP;
 }
 
 // Creates attachment(s) from the forwarded message(s)

program/steps/mail/func.inc

@@ -1083,13 +1083,13 @@ function rcmail_part_image_type($part)
 /**
  * Modify a HTML message that it can be displayed inside a HTML page
  */
-function rcmail_html4inline($body, $args)
+function rcmail_html4inline($body, &$args)
 {
     $last_pos = 0;
     $cont_id  = $args['container_id'] . ($args['body_class'] ? ' div.' . $args['body_class'] : '');
 
     // find STYLE tags
-    while (($pos = stripos($body, '<style', $last_pos)) && ($pos2 = stripos($body, '</style>', $pos))) {
+    while (($pos = stripos($body, '<style', $last_pos)) !== false && ($pos2 = stripos($body, '</style>', $pos+1))) {
         $pos = strpos($body, '>', $pos) + 1;
         $len = $pos2 - $pos;
 
@@ -1101,37 +1101,22 @@ function rcmail_html4inline($body, $args)
         $last_pos = $pos2 + strlen($styles) - $len;
     }
 
-    $body = preg_replace(array(
-            // add comments around html and other tags
-            '/(<!DOCTYPE[^>]*>)/i',
-            '/(<\?xml[^>]*>)/i',
-            '/(<\/?html[^>]*>)/i',
-            '/(<\/?head[^>]*>)/i',
-            '/(<title[^>]*>.*<\/title>)/Ui',
-            '/(<\/?meta[^>]*>)/i',
-            // quote <? of php and xml files that are specified as text/html
-            '/<\?/',
-            '/\?>/',
-            // replace <body> with <div>
-            '/<body([^>]*)>/i',
-            '/<\/body>/i',
-        ),
-        array(
-            '<!--\\1-->',
-            '<!--\\1-->',
-            '<!--\\1-->',
-            '<!--\\1-->',
-            '<!--\\1-->',
-            '<!--\\1-->',
-            '&lt;?',
-            '?&gt;',
-            '<div class="' . $args['body_class'] . '"\\1>',
-            '</div>',
-        ),
-        $body);
+    $replace = array(
+        // add comments around html and other tags
+        '/(<!DOCTYPE[^>]*>)/i'          => '<!--\\1-->',
+        '/(<\?xml[^>]*>)/i'             => '<!--\\1-->',
+        '/(<\/?html[^>]*>)/i'           => '<!--\\1-->',
+        '/(<\/?head[^>]*>)/i'           => '<!--\\1-->',
+        '/(<title[^>]*>.*<\/title>)/Ui' => '<!--\\1-->',
+        '/(<\/?meta[^>]*>)/i'           => '<!--\\1-->',
+        // quote <? of php and xml files that are specified as text/html
+        '/<\?/' => '&lt;?',
+        '/\?>/' => '?&gt;',
+    );
+
+    $regexp = '/<body([^>]*)/';
 
     // Handle body attributes that doesn't play nicely with div elements
-    $regexp = '/<div class="' . preg_quote($args['body_class'], '/') . '"([^>]*)/';
     if (preg_match($regexp, $body, $m)) {
         $style = array();
         $attrs = $m[0];
@@ -1139,13 +1124,13 @@ function rcmail_html4inline($body, $args)
         // Get bgcolor, we'll set it as background-color of the message container
         if ($m[1] && preg_match('/bgcolor=["\']*([a-z0-9#]+)["\']*/i', $attrs, $mb)) {
             $style['background-color'] = $mb[1];
-            $attrs = preg_replace('/bgcolor=["\']*[a-z0-9#]+["\']*/i', '', $attrs);
+            $attrs = preg_replace('/\s?bgcolor=["\']*[a-z0-9#]+["\']*/i', '', $attrs);
         }
 
         // Get background, we'll set it as background-image of the message container
         if ($m[1] && preg_match('/background=["\']*([^"\'>\s]+)["\']*/', $attrs, $mb)) {
             $style['background-image'] = 'url('.$mb[1].')';
-            $attrs = preg_replace('/background=["\']*([^"\'>\s]+)["\']*/', '', $attrs);
+            $attrs = preg_replace('/\s?background=["\']*([^"\'>\s]+)["\']*/', '', $attrs);
         }
 
         if (!empty($style)) {
@@ -1171,15 +1156,28 @@ function rcmail_html4inline($body, $args)
                 $style[$idx] = $idx . ': ' . $val;
             }
 
-            $attributes['style'] = implode('; ', $style);
+            $args['container_attrib']['style'] = implode('; ', $style);
         }
+
+        // replace <body> with <div>
+        if (!empty($args['body_class'])) {
+            $replace['/<body([^>]*)>/i'] = '<div class="' . $args['body_class'] . '"\\1>';
+        }
+        else {
+            $replace['/<body/i'] = '<div';
+        }
+
+        $replace['/<\/body>/i'] = '</div>';
     }
     // make sure there's 'rcmBody' div, we need it for proper css modification
     // its name is hardcoded in rcmail_message_body() also
-    else {
+    else if (!empty($args['body_class'])) {
         $body = '<div class="' . $args['body_class'] . '">' . $body . '</div>';
     }
 
+    // Clean up, and replace <body> with <div>
+    $body = preg_replace(array_keys($replace), array_values($replace), $body);
+
     return $body;
 }
 

program/steps/mail/show.inc

@@ -685,7 +685,7 @@ function rcmail_message_body($attrib)
                     $body = rcmail_html4inline($body, $body_args);
                 }
 
-                $out .= html::div($container_attrib, $plugin['prefix'] . $body);
+                $out .= html::div($body_args['container_attrib'], $plugin['prefix'] . $body);
             }
         }
     }

tests/MailFunc.php

@@ -41,8 +41,10 @@ class MailFunc extends PHPUnit_Framework_TestCase
         $part = $this->get_html_part('src/htmlbody.txt');
         $part->replaces = array('ex1.jpg' => 'part_1.2.jpg', 'ex2.jpg' => 'part_1.2.jpg');
 
+        $params = array('container_id' => 'foo');
+
         // render HTML in normal mode
-        $html = rcmail_html4inline(rcmail_print_body($part->body, $part, array('safe' => false)), array('container_id' => 'foo'));
+        $html = rcmail_html4inline(rcmail_print_body($part->body, $part, array('safe' => false)), $params);
 
         $this->assertRegExp('/src="'.$part->replaces['ex1.jpg'].'"/', $html, "Replace reference to inline image");
         $this->assertRegExp('#background="program/resources/blocked.gif"#', $html, "Replace external background image");
@@ -56,7 +58,7 @@ class MailFunc extends PHPUnit_Framework_TestCase
         $this->assertTrue($GLOBALS['REMOTE_OBJECTS'], "Remote object detected");
 
         // render HTML in safe mode
-        $html2 = rcmail_html4inline(rcmail_print_body($part->body, $part, array('safe' => true)), array('container_id' => 'foo'));
+        $html2 = rcmail_html4inline(rcmail_print_body($part->body, $part, array('safe' => true)), $params);
 
         $this->assertRegExp('/<style [^>]+>/', $html2, "Allow styles in safe mode");
         $this->assertRegExp('#src="http://evilsite.net/mailings/ex3.jpg"#', $html2, "Allow external images in HTML (safe mode)");
@@ -77,7 +79,9 @@ class MailFunc extends PHPUnit_Framework_TestCase
         $this->assertNotRegExp('/\son[a-z]+/', $washed, "Remove on* attributes");
         $this->assertNotContains('onload', $washed, "Handle invalid style");
 
-        $html = rcmail_html4inline($washed, array('container_id' => 'foo'));
+        $params = array('container_id' => 'foo');
+        $html   = rcmail_html4inline($washed, $params);
+
         $this->assertNotRegExp('/onclick="return rcmail.command(\'compose\',\'xss@somehost.net\',this)"/', $html, "Clean mailto links");
         $this->assertNotRegExp('/alert/', $html, "Remove alerts");
     }
@@ -88,9 +92,9 @@ class MailFunc extends PHPUnit_Framework_TestCase
      */
     function test_html_xss2()
     {
-        $part = $this->get_html_part('src/BID-26800.txt');
-        $washed = rcmail_html4inline(rcmail_print_body($part->body, $part, array('safe' => true)),
-             array('container_id' => 'dabody', 'safe' => true));
+        $part   = $this->get_html_part('src/BID-26800.txt');
+        $params = array('container_id' => 'dabody', 'safe' => true);
+        $washed = rcmail_html4inline(rcmail_print_body($part->body, $part, array('safe' => true)), $params);
 
         $this->assertNotRegExp('/alert|expression|javascript|xss/', $washed, "Remove evil style blocks");
         $this->assertNotRegExp('/font-style:italic/', $washed, "Allow valid styles");
@@ -110,6 +114,20 @@ class MailFunc extends PHPUnit_Framework_TestCase
         $this->assertNotRegExp('/vbscript:/', $washed, "Remove vbscript: links");
     }
 
+    /**
+     * Test handling of body style attributes
+     */
+    function test_html4inline_body_style()
+    {
+        $html   = '<body background="test" bgcolor="#fff" style="font-size:11px"><p>test</p></body>';
+        $params = array('container_id' => 'foo');
+        $html   = rcmail_html4inline($html, $params);
+
+        $this->assertRegExp('/<div style="font-size:11px">/', $html, "Body attributes");
+        $this->assertArrayHasKey('container_attrib', $params, "'container_attrib' param set");
+        $this->assertSame('background-color: #fff; background-image: url(test)', $params['container_attrib']['style'], "Body style");
+    }
+
     /**
      * Test washtml class on non-unicode characters (#1487813)
      * @group iconv
@@ -177,9 +195,10 @@ class MailFunc extends PHPUnit_Framework_TestCase
     function test_mailto()
     {
         $part = $this->get_html_part('src/mailto.txt');
+        $params = array('container_id' => 'foo');
 
         // render HTML in normal mode
-        $html = rcmail_html4inline(rcmail_print_body($part->body, $part, array('safe' => false)), array('container_id' => 'foo'));
+        $html = rcmail_html4inline(rcmail_print_body($part->body, $part, array('safe' => false)), $params);
 
         $mailto = '<a href="mailto:me@me.com"'
             .' onclick="return rcmail.command(\'compose\',\'me@me.com?subject=this is the subject&amp;body=this is the body\',this)" rel="noreferrer">e-mail</a>';