aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPaweł Sroka <srokap@gmail.com>2014-01-01 13:12:24 +0100
committerPaweł Sroka <srokap@gmail.com>2014-01-01 13:12:24 +0100
commit53509917fd2119e17209179aae6d54b64dd2d244 (patch)
treeaac2e883578b78796686728ae3beed5b2a35a9a4
parent7006294fcbfab450289403b6519edb9d5d30ff35 (diff)
parent7cacdc8bc26c98a58dc8986acfd911d6542608af (diff)
downloadelgg-53509917fd2119e17209179aae6d54b64dd2d244.tar.gz
elgg-53509917fd2119e17209179aae6d54b64dd2d244.tar.bz2
Merged in libxml18 (pull request #8)
Disable loading external entities during XML parsing
-rw-r--r--engine/classes/ElggAutoP.php14
-rw-r--r--engine/classes/ElggXMLElement.php8
-rw-r--r--engine/tests/regression/trac_bugs.php29
-rw-r--r--engine/tests/test_files/xxe/external_entity.txt1
-rw-r--r--engine/tests/test_files/xxe/request.xml8
5 files changed, 58 insertions, 2 deletions
diff --git a/engine/classes/ElggAutoP.php b/engine/classes/ElggAutoP.php
index 71536c433..05842d1b2 100644
--- a/engine/classes/ElggAutoP.php
+++ b/engine/classes/ElggAutoP.php
@@ -110,12 +110,19 @@ class ElggAutoP {
// http://www.php.net/manual/en/domdocument.loadhtml.php#95463
libxml_use_internal_errors(true);
+ // Do not load entities. May be unnecessary, better safe than sorry
+ $disable_load_entities = libxml_disable_entity_loader(true);
+
if (!$this->_doc->loadHTML("<html><meta http-equiv='content-type' "
. "content='text/html; charset={$this->encoding}'><body>{$html}</body>"
. "</html>")) {
+
+ libxml_disable_entity_loader($disable_load_entities);
return false;
}
+ libxml_disable_entity_loader($disable_load_entities);
+
$this->_xpath = new DOMXPath($this->_doc);
// start processing recursively at the BODY element
$nodeList = $this->_xpath->query('//body[1]');
@@ -135,9 +142,16 @@ class ElggAutoP {
// re-parse so we can handle new AUTOP elements
+ // Do not load entities. May be unnecessary, better safe than sorry
+ $disable_load_entities = libxml_disable_entity_loader(true);
+
if (!$this->_doc->loadHTML($html)) {
+ libxml_disable_entity_loader($disable_load_entities);
return false;
}
+
+ libxml_disable_entity_loader($disable_load_entities);
+
// must re-create XPath object after DOM load
$this->_xpath = new DOMXPath($this->_doc);
diff --git a/engine/classes/ElggXMLElement.php b/engine/classes/ElggXMLElement.php
index 6f2633e25..cbd3fc5ce 100644
--- a/engine/classes/ElggXMLElement.php
+++ b/engine/classes/ElggXMLElement.php
@@ -20,7 +20,12 @@ class ElggXMLElement {
if ($xml instanceof SimpleXMLElement) {
$this->_element = $xml;
} else {
+ // do not load entities
+ $disable_load_entities = libxml_disable_entity_loader(true);
+
$this->_element = new SimpleXMLElement($xml);
+
+ libxml_disable_entity_loader($disable_load_entities);
}
}
@@ -123,5 +128,4 @@ class ElggXMLElement {
}
return false;
}
-
-} \ No newline at end of file
+}
diff --git a/engine/tests/regression/trac_bugs.php b/engine/tests/regression/trac_bugs.php
index ef1348cf6..689275661 100644
--- a/engine/tests/regression/trac_bugs.php
+++ b/engine/tests/regression/trac_bugs.php
@@ -373,4 +373,33 @@ class ElggCoreRegressionBugsTest extends ElggCoreUnitTest {
//delete group and annotations
$group->delete();
}
+
+ public function test_ElggXMLElement_does_not_load_external_entities() {
+ $elLast = libxml_disable_entity_loader(false);
+
+ // build payload that should trigger loading of external entity
+ $payload = file_get_contents(dirname(dirname(__FILE__)) . '/test_files/xxe/request.xml');
+ $path = realpath(dirname(dirname(__FILE__)) . '/test_files/xxe/external_entity.txt');
+ $path = str_replace('\\', '/', $path);
+ if ($path[0] != '/') {
+ $path = '/' . $path;
+ }
+ $path = 'file://' . $path;
+ $payload = sprintf($payload, $path);
+
+ // make sure we can actually this in this environment
+ $element = new SimpleXMLElement($payload);
+ $can_load_entity = preg_match('/secret/', (string)$element->methodName);
+
+ $this->skipUnless($can_load_entity, "XXE vulnerability cannot be tested on this system");
+
+ if ($can_load_entity) {
+ $el = new ElggXMLElement($payload);
+ $chidren = $el->getChildren();
+ $content = $chidren[0]->getContent();
+ $this->assertNoPattern('/secret/', $content);
+ }
+
+ libxml_disable_entity_loader($elLast);
+ }
}
diff --git a/engine/tests/test_files/xxe/external_entity.txt b/engine/tests/test_files/xxe/external_entity.txt
new file mode 100644
index 000000000..536aca34d
--- /dev/null
+++ b/engine/tests/test_files/xxe/external_entity.txt
@@ -0,0 +1 @@
+secret \ No newline at end of file
diff --git a/engine/tests/test_files/xxe/request.xml b/engine/tests/test_files/xxe/request.xml
new file mode 100644
index 000000000..4390f9db2
--- /dev/null
+++ b/engine/tests/test_files/xxe/request.xml
@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<!DOCTYPE foo [
+<!ELEMENT methodName ANY >
+<!ENTITY xxe SYSTEM "%s" >
+]>
+<methodCall>
+ <methodName>test&xxe;test</methodName>
+</methodCall>