CVE-2023-38490: Improper Restriction of Recursive Entity References in DTDs ('XML Entity Expansion') (CWE-776)
value: Kirby is a content management system. A vulnerability in versions prior to 3.5.8.3, 3.6.6.3, 3.7.5.2, 3.8.4.1, and 3.9.6 only affects Kirby sites that use the `Xml` data handler (e.g. `Data::decode($string, 'xml')`) or the `Xml::parse()` method in site or plugin code. The Kirby core does not use any of the affected methods.

XML External Entities (XXE) is a little used feature in the XML markup language that allows to include data from external files in an XML structure. If the name of the external file can be controlled by an attacker, this becomes a vulnerability that can be abused for various system impacts like the disclosure of internal or confidential data that is stored on the server (arbitrary file disclosure) or to perform network requests on behalf of the server (server-side request forgery, SSRF).

Kirby's `Xml::parse()` method used PHP's `LIBXML_NOENT` constant, which enabled the processing of XML external entities during the parsing operation. The `Xml::parse()` method is used in the `Xml` data handler (e.g. `Data::decode($string, 'xml')`). Both the vulnerable method and the data handler are not used in the Kirby core. However they may be used in site or plugin code, e.g. to parse RSS feeds or other XML files. If those files are of an external origin (e.g. uploaded by a user or retrieved from an external URL), attackers may be able to include an external entity in the XML file that will then be processed in the parsing process. Kirby sites that don't use XML parsing in site or plugin code are *not* affected.

The problem has been patched in Kirby 3.5.8.3, 3.6.6.3, 3.7.5.2, 3.8.4.1, and 3.9.6. In all of the mentioned releases, the maintainers have removed the `LIBXML_NOENT` constant as processing of external entities is out of scope of the parsing logic. This protects all uses of the method against the described vulnerability.
CWE description: The product uses XML documents and allows their structure to be defined with a Document Type Definition (DTD), but it does not properly control the number of recursive definitions of entities.
CVSS v3 base score: 10
Published: 2023-07-27T15:15Z
Repository: https://github.com/getkirby/kirby
Language: PHP
Files changed: src/Toolkit/Xml.php, tests/Toolkit/XmlTest.php, tests/Toolkit/fixtures/xml/billion-laughs.xml
Commit date: 2023-07-26 21:17:35 +0200
Commit message: Fix XXE vulnerability

https://github.com/getkirby/kirby/security/advisories/GHSA-q386-w6fg-gmgp

Diff: diff --git a/src/Toolkit/Xml.php b/src/Toolkit/Xml.php
index 011fada8d..a4954b772 100644
--- a/src/Toolkit/Xml.php
+++ b/src/Toolkit/Xml.php
@@ -269,11 +269,11 @@ class Xml
 	 *
 	 * @return array|null Parsed array or `null` on error
 	 */
 	public static function parse(string $xml): array|null
 	{
-		$xml = @simplexml_load_string($xml, 'SimpleXMLElement', LIBXML_NOENT);
+		$xml = @simplexml_load_string($xml);
 
 		if (is_object($xml) !== true) {
 			return null;
 		}
 
diff --git a/tests/Toolkit/XmlTest.php b/tests/Toolkit/XmlTest.php
index 6c0c6c988..7959e02e5 100644
--- a/tests/Toolkit/XmlTest.php
+++ b/tests/Toolkit/XmlTest.php
@@ -137,10 +137,47 @@ class XmlTest extends TestCase
 		$this->assertStringEqualsFile($fixtures . '/simpsons.xml', Xml::create($data, 'simpsons'));
 
 		$this->assertNull(Xml::parse('<this>is invalid</that>'));
 	}
 
+	/**
+	 * @covers ::parse
+	 */
+	public function testParseEntities()
+	{
+		$xml   = '<!DOCTYPE d [<!ENTITY e "bar">]><x>this is a file: foo &e; (with entities)</x>';
+		$array = Xml::parse($xml);
+
+		$this->assertSame([
+			'@name' => 'x',
+			'@value' => 'this is a file: foo bar (with entities)'
+		], $array);
+	}
+
+	/**
+	 * @covers ::parse
+	 */
+	public function testParseRecursiveEntities()
+	{
+		$xml = file_get_contents(__DIR__ . '/fixtures/xml/billion-laughs.xml');
+		$this->assertNull(Xml::parse($xml));
+	}
+
+	/**
+	 * @covers ::parse
+	 */
+	public function testParseXXE()
+	{
+		$xml   = '<!DOCTYPE d [<!ENTITY e SYSTEM "' . __FILE__ . '">]><x>this is a file: &e; with an XXE vulnerability</x>';
+		$array = Xml::parse($xml);
+
+		$this->assertSame([
+			'@name' => 'x',
+			'@value' => 'this is a file:  with an XXE vulnerability'
+		], $array);
+	}
+
 	/**
 	 * @covers ::encode
 	 * @covers ::decode
 	 */
 	public function testEncodeDecode()
diff --git a/tests/Toolkit/fixtures/xml/billion-laughs.xml b/tests/Toolkit/fixtures/xml/billion-laughs.xml
new file mode 100644
index 000000000..6cdc29cf2
--- /dev/null
+++ b/tests/Toolkit/fixtures/xml/billion-laughs.xml
@@ -0,0 +1,9 @@
+<!DOCTYPE xml [
+  <!ENTITY lol "lol">
+  <!ENTITY lol2 "&lol;&lol;&lol;&lol;&lol;&lol;">
+  <!ENTITY lol3 "&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;">
+  <!ENTITY lol4 "&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;">
+]>
+<xml>
+  <p>&lol4;</p>
+</xml>

Vulnerable code: 		$xml = @simplexml_load_string($xml, 'SimpleXMLElement', LIBXML_NOENT);
Fixed code: 		$xml = @simplexml_load_string($xml);
	/**
	 * @covers ::parse
	 */
	public function testParseEntities()
	{
		$xml   = '<!DOCTYPE d [<!ENTITY e "bar">]><x>this is a file: foo &e; (with entities)</x>';
		$array = Xml::parse($xml);

		$this->assertSame([
			'@name' => 'x',
			'@value' => 'this is a file: foo bar (with entities)'
		], $array);
	}

	/**
	 * @covers ::parse
	 */
	public function testParseRecursiveEntities()
	{
		$xml = file_get_contents(__DIR__ . '/fixtures/xml/billion-laughs.xml');
		$this->assertNull(Xml::parse($xml));
	}

	/**
	 * @covers ::parse
	 */
	public function testParseXXE()
	{
		$xml   = '<!DOCTYPE d [<!ENTITY e SYSTEM "' . __FILE__ . '">]><x>this is a file: &e; with an XXE vulnerability</x>';
		$array = Xml::parse($xml);

		$this->assertSame([
			'@name' => 'x',
			'@value' => 'this is a file:  with an XXE vulnerability'
		], $array);
	}

<!DOCTYPE xml [
  <!ENTITY lol "lol">
  <!ENTITY lol2 "&lol;&lol;&lol;&lol;&lol;&lol;">
  <!ENTITY lol3 "&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;">
  <!ENTITY lol4 "&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;">
]>
<xml>
  <p>&lol4;</p>
</xml>