CVE-2014-1439: Other (NVD-CWE-Other)
value: The libxml_disable_entity_loader function in runtime/ext/ext_simplexml.cpp in HipHop Virtual Machine for PHP (HHVM) before 2.4.0 and 2.3.x before 2.3.3 does not properly disable a certain libxml handler, which allows remote attackers to conduct XML External Entity (XXE) attacks.
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
Published: 2014-02-05T19:55Z
Repository: https://github.com/facebook/hhvm
Language: PHP
Files changed: hphp/runtime/ext/ext_simplexml.cpp, hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php, hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php.expectf, hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php.skipif, hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader_payload.txt
Commit date: 2014-01-07 10:39:32 -0800
Commit message: Fix libxml_disable_entity_loader()

This wasn't calling requestInit and setting the libxml handler no null.
So the first time an error came along it would reset the handler from
no-op to reading again.

This is a much better fix, we set our custom handler in requestInit and
when libxml_disable_entity_loader we store that state as a member bool
ensuring requestInit is always called to set our own handler.

If the handler isn't inserted then the behavious is as before. The only
time this could go pear shaped is say we wanted to make the default be
off. In that case we'd need a global requestInit that is always called
since there are libxml references everywhere.

Reviewed By: @jdelong

Differential Revision: D1116686

Diff: diff --git a/hphp/runtime/ext/ext_simplexml.cpp b/hphp/runtime/ext/ext_simplexml.cpp
index 790e992a7..91060218b 100644
--- a/hphp/runtime/ext/ext_simplexml.cpp
+++ b/hphp/runtime/ext/ext_simplexml.cpp
@@ -1244,10 +1244,13 @@ void c_LibXMLError::t___construct() {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // libxml
 
+static xmlParserInputBufferPtr
+hphp_libxml_input_buffer(const char *URI, xmlCharEncoding enc);
+
 class xmlErrorVec : public std::vector<xmlError> {
 public:
   ~xmlErrorVec() {
     reset();
   }
@@ -1263,17 +1266,19 @@ public:
 class LibXmlErrors : public RequestEventHandler {
 public:
   virtual void requestInit() {
     m_use_error = false;
     m_errors.reset();
-    xmlParserInputBufferCreateFilenameDefault(nullptr);
+    m_entity_loader_disabled = false;
+    xmlParserInputBufferCreateFilenameDefault(hphp_libxml_input_buffer);
   }
   virtual void requestShutdown() {
     m_use_error = false;
     m_errors.reset();
   }
 
+  bool m_entity_loader_disabled;
   bool m_use_error;
   xmlErrorVec m_errors;
 };
 
 IMPLEMENT_STATIC_REQUEST_LOCAL(LibXmlErrors, s_libxml_errors);
@@ -1372,22 +1377,22 @@ bool f_libxml_use_internal_errors(CVarRef use_errors /* = null_variant */) {
   }
   return ret;
 }
 
 static xmlParserInputBufferPtr
-hphp_libxml_input_buffer_noload(const char *URI, xmlCharEncoding enc) {
-  return nullptr;
+hphp_libxml_input_buffer(const char *URI, xmlCharEncoding enc) {
+  if (s_libxml_errors->m_entity_loader_disabled) {
+    return nullptr;
+  }
+  return __xmlParserInputBufferCreateFilename(URI, enc);
 }
 
 bool f_libxml_disable_entity_loader(bool disable /* = true */) {
-  xmlParserInputBufferCreateFilenameFunc old;
+  bool old = s_libxml_errors->m_entity_loader_disabled;
 
-  if (disable) {
-    old = xmlParserInputBufferCreateFilenameDefault(hphp_libxml_input_buffer_noload);
-  } else {
-    old = xmlParserInputBufferCreateFilenameDefault(nullptr);
-  }
-  return (old == hphp_libxml_input_buffer_noload);
+  s_libxml_errors->m_entity_loader_disabled = disable;
+
+  return old;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 }
diff --git a/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php b/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php
new file mode 100644
index 000000000..95872eb89
--- /dev/null
+++ b/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php
@@ -0,0 +1,25 @@
+<?php
+
+$xml = <<<EOT
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE test [<!ENTITY xxe SYSTEM "XXE_URI">]>
+<foo>&xxe;</foo>
+EOT;
+
+$xml = str_replace('XXE_URI', __DIR__ . '/libxml_disable_entity_loader_payload.txt', $xml);
+
+function parseXML($xml) {
+  $doc = new DOMDocument();
+  $doc->resolveExternals = true;
+  $doc->substituteEntities = true;
+  $doc->validateOnParse = false;
+  $doc->loadXML($xml, 0);
+  return $doc->saveXML();
+}
+
+var_dump(strpos(parseXML($xml), 'SECRET_DATA') !== false);
+var_dump(libxml_disable_entity_loader(true));
+var_dump(strpos(parseXML($xml), 'SECRET_DATA') === false);
+
+echo "Done\n";
+?>
diff --git a/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php.expectf b/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php.expectf
new file mode 100644
index 000000000..54de637bc
--- /dev/null
+++ b/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php.expectf
@@ -0,0 +1,7 @@
+bool(true)
+bool(false)
+I/O warning : failed to load external entity "%s"
+HipHop Warning: %s
+HipHop Warning: %s
+bool(true)
+Done
diff --git a/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php.skipif b/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php.skipif
new file mode 100644
index 000000000..1086e1001
--- /dev/null
+++ b/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader.php.skipif
@@ -0,0 +1 @@
+<?php if (!extension_loaded('simplexml') || !extension_loaded('dom') || defined('PHP_WINDOWS_VERSION_MAJOR')) die('skip'); ?>
diff --git a/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader_payload.txt b/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader_payload.txt
new file mode 100644
index 000000000..3b8a43cc1
--- /dev/null
+++ b/hphp/test/zend/good/ext/libxml/tests/libxml_disable_entity_loader_payload.txt
@@ -0,0 +1 @@
+SECRET_DATA
\ No newline at end of file

Vulnerable code:     xmlParserInputBufferCreateFilenameDefault(nullptr);
hphp_libxml_input_buffer_noload(const char *URI, xmlCharEncoding enc) {
  return nullptr;
  xmlParserInputBufferCreateFilenameFunc old;
  if (disable) {
    old = xmlParserInputBufferCreateFilenameDefault(hphp_libxml_input_buffer_noload);
  } else {
    old = xmlParserInputBufferCreateFilenameDefault(nullptr);
  }
  return (old == hphp_libxml_input_buffer_noload);
Fixed code: static xmlParserInputBufferPtr
hphp_libxml_input_buffer(const char *URI, xmlCharEncoding enc);

    m_entity_loader_disabled = false;
    xmlParserInputBufferCreateFilenameDefault(hphp_libxml_input_buffer);
  bool m_entity_loader_disabled;
hphp_libxml_input_buffer(const char *URI, xmlCharEncoding enc) {
  if (s_libxml_errors->m_entity_loader_disabled) {
    return nullptr;
  }
  return __xmlParserInputBufferCreateFilename(URI, enc);
  bool old = s_libxml_errors->m_entity_loader_disabled;
  s_libxml_errors->m_entity_loader_disabled = disable;

  return old;
<?php

$xml = <<<EOT
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE test [<!ENTITY xxe SYSTEM "XXE_URI">]>
<foo>&xxe;</foo>
EOT;

$xml = str_replace('XXE_URI', __DIR__ . '/libxml_disable_entity_loader_payload.txt', $xml);

function parseXML($xml) {
  $doc = new DOMDocument();
  $doc->resolveExternals = true;
  $doc->substituteEntities = true;
  $doc->validateOnParse = false;
  $doc->loadXML($xml, 0);
  return $doc->saveXML();
}

var_dump(strpos(parseXML($xml), 'SECRET_DATA') !== false);
var_dump(libxml_disable_entity_loader(true));
var_dump(strpos(parseXML($xml), 'SECRET_DATA') === false);

echo "Done\n";
?>
bool(true)
bool(false)
I/O warning : failed to load external entity "%s"
HipHop Warning: %s
HipHop Warning: %s
bool(true)
Done
<?php if (!extension_loaded('simplexml') || !extension_loaded('dom') || defined('PHP_WINDOWS_VERSION_MAJOR')) die('skip'); ?>
SECRET_DATA