keystoneauth auth plugins should not use etree XML parsing

[Brant Knudson]

XML parsing is surprisingly difficult and fraught with danger, for example entity expansion makes it easy to cause a lot of memory to be used and therefore crash your system. keystoneclient is using etree parsing which has these potential issues, although in the case of keystoneclient it's the response from the IdP which I think is generally trusted.

This is in python-keystoneclient/keystoneclient/contrib/auth/v3/saml2.py

There's a defusedxml parser that has protections against these attacks and should therefore be used instead if possible - https://pypi.python.org/pypi/defusedxml - the docs for this page also include some examples of other possible attacks.

This was caught by bandit 0.17.0.

I'm going to start this out as private security so we can think about it some more before it goes public, even though it's probably not something that needs an issue since I think the source is generally trusted. If you can't trust your IdP then who can you trust?

[Tristan Cacqueray (tristan-cacqueray)]

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

[Tristan Cacqueray (tristan-cacqueray)]

So this affect the saml2 authentication plugin and the exploit needs to first impersonate an identity provider in order to answer with malicious xml content ?

[Brant Knudson (blk-u)]

It can either be the Service Provider response or the Identity Provider response. These values are both parsed using lxml.etree.XML.

[Brant Knudson (blk-u)]

keystoneauth has this same code.

[Tristan Cacqueray (tristan-cacqueray)]

Adding keystone-coresec for further investigation.

So in order to impersonate Service Provider response or the Identity Provider response, these endpoints need to be in http right ? If so, this is likely a C1 type of bug (according to https://security.openstack.org/vmt-process.html#incident-report-taxonomy ).

[Jeremy Stanley (fungi)]

So does this mean that (say in a federated auth model) any malicious actor at a configured IdP can crash services calling into keystoneauth?

[Brant Knudson (blk-u)]

Jeremy - my understanding is that, yes, if you control the SP or the IdP then you can crash any application using keystoneauth / keystoneclient when the system is configured for SAML authentication (federation) by crafting a response to exploit the expansion function in lxml.etree. It's possible the SP/IdP might even be able to read a file off the system via external entity expansion... I'll have to look at the code some more... the client takes the value sent to it and then modifies it for reuse without getting rid of stuff that would be invalid or ignored as far as I can tell.

If somebody controls the SP and/or IdP then there's probably all sorts of other badness that they can get up to that's worse than crashing the applications. Keystone does try to protect the deployment from some of these things (for example, the mapping needs to be valid and will therefore limit what the SP and IdP can get up to... a sane configuration would prevent the SP/IdP from becoming admin through the mapping).

So, keystone will protect against some bad actions by SP/IdP. The deployer might think that this makes them "safe" and trust their external IdP. The SP/IdP admin could exploit this to crash their (or their customers') systems (or read files off their systems?) by crafting bad responses.

Tristan - I wouldn't object to a C1 classification. I don't know of a real exploit here, this is all speculative.

[Brant Knudson (blk-u)]

Found an interesting reference: http://blog.sendsafely.com/web-based-single-sign-on-and-the-dangers-of-saml-xml-parsing

[Morgan Fainberg (mdrnstm)]

So, this likely can't crash keystone but could cause issues with the client. Depends on how apache_saml2 processing handles this problem.

This is not likely to hit anything outside of client(s). It doesn't change the risk of ugly things.

[Morgan Fainberg (mdrnstm)]

This also likely can be made public, but I'm happy to let brant think that over a bit more.

[Morgan Fainberg (mdrnstm)]

Ah you did say it was only consumers of ksa not keystoneserver. Sorry.

[Tristan Cacqueray (tristan-cacqueray)]

I've removed the privacy settings and put the OSSA task as Won't Fix based on above comments. This can be put back to incomplete if the situation changes.

[Morgan Fainberg (mdrnstm)]

Marking this as "wont fix" against KSC as it shouldn't be needed long term (Session, AuthPlugins, and CLI are all being removed soon/are deprecated)

[Jamie Lennox (jamielennox)]

Unfortunately defusedxml is not part of global-requirements and that's not a battle i care to have. However we are also wanting to move away from lxml to remove the C dependencies that lxml bring. So alternative options would be appreciated.

For a shorter term fix, googling the issue brings up [1], suggesting we can do:

    from lxml import etree
    parser = etree.XMLParser(resolve_entities=False)

I think (i haven't looked closely) that this should be safe for SAML. Can we update bandit to check for this specifically?

[1] http://mikeknoop.com/lxml-xxe-exploit/

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/536761

[Morgan Fainberg (mdrnstm)]

Unassigning due to inactivity

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Gage Hugo <email address hidden>" on branch: master
Review: https://review.opendev.org/c/openstack/keystoneauth/+/536761
Reason: Abandoning since there hasn't been any recent activity, if anyone wants to continue this work, please feel free to restore this or create a new change.