Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix XSS issue in SVG images handling (#1490625)
  • Loading branch information
alecpl committed Jan 6, 2016
1 parent f04b56f commit 40d7342
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 26 deletions.
1 change: 1 addition & 0 deletions CHANGELOG
Expand Up @@ -10,6 +10,7 @@ CHANGELOG Roundcube Webmail
- Fix .htaccess rewrite rules to not block .well-known URIs (#1490615)
- Fix mail view scaling on iOS (#1490551)
- Fix PHP7 warning "session_start(): Session callback expects true/false return value" (#1490624)
- Fix XSS issue in SVG images handling (#1490625)

RELEASE 1.2-beta
----------------
Expand Down
101 changes: 75 additions & 26 deletions program/steps/mail/get.inc
Expand Up @@ -93,6 +93,11 @@ else if ($_GET['_thumb']) {
$mimetype = 'image/' . $imgtype;
unlink($orig_name);
}
else if (stripos($mimetype, 'image/svg') === 0) {
$content = rcmail_svg_filter(file_get_contents($orig_name));
file_put_contents($cache_file, $content);
unlink($orig_name);
}
else {
rename($orig_name, $cache_file);
}
Expand Down Expand Up @@ -329,7 +334,7 @@ else if (strlen($part_id)) {
}

// convert image to jpeg and send it to the browser
if ($saved) {
if ($sent = $saved) {
$image = new rcube_image($file_path);
if ($image->convert(rcube_image::TYPE_JPG, $file_path)) {
header("Content-Length: " . filesize($file_path));
Expand All @@ -338,32 +343,8 @@ else if (strlen($part_id)) {
unlink($file_path);
}
}
// do content filtering to avoid XSS through fake images
else if (!empty($_REQUEST['_embed']) && $browser->ie && $browser->ver <= 8) {
if ($body) {
echo preg_match('/<(script|iframe|object)/i', $body) ? '' : $body;
$sent = true;
}
else if ($part->size) {
$stdout = fopen('php://output', 'w');
stream_filter_register('rcube_content', 'rcube_content_filter') or die('Failed to register content filter');
stream_filter_append($stdout, 'rcube_content');
$sent = $MESSAGE->get_part_body($part->mime_id, true, 0, $stdout);
}
}
// send part as-it-is
else {
if ($body && empty($plugin['download'])) {
header("Content-Length: " . strlen($body));
echo $body;
$sent = true;
}
else if ($part->size) {
// Don't be tempted to set Content-Length to $part->d_parameters['size'] (#1490482)
// RFC2183 says "The size parameter indicates an approximate size"

$sent = $MESSAGE->get_part_body($part->mime_id, false, 0, -1);
}
$sent = rcmail_message_part_output($body, $part, $mimetype, $plugin['download']);
}

// check connection status
Expand Down Expand Up @@ -475,3 +456,71 @@ function rcmail_message_part_frame($attrib)

return html::iframe($attrib);
}

/**
* Output attachment body with content filtering
*/
function rcmail_message_part_output($body, $part, $mimetype, $download)
{
global $MESSAGE, $RCMAIL;

if (!$part->size && !$body) {
return false;
}

$browser = $RCMAIL->output->browser;
$secure = stripos($mimetype, 'image/') === false || $download;

// Remove <script> in SVG images
if (!$secure && stripos($mimetype, 'image/svg') === 0) {
if (!$body) {
$body = $MESSAGE->get_part_body($part->mime_id, false);
if (empty($body)) {
return false;
}
}

echo rcmail_svg_filter($body);
return true;
}

// Remove dangerous content in images for older IE (to be removed)
if (!$secure && $browser->ie && $browser->ver <= 8) {
if ($body) {
echo preg_match('/<(script|iframe|object)/i', $body) ? '' : $body;
return true;
}
else {
$stdout = fopen('php://output', 'w');
stream_filter_register('rcube_content', 'rcube_content_filter') or die('Failed to register content filter');
stream_filter_append($stdout, 'rcube_content');
return $MESSAGE->get_part_body($part->mime_id, true, 0, $stdout);
}
}

if ($body && !$download) {
header("Content-Length: " . strlen($body));
echo $body;
return true;
}

// Don't be tempted to set Content-Length to $part->d_parameters['size'] (#1490482)
// RFC2183 says "The size parameter indicates an approximate size"

return $MESSAGE->get_part_body($part->mime_id, false, 0, -1);
}

/**
* Remove <script> in SVG images
*/
function rcmail_svg_filter($body)
{
$dom = new DOMDocument;
$dom->loadXML($body);

foreach ($dom->getElementsByTagName('script') as $node) {
$node->parentNode->removeChild($node);
}

return $dom->saveXML() ?: '';
}

1 comment on commit 40d7342

@thomascube
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start! Removing script nodes, however, is just the beginning. XSS code can also be in node attributes like onclick, onmouseover, href="javascript:, etc. or even in CSS url() as we learned with HTML messages.

So traversing the entire DOM is probably necessary to provide protection that goes beyond the one example we received.

Please sign in to comment.