Vulnerable to XXE in common configuration

[marcpare]

Hey there, ran into an XXE issue during a Burp scan that traced back to pysaml2. Not sure if it's something you would consider fixing on your end, but I figured it would be helpful to at least be aware.

Code Version

4.5.0

Expected Behavior

pysaml2 should reject all XXE attempts.

Current Behavior

It is possible to trigger external HTTP requests from the function saml_client.parse_authn_request_response (and possibly others) by adding the following to POST responses:

<!DOCTYPE saml2p:response PUBLIC "-//B/A/EN" "http://example.com/uhoh">

Possible Solution

So, the root cause it pretty interesting. It's the calls to xmlsec1 to verify payloads that make the external request. This is a known vulnerability (CVE-2017-1000061)

Versions of xmlsec1 below 1.2.23 are vulnerable. Unfortunately, secure versions aren't available in every package manager by default. In our case, the system was running Ubuntu xenial (by way of Heroku's Cedar stack), which was giving us version 1.2.20 when installed from apt. I'm sure there are lots more folks running systems like this out there, and it may be worthwhile to still be secure by default in this case.

Adding the flag forbid_dtd to the defusedxml calls would prevent the XXE vulnerability in cases of an old xmlsec1:

envelope = defusedxml.ElementTree.fromstring(text, forbid_dtd=True)

This causes pysaml2 to bail before calling out to xmlsec1.

This appears to only break tests that are supposed to fail for XXE vulnerabilities since a different error message is returned.

Steps to Reproduce

I can provide a runnable example if desired, but wanted to test the waters first.

Thanks!

[jkakavas]

👍 for explicitly disallowing DTD so that we can trip as soon as possible on malicious input. In any case there is no use case where DTDs are neccessary in the XML messages used in SAML 2 Web Browser SSO profile AFAIK.

[marcpare]

Great, thanks for the feedback @jkakavas. I'll keep an eye on #498 getting merged to see if this still applies. Would be great for the fix to be a one line change!

[peppelinux]

Debian 10 and newer distributions now fixed this in xmlsec1, by the way is always good to have another filter on xml

[spaceone]

Is it possible via this XXE to gain any local files?

[peppelinux]

Good question, I think that it's also related to this #603, where you put:

<?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>

[spaceone]

Yes, I was researching for ways to get (config) files. It's much more critical then.

[peppelinux]

I think that with the newer xmlsec1 release and gnu/linux distribution this have been fixed definitively.
@marcpare is there something that would be useful to keep this thread open?

@c00kiemon5ter this is a sensible topic, is there any update about #509 or the latest xmlsec1 does it fine as they are?

[peppelinux]

The only way to enable XXE features in xmlsec1 would be enable it arbitrarly:

we're more relaxed now.
Going to close this, it was very interesting to dig out