From 132e9b3e2fad361ebb4b502b6a37d34d013bfba3 Mon Sep 17 00:00:00 2001
From: Damian Mooyman <damian@silverstripe.com>
Date: Thu, 29 Oct 2015 11:53:44 +1300
Subject: [PATCH] [ss-2015-021] Fix rewrite hash links XSS

---
 tests/view/SSViewerTest.php   | 29 +++++++++++++++++++++++------
 view/SSTemplateParser.php     | 10 +++++++---
 view/SSTemplateParser.php.inc | 10 +++++++---
 view/SSViewer.php             |  6 +++---
 4 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php
index 39a3dd412..a816f6802 100644
--- a/tests/view/SSViewerTest.php
+++ b/tests/view/SSViewerTest.php
@@ -2,6 +2,13 @@
 
 class SSViewerTest extends SapphireTest {
 
+	/**
+	 * Backup of $_SERVER global
+	 *
+	 * @var array
+	 */
+	protected $oldServer = array();
+
 	protected $extraDataObjects = array(
 		'SSViewerTest_Object',
 	);
@@ -10,6 +17,12 @@ class SSViewerTest extends SapphireTest {
 		parent::setUp();
 		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();
 	}
 
 	/**
@@ -1156,10 +1169,13 @@ after')
 		$orig = Config::inst()->get('SSViewer', 'rewrite_hash_links'); 
 		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()
-		$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';
 
@@ -1227,10 +1243,11 @@ after')
 		$obj = new ViewableData();
 		$obj->InsertedLink = '<a class="inserted" href="#anchor">InsertedLink</a>';
 		$result = $tmpl->process($obj);
-		$this->assertContains(
-			'<a class="inserted" href="<?php echo Convert::raw2att(',
-			$result
-		);
+
+		$code = <<<'EOC'
+<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
 		// $this->assertContains(
 		// 	'<a class="inline" href="<?php echo str_replace(',
diff --git a/view/SSTemplateParser.php b/view/SSTemplateParser.php
index 62f38277c..f9eb592b7 100644
--- a/view/SSTemplateParser.php
+++ b/view/SSTemplateParser.php
@@ -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
 		// 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(
 			'/(<a[^>]+href *= *)"#/i',
-			'\\1"\' . (Config::inst()->get(\'SSViewer\', \'rewrite_hash_links\') ?' .
-			' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") .
-				\'#',
+			'\\1"\' . ' . addcslashes($code, '\\')  . ' . \'#',
 			$text
 		);
 
diff --git a/view/SSTemplateParser.php.inc b/view/SSTemplateParser.php.inc
index bc7640669..d4a654082 100644
--- a/view/SSTemplateParser.php.inc
+++ b/view/SSTemplateParser.php.inc
@@ -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
 		// 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(
 			'/(<a[^>]+href *= *)"#/i',
-			'\\1"\' . (Config::inst()->get(\'SSViewer\', \'rewrite_hash_links\') ?' .
-			' Convert::raw2att( $_SERVER[\'REQUEST_URI\'] ) : "") .
-				\'#',
+			'\\1"\' . ' . addcslashes($code, '\\')  . ' . \'#',
 			$text
 		);
 
diff --git a/view/SSViewer.php b/view/SSViewer.php
index f84343d84..9556906a7 100644
--- a/view/SSViewer.php
+++ b/view/SSViewer.php
@@ -1129,10 +1129,10 @@ class SSViewer implements Flushable {
 		$rewrite = Config::inst()->get('SSViewer', 'rewrite_hash_links');
 		if($this->rewriteHashlinks && $rewrite) {
 			if(strpos($output, '<base') !== false) {
-				if($rewrite === 'php') { 
-					$thisURLRelativeToBase = "<?php echo Convert::raw2att(\$_SERVER['REQUEST_URI']); ?>";
+				if($rewrite === 'php') {
+					$thisURLRelativeToBase = "<?php echo Convert::raw2att(preg_replace(\"/^(\\\\/)+/\", \"/\", \$_SERVER['REQUEST_URI'])); ?>";
 				} else { 
-					$thisURLRelativeToBase = Convert::raw2att($_SERVER['REQUEST_URI']);
+					$thisURLRelativeToBase = Convert::raw2att(preg_replace("/^(\\/)+/", "/", $_SERVER['REQUEST_URI']));
 				}
 
 				$output = preg_replace('/(<a[^>]+href *= *)"#/i', '\\1"' . $thisURLRelativeToBase . '#', $output);