Skip to content

Commit a71bf2e

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

File tree

3 files changed

+212
-29
lines changed

3 files changed

+212
-29
lines changed

Diff for: CHANGELOG

+2
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ CHANGELOG Roundcube Webmail
1313
- Fix handling links without defined protocol (#7454)
1414
- Fix paging of search results on IMAP servers with no SORT capability (#7462)
1515
- Fix detecting special folders on servers with both SPECIAL-USE and LIST-STATUS (#7525)
16+
- Security: Fix cross-site scripting (XSS) via HTML messages with malicious svg content [CVE-2020-16145]
17+
- Security: Fix cross-site scripting (XSS) via HTML messages with malicious math content
1618

1719
RELEASE 1.4.7
1820
-------------

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

+58-2
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,30 @@ private function wash_uri($uri, $blocked_source = false, $is_image = true)
412412
return $this->config['blocked_src'];
413413
}
414414
}
415-
else if ($is_image && preg_match('/^data:image.+/i', $uri)) { // RFC2397
415+
else if ($is_image && preg_match('/^data:image\/([^,]+),(.+)$/i', $uri, $matches)) { // RFC2397
416+
// svg images can be insecure, we'll sanitize them
417+
if (stripos($matches[1], 'svg') !== false) {
418+
$svg = $matches[2];
419+
420+
if (stripos($matches[1], ';base64') !== false) {
421+
$svg = base64_decode($svg);
422+
$type = $matches[1];
423+
}
424+
else {
425+
$type = $matches[1] . ';base64';
426+
}
427+
428+
$washer = new self($this->config);
429+
$svg = $washer->wash($svg);
430+
431+
// Invalid svg content
432+
if (empty($svg)) {
433+
return null;
434+
}
435+
436+
return 'data:image/' . $type . ',' . base64_encode($svg);
437+
}
438+
416439
return $uri;
417440
}
418441
}
@@ -451,7 +474,7 @@ private function wash_link($href)
451474
*/
452475
private function is_link_attribute($tag, $attr)
453476
{
454-
return ($tag == 'a' || $tag == 'area') && $attr == 'href';
477+
return $attr === 'href';
455478
}
456479

457480
/**
@@ -468,6 +491,7 @@ private function is_image_attribute($tag, $attr)
468491
|| $attr == 'color-profile' // SVG
469492
|| ($attr == 'poster' && $tag == 'video')
470493
|| ($attr == 'src' && preg_match('/^(img|image|source|input|video|audio)$/i', $tag))
494+
|| ($tag == 'use' && $attr == 'href') // SVG
471495
|| ($tag == 'image' && $attr == 'href'); // SVG
472496
}
473497

@@ -485,6 +509,31 @@ private function is_funciri_attribute($tag, $attr)
485509
'marker-end', 'marker-mid', 'clip-path', 'mask', 'cursor'));
486510
}
487511

512+
/**
513+
* Check if a specified element has an attribute with specified value.
514+
* Do it in case-insensitive manner.
515+
*
516+
* @param DOMElement $node The element
517+
* @param string $attr_name The attribute name
518+
* @param string $attr_value The attribute value to find
519+
*
520+
* @return bool True if the specified attribute exists and has the expected value
521+
*/
522+
private static function attribute_value($node, $attr_name, $attr_value)
523+
{
524+
$attr_name = strtolower($attr_name);
525+
526+
foreach ($node->attributes as $name => $attr) {
527+
if (strtolower($name) === $attr_name) {
528+
if (strtolower($attr_value) === strtolower($attr->nodeValue)) {
529+
return true;
530+
}
531+
}
532+
}
533+
534+
return false;
535+
}
536+
488537
/**
489538
* The main loop that recurse on a node tree.
490539
* It output only allowed tags with allowed attributes and allowed inline styles
@@ -532,6 +581,13 @@ private function dumpHtml($node, $level = 20)
532581

533582
$node->setAttribute('href', (string) $uri);
534583
}
584+
else if (in_array($tagName, array('animate', 'animatecolor', 'set', 'animatetransform'))
585+
&& self::attribute_value($node, 'attributename', 'href')
586+
) {
587+
// Insecure svg tags
588+
$dump .= "<!-- $tagName blocked -->";
589+
break;
590+
}
535591

536592
if ($callback = $this->handlers[$tagName]) {
537593
$dump .= call_user_func($callback, $tagName,

Diff for: tests/Framework/Washtml.php

+152-27
Original file line numberDiff line numberDiff line change
@@ -317,41 +317,166 @@ function test_wash_svg()
317317
}
318318

319319
/**
320-
* Test SVG cleanup
320+
* Test cases for SVG tests
321321
*/
322-
function test_wash_svg2()
322+
function data_wash_svg_tests()
323323
{
324-
$svg = '<head xmlns="&quot;&gt;&lt;script&gt;alert(document.domain)&lt;/script&gt;"><svg></svg></head>';
325-
$exp = '<!-- html ignored --><!-- head ignored --><svg xmlns="http://www.w3.org/1999/xhtml"></svg>';
326-
327-
$washer = new rcube_washtml;
328-
$washed = $washer->wash($svg);
329-
330-
$this->assertSame($washed, $exp, "SVG content");
331-
332-
$svg = '<head xmlns="&quot; onload=&quot;alert(document.domain)">Hello victim!<svg></svg></head>';
333-
$exp = '<!-- html ignored --><!-- head ignored -->Hello victim!<svg xmlns="http://www.w3.org/1999/xhtml"></svg>';
334-
335-
$washer = new rcube_washtml;
336-
$washed = $washer->wash($svg);
337-
338-
$this->assertSame($washed, $exp, "SVG content");
339-
340-
$svg = '<p>Hello victim!<svg xmlns="&quot; onload=&quot;alert(document.domain)"></svg></p>';
341-
$exp = '<p>Hello victim!<svg /></p>';
324+
$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>";
325+
326+
return [
327+
[
328+
'<head xmlns="&quot;&gt;&lt;script&gt;alert(document.domain)&lt;/script&gt;"><svg></svg></head>',
329+
'<!-- html ignored --><!-- head ignored --><svg xmlns="http://www.w3.org/1999/xhtml"></svg>'
330+
],
331+
[
332+
'<head xmlns="&quot; onload=&quot;alert(document.domain)">Hello victim!<svg></svg></head>',
333+
'<!-- html ignored --><!-- head ignored -->Hello victim!<svg xmlns="http://www.w3.org/1999/xhtml"></svg>'
334+
],
335+
[
336+
'<p>Hello victim!<svg xmlns="&quot; onload=&quot;alert(document.domain)"></svg></p>',
337+
'<p>Hello victim!<svg /></p>'
338+
],
339+
[
340+
'<html><p>Hello victim!<svg xmlns="&quot; onload=&quot;alert(document.domain)"></svg></p>',
341+
'<!-- html ignored --><!-- body ignored --><p>Hello victim!<svg xmlns="http://www.w3.org/1999/xhtml"></svg></p>'
342+
],
343+
[
344+
'<svg xmlns="&quot; onload=&quot;alert(document.domain)" />',
345+
'<svg xmlns="&quot; onload=&quot;alert(document.domain)" />'
346+
],
347+
[
348+
'<html><svg xmlns="&quot; onload=&quot;alert(document.domain)" />',
349+
'<!-- html ignored --><!-- body ignored --><svg xmlns="http://www.w3.org/1999/xhtml"></svg>'
350+
],
351+
[
352+
'<svg><a xlink:href="javascript:alert(1)"><text x="20" y="20">XSS</text></a></svg>',
353+
'<svg><a x-washed="xlink:href"><text x="20" y="20">XSS</text></a></svg>'
354+
],
355+
[
356+
'<html><svg><a xlink:href="javascript:alert(1)"><text x="20" y="20">XSS</text></a></svg>',
357+
'<!-- 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>'
358+
],
359+
[
360+
'<svg><animate xlink:href="#xss" attributeName="href" values="javascript:alert(1)" />'
361+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
362+
'<svg><!-- animate blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
363+
],
364+
[
365+
'<html><svg><animate xlink:href="#xss" attributeName="href" values="javascript:alert(1)" />'
366+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
367+
'<!-- html ignored --><!-- body ignored --><svg xmlns="http://www.w3.org/1999/xhtml">'
368+
. '<!-- animate blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
369+
],
370+
[
371+
'<svg><animate xlink:href="#xss" attributeName="href" from="javascript:alert(1)" to="1" />'
372+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
373+
'<svg><!-- animate blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
374+
],
375+
[
376+
'<svg><set xlink:href="#xss" attributeName="href" from="?" to="javascript:alert(1)" />'
377+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
378+
'<svg><!-- set blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
379+
],
380+
[
381+
'<svg><animate xlink:href="#xss" attributename="href" dur="5s" repeatCount="indefinite" keytimes="0;0;1" values="https://portswigger.net?;javascript:alert(1);0" />'
382+
. '<a id="xss"><text x="20" y="20">XSS</text></a></svg>',
383+
'<svg><!-- animate blocked --><a id="xss"><text x="20" y="20">XSS</text></a></svg>',
384+
],
385+
[
386+
"<svg><use href=\"data:image/svg+xml,&lt;svg id='x' xmlns='http://www.w3.org/2000/svg' "
387+
. "xmlns:xlink='http://www.w3.org/1999/xlink' width='100' height='100'&gt;&lt;a xlink:href='javascript:alert(1)'&gt;"
388+
. "&lt;rect x='0' y='0' width='100' height='100' /&gt;&lt;/a&gt;&lt;/svg&gt;\"></use></svg>",
389+
"<svg><use href=\"data:image/svg+xml;base64,PHN2ZyB4bWxuczp4bGluaz0iaHR0cDovL3d3dy53"
390+
. "My5vcmcvMTk5OS94bGluayIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIiBpZD0ie"
391+
. "CIgd2lkdGg9IjEwMCIgaGVpZ2h0PSIxMDAiPjxhIHgtd2FzaGVkPSJ4bGluazpocmVmIj48cmVjdC"
392+
. "B4PSIwIiB5PSIwIiB3aWR0aD0iMTAwIiBoZWlnaHQ9IjEwMCIgLz48L2E+PC9zdmc+\" /></svg>"
393+
],
394+
[
395+
"<svg><use href=\"data:image/svg+xml;base64," . base64_encode($svg1) . "\"></use></svg>",
396+
"<svg><use href=\"data:image/svg+xml;base64,PHN2ZyBpZD0ieCIgd2lkdGg9IjEwMCIgaGVpZ2h"
397+
. "0PSIxMDAiPjxhIHgtd2FzaGVkPSJ4bGluazpocmVmIj48cmVjdCB4PSIwIiB5PSIwIiB3aWR0aD0"
398+
. "iMTAwIiBoZWlnaHQ9IjEwMCIgLz48L2E+PC9zdmc+\" /></svg>"
399+
],
400+
[
401+
'<svg><script href="data:text/javascript,alert(1)" /><text x="20" y="20">XSS</text></svg>',
402+
'<svg><!-- script not allowed --><text x="20" y="20">XSS</text></svg>'
403+
],
404+
];
405+
}
342406

407+
/**
408+
* Test SVG cleanup
409+
*
410+
* @dataProvider data_wash_svg_tests
411+
*/
412+
function test_wash_svg_tests($input, $expected)
413+
{
343414
$washer = new rcube_washtml;
344-
$washed = $washer->wash($svg);
415+
$washed = $washer->wash($input);
345416

346-
$this->assertSame($washed, $exp, "SVG content");
417+
$this->assertSame($expected, $washed, "SVG content");
418+
}
347419

348-
$svg = '<svg xmlns="&quot; onload=&quot;alert(document.domain)" />';
349-
$exp = '<svg xmlns="&quot; onload=&quot;alert(document.domain)" />';
420+
/**
421+
* Test cases for various XSS issues
422+
*/
423+
function data_wash_xss_tests()
424+
{
425+
return [
426+
[
427+
'<html><base href="javascript:/a/-alert(1)///////"><a href="../lol/safari.html">test</a>',
428+
'<!-- html ignored --><body><!-- base ignored --><a x-washed="href">test</a></body>'
429+
],
430+
[
431+
'<html><math><x href="javascript:alert(1)">blah</x>',
432+
'<!-- html ignored --><body><math><!-- x ignored -->blah</math></body>'
433+
],
434+
[
435+
'<html><a href="j&#x61vascript:alert(1)">XSS</a>',
436+
'<!-- html ignored --><body><a x-washed="href">XSS</a></body>'
437+
],
438+
[
439+
'<html><a href="&#x6a avascript:alert(1)">XSS</a>',
440+
'<!-- html ignored --><body><a x-washed="href">XSS</a></body>'
441+
],
442+
[
443+
'<html><a href="&#x6a avascript:alert(1)">XSS</a>',
444+
'<!-- html ignored --><body><a x-washed="href">XSS</a></body>'
445+
],
446+
[
447+
'<html><body background="javascript:alert(1)">',
448+
'<!-- html ignored --><body x-washed="background"></body>'
449+
],
450+
[
451+
'<html><math href="javascript:alert(location);"><mi>clickme</mi></math>',
452+
'<!-- html ignored --><body><math x-washed="href"><mi>clickme</mi></math></body>',
453+
],
454+
[
455+
'<html><math><mstyle href="javascript:alert(location);"><mi>clickme</mi></mstyle></math>',
456+
'<!-- html ignored --><body><math><mstyle x-washed="href"><mi>clickme</mi></mstyle></math></body>',
457+
],
458+
[
459+
'<html><math><msubsup href="javascript:alert(location);"><mi>clickme</mi></msubsup></math>',
460+
'<!-- html ignored --><body><math><msubsup x-washed="href"><mi>clickme</mi></msubsup></math></body>',
461+
],
462+
[
463+
'<html><math><ms HREF="javascript:alert(location);">clickme</ms></math>',
464+
'<!-- html ignored --><body><math><ms x-washed="href">clickme</ms></math></body>',
465+
],
466+
];
467+
}
350468

351-
$washer = new rcube_washtml;
352-
$washed = $washer->wash($svg);
469+
/**
470+
* Test various XSS issues
471+
*
472+
* @dataProvider data_wash_xss_tests
473+
*/
474+
function test_wash_xss_tests($input, $expected)
475+
{
476+
$washer = new rcube_washtml(['allow_remote' => true, 'html_elements' => ['body']]);
477+
$washed = $washer->wash($input);
353478

354-
$this->assertSame($washed, $exp, "SVG content");
479+
$this->assertSame($expected, $washed, "XSS issues");
355480
}
356481

357482
/**

0 commit comments

Comments
 (0)