BUG Fix SS-2014-017

silverstripe · Mar 20, 2015 · 7f983c2 · 7f983c2
1 parent 80fc55d
commit 7f983c2
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 5 deletions.
diff --git a/core/Convert.php b/core/Convert.php
@@ -167,15 +167,32 @@ public static function json2array($val) {
 
 	/**
 	 * 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
@@ -238,4 +238,55 @@ public function testRaw2JSON() {
 			Convert::raw2json($value)
 		);
 	}
+
+	public function testXML2Array() {
+		// Ensure an XML file at risk of entity expansion can be avoided safely
+		$inputXML = <<<XML
+<?xml version="1.0"?>
+<!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING">]>
+<results>
+    <result>Now include &long; lots of times to expand the in-memory size of this XML structure</result>
+    <result>&long;&long;&long;</result>
+</results>
+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
+		);
+	}
 }