Is pysaml2 affected by CVE-2017-11427?

[johnwheeler]

Hello,

Just a quick sanity check to ensure pysaml2 isn't affected by the SAML vulnerabilities announced:

https://www.kb.cert.org/vuls/id/475445

The note does not mention pysaml2, but it would be great to hear it from the horse's mouth.

I apologize if this is not the appropriate venue for this type of query.

John

[logston]

I think so.

If I understand what I think I do...

StatusResponse (which is subclassed by AuthnResponse here gets its signature_check method from self.sec.correctly_signed_response here.

This, in turn, calls samlp.any_response_from_string here

pysaml2/src/saml2/sigver.py

Line 1710 in 86cf3a4

def correctly_signed_response(self, decoded_xml, must=False, origdoc=None,

That in turn calls response_from_string here

pysaml2/src/saml2/samlp.py

Line 1843 in fd7a4f6

for func in [status_response_type__from_string, response_from_string,

That in turn calls saml2.create_class_from_xml_string(Response, xml_string) here

pysaml2/src/saml2/samlp.py

Line 1620 in fd7a4f6

return saml2.create_class_from_xml_string(Response, xml_string)

That in turn calls defusedxml.ElementTree.fromstring(xml_string) here

pysaml2/src/saml2/__init__.py

Line 91 in 847e970

tree = defusedxml.ElementTree.fromstring(xml_string)

... which is the same library that OneLogin/python-saml had to patch here:

SAML-Toolkits/python-saml@fad881b

I think direct access of attributes on this DOM are risky.

Instead, something like etree.strip_tags(node, etree.Comment) needs to be done for each text node in the DOM. See: SAML-Toolkits/python-saml@fad881b#diff-7b29e1b5445a77441bbcbbd2b1b1bfd5R161

[bqumsiyeh]

I've been trying to reproduce, but I have not been able to yet. From what I can tell, the comments are stripped out appropriately.

In my local environment, I intercepted the SAML Response, and injected the comment in the user value. Here is a simple version of the POC:

...
# Get the SAMLResponse from the request
saml_response = request.POST['SAMLResponse']

# Base64 decode the SAML Assertion
decoded_saml_response = base64.b64decode(saml_response).decode('utf-8')

# Spoof the user
spoofed = decoded_saml_response.replace('user@user.com.evil.com', "user@user.com<!---->.evil.com")

# Encode back to base64
spoofed_saml_response = base64.b64encode(spoofed.encode('utf-8')).decode('utf-8')

# Parse and verify, as usual, with the spoofed SAML response
saml_client = saml_client_for(idp_name, retreived_metadata_url, request)
authn_response = saml_client.parse_authn_request_response(spoofed_saml_response, entity.BINDING_HTTP_POST)
authn_response.get_identity()

print(authn_response.name_id.text)
# This prints "user@user.com.evil.com"

[dcripplinger]

Also confirming in my system that pysaml2 already handles the injected comment fine, and the text before and after the comment are combined for the intended nameid. Same thing for an email attribute.

[Plazmaz]

In that case, should this be closed? Do we need more confirmation here @c00kiemon5ter?

[dustin-decker]

Looks okay from @bqumsiyeh's attempt. This should probably be rolled into a pysaml2 test so people can easily verify behavior and if there are any changes to the XML library down the road you can be confident that it is still covered.