From ee9bddb808df6d27db4d56bb5d522dcfe6788715 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 20 Mar 2015 17:30:37 +1300 Subject: [PATCH 1/3] BUG Fix SS-2015-010 --- control/Director.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/control/Director.php b/control/Director.php index cc28ec8f2..7651ec2ef 100644 --- a/control/Director.php +++ b/control/Director.php @@ -796,14 +796,10 @@ class Director implements TemplateGlobalProvider { * @param string $destURL - The URL to redirect to */ protected static function force_redirect($destURL) { - $response = new SS_HTTPResponse( - "

Your browser is not accepting header redirects

". - "

Please click here", - 301 - ); + $response = new SS_HTTPResponse(); + $response->redirect($destURL, 301); HTTP::add_cache_headers($response); - $response->addHeader('Location', $destURL); // TODO: Use an exception - ATM we can be called from _config.php, before Director#handleRequest's try block $response->output(); From 604c32871202064a4aa12c3b3fd58140231685e5 Mon Sep 17 00:00:00 2001 From: Christopher Pitt Date: Fri, 20 Mar 2015 18:17:51 +1300 Subject: [PATCH 2/3] Fixed XSS vulnerability relating to rewrite_hash --- tests/view/SSViewerTest.php | 19 ++++++++++++++++--- view/SSTemplateParser.php | 2 +- view/SSTemplateParser.php.inc | 2 +- view/SSViewer.php | 4 ++-- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php index d8349c50d..550eaa046 100644 --- a/tests/view/SSViewerTest.php +++ b/tests/view/SSViewerTest.php @@ -1131,8 +1131,10 @@ after') 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 @@ after') <% base_tag %> + ExternalInlineLink + $ExternalInsertedLink InlineLink $InsertedLink @@ -1151,15 +1155,24 @@ after') $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, @@ -1192,7 +1205,7 @@ after') $obj->InsertedLink = 'InsertedLink'; $result = $tmpl->process($obj); $this->assertContains( - '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 index 74fc27bbe..6bb7c550d 100644 --- a/view/SSTemplateParser.php.inc +++ b/view/SSTemplateParser.php.inc @@ -1138,7 +1138,7 @@ class SSTemplateParser extends Parser implements TemplateParser { $text = preg_replace( '/(]+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 index 8a4ebf052..d5fdb68bb 100644 --- a/view/SSViewer.php +++ b/view/SSViewer.php @@ -1109,9 +1109,9 @@ class SSViewer implements Flushable { if($this->rewriteHashlinks && $rewrite) { if(strpos($output, ']+href *= *)"#/i', '\\1"' . $thisURLRelativeToBase . '#', $output); From 7f983c2bae1dc78ca7217e9af364b2fb71dcefe8 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 20 Mar 2015 17:21:59 +1300 Subject: [PATCH 3/3] BUG Fix SS-2014-017 --- core/Convert.php | 27 ++++++++++++++++---- tests/core/ConvertTest.php | 51 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/core/Convert.php b/core/Convert.php index 819d2abd0..89a4367a6 100644 --- a/core/Convert.php +++ b/core/Convert.php @@ -167,15 +167,32 @@ class Convert { /** * Converts an XML string to a PHP array + * See http://phpsecurity.readthedocs.org/en/latest/Injection-Attacks.html#xml-external-entity-injection * * @uses recursiveXMLToArray() - * @param string - * + * @param string $val + * @param boolean $disableDoctypes Disables the use of DOCTYPE, and will trigger an error if encountered. + * false by default. + * @param boolean $disableExternals Disables the loading of external entities. false by default. * @return array */ - public static function xml2array($val) { - $xml = new SimpleXMLElement($val); - return self::recursiveXMLToArray($xml); + public static function xml2array($val, $disableDoctypes = false, $disableExternals = false) { + // Check doctype + if($disableDoctypes && preg_match('/\<\!DOCTYPE.+]\>/', $val)) { + throw new InvalidArgumentException('XML Doctype parsing disabled'); + } + + // Disable external entity loading + if($disableExternals) $oldVal = libxml_disable_entity_loader($disableExternals); + try { + $xml = new SimpleXMLElement($val); + $result = self::recursiveXMLToArray($xml); + } catch(Exception $ex) { + if($disableExternals) libxml_disable_entity_loader($oldVal); + throw $ex; + } + if($disableExternals) libxml_disable_entity_loader($oldVal); + return $result; } /** diff --git a/tests/core/ConvertTest.php b/tests/core/ConvertTest.php index 35141940d..baffd69c2 100644 --- a/tests/core/ConvertTest.php +++ b/tests/core/ConvertTest.php @@ -238,4 +238,55 @@ class ConvertTest extends SapphireTest { Convert::raw2json($value) ); } + + public function testXML2Array() { + // Ensure an XML file at risk of entity expansion can be avoided safely + $inputXML = << +]> + + Now include &long; lots of times to expand the in-memory size of this XML structure + &long;&long;&long; + +XML + ; + try { + Convert::xml2array($inputXML, true); + } catch(Exception $ex) {} + $this->assertTrue( + isset($ex) + && $ex instanceof InvalidArgumentException + && $ex->getMessage() === 'XML Doctype parsing disabled' + ); + + // Test without doctype validation + $expected = array( + 'result' => array( + "Now include SOME_SUPER_LONG_STRING lots of times to expand the in-memory size of this XML structure", + array( + 'long' => array( + array( + 'long' => 'SOME_SUPER_LONG_STRING' + ), + array( + 'long' => 'SOME_SUPER_LONG_STRING' + ), + array( + 'long' => 'SOME_SUPER_LONG_STRING' + ) + ) + ) + ) + ); + $result = Convert::xml2array($inputXML, false, true); + $this->assertEquals( + $expected, + $result + ); + $result = Convert::xml2array($inputXML, false, false); + $this->assertEquals( + $expected, + $result + ); + } }