Skip to content

Commit d44ca23

Browse files
committed
Fix cross-site scripting (XSS) via HTML messages with malicious svg or math content
1 parent abddddb commit d44ca23

File tree

3 files changed

+215
-29
lines changed

3 files changed

+215
-29
lines changed

Diff for: CHANGELOG

+3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
CHANGELOG Roundcube Webmail
22
===========================
33

4+
- Security: Fix cross-site scripting (XSS) via HTML messages with malicious svg content [CVE-2020-16145]
5+
- Security: Fix cross-site scripting (XSS) via HTML messages with malicious math content
6+
47
RELEASE 1.3.14
58
--------------
69
- Security: Fix cross-site scripting (XSS) via HTML messages with malicious svg/namespace

Diff for: program/lib/Roundcube/rcube_washtml.php

+60-2
Original file line numberDiff line numberDiff line change
@@ -390,7 +390,30 @@ private function wash_uri($uri, $blocked_source = false)
390390
return $this->config['blocked_src'];
391391
}
392392
}
393-
else if (preg_match('/^data:image.+/i', $uri)) { // RFC2397
393+
else if (preg_match('/^data:image\/([^,]+),(.+)$/i', $uri, $matches)) { // RFC2397
394+
// svg images can be insecure, we'll sanitize them
395+
if (stripos($matches[1], 'svg') !== false) {
396+
$svg = $matches[2];
397+
398+
if (stripos($matches[1], ';base64') !== false) {
399+
$svg = base64_decode($svg);
400+
$type = $matches[1];
401+
}
402+
else {
403+
$type = $matches[1] . ';base64';
404+
}
405+
406+
$washer = new self($this->config);
407+
$svg = $washer->wash($svg);
408+
409+
// Invalid svg content
410+
if (empty($svg)) {
411+
return null;
412+
}
413+
414+
return 'data:image/' . $type . ',' . base64_encode($svg);
415+
}
416+
394417
return $uri;
395418
}
396419
}
@@ -400,7 +423,7 @@ private function wash_uri($uri, $blocked_source = false)
400423
*/
401424
private function is_link_attribute($tag, $attr)
402425
{
403-
return ($tag == 'a' || $tag == 'area') && $attr == 'href';
426+
return $attr === 'href';
404427
}
405428

406429
/**
@@ -412,6 +435,7 @@ private function is_image_attribute($tag, $attr)
412435
|| $attr == 'color-profile' // SVG
413436
|| ($attr == 'poster' && $tag == 'video')
414437
|| ($attr == 'src' && preg_match('/^(img|image|source|input|video|audio)$/i', $tag))
438+
|| ($tag == 'use' && $attr == 'href') // SVG
415439
|| ($tag == 'image' && $attr == 'href'); // SVG
416440
}
417441

@@ -424,6 +448,31 @@ private function is_funciri_attribute($tag, $attr)
424448
'marker-end', 'marker-mid', 'clip-path', 'mask', 'cursor'));
425449
}
426450

451+
/**
452+
* Check if a specified element has an attribute with specified value.
453+
* Do it in case-insensitive manner.
454+
*
455+
* @param DOMElement $node The element
456+
* @param string $attr_name The attribute name
457+
* @param string $attr_value The attribute value to find
458+
*
459+
* @return bool True if the specified attribute exists and has the expected value
460+
*/
461+
private static function attribute_value($node, $attr_name, $attr_value)
462+
{
463+
$attr_name = strtolower($attr_name);
464+
465+
foreach ($node->attributes as $name => $attr) {
466+
if (strtolower($name) === $attr_name) {
467+
if (strtolower($attr_value) === strtolower($attr->nodeValue)) {
468+
return true;
469+
}
470+
}
471+
}
472+
473+
return false;
474+
}
475+
427476
/**
428477
* The main loop that recurse on a node tree.
429478
* It output only allowed tags with allowed attributes and allowed inline styles
@@ -458,6 +507,15 @@ private function dumpHtml($node, $level = 20)
458507
switch ($node->nodeType) {
459508
case XML_ELEMENT_NODE: //Check element
460509
$tagName = strtolower($node->nodeName);
510+
511+
if (in_array($tagName, array('animate', 'animatecolor', 'set', 'animatetransform'))
512+
&& self::attribute_value($node, 'attributename', 'href')
513+
) {
514+
// Insecure svg tags
515+
$dump .= "<!-- $tagName blocked -->";
516+
break;
517+
}
518+
461519
if ($callback = $this->handlers[$tagName]) {
462520
$dump .= call_user_func($callback, $tagName,
463521
$this->wash_attribs($node), $this->dumpHtml($node, $level), $this);

Diff for: tests/Framework/Washtml.php

+152-27
Original file line numberDiff line numberDiff line change
@@ -288,41 +288,166 @@ function test_wash_svg()
288288
}
289289

290290
/**
291-
* Test SVG cleanup
291+
* Test cases for SVG tests
292292
*/
293-
function test_wash_svg2()
293+
function data_wash_svg_tests()
294294
{
295-
$svg = '<head xmlns="&quot;&gt;&lt;script&gt;alert(document.domain)&lt;/script&gt;"><svg></svg></head>';
296-
$exp = '<!-- html ignored --><!-- head ignored --><svg xmlns="http://www.w3.org/1999/xhtml"></svg>';
297-
298-
$washer = new rcube_washtml;
299-
$washed = $washer->wash($svg);
300-
301-
$this->assertSame($washed, $exp, "SVG content");
302-
303-
$svg = '<head xmlns="&quot; onload=&quot;alert(document.domain)">Hello victim!<svg></svg></head>';
304-
$exp = '<!-- html ignored --><!-- head ignored -->Hello victim!<svg xmlns="http://www.w3.org/1999/xhtml"></svg>';
305-
306-
$washer = new rcube_washtml;
307-
$washed = $washer->wash($svg);
308-
309-
$this->assertSame($washed, $exp, "SVG content");
310-
311-
$svg = '<p>Hello victim!<svg xmlns="&quot; onload=&quot;alert(document.domain)"></svg></p>';
312-
$exp = '<p>Hello victim!<svg /></p>';
295+
$svg1 = "<svg id='x' width='100' height='100'><a xlink:href='javascript:alert(1)'><rect x='0' y='0' width='100' height='100' /></a></svg>";
296+
297+
return [
298+
[
299+
'<head xmlns="&quot;&gt;&lt;script&gt;alert(document.domain)&lt;/script&gt;"><svg></svg></head>',
300+
'<!-- html ignored --><!-- head ignored --><svg xmlns="http://www.w3.org/1999/xhtml"></svg>'
301+
],
302+
[
303+
'<head xmlns="&quot; onload=&quot;alert(document.domain)">Hello victim!<svg></svg></head>',
304+
'<!-- html ignored --><!-- head ignored -->Hello victim!<svg xmlns="http://www.w3.org/1999/xhtml"></svg>'
305+
],
306+
[
307+
'<p>Hello victim!<svg xmlns="&quot; onload=&quot;alert(document.domain)"></svg></p>',
308+
'<p>Hello victim!<svg /></p>'
309+
],
310+
[
311+
'<html><p>Hello victim!<svg xmlns="&quot; onload=&quot;alert(document.domain)"></svg></p>',
312+
'<!-- html ignored --><!-- body ignored --><p>Hello victim!<svg xmlns="http://www.w3.org/1999/xhtml"></svg></p>'
313+
],
314+
[
315+
'<svg xmlns="&quot; onload=&quot;alert(document.domain)" />',
316+
'<svg xmlns="&quot; onload=&quot;alert(document.domain)" />'
317+
],
318+
[
319+
'<html><svg xmlns="&quot; onload=&quot;alert(document.domain)" />',
320+
'<!-- html ignored --><!-- body ignored --><svg xmlns="http://www.w3.org/1999/xhtml"></svg>'
321+
],
322+
[
323+
'<svg><a xlink:href="javascript:alert(1)"><text x="20" y="20">XSS</text></a></svg>',
324+
'<svg><a x-washed="xlink:href"><text x="20" y="20">XSS</text></a></svg>'
325+
],
326+
[
327+
'<html><svg><a xlink:href="javascript:alert(1)"><text x="20" y="20">XSS</text></a></svg>',
328+
'<!-- html ignored --><!-- body ignored --><svg xmlns="http://www.w3.org/1999/xhtml"><a x-washed="xlink:href"><text x="20" y="20">XSS</text></a></svg>'
329+
],
330+
[
331+
'<svg><animate xlink:href="#xss" attributeName="href" values="javascript:alert(1)" />'
332+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
333+
'<svg><!-- animate blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
334+
],
335+
[
336+
'<html><svg><animate xlink:href="#xss" attributeName="href" values="javascript:alert(1)" />'
337+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
338+
'<!-- html ignored --><!-- body ignored --><svg xmlns="http://www.w3.org/1999/xhtml">'
339+
. '<!-- animate blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
340+
],
341+
[
342+
'<svg><animate xlink:href="#xss" attributeName="href" from="javascript:alert(1)" to="1" />'
343+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
344+
'<svg><!-- animate blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
345+
],
346+
[
347+
'<svg><set xlink:href="#xss" attributeName="href" from="?" to="javascript:alert(1)" />'
348+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
349+
'<svg><!-- set blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
350+
],
351+
[
352+
'<svg><animate xlink:href="#xss" attributename="href" dur="5s" repeatCount="indefinite" keytimes="0;0;1" values="https://portswigger.net?;javascript:alert(1);0" />'
353+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
354+
'<svg><!-- animate blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
355+
],
356+
[
357+
"<svg><use href=\"data:image/svg+xml,&lt;svg id='x' xmlns='http://www.w3.org/2000/svg' "
358+
. "xmlns:xlink='http://www.w3.org/1999/xlink' width='100' height='100'&gt;&lt;a xlink:href='javascript:alert(1)'&gt;"
359+
. "&lt;rect x='0' y='0' width='100' height='100' /&gt;&lt;/a&gt;&lt;/svg&gt;\"></use></svg>",
360+
"<svg><use href=\"data:image/svg+xml;base64,PHN2ZyB4bWxuczp4bGluaz0iaHR0cDovL3d3dy53"
361+
. "My5vcmcvMTk5OS94bGluayIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIiBpZD0ie"
362+
. "CIgd2lkdGg9IjEwMCIgaGVpZ2h0PSIxMDAiPjxhIHgtd2FzaGVkPSJ4bGluazpocmVmIj48cmVjdC"
363+
. "B4PSIwIiB5PSIwIiB3aWR0aD0iMTAwIiBoZWlnaHQ9IjEwMCIgLz48L2E+PC9zdmc+\" /></svg>"
364+
],
365+
[
366+
"<svg><use href=\"data:image/svg+xml;base64," . base64_encode($svg1) . "\"></use></svg>",
367+
"<svg><use href=\"data:image/svg+xml;base64,PHN2ZyBpZD0ieCIgd2lkdGg9IjEwMCIgaGVpZ2h"
368+
. "0PSIxMDAiPjxhIHgtd2FzaGVkPSJ4bGluazpocmVmIj48cmVjdCB4PSIwIiB5PSIwIiB3aWR0aD0"
369+
. "iMTAwIiBoZWlnaHQ9IjEwMCIgLz48L2E+PC9zdmc+\" /></svg>"
370+
],
371+
[
372+
'<svg><script href="data:text/javascript,alert(1)" /><text x="20" y="20">XSS</text></svg>',
373+
'<svg><!-- script not allowed --><text x="20" y="20">XSS</text></svg>'
374+
],
375+
];
376+
}
313377

378+
/**
379+
* Test SVG cleanup
380+
*
381+
* @dataProvider data_wash_svg_tests
382+
*/
383+
function test_wash_svg_tests($input, $expected)
384+
{
314385
$washer = new rcube_washtml;
315-
$washed = $washer->wash($svg);
386+
$washed = $washer->wash($input);
316387

317-
$this->assertSame($washed, $exp, "SVG content");
388+
$this->assertSame($expected, $washed, "SVG content");
389+
}
318390

319-
$svg = '<svg xmlns="&quot; onload=&quot;alert(document.domain)" />';
320-
$exp = '<svg xmlns="&quot; onload=&quot;alert(document.domain)" />';
391+
/**
392+
* Test cases for various XSS issues
393+
*/
394+
function data_wash_xss_tests()
395+
{
396+
return [
397+
[
398+
'<html><base href="javascript:/a/-alert(1)///////"><a href="../lol/safari.html">test</a>',
399+
'<!-- html ignored --><body><!-- base ignored --><a x-washed="href">test</a></body>'
400+
],
401+
[
402+
'<html><math><x href="javascript:alert(1)">blah</x>',
403+
'<!-- html ignored --><body><math><!-- x ignored -->blah</math></body>'
404+
],
405+
[
406+
'<html><a href="j&#x61vascript:alert(1)">XSS</a>',
407+
'<!-- html ignored --><body><a x-washed="href">XSS</a></body>'
408+
],
409+
[
410+
'<html><a href="&#x6a avascript:alert(1)">XSS</a>',
411+
'<!-- html ignored --><body><a x-washed="href">XSS</a></body>'
412+
],
413+
[
414+
'<html><a href="&#x6a avascript:alert(1)">XSS</a>',
415+
'<!-- html ignored --><body><a x-washed="href">XSS</a></body>'
416+
],
417+
[
418+
'<html><body background="javascript:alert(1)">',
419+
'<!-- html ignored --><body x-washed="background"></body>'
420+
],
421+
[
422+
'<html><math href="javascript:alert(location);"><mi>clickme</mi></math>',
423+
'<!-- html ignored --><body><math x-washed="href"><mi>clickme</mi></math></body>',
424+
],
425+
[
426+
'<html><math><mstyle href="javascript:alert(location);"><mi>clickme</mi></mstyle></math>',
427+
'<!-- html ignored --><body><math><mstyle x-washed="href"><mi>clickme</mi></mstyle></math></body>',
428+
],
429+
[
430+
'<html><math><msubsup href="javascript:alert(location);"><mi>clickme</mi></msubsup></math>',
431+
'<!-- html ignored --><body><math><msubsup x-washed="href"><mi>clickme</mi></msubsup></math></body>',
432+
],
433+
[
434+
'<html><math><ms HREF="javascript:alert(location);">clickme</ms></math>',
435+
'<!-- html ignored --><body><math><ms x-washed="href">clickme</ms></math></body>',
436+
],
437+
];
438+
}
321439

322-
$washer = new rcube_washtml;
323-
$washed = $washer->wash($svg);
440+
/**
441+
* Test various XSS issues
442+
*
443+
* @dataProvider data_wash_xss_tests
444+
*/
445+
function test_wash_xss_tests($input, $expected)
446+
{
447+
$washer = new rcube_washtml(['allow_remote' => true, 'html_elements' => ['body']]);
448+
$washed = $washer->wash($input);
324449

325-
$this->assertSame($washed, $exp, "SVG content");
450+
$this->assertSame($expected, $washed, "XSS issues");
326451
}
327452

328453
/**

0 commit comments

Comments
 (0)