tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

Fixed XSS vulnerability relating to rewrite_hash

Browse Source
This commit is contained in:
Christopher Pitt 2015-03-20 18:17:51 +13:00 committed by Damian Mooyman
parent 11521fb92d
commit bdef4fc7a5
4 changed files with 30 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
tests/view/SSViewerTest.php
View File

@ -910,6 +910,8 @@ after')
$oldRewriteHashLinks = SSViewer::getOption('rewriteHashlinks'); $oldRewriteHashLinks = SSViewer::getOption('rewriteHashlinks');
SSViewer::setOption('rewriteHashlinks', true); SSViewer::setOption('rewriteHashlinks', true);
$_SERVER['REQUEST_URI'] = 'http://path/to/file?foo"onclick="alert(\'xss\')""';
// Emulate SSViewer::process() // Emulate SSViewer::process()
$base = Convert::raw2att($_SERVER['REQUEST_URI']); $base = Convert::raw2att($_SERVER['REQUEST_URI']);
@ -920,22 +922,39 @@ after')
<html> <html>
<head><% base_tag %></head> <head><% base_tag %></head>
<body> <body>
<a class="external-inline" href="http://google.com#anchor">ExternalInlineLink</a>
$ExternalInsertedLink
<a class="inline" href="#anchor">InlineLink</a> <a class="inline" href="#anchor">InlineLink</a>
$InsertedLink $InsertedLink
<svg><use xlink:href="#sprite"></use></svg>
<body> <body>
</html>'); </html>');
$tmpl = new SSViewer($tmplFile); $tmpl = new SSViewer($tmplFile);
$obj = new ViewableData(); $obj = new ViewableData();
$obj->InsertedLink = '<a class="inserted" href="#anchor">InsertedLink</a>'; $obj->InsertedLink = '<a class="inserted" href="#anchor">InsertedLink</a>';
$obj->ExternalInsertedLink = '<a class="external-inserted" href="http://google.com#anchor">ExternalInsertedLink</a>';
$result = $tmpl->process($obj); $result = $tmpl->process($obj);
$this->assertContains( $this->assertContains(
'<a class="inserted" href="' . $base . '#anchor">InsertedLink</a>', '<a class="inserted" href="' . $base . '#anchor">InsertedLink</a>',
$result $result
); );
$this->assertContains(
'<a class="external-inserted" href="http://google.com#anchor">ExternalInsertedLink</a>',
$result
);
$this->assertContains( $this->assertContains(
'<a class="inline" href="' . $base . '#anchor">InlineLink</a>', '<a class="inline" href="' . $base . '#anchor">InlineLink</a>',
$result $result
); );
$this->assertContains(
'<a class="external-inline" href="http://google.com#anchor">ExternalInlineLink</a>',
$result
);
$this->assertContains(
'<svg><use xlink:href="#sprite"></use></svg>',
$result,
'SSTemplateParser should only rewrite anchor hrefs'
);
unlink($tmplFile); unlink($tmplFile);
@ -962,7 +981,7 @@ after')
$obj->InsertedLink = '<a class="inserted" href="#anchor">InsertedLink</a>'; $obj->InsertedLink = '<a class="inserted" href="#anchor">InsertedLink</a>';
$result = $tmpl->process($obj); $result = $tmpl->process($obj);
$this->assertContains( $this->assertContains(
'<a class="inserted" href="<?php echo strip_tags(', '<a class="inserted" href="<?php echo Convert::raw2att(',
$result $result
); );
// TODO Fix inline links in PHP mode // TODO Fix inline links in PHP mode

5
view/SSTemplateParser.php
View File

@ -4471,8 +4471,9 @@ class SSTemplateParser extends Parser {
// TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this // TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this
// non-dynamically calculated // non-dynamically calculated
$text = preg_replace( $text = preg_replace(
'/href\s*\=\s*\"\#/', '/(<a[^>]+href *= *)"#/i',
'href="\' . (SSViewer::$options[\'rewriteHashlinks\'] ? strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . '\\1"\' . (SSViewer::$options[\'rewriteHashlinks\'] ?' .
' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") .
\'#', \'#',
$text $text
); );

5
view/SSTemplateParser.php.inc
View File

@ -1005,8 +1005,9 @@ class SSTemplateParser extends Parser {
// TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this // TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this
// non-dynamically calculated // non-dynamically calculated
$text = preg_replace( $text = preg_replace(
'/href\s*\=\s*\"\#/', '/(<a[^>]+href *= *)"#/i',
'href="\' . (SSViewer::$options[\'rewriteHashlinks\'] ? strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . '\\1"\' . (SSViewer::$options[\'rewriteHashlinks\'] ?' .
' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") .
\'#', \'#',
$text $text
); );

4
view/SSViewer.php
View File

@ -928,9 +928,9 @@ class SSViewer {
if($this->rewriteHashlinks && self::$options['rewriteHashlinks']) { if($this->rewriteHashlinks && self::$options['rewriteHashlinks']) {
if(strpos($output, '<base') !== false) { if(strpos($output, '<base') !== false) {
if(SSViewer::$options['rewriteHashlinks'] === 'php') { if(SSViewer::$options['rewriteHashlinks'] === 'php') {
$thisURLRelativeToBase = "<?php echo strip_tags(\$_SERVER['REQUEST_URI']); ?>"; $thisURLRelativeToBase = "<?php echo Convert::raw2att(\$_SERVER['REQUEST_URI']); ?>";
} else { } else {
$thisURLRelativeToBase = strip_tags($_SERVER['REQUEST_URI']); $thisURLRelativeToBase = Convert::raw2att($_SERVER['REQUEST_URI']);
} }
$output = preg_replace('/(<a[^>]+href *= *)"#/i', '\\1"' . $thisURLRelativeToBase . '#', $output); $output = preg_replace('/(<a[^>]+href *= *)"#/i', '\\1"' . $thisURLRelativeToBase . '#', $output);