AttributeError: 'NoneType' object has no attribute 'version'

[spaceone]

The error handling for invalid SAML messages is broken:

Traceback (most recent call last):
    response = self.sp.parse_authn_request_response(message, binding, self.outstanding_queries)
  File "/usr/lib/python2.7/dist-packages/saml2/client_base.py", line 711, in parse_authn_request_response
    binding, **kwargs)
  File "/usr/lib/python2.7/dist-packages/saml2/entity.py", line 1202, in _parse_response
    response = response.verify(keys)
  File "/usr/lib/python2.7/dist-packages/saml2/response.py", line 1041, in verify
    res = self._verify()
  File "/usr/lib/python2.7/dist-packages/saml2/response.py", line 400, in _verify
    assert self.response.version == "2.0"
AttributeError: 'NoneType' object has no attribute 'version'

We would like to present a user friendly error message instead of this traceback in case of errors. Would be nice to mask this better, there are exceptions for signature error etc.

Reproducible by e.g. prepending anything invalid to the SAML message. e.g.:
<?xml version="1.0" encoding="ISO-8859-1"?><!DOCTYPE saml2p:response [<!ELEMENT saml2p:response ANY ><!ENTITY xxe SYSTEM "file:///etc/shadow" >] ><saml2p:response>&xxe;</saml2p:response>

Code Version

v4.7.0

[peppelinux]

Hi @spaceone I would like to ask you to check this behaviour on the last pysaml2 release and if possibile give us updates. That's something interesting that would be patched soon

[peppelinux]

@spaceone ping
Should we have a unit test to cover this behaviour?

[spaceone]

With pysaml2 4.5.0 the traceback changed into:

    response = self.sp.parse_authn_request_response(message, binding, self.outstanding_queries)
  File "/usr/lib/python3/dist-packages/saml2/client_base.py", line 702, in parse_authn_request_response
    binding, **kwargs)
  File "/usr/lib/python3/dist-packages/saml2/entity.py", line 1138, in _parse_response
    response = response.loads(xmlstr, False, origxml=origxml)
  File "/usr/lib/python3/dist-packages/saml2/response.py", line 512, in loads
    self._loads(xmldata, decode, origxml)
  File "/usr/lib/python3/dist-packages/saml2/response.py", line 337, in _loads
    **args)
  File "/usr/lib/python3/dist-packages/saml2/sigver.py", line 1776, in correctly_signed_response
    response = samlp.any_response_from_string(decoded_xml)
  File "/usr/lib/python3/dist-packages/saml2/samlp.py", line 1847, in any_response_from_string
    resp = func(xmlstr)
  File "/usr/lib/python3/dist-packages/saml2/samlp.py", line 1446, in status_response_type__from_string
    return saml2.create_class_from_xml_string(StatusResponseType_, xml_string)
  File "/usr/lib/python3/dist-packages/saml2/__init__.py", line 91, in create_class_from_xml_string
    tree = defusedxml.ElementTree.fromstring(xml_string)
  File "/usr/lib/python3/dist-packages/defusedxml/common.py", line 117, in fromstring
    parser.feed(text)
  File "/usr/lib/python3.7/xml/etree/ElementTree.py", line 1630, in feed
    self.parser.Parse(data, 0)
  File "../Modules/pyexpat.c", line 506, in EntityDecl
  File "/usr/lib/python3/dist-packages/defusedxml/ElementTree.py", line 90, in defused_entity_decl
    raise EntitiesForbidden(name, value, base, sysid, pubid, notation_name)
defusedxml.common.EntitiesForbidden: EntitiesForbidden(name='xxe', system_id='file:///etc/shadow', public_id=None)

This is nice, as I can handle this.
But the message above is only one example to trigger this problem. I would have to try a lot other regular SAML responses as well, to prove the error still exists.

[spaceone]

With the current git master (6.3.1 / fc42b2a) I get the following exception:

Traceback (most recent call last):
…
    response = self.sp.parse_authn_request_response(message, binding, self.outstanding_queries)
  File "/usr/lib/python3/dist-packages/saml2/client_base.py", line 719, in parse_authn_request_response
    binding, **kwargs)
  File "/usr/lib/python3/dist-packages/saml2/entity.py", line 1197, in _parse_response
    response.verify(keys)
  File "/usr/lib/python3/dist-packages/saml2/response.py", line 1034, in verify
    res = self._verify()
  File "/usr/lib/python3/dist-packages/saml2/response.py", line 406, in _verify
    self.response.destination not in self.return_addrs:
TypeError: argument of type 'NoneType' is not iterable

To reach this I had to set want_response_signed=False

I used this SAMLResponse message:

<?xml version="1.0" encoding="ISO-8859-1"?><samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="_8e8dc5f69a98cc4c1ff3427e5ce34606fd672f91e6" Version="2.0" IssueInstant="2014-07-17T01:01:48Z" Destination="http://example.com/" InResponseTo="foo"/>

fY9Na8MwEET/iti7bcnxhyzihEAuhpRCE3roJQh51RrslfEqJT%2B/biDQU2FOA2%2BYt93fp1F848JDoBZUKkEgudAP9NlCd35NtC6bRMF%2Bt2U7jbN5Q54DMYoVJDaPsoXbQiZYHtiQnZBNdOZ8eDmZPJVmXkIMLozwB/mfsMy4xPURiO7YwlWj7l3pq8Y22rnCKe83RV5j6XBTVLLyfVXnvlFYgXh/uuS/Lh3zDTviaCmulVRFIutE1RepzJpCf4A4IseBbHxQXzHOJsvwbqd5xNSFKVtX6Gl9CS34ECDb/QA%3D```

[peppelinux]

I'd suggest to consult @c00kiemon5ter for a PR that would handle this, like:

# saml2/response.py", line 406
if not getattr(self.response, 'destination', None):
    logger.error|warning('some troubles')
    raise ThatExceptionThatHopefullyAlreadyIsInpySAML2Exceptions('a generic message')

Not at last, about that assert self.response.version == "2.0":
I don't think that pysaml2 still have assert statements in it, @c00kiemon5ter have merged a PR on this

[spaceone]

Some starting point:

diff --git src/saml2/entity.py src/saml2/entity.py
index 672ad6f7..de841c86 100644
--- src/saml2/entity.py
+++ src/saml2/entity.py
@@ -1108,7 +1108,7 @@ class Entity(HTTPBase):
 
         response = None
         if not xmlstr:
-            return response
+            raise TypeError('empty xmlstr')
 
         if "return_addrs" not in kwargs:
             bindings = {
@@ -1131,7 +1131,7 @@ class Entity(HTTPBase):
 
         xmlstr = self.unravel(xmlstr, binding, response_cls.msgtype)
         if not xmlstr:  # Not a valid reponse
-            return None
+            raise SomeException('Not a valid response because not unravel() ...')
 
         try:
             response_is_signed = False
@@ -1167,9 +1167,6 @@ class Entity(HTTPBase):
 
         logger.debug("XMLSTR: %s", xmlstr)
 
-        if not response:
-            return response
-
         keys = None
         if outstanding_certs:
             try:
diff --git src/saml2/response.py src/saml2/response.py
index 5fd8043a..5193ae30 100644
--- src/saml2/response.py
+++ src/saml2/response.py
@@ -255,7 +255,7 @@ class StatusResponse(object):
     def __init__(self, sec_context, return_addrs=None, timeslack=0,
             request_id=0, asynchop=True, conv_info=None):
         self.sec = sec_context
-        self.return_addrs = return_addrs
+        self.return_addrs = return_addrs or []
 
         self.timeslack = timeslack
         self.request_id = request_id

I think parse_response should never return None but instead always raise exceptions for invalid things.
It seems that has already be done for most cases, so the if not response: block can be removed.

[peppelinux]

I think parse_response should never return None but instead always raise exceptions for invalid things.
It seems that has already be done for most cases, so the if not response: block can be removed.

Yes, I would prefer your solution, if something went wrong simply raise exception!