From bdef4fc7a548c7c243ff86f2db7c16f301a6f120 Mon Sep 17 00:00:00 2001 From: Christopher Pitt Date: Fri, 20 Mar 2015 18:17:51 +1300 Subject: [PATCH] Fixed XSS vulnerability relating to rewrite_hash --- tests/view/SSViewerTest.php | 25 ++++++++++++++++++++++--- view/SSTemplateParser.php | 5 +++-- view/SSTemplateParser.php.inc | 5 +++-- view/SSViewer.php | 4 ++-- 4 files changed, 30 insertions(+), 9 deletions(-) diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php index 046bb4b68..060fac662 100644 --- a/tests/view/SSViewerTest.php +++ b/tests/view/SSViewerTest.php @@ -909,7 +909,9 @@ after') public function testRewriteHashlinks() { $oldRewriteHashLinks = SSViewer::getOption('rewriteHashlinks'); SSViewer::setOption('rewriteHashlinks', true); - + + $_SERVER['REQUEST_URI'] = 'http://path/to/file?foo"onclick="alert(\'xss\')""'; + // Emulate SSViewer::process() $base = Convert::raw2att($_SERVER['REQUEST_URI']); @@ -920,23 +922,40 @@ after') <% base_tag %> + ExternalInlineLink + $ExternalInsertedLink InlineLink $InsertedLink + '); $tmpl = new SSViewer($tmplFile); $obj = new ViewableData(); $obj->InsertedLink = 'InsertedLink'; + $obj->ExternalInsertedLink = 'ExternalInsertedLink'; $result = $tmpl->process($obj); $this->assertContains( 'InsertedLink', $result ); + $this->assertContains( + 'ExternalInsertedLink', + $result + ); $this->assertContains( 'InlineLink', $result ); - + $this->assertContains( + 'ExternalInlineLink', + $result + ); + $this->assertContains( + '', + $result, + 'SSTemplateParser should only rewrite anchor hrefs' + ); + unlink($tmplFile); SSViewer::setOption('rewriteHashlinks', $oldRewriteHashLinks); @@ -962,7 +981,7 @@ after') $obj->InsertedLink = 'InsertedLink'; $result = $tmpl->process($obj); $this->assertContains( - ']+href *= *)"#/i', + '\\1"\' . (SSViewer::$options[\'rewriteHashlinks\'] ?' . + ' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") . \'#', $text ); diff --git a/view/SSTemplateParser.php.inc b/view/SSTemplateParser.php.inc index e58de6031..039e22a9d 100644 --- a/view/SSTemplateParser.php.inc +++ b/view/SSTemplateParser.php.inc @@ -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 // non-dynamically calculated $text = preg_replace( - '/href\s*\=\s*\"\#/', - 'href="\' . (SSViewer::$options[\'rewriteHashlinks\'] ? strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . + '/(]+href *= *)"#/i', + '\\1"\' . (SSViewer::$options[\'rewriteHashlinks\'] ?' . + ' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") . \'#', $text ); diff --git a/view/SSViewer.php b/view/SSViewer.php index 9c02253f3..3e43ab0d5 100644 --- a/view/SSViewer.php +++ b/view/SSViewer.php @@ -928,9 +928,9 @@ class SSViewer { if($this->rewriteHashlinks && self::$options['rewriteHashlinks']) { if(strpos($output, ']+href *= *)"#/i', '\\1"' . $thisURLRelativeToBase . '#', $output);