From 6e09a25d9b4b7aa7a506853210a9a14100b8bc9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20R=C3=BCchel?= Date: Mon, 31 Oct 2016 11:56:48 +1100 Subject: [PATCH] Fix XXE in XML parsing (related to #366) This fixes XXE issues on anything where pysaml2 parses XML directly as part of issue #366. It doesn't address the xmlsec issues discussed on that ticket as they are out of reach of a direct fix and need the underlying library to fix this issue. --- setup.py | 1 + src/saml2/__init__.py | 5 +++-- src/saml2/pack.py | 3 ++- src/saml2/soap.py | 7 ++++--- tests/test_03_saml2.py | 27 ++++++++++++++++++++++++++ tests/test_43_soap.py | 43 +++++++++++++++++++++++++++++++++++++++++ tests/test_51_client.py | 15 ++++++++++++++ 7 files changed, 95 insertions(+), 6 deletions(-) diff --git a/setup.py b/setup.py index 8d39207..de96cae 100755 --- a/setup.py +++ b/setup.py @@ -18,6 +18,7 @@ install_requires = [ 'pytz', 'pyOpenSSL', 'python-dateutil', + 'defusedxml', 'six' ] diff --git a/src/saml2/__init__.py b/src/saml2/__init__.py index 6833d7e..b246caa 100644 --- a/src/saml2/__init__.py +++ b/src/saml2/__init__.py @@ -36,6 +36,7 @@ except ImportError: import cElementTree as ElementTree except ImportError: from elementtree import ElementTree +import defusedxml.ElementTree root_logger = logging.getLogger(__name__) root_logger.level = logging.NOTSET @@ -87,7 +88,7 @@ def create_class_from_xml_string(target_class, xml_string): """ if not isinstance(xml_string, six.binary_type): xml_string = xml_string.encode('utf-8') - tree = ElementTree.fromstring(xml_string) + tree = defusedxml.ElementTree.fromstring(xml_string) return create_class_from_element_tree(target_class, tree) @@ -269,7 +270,7 @@ class ExtensionElement(object): def extension_element_from_string(xml_string): - element_tree = ElementTree.fromstring(xml_string) + element_tree = defusedxml.ElementTree.fromstring(xml_string) return _extension_element_from_element_tree(element_tree) diff --git a/src/saml2/pack.py b/src/saml2/pack.py index e4c1462..728a516 100644 --- a/src/saml2/pack.py +++ b/src/saml2/pack.py @@ -37,6 +37,7 @@ except ImportError: import cElementTree as ElementTree except ImportError: from elementtree import ElementTree +import defusedxml.ElementTree NAMESPACE = "http://schemas.xmlsoap.org/soap/envelope/" FORM_SPEC = """
@@ -235,7 +236,7 @@ def parse_soap_enveloped_saml(text, body_class, header_class=None): :param text: The SOAP object as XML :return: header parts and body as saml.samlbase instances """ - envelope = ElementTree.fromstring(text) + envelope = defusedxml.ElementTree.fromstring(text) assert envelope.tag == '{%s}Envelope' % NAMESPACE # print(len(envelope)) diff --git a/src/saml2/soap.py b/src/saml2/soap.py index c1be544..055c690 100644 --- a/src/saml2/soap.py +++ b/src/saml2/soap.py @@ -19,6 +19,7 @@ except ImportError: except ImportError: #noinspection PyUnresolvedReferences from elementtree import ElementTree +import defusedxml.ElementTree logger = logging.getLogger(__name__) @@ -133,7 +134,7 @@ def parse_soap_enveloped_saml_thingy(text, expected_tags): :param expected_tags: What the tag of the SAML thingy is expected to be. :return: SAML thingy as a string """ - envelope = ElementTree.fromstring(text) + envelope = defusedxml.ElementTree.fromstring(text) # Make sure it's a SOAP message assert envelope.tag == '{%s}Envelope' % soapenv.NAMESPACE @@ -183,7 +184,7 @@ def class_instances_from_soap_enveloped_saml_thingies(text, modules): :return: The body and headers as class instances """ try: - envelope = ElementTree.fromstring(text) + envelope = defusedxml.ElementTree.fromstring(text) except Exception as exc: raise XmlParseError("%s" % exc) @@ -209,7 +210,7 @@ def open_soap_envelope(text): :return: dictionary with two keys "body"/"header" """ try: - envelope = ElementTree.fromstring(text) + envelope = defusedxml.ElementTree.fromstring(text) except Exception as exc: raise XmlParseError("%s" % exc) diff --git a/tests/test_03_saml2.py b/tests/test_03_saml2.py index 136161a..a71eb3c 100644 --- a/tests/test_03_saml2.py +++ b/tests/test_03_saml2.py @@ -17,6 +17,7 @@ except ImportError: import cElementTree as ElementTree except ImportError: from elementtree import ElementTree +from defusedxml.common import EntitiesForbidden ITEMS = { NameID: [""" @@ -166,6 +167,19 @@ def test_create_class_from_xml_string_wrong_class_spec(): assert kl == None +def test_create_class_from_xml_string_xxe(): + xml = """ + + + + ]> + &lol1; + """ + with raises(EntitiesForbidden) as err: + create_class_from_xml_string(NameID, xml) + + def test_ee_1(): ee = saml2.extension_element_from_string( """bar""") @@ -454,6 +468,19 @@ def test_ee_7(): assert nid.text.strip() == "http://federationX.org" +def test_ee_xxe(): + xml = """ + + + + ]> + &lol1; + """ + with raises(EntitiesForbidden): + saml2.extension_element_from_string(xml) + + def test_extension_element_loadd(): ava = {'attributes': {}, 'tag': 'ExternalEntityAttributeAuthority', diff --git a/tests/test_43_soap.py b/tests/test_43_soap.py index 4cde3d6..bf66a1d 100755 --- a/tests/test_43_soap.py +++ b/tests/test_43_soap.py @@ -12,9 +12,13 @@ except ImportError: import cElementTree as ElementTree except ImportError: from elementtree import ElementTree +from defusedxml.common import EntitiesForbidden + +from pytest import raises import saml2.samlp as samlp from saml2.samlp import NAMESPACE as SAMLP_NAMESPACE +from saml2 import soap NAMESPACE = "http://schemas.xmlsoap.org/soap/envelope/" @@ -66,3 +70,42 @@ def test_make_soap_envelope(): assert len(body) == 1 saml_part = body[0] assert saml_part.tag == '{%s}AuthnRequest' % SAMLP_NAMESPACE + + +def test_parse_soap_enveloped_saml_thingy_xxe(): + xml = """ + + + + ]> + &lol1; + """ + with raises(EntitiesForbidden): + soap.parse_soap_enveloped_saml_thingy(xml, None) + + +def test_class_instances_from_soap_enveloped_saml_thingies_xxe(): + xml = """ + + + + ]> + &lol1; + """ + with raises(soap.XmlParseError): + soap.class_instances_from_soap_enveloped_saml_thingies(xml, None) + + +def test_open_soap_envelope_xxe(): + xml = """ + + + + ]> + &lol1; + """ + with raises(soap.XmlParseError): + soap.open_soap_envelope(xml) diff --git a/tests/test_51_client.py b/tests/test_51_client.py index f695816..13cef7c 100644 --- a/tests/test_51_client.py +++ b/tests/test_51_client.py @@ -7,6 +7,7 @@ import six from future.backports.urllib.parse import parse_qs from future.backports.urllib.parse import urlencode from future.backports.urllib.parse import urlparse +from pytest import raises from saml2.argtree import add_path from saml2.cert import OpenSSLWrapper @@ -25,6 +26,7 @@ from saml2.assertion import Assertion from saml2.authn_context import INTERNETPROTOCOLPASSWORD from saml2.client import Saml2Client from saml2.config import SPConfig +from saml2.pack import parse_soap_enveloped_saml from saml2.response import LogoutResponse from saml2.saml import NAMEID_FORMAT_PERSISTENT, EncryptedAssertion, Advice from saml2.saml import NAMEID_FORMAT_TRANSIENT @@ -38,6 +40,8 @@ from saml2.s_utils import do_attribute_statement from saml2.s_utils import factory from saml2.time_util import in_a_while, a_while_ago +from defusedxml.common import EntitiesForbidden + from fakeIDP import FakeIDP from fakeIDP import unpack_form from pathutils import full_path @@ -1552,6 +1556,17 @@ class TestClientWithDummy(): 'http://www.example.com/login' assert ac.authn_context_class_ref.text == INTERNETPROTOCOLPASSWORD +def test_parse_soap_enveloped_saml_xxe(): + xml = """ + + + + ]> + &lol1; + """ + with raises(EntitiesForbidden): + parse_soap_enveloped_saml(xml, None) # if __name__ == "__main__": # tc = TestClient()