Fixed XSS vulnerability relating to rewrite_hash

silverstripe · Mar 20, 2015 · 604c328 · 604c328
1 parent b336415
commit 604c328
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 7 deletions.
diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php
@@ -1131,8 +1131,10 @@ public function testThemeRetrieval() {
 
 	public function testRewriteHashlinks() {
 		$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\')""';
+
 		// Emulate SSViewer::process()
 		$base = Convert::raw2att($_SERVER['REQUEST_URI']);
 
@@ -1143,6 +1145,8 @@ public function testRewriteHashlinks() {
 			<html>
 				<head><% base_tag %></head>
 				<body>
+				<a class="external-inline" href="http://google.com#anchor">ExternalInlineLink</a>
+				$ExternalInsertedLink
 				<a class="inline" href="#anchor">InlineLink</a>
 				$InsertedLink
 				<svg><use xlink:href="#sprite"></use></svg>
@@ -1151,15 +1155,24 @@ public function testRewriteHashlinks() {
 		$tmpl = new SSViewer($tmplFile);
 		$obj = new ViewableData();
 		$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);
 		$this->assertContains(
 			'<a class="inserted" href="' . $base . '#anchor">InsertedLink</a>',
 			$result
 		);
+		$this->assertContains(
+			'<a class="external-inserted" href="http://google.com#anchor">ExternalInsertedLink</a>',
+			$result
+		);
 		$this->assertContains(
 			'<a class="inline" href="' . $base . '#anchor">InlineLink</a>',
 			$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,
@@ -1192,7 +1205,7 @@ public function testRewriteHashlinksInPhpMode() {
 		$obj->InsertedLink = '<a class="inserted" href="#anchor">InsertedLink</a>';
 		$result = $tmpl->process($obj);
 		$this->assertContains(
-			'<a class="inserted" href="<?php echo strip_tags(',
+			'<a class="inserted" href="<?php echo Convert::raw2att(',
 			$result
 		);
 		// TODO Fix inline links in PHP mode

diff --git a/view/SSTemplateParser.php b/view/SSTemplateParser.php
@@ -4684,7 +4684,7 @@ function Text__finalise(&$res) {
 		$text = preg_replace(
 			'/(<a[^>]+href *= *)"#/i',
 			'\\1"\' . (Config::inst()->get(\'SSViewer\', \'rewrite_hash_links\') ?' .
-			' strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . 
+			' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") .
 				\'#',
 			$text
 		);

diff --git a/view/SSTemplateParser.php.inc b/view/SSTemplateParser.php.inc
@@ -1138,7 +1138,7 @@ class SSTemplateParser extends Parser implements TemplateParser {
 		$text = preg_replace(
 			'/(<a[^>]+href *= *)"#/i',
 			'\\1"\' . (Config::inst()->get(\'SSViewer\', \'rewrite_hash_links\') ?' .
-			' strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . 
+			' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") .
 				\'#',
 			$text
 		);

diff --git a/view/SSViewer.php b/view/SSViewer.php
@@ -1109,9 +1109,9 @@ public function process($item, $arguments = null, $inheritedScope = null) {
 		if($this->rewriteHashlinks && $rewrite) {
 			if(strpos($output, '<base') !== false) {
 				if($rewrite === 'php') { 
-					$thisURLRelativeToBase = "<?php echo strip_tags(\$_SERVER['REQUEST_URI']); ?>"; 
+					$thisURLRelativeToBase = "<?php echo Convert::raw2att(\$_SERVER['REQUEST_URI']); ?>";
 				} else { 
-					$thisURLRelativeToBase = strip_tags($_SERVER['REQUEST_URI']); 
+					$thisURLRelativeToBase = Convert::raw2att($_SERVER['REQUEST_URI']);
 				}
 
 				$output = preg_replace('/(<a[^>]+href *= *)"#/i', '\\1"' . $thisURLRelativeToBase . '#', $output);