Permalink
Browse files

disallow XML document types

  • Loading branch information...
schmittjoh committed Aug 27, 2012
1 parent 1f308a5 commit 5a9b7e6b62143950726f642da23d7eebf850e389
Showing with 24 additions and 14 deletions.
  1. +9 −0 Serializer/XmlDeserializationVisitor.php
  2. +15 −14 Tests/Serializer/XmlSerializationTest.php
@@ -62,6 +62,15 @@ public function prepare($data)
{
$previous = libxml_use_internal_errors(true);
$previousEntityLoaderState = libxml_disable_entity_loader($this->disableExternalEntities);
+
+ $dom = new \DOMDocument();
+ $dom->loadXML($data);
+ foreach ($dom->childNodes as $child) {
+ if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
+ throw new \InvalidArgumentException('Document types are not allowed.');
+ }
+ }
+
$doc = simplexml_load_string($data);
libxml_use_internal_errors($previous);
libxml_disable_entity_loader($previousEntityLoaderState);
@@ -63,20 +63,28 @@ public function testPropertyIsCollectionOfObjectsWithAttributeAndValue()
$this->assertEquals($this->getContent('person_collection'), $this->serialize($personCollection));
}
+ /**
+ * @expectedException \InvalidArgumentException
+ * @expectedExceptionMessage Document types are not allowed
+ */
public function testExternalEntitiesAreDisabledByDefault()
{
- $currentDir = getcwd();
- chdir(__DIR__);
- $entity = $this->deserialize('<?xml version="1.0"?>
+ $this->deserialize('<?xml version="1.0"?>
<!DOCTYPE author [
<!ENTITY foo SYSTEM "php://filter/read=convert.base64-encode/resource='.basename(__FILE__).'">
]>
<result>
&foo;
- </result>', 'JMS\SerializerBundle\Tests\Serializer\ExternalEntityTest');
- chdir($currentDir);
+ </result>', 'stdClass');
+ }
- $this->assertEquals('', trim($entity->foo));
+ /**
+ * @expectedException \InvalidArgumentException
+ * @expectedExceptionMessage Document types are not allowed
+ */
+ public function testDocumentTypesAreNotAllowed()
+ {
+ $this->deserialize('<?xml version="1.0"?><!DOCTYPE foo><foo></foo>', 'stdClass');
}
public function testVirtualAttributes() {
@@ -122,11 +130,4 @@ protected function getFormat()
{
return 'xml';
}
-}
-
-class ExternalEntityTest
-{
- /** @Type("string") @XmlValue */
- public $foo;
-}
-
+}

8 comments on commit 5a9b7e6

@michelsalib

This comment has been minimized.

Show comment
Hide comment
@michelsalib

michelsalib Sep 28, 2012

Contributor

Hi @schmittjoh, can you explain why you made this choice?

Contributor

michelsalib replied Sep 28, 2012

Hi @schmittjoh, can you explain why you made this choice?

@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh Sep 28, 2012

Owner

It's a security precaution.

We could make it configurable, but it only makes sense to allow them if you tightly control what kind of data gets passed to the serializer.

Owner

schmittjoh replied Sep 28, 2012

It's a security precaution.

We could make it configurable, but it only makes sense to allow them if you tightly control what kind of data gets passed to the serializer.

@michelsalib

This comment has been minimized.

Show comment
Hide comment
@michelsalib

michelsalib Sep 28, 2012

Contributor
Contributor

michelsalib replied Sep 28, 2012

@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh Sep 28, 2012

Owner

Can we implement a whitelisting approach where you can allow certain doc types, but disallow everything else?

Owner

schmittjoh replied Sep 28, 2012

Can we implement a whitelisting approach where you can allow certain doc types, but disallow everything else?

@michelsalib

This comment has been minimized.

Show comment
Hide comment
@michelsalib

michelsalib Sep 28, 2012

Contributor
Contributor

michelsalib replied Sep 28, 2012

@michelsalib

This comment has been minimized.

Show comment
Hide comment
@michelsalib

michelsalib Sep 28, 2012

Contributor

Here is an example of what I want to add to the whitelist:

<!DOCTYPE ICECAT-interface SYSTEM "http://data.icecat.biz/dtd/ICECAT-interface_response.dtd">

Is this configuration ok ?

jms_serializer:
    xml_document:
        whitelist:
            - ICECAT-interface
Contributor

michelsalib replied Sep 28, 2012

Here is an example of what I want to add to the whitelist:

<!DOCTYPE ICECAT-interface SYSTEM "http://data.icecat.biz/dtd/ICECAT-interface_response.dtd">

Is this configuration ok ?

jms_serializer:
    xml_document:
        whitelist:
            - ICECAT-interface
@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh Sep 29, 2012

Owner

I think you need to require the entire doc type in the whitelist.

Owner

schmittjoh replied Sep 29, 2012

I think you need to require the entire doc type in the whitelist.

@michelsalib

This comment has been minimized.

Show comment
Hide comment
@michelsalib

michelsalib Oct 2, 2012

Contributor

I did a first draft, I should be able to submit tonight.

Contributor

michelsalib replied Oct 2, 2012

I did a first draft, I should be able to submit tonight.

Please sign in to comment.