From d53447f7e6b3277f3249d9a70e56ec01a90c3a60 Mon Sep 17 00:00:00 2001 From: Steve Clay Date: Thu, 11 Jul 2013 13:24:01 -0400 Subject: Disable loading external entities during XML parsing --- engine/classes/ElggAutoP.php | 14 ++++++++++++++ engine/classes/ElggXMLElement.php | 8 ++++++-- engine/tests/regression/trac_bugs.php | 10 ++++++++++ engine/tests/test_files/xxe/external_entity.txt | 1 + engine/tests/test_files/xxe/request.xml | 8 ++++++++ 5 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 engine/tests/test_files/xxe/external_entity.txt create mode 100644 engine/tests/test_files/xxe/request.xml 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}" . "")) { + + 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..e6773c8af 100644 --- a/engine/tests/regression/trac_bugs.php +++ b/engine/tests/regression/trac_bugs.php @@ -373,4 +373,14 @@ class ElggCoreRegressionBugsTest extends ElggCoreUnitTest { //delete group and annotations $group->delete(); } + + public function test_ElggXMLElement_does_not_load_external_entities() { + $payload = file_get_contents(dirname(dirname(__FILE__)) . '/test_files/xxe/request.xml'); + $payload = sprintf($payload, 'file://' . realpath(dirname(dirname(__FILE__)) . '/test_files/xxe/external_entity.txt')); + + $el = new ElggXMLElement($payload); + $chidren = $el->getChildren(); + $content = $chidren[0]->getContent(); + $this->assertNoPattern('/secret/', $content); + } } 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 @@ + + + +]> + + test&xxe;test + -- cgit v1.2.3 From 6eec301f33ff3e618d591d429de7edf30277e972 Mon Sep 17 00:00:00 2001 From: Paweł Sroka Date: Tue, 23 Jul 2013 08:28:30 +0200 Subject: Enhanced test --- engine/tests/regression/trac_bugs.php | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/engine/tests/regression/trac_bugs.php b/engine/tests/regression/trac_bugs.php index e6773c8af..ea39253df 100644 --- a/engine/tests/regression/trac_bugs.php +++ b/engine/tests/regression/trac_bugs.php @@ -375,12 +375,26 @@ class ElggCoreRegressionBugsTest extends ElggCoreUnitTest { } public function test_ElggXMLElement_does_not_load_external_entities() { + $elLast = libxml_disable_entity_loader(false); + $payload = file_get_contents(dirname(dirname(__FILE__)) . '/test_files/xxe/request.xml'); - $payload = sprintf($payload, 'file://' . realpath(dirname(dirname(__FILE__)) . '/test_files/xxe/external_entity.txt')); + $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); $el = new ElggXMLElement($payload); $chidren = $el->getChildren(); $content = $chidren[0]->getContent(); $this->assertNoPattern('/secret/', $content); + + //make sure the test is valid + $element = new SimpleXMLElement($payload); + $this->assertPattern('/secret/', (string)$element->methodName); + + libxml_disable_entity_loader($elLast); } } -- cgit v1.2.3 From 7cacdc8bc26c98a58dc8986acfd911d6542608af Mon Sep 17 00:00:00 2001 From: Steve Clay Date: Wed, 31 Jul 2013 13:34:55 -0400 Subject: Emit notice if XXE can't be tested and skip test --- engine/tests/regression/trac_bugs.php | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/engine/tests/regression/trac_bugs.php b/engine/tests/regression/trac_bugs.php index ea39253df..689275661 100644 --- a/engine/tests/regression/trac_bugs.php +++ b/engine/tests/regression/trac_bugs.php @@ -377,6 +377,7 @@ class ElggCoreRegressionBugsTest extends ElggCoreUnitTest { 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); @@ -384,16 +385,20 @@ class ElggCoreRegressionBugsTest extends ElggCoreUnitTest { $path = '/' . $path; } $path = 'file://' . $path; - $payload = sprintf($payload, $path); + $payload = sprintf($payload, $path); - $el = new ElggXMLElement($payload); - $chidren = $el->getChildren(); - $content = $chidren[0]->getContent(); - $this->assertNoPattern('/secret/', $content); - - //make sure the test is valid + // make sure we can actually this in this environment $element = new SimpleXMLElement($payload); - $this->assertPattern('/secret/', (string)$element->methodName); + $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); } -- cgit v1.2.3