Add a safe_minidom_parse_string function.

Adds a new utils.safe_minidom_parse_string function and
updates external API facing Cinder modules to use it.
This ensures we have safe defaults on our incoming API XML parsing.

Internally safe_minidom_parse_string uses a ProtectedExpatParser
class to disable DTDs and entities from being parsed when using
minidom.

Fixes LP Bug #1100282.

Change-Id: Iff8340033c8e8db58184944a1bf705e16b8b3e03

diff --git a/cinder/api/common.py b/cinder/api/common.py
index 2ee4a9c62f306e320e3151b9765ffe547da7436d..94bf594c00b7fdeba2466b8d692f91b87dc3bc0f 100644 (file)
--- a/cinder/api/common.py
+++ b/cinder/api/common.py
@@ -25,7 +25,7 @@ from cinder.api.openstack import wsgi
 from cinder.api import xmlutil
 from cinder import flags
 from cinder.openstack.common import log as logging
-from xml.dom import minidom
+from cinder import utils
 
 
 LOG = logging.getLogger(__name__)
@@ -244,7 +244,7 @@ class ViewBuilder(object):
 
 class MetadataDeserializer(wsgi.MetadataXMLDeserializer):
     def deserialize(self, text):
-        dom = minidom.parseString(text)
+        dom = utils.safe_minidom_parse_string(text)
         metadata_node = self.find_first_child_named(dom, "metadata")
         metadata = self.extract_metadata(metadata_node)
         return {'body': {'metadata': metadata}}
@@ -252,7 +252,7 @@ class MetadataDeserializer(wsgi.MetadataXMLDeserializer):
 
 class MetaItemDeserializer(wsgi.MetadataXMLDeserializer):
     def deserialize(self, text):
-        dom = minidom.parseString(text)
+        dom = utils.safe_minidom_parse_string(text)
         metadata_item = self.extract_metadata(dom)
         return {'body': {'meta': metadata_item}}
 
@@ -270,7 +270,7 @@ class MetadataXMLDeserializer(wsgi.XMLDeserializer):
         return metadata
 
     def _extract_metadata_container(self, datastring):
-        dom = minidom.parseString(datastring)
+        dom = utils.safe_minidom_parse_string(datastring)
         metadata_node = self.find_first_child_named(dom, "metadata")
         metadata = self.extract_metadata(metadata_node)
         return {'body': {'metadata': metadata}}
@@ -282,7 +282,7 @@ class MetadataXMLDeserializer(wsgi.XMLDeserializer):
         return self._extract_metadata_container(datastring)
 
     def update(self, datastring):
-        dom = minidom.parseString(datastring)
+        dom = utils.safe_minidom_parse_string(datastring)
         metadata_item = self.extract_metadata(dom)
         return {'body': {'meta': metadata_item}}
 

diff --git a/cinder/api/contrib/hosts.py b/cinder/api/contrib/hosts.py
index 40a4e9cc93712d87d35af302b7d5a5705a7d0f2f..2c68c71c9efd1ab9d8fac06bb80eb169be49f1ff 100644 (file)
--- a/cinder/api/contrib/hosts.py
+++ b/cinder/api/contrib/hosts.py
@@ -16,7 +16,6 @@
 """The hosts admin extension."""
 
 import webob.exc
-from xml.dom import minidom
 from xml.parsers import expat
 
 from cinder.api import extensions
@@ -79,7 +78,7 @@ class HostShowTemplate(xmlutil.TemplateBuilder):
 class HostDeserializer(wsgi.XMLDeserializer):
     def default(self, string):
         try:
-            node = minidom.parseString(string)
+            node = utils.safe_minidom_parse_string(string)
         except expat.ExpatError:
             msg = _("cannot understand XML")
             raise exception.MalformedRequestBody(reason=msg)

diff --git a/cinder/api/contrib/volume_actions.py b/cinder/api/contrib/volume_actions.py
index ae75885ff65962264831131037eee9d9f48429ca..50587c3692673c3f7e633f3d4a910996a949ffc5 100644 (file)
--- a/cinder/api/contrib/volume_actions.py
+++ b/cinder/api/contrib/volume_actions.py
@@ -13,7 +13,6 @@
 #   under the License.
 
 import webob
-from xml.dom import minidom
 
 from cinder.api import extensions
 from cinder.api.openstack import wsgi
@@ -22,6 +21,7 @@ from cinder import exception
 from cinder import flags
 from cinder.openstack.common import log as logging
 from cinder.openstack.common.rpc import common as rpc_common
+from cinder import utils
 from cinder import volume
 
 
@@ -54,7 +54,7 @@ class VolumeToImageSerializer(xmlutil.TemplateBuilder):
 class VolumeToImageDeserializer(wsgi.XMLDeserializer):
     """Deserializer to handle xml-formatted requests."""
     def default(self, string):
-        dom = minidom.parseString(string)
+        dom = utils.safe_minidom_parse_string(string)
         action_node = dom.childNodes[0]
         action_name = action_node.tagName
 

diff --git a/cinder/api/openstack/wsgi.py b/cinder/api/openstack/wsgi.py
index 688fed86f7db48e4712da0e7c13f17dc9b8b08f9..cc882c826ac0fb8a2dfd1ab71bb945c9896c6187 100644 (file)
--- a/cinder/api/openstack/wsgi.py
+++ b/cinder/api/openstack/wsgi.py
@@ -23,6 +23,7 @@ import webob
 from cinder import exception
 from cinder.openstack.common import jsonutils
 from cinder.openstack.common import log as logging
+from cinder import utils
 from cinder import wsgi
 
 from lxml import etree
@@ -151,7 +152,7 @@ class XMLDeserializer(TextDeserializer):
         plurals = set(self.metadata.get('plurals', {}))
 
         try:
-            node = minidom.parseString(datastring).childNodes[0]
+            node = utils.safe_minidom_parse_string(datastring).childNodes[0]
             return {node.nodeName: self._from_xml_node(node, plurals)}
         except expat.ExpatError:
             msg = _("cannot understand XML")
@@ -548,7 +549,7 @@ def action_peek_json(body):
 def action_peek_xml(body):
     """Determine action to invoke."""
 
-    dom = minidom.parseString(body)
+    dom = utils.safe_minidom_parse_string(body)
     action_node = dom.childNodes[0]
 
     return action_node.tagName

diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py
index b695d78d0e93bd9ad248c75a36c63bc4972fbaa3..a60f1a4c4c64cf28daa854fd19ab7eae772cd7e2 100644 (file)
--- a/cinder/api/v1/volumes.py
+++ b/cinder/api/v1/volumes.py
@@ -17,7 +17,6 @@
 
 import webob
 from webob import exc
-from xml.dom import minidom
 
 from cinder.api import common
 from cinder.api.openstack import wsgi
@@ -26,6 +25,7 @@ from cinder import exception
 from cinder import flags
 from cinder.openstack.common import log as logging
 from cinder.openstack.common import uuidutils
+from cinder import utils
 from cinder import volume
 from cinder.volume import volume_types
 
@@ -204,7 +204,7 @@ class CreateDeserializer(CommonDeserializer):
 
     def default(self, string):
         """Deserialize an xml-formatted volume create request."""
-        dom = minidom.parseString(string)
+        dom = utils.safe_minidom_parse_string(string)
         volume = self._extract_volume(dom)
         return {'body': {'volume': volume}}
 

diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py
index 16474447977f78ea473358421302469960349abd..bd2e4e91b3851615f72b1a39c7ef8a3e01345138 100644 (file)
--- a/cinder/api/v2/volumes.py
+++ b/cinder/api/v2/volumes.py
@@ -17,7 +17,6 @@
 
 import webob
 from webob import exc
-from xml.dom import minidom
 
 from cinder.api import common
 from cinder.api.openstack import wsgi
@@ -27,6 +26,7 @@ from cinder import exception
 from cinder import flags
 from cinder.openstack.common import log as logging
 from cinder.openstack.common import uuidutils
+from cinder import utils
 from cinder import volume
 from cinder.volume import volume_types
 
@@ -119,7 +119,7 @@ class CreateDeserializer(CommonDeserializer):
 
     def default(self, string):
         """Deserialize an xml-formatted volume create request."""
-        dom = minidom.parseString(string)
+        dom = utils.safe_minidom_parse_string(string)
         volume = self._extract_volume(dom)
         return {'body': {'volume': volume}}
 

diff --git a/cinder/tests/test_utils.py b/cinder/tests/test_utils.py
index e559d8e44f50232b61c34e261518b21f161b79fa..76aee6d1991e0643bf3a2f442afedb2f5aa6836c 100644 (file)
--- a/cinder/tests/test_utils.py
+++ b/cinder/tests/test_utils.py
@@ -426,6 +426,39 @@ class GenericUtilsTestCase(test.TestCase):
         result = utils.service_is_up(service)
         self.assertFalse(result)
 
+    def test_safe_parse_xml(self):
+
+        normal_body = ("""
+                 <?xml version="1.0" ?><foo>
+                    <bar>
+                        <v1>hey</v1>
+                        <v2>there</v2>
+                    </bar>
+                </foo>""").strip()
+
+        def killer_body():
+            return (("""<!DOCTYPE x [
+                    <!ENTITY a "%(a)s">
+                    <!ENTITY b "%(b)s">
+                    <!ENTITY c "%(c)s">]>
+                <foo>
+                    <bar>
+                        <v1>%(d)s</v1>
+                    </bar>
+                </foo>""") % {
+                'a': 'A' * 10,
+                'b': '&a;' * 10,
+                'c': '&b;' * 10,
+                'd': '&c;' * 9999,
+            }).strip()
+
+        dom = utils.safe_minidom_parse_string(normal_body)
+        self.assertEqual(normal_body, str(dom.toxml()))
+
+        self.assertRaises(ValueError,
+                          utils.safe_minidom_parse_string,
+                          killer_body())
+
     def test_xhtml_escape(self):
         self.assertEqual('&quot;foo&quot;', utils.xhtml_escape('"foo"'))
         self.assertEqual('&apos;foo&apos;', utils.xhtml_escape("'foo'"))

diff --git a/cinder/utils.py b/cinder/utils.py
index bd12966543b5f599a6f8df88fd938876384bdb11..f5164acc35ddea8d7920b3a920cb1b1e9a367e37 100644 (file)
--- a/cinder/utils.py
+++ b/cinder/utils.py
@@ -41,6 +41,10 @@ import tempfile
 import time
 import types
 import warnings
+from xml.dom import minidom
+from xml.parsers import expat
+from xml import sax
+from xml.sax import expatreader
 from xml.sax import saxutils
 
 from eventlet import event
@@ -622,6 +626,46 @@ class LoopingCall(object):
         return self.done.wait()
 
 
+class ProtectedExpatParser(expatreader.ExpatParser):
+    """An expat parser which disables DTD's and entities by default."""
+
+    def __init__(self, forbid_dtd=True, forbid_entities=True,
+                 *args, **kwargs):
+        # Python 2.x old style class
+        expatreader.ExpatParser.__init__(self, *args, **kwargs)
+        self.forbid_dtd = forbid_dtd
+        self.forbid_entities = forbid_entities
+
+    def start_doctype_decl(self, name, sysid, pubid, has_internal_subset):
+        raise ValueError("Inline DTD forbidden")
+
+    def entity_decl(self, entityName, is_parameter_entity, value, base,
+                    systemId, publicId, notationName):
+        raise ValueError("<!ENTITY> forbidden")
+
+    def unparsed_entity_decl(self, name, base, sysid, pubid, notation_name):
+        # expat 1.2
+        raise ValueError("<!ENTITY> forbidden")
+
+    def reset(self):
+        expatreader.ExpatParser.reset(self)
+        if self.forbid_dtd:
+            self._parser.StartDoctypeDeclHandler = self.start_doctype_decl
+        if self.forbid_entities:
+            self._parser.EntityDeclHandler = self.entity_decl
+            self._parser.UnparsedEntityDeclHandler = self.unparsed_entity_decl
+
+
+def safe_minidom_parse_string(xml_string):
+    """Parse an XML string using minidom safely.
+
+    """
+    try:
+        return minidom.parseString(xml_string, parser=ProtectedExpatParser())
+    except sax.SAXParseException as se:
+        raise expat.ExpatError()
+
+
 def xhtml_escape(value):
     """Escapes a string so it is valid within XML or XHTML.