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

[ss-2015-021] Fix rewrite hash links XSS

Browse Source
This commit is contained in:
Damian Mooyman 2015-10-29 11:53:44 +13:00
parent e68eb7e45d
commit 97f21fddb3
4 changed files with 47 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
tests/view/SSViewerTest.php
View File

@ -1,9 +1,28 @@
<?php <?php
class SSViewerTest extends SapphireTest { class SSViewerTest extends SapphireTest {
/**
* Backup of $_SERVER global
*
* @var array
*/
protected $oldServer = array();
protected $extraDataObjects = array(
'SSViewerTest_Object',
);
public function setUp() { public function setUp() {
parent::setUp(); parent::setUp();
Config::inst()->update('SSViewer', 'source_file_comments', false); Config::inst()->update('SSViewer', 'source_file_comments', false);
Config::inst()->update('SSViewer_FromString', 'cache_template', false);
$this->oldServer = $_SERVER;
}
public function tearDown() {
$_SERVER = $this->oldServer;
parent::tearDown();
} }
/** /**
@ -1160,10 +1179,13 @@ after')
$orig = Config::inst()->get('SSViewer', 'rewrite_hash_links'); $orig = Config::inst()->get('SSViewer', 'rewrite_hash_links');
Config::inst()->update('SSViewer', 'rewrite_hash_links', true); Config::inst()->update('SSViewer', 'rewrite_hash_links', true);
$_SERVER['REQUEST_URI'] = 'http://path/to/file?foo"onclick="alert(\'xss\')""'; $_SERVER['HTTP_HOST'] = 'www.mysite.com';
$_SERVER['REQUEST_URI'] = '//file.com?foo"onclick="alert(\'xss\')""';
// Emulate SSViewer::process() // Emulate SSViewer::process()
$base = Convert::raw2att($_SERVER['REQUEST_URI']); // Note that leading double slashes have been rewritten to prevent these being mis-interepreted
// as protocol-less absolute urls
$base = Convert::raw2att('/file.com?foo"onclick="alert(\'xss\')""');
$tmplFile = TEMP_FOLDER . '/SSViewerTest_testRewriteHashlinks_' . sha1(rand()) . '.ss'; $tmplFile = TEMP_FOLDER . '/SSViewerTest_testRewriteHashlinks_' . sha1(rand()) . '.ss';
@ -1231,10 +1253,11 @@ after')
$obj = new ViewableData(); $obj = new ViewableData();
$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(
'<a class="inserted" href="<?php echo Convert::raw2att(', $code = <<<'EOC'
$result <a class="inserted" href="<?php echo Convert::raw2att(preg_replace("/^(\/)+/", "/", $_SERVER['REQUEST_URI'])); ?>#anchor">InsertedLink</a>
); EOC;
$this->assertContains($code, $result);
// TODO Fix inline links in PHP mode // TODO Fix inline links in PHP mode
// $this->assertContains( // $this->assertContains(
// '<a class="inline" href="<?php echo str_replace(', // '<a class="inline" href="<?php echo str_replace(',

10
view/SSTemplateParser.php
View File

@ -4681,11 +4681,15 @@ class SSTemplateParser extends Parser implements TemplateParser {
// 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
$code = <<<'EOC'
(\Config::inst()->get('SSViewer', 'rewrite_hash_links')
? \Convert::raw2att( preg_replace("/^(\\/)+/", "/", $_SERVER['REQUEST_URI'] ) )
: "")
EOC;
// Because preg_replace replacement requires escaped slashes, addcslashes here
$text = preg_replace( $text = preg_replace(
'/(<a[^>]+href *= *)"#/i', '/(<a[^>]+href *= *)"#/i',
'\\1"\' . (Config::inst()->get(\'SSViewer\', \'rewrite_hash_links\') ?' . '\\1"\' . ' . addcslashes($code, '\\') . ' . \'#',
' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") .
\'#',
$text $text
); );

10
view/SSTemplateParser.php.inc
View File

@ -1135,11 +1135,15 @@ class SSTemplateParser extends Parser implements TemplateParser {
// 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
$code = <<<'EOC'
(\Config::inst()->get('SSViewer', 'rewrite_hash_links')
? \Convert::raw2att( preg_replace("/^(\\/)+/", "/", $_SERVER['REQUEST_URI'] ) )
: "")
EOC;
// Because preg_replace replacement requires escaped slashes, addcslashes here
$text = preg_replace( $text = preg_replace(
'/(<a[^>]+href *= *)"#/i', '/(<a[^>]+href *= *)"#/i',
'\\1"\' . (Config::inst()->get(\'SSViewer\', \'rewrite_hash_links\') ?' . '\\1"\' . ' . addcslashes($code, '\\') . ' . \'#',
' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") .
\'#',
$text $text
); );

4
view/SSViewer.php
View File

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