[OSSA 2013-004] DoS through XML entity expansion (CVE-2013-1664)

Bug #1100282 reported by Thierry Carrez
292
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Cinder
Fix Released
High
Dan Prince
Folsom
Fix Released
High
Dan Prince
OpenStack Compute (nova)
Fix Released
High
Dan Prince
Folsom
Fix Released
High
Dan Prince
OpenStack Identity (keystone)
Fix Released
High
Dolph Mathews
Essex
Fix Released
High
Dan Prince
Folsom
Fix Released
High
Dolph Mathews
OpenStack Security Advisory
Fix Released
Undecided
Thierry Carrez
neutron
Fix Released
High
Davanum Srinivas (DIMS)
oslo-incubator
Fix Released
High
Davanum Srinivas (DIMS)
Grizzly
Fix Released
High
Davanum Srinivas (DIMS)

Bug Description

Jonathan Murray from NCC Group reported that you can DoS keystone servers using XML entities in Keystone requests.

[ Joshua Harlow from Yahoo! independently reported the same issue plaguing Nova (using minidom). ]

POST /v2.0/tokens HTTP/1.1
content-type: application/xml

<!DOCTYPE foo [
<!ENTITY a "AAAA lots of As AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAvAAAAAAAAAA" >
<!ENTITY b "&a;&a;&a;&a;&a;&a;&a;&a;" >
<!ENTITY c "&b;&b;&b;&b;&b;&b;&b;&b;" >
]>
<auth>
<tenantName>&c;</tenantName>
<passwordCredentials>
<username>&c;</username>
<username>&c;</username>
<username>&c;</username>
<username>&c;</username>
<password>&c;</password>
<somethingElse>&c;</somethingElse>
<somethingElse1>&c;</somethingElse1>
<somethingElse2>&c;</somethingElse2>
</passwordCredentials>
</auth>

In that precise case it might be an issue with the XML library we use, although it sounds generally safer to disable parsing ENTITY blocks entirely if we can.

Revision history for this message
Thierry Carrez (ttx) wrote :

Looks like we could pass an etree.XMLParser with resolve_entities=False to etree.fromstring. Thoughts ?

Dolph Mathews (dolph)
Changed in keystone:
status: New → Confirmed
assignee: nobody → Dolph Mathews (dolph)
Revision history for this message
Dolph Mathews (dolph) wrote :

I wrote a 4KB request similar to the above that took about 10 minutes to consume > 1 GB server memory.

Thierry's suggestion fixes the DoS vulnerability, but then the same request will still return a 500 as the subsequent code doesn't know how to deal with XML Entities.

The attached patch completely ignores XML entities, comments, and processing instructions -- the sample request above now results in a quick 401 Unauthorized, as expected.

Revision history for this message
Dolph Mathews (dolph) wrote :

Backport to stable/folsom

Revision history for this message
Dolph Mathews (dolph) wrote :

Backport to stable/essex

Revision history for this message
Thierry Carrez (ttx) wrote :

Awesome, Dolph! keystone-core please +1 the proposed patches.

Anyone with spare cycles to check which OTHER openstack projects are affected by the same issue ?
lxml is used in cinder, glance, nova and oslo. If affected I'd rather do them all in one go.

Revision history for this message
Thierry Carrez (ttx) wrote :

Proposed combined impact description with bug 1100279:

========
Title: Information leak and Denial of Service using XML entities
Reporter: Jonathan Murray (NCC Group)
Products: Keystone
Affects: All versions

Description:
Jonathan Murray from NCC Group reported a vulnerability in the parsing of XML requests in Keystone. By using entities in XML requests, an unauthenticated attacker may crash the Keystone API server, resulting in a denial of service. Authenticated attackers may also leverage XML entities to read the content of a local file on the Keystone API server.
=========

Ramdom questions: there is no way of disabling XML requests completely, right ?

Revision history for this message
Dolph Mathews (dolph) wrote :

Since essex, there is definitely a way to disable XML. We opt to translate XML<-->JSON in middleware so that the application itself only has to speak JSON.

It's included by keystone.conf.sample, but you can remove the keystone.middleware:XmlBodyMiddleware from your application pipelines to completely disable XML support.

Revision history for this message
Thierry Carrez (ttx) wrote :

OK then I propose the following impact description:

========
Title: Information leak and Denial of Service using XML entities
Reporter: Jonathan Murray (NCC Group)
Products: Keystone
Affects: All versions

Description:
Jonathan Murray from NCC Group reported a vulnerability in the parsing of XML requests in Keystone. By using entities in XML requests, an unauthenticated attacker may crash the Keystone API server, resulting in a denial of service. Authenticated attackers may also leverage XML entities to read the content of a local file on the Keystone API server. This only affects servers with XML support enabled (with keystone.middleware:XmlBodyMiddleware in the WSGI pipeline).
=========

Remaining tasks before disclosure:
Keystone Core: please +1 proposed patches
Anyone: please +1 proposed impact description
VMT: Analyze other projects to make sure they don't suffer from the same issue

Revision history for this message
Dolph Mathews (dolph) wrote :

I think the description is accurate, but you could be more specific, I suppose. Instead of:

> an unauthenticated attacker may crash the Keystone API server, resulting in a denial of service.

... change to:

> an unauthenticated attacker may consume excessive resources on the Keystone API server, resulting in a denial of service and potentially a crash.

Revision history for this message
Thierry Carrez (ttx) wrote :

I like it. here it goes:

========
Title: Information leak and Denial of Service using XML entities
Reporter: Jonathan Murray (NCC Group)
Products: Keystone
Affects: All versions

Description:
Jonathan Murray from NCC Group reported a vulnerability in the parsing of XML requests in Keystone. By using entities in XML requests, an unauthenticated attacker may consume excessive resources on the Keystone API server, resulting in a denial of service and potentially a crash. Authenticated attackers may also leverage XML entities to read the content of a local file on the Keystone API server. This only affects servers with XML support enabled (with keystone.middleware:XmlBodyMiddleware in the WSGI pipeline).
=========

Mark McLoughlin (markmc)
Changed in keystone:
milestone: none → 2012.2.3
Revision history for this message
Dolph Mathews (dolph) wrote :

+1

Revision history for this message
Adam Young (ayoung) wrote :

+1

Revision history for this message
Dan Prince (dan-prince) wrote :

I'm running similar requests on Fedora and I can't seem to reproduce this. I'm wondering if the underlying version of python-lxml is also part of what is causing the problems.

[root@nova1 ~]# rpm -qi python-lxml
Name : python-lxml
Version : 2.3.3

Which version are you guys using?

Revision history for this message
Dan Prince (dan-prince) wrote :

Okay. I got it to happen. Just had to tune things a bit....

Revision history for this message
Dolph Mathews (dolph) wrote :

FWIW:

  $ pip freeze | grep lxml
  lxml==3.1beta1

Revision history for this message
Dan Prince (dan-prince) wrote :

ttx:

I tested this today with Nova using a similar XML request (lots of entity expansions) and cause the same exploit. Essentially the request took forever and pegged the CPU on the machine.

We are going to need a different fix in Nova though because minidom is actually the XML call we use there:

    dom = minidom.parseString(body)

Both Cinder and Quantum will have similar issues due to the fact that they use minidom.parseString as well. So whatever we do to fix minidom in Nova should work there as well.

From what I can tell Glance is clean though.

----

I'm looking into a fix for the minidom issue... shall I post the patches into this ticket?

Revision history for this message
Thierry Carrez (ttx) wrote :

Thanks for looking into this. Yes, you can post here, and I'll create tasks for all affected projects. Looks like getting this one right will push us after 2012.2.3, but it's better this way.

Mark McLoughlin (markmc)
Changed in keystone:
milestone: 2012.2.3 → none
Revision history for this message
Thierry Carrez (ttx) wrote :

Joshua Harlow independently reported the Nova issue on another bug, and had a proposed way of fixing it:

" Possibly fixed by simply adjusting the following: http://docs.python.org/2/library/pyexpat.html?highlight=xml.parsers#xml.parsers.expat.xmlparser.SetParamEntityParsing "

Changed in keystone:
importance: Undecided → High
Changed in nova:
status: New → Confirmed
importance: Undecided → High
description: updated
Thierry Carrez (ttx)
Changed in cinder:
status: New → Confirmed
importance: Undecided → High
Changed in quantum:
status: New → Confirmed
importance: Undecided → High
Changed in keystone:
status: Confirmed → Triaged
Revision history for this message
Doug Hellmann (doug-hellmann) wrote :

Has this issue been raised to the Python-dev security team? http://www.python.org/news/security/

I mentioned it to one member of the team, and they're aware of the issue, but weren't aware that it affected us.

Revision history for this message
Doug Hellmann (doug-hellmann) wrote :

It also looks like this may affect libraries other than minidom. The python-dev team (Christian Heimes) is working on a patch. I would imagine we could get a copy of that patch if we contact them.

Revision history for this message
Joshua Harlow (harlowja) wrote :

Same here, looking for someway to get minidom to stop doing what its doing, more patches the better :)

Revision history for this message
Joshua Harlow (harlowja) wrote :

This example causes the expat library to turn off entity expansion.

Looking at the c code there is an odd section that this causes to run.

void XMLCALL
XML_SetDefaultHandler(XML_Parser parser,
                      XML_DefaultHandler handler)
{
  defaultHandler = handler;
  defaultExpandInternalEntities = XML_FALSE;
}

Basically be setting the default handler to anything it will cause expansion to stop.
Normal xml documents seem to flow through just fine.

Revision history for this message
Joshua Harlow (harlowja) wrote :

We could just monkey-patch the previous SafeExpatParser over the ExpatParser and all minidom calls should proceed without expansion, thats one way (or people can be forced to use this parser, wherever a parse occurs).

Revision history for this message
Joshua Harlow (harlowja) wrote :

This fix is a little different, just by causing the C function to be activated (and then restoring the previous handler) will accomplish the same goal (odd stuff....)

Revision history for this message
Thierry Carrez (ttx) wrote :

@Doug: I'm not sure this should be considered a Python vulnerability, unless there is no way of disabling parsing of extensions in minidom. You could argue that the default should be safer, or that documentation should be clearer...

Who do you recommend we talk to at this point ? Security team ? Christian (email?) ?

Dan Prince (dan-prince)
Changed in nova:
assignee: nobody → Dan Prince (dan-prince)
Revision history for this message
Doug Hellmann (doug-hellmann) wrote :

@Thierry, Based on the informal conversation I had with Jesse Noller yesterday I definitely recommend contacting the security team. Given that they are working on some sort of patch already, they may have done analysis that tells them this is an issue with the stdlib. At the very least the example cases uncovered by this bug report could give them more tests for that patch. If it turns out the solution is better runtime configuration, we may be able to influence their default settings or the API for configuring the parser.

In any case, Jesse strongly encouraged us to talk to them. I would do it, but it would be better for someone with stronger knowledge of the issue to have that conversation.

Revision history for this message
Joshua Harlow (harlowja) wrote :

So a little of how I traced this down.

  - Figured out what exactly the minidom parseString was doing when it created a parser (when none was provided).
  - This seemed to then go into the code @ http://svn.python.org/view/python/trunk/Lib/xml/dom/minidom.py?revision=75305&view=markup#l1917
  - Note that it seems to jump into 2 different DOM impls depending on if a parser is provided or not so first I tried to see if I could
    monkey-patch out the parser it was 'creating' when no parser was selected, basically by trying to patch out the function @
    http://svn.python.org/view/python/trunk/Lib/xml/dom/expatbuilder.py?revision=50941&view=markup#l932
  - This is how I then noticed that http://svn.python.org/view/python/trunk/Lib/xml/dom/expatbuilder.py?revision=50941&view=markup#l155
    is what is actually creating the underlying parser (so I was trying to then adjust settings in that underlying parser that would
    make it work like we expected). This is where I realized that self._parser.SetParamEntityParsing(expat.XML_PARAM_ENTITY_PARSING_NEVER)
    isn't actually doing anything, I didn't dive to much into the C code to figure out exactly why this call isn't actually changing anything
    but from initial dive I found http://svn.python.org/view/python/trunk/Modules/expat/xmlparse.c?view=markup#l2215 which seems to be the
    entity expansion/reference code, note from that code there is logic around 'XML_ERROR_RECURSIVE_ENTITY_REF;' but this doesn't stop the
    case we are seeing that actually isn't recursive. This code then eventually calls http://svn.python.org/view/python/trunk/Modules/expat/xmlparse.c?view=markup#l4665
    which then starts the whole 'doContent()' function over again.
  - So then I was looking back at that C code @ line http://svn.python.org/view/python/trunk/Modules/expat/xmlparse.c?view=markup#l2225
    and was like it seems to be checking 'else if (defaultHandler)' and then stopping entity expansion right there if said handler actually
    exists, which I was like well thats odd. So then I started seeing about replacing this default handler (which apparently does not exist
    on said parsers unless set). This is how I then started looking at http://svn.python.org/view/python/trunk/Modules/expat/xmlparse.c?revision=77680&view=markup#l1271
    and seeing if I could just set any handler on this parser to stop it from doing what it was doing, so this is how I discovered that setting any
    default handler will cause 'defaultExpandInternalEntities = XML_TRUE;' to be called, which is then how i stumbled upon
    http://svn.python.org/view/python/trunk/Modules/expat/xmlparse.c?view=markup#l2257 and this resulted in me messing with the default handler to see what I could
    set (anything actually) to turn off entity expansion.

End of chapter, josh vs the DTD beast.

Revision history for this message
Dan Prince (dan-prince) wrote :

@harlowja: Thanks for the info.

@ttx: I've had similar luck in trying to get minidom to stop parsing XML entities. Nothing obvious seems to work... What Josh posted in fix.py definately stops entity expansion. This approach definately falls into the "work around" category but given the lack of a better solution I'd say we go with it for now. If we get a better solution from the Python security team we can drop it in as a replacement too. I'm going to post some Nova patches which use this approach for Grizzly (above), Folsom, and Essex this afternoon.

Revision history for this message
Dan Prince (dan-prince) wrote :

I'm seeing a minor XML formatting issue. For the most part tests are passing... but I'm having to change two test cases when using the custom minidom parser. This fix is to do this...

+++ b/nova/tests/integrated/api_samples/os-security-groups/security-group-post-r
@@ -1,5 +1,3 @@
 <security_group name="%(group_name)s">
- <description>
- description
- </description>
+ <description>description</description>
 </security_group>

Revision history for this message
Joshua Harlow (harlowja) wrote :

I also tried tracking down a little of what the 3 enums were doing @ http://svn.python.org/view/python/trunk/Modules/expat/expat.h?revision=47033&view=markup#l853 but didn't go to far, I tried seeing if somehow those enums weren't getting mapped correctly (they should be 0, 1, 2 in typical c compilers) but that didn't change anything.

Some points these affect:

http://svn.python.org/view/python/trunk/Modules/expat/xmlparse.c?revision=77680&view=markup#l1409

The only real place this seems used:

http://svn.python.org/view/python/trunk/Modules/expat/xmlparse.c?revision=77680&view=markup#l3328

Note that its not used in the actual doContent function, http://svn.python.org/view/python/trunk/Modules/expat/xmlparse.c?revision=77680&view=markup#l2139 but I might have missed something.

Revision history for this message
Dan Prince (dan-prince) wrote :

Okay. Fixed the XML formatting issue. Had to tweak Nova's wsgi.py extract_text function a bit. Upcoming grizzly patch is better.

Dan Prince (dan-prince)
Changed in nova:
status: Confirmed → In Progress
Changed in cinder:
assignee: nobody → Dan Prince (dan-prince)
status: Confirmed → In Progress
Revision history for this message
Doug Hellmann (doug-hellmann) wrote :

@Joshua - The latest versions of Python are in hg. I don't know if they've been mirroring hg changes back to the svn repository. See http://hg.python.org/cpython/file/5bf91dfb1e34/Modules/expat/xmlparse.c for the Python 2.7 version of xmlparse.c.

Revision history for this message
Joshua Harlow (harlowja) wrote :

@Doug - good to know, looks like similar functions, but newer updated files, thx!

Revision history for this message
Christian Heimes (heimes) wrote :

Hello,

I'm a Python core developer and member of the Python Security Response Team. I have found the vulnerability in Python's standard library XML parser and libexpat a couple of months ago. I have been working on a patch for a while but the patch is not ready yet. The libexpat and stdlib's XML parser are vulnerable to three kinds of XML attacks related to inline DTDs and ENTITY.

A quick and dirty workaround is to reject all XML data that contain the strings '<!ENTITY' and its utf-16-be, utf-16-le and utf-32 variants. That may generate false positive hits in CDATA sections but the likelihood is very small. Your code should also limit the maximum length of the input string and make sure it's not vulnerable to gzip bombs.

I'm going to read this thread now ...

Christian

Revision history for this message
Christian Heimes (heimes) wrote :

Two quicks comments.

1) It's sufficient and a little bit faster to write:

class ExpatParserNoEntity(ExpatParser):
    def reset(self):
        ExpatParser.reset(self)
        self._parser.DefaultHandler = None

None disables the feature completely

2) You should definitely disable entity expansion in LXML. Although libxml2 protects from excessive exponential expansion it's still vulnerable to quadratic blowup. I'm in contact with a libxml2 developer.

Revision history for this message
Christian Heimes (heimes) wrote :

I compared your patches with my patch for Python's standard library. Your approach seems to work well but I'm not sure if it intercepts all possible attacks. When you overwrite DefaultHandler you don't get any error messages when an entity occurs in the XML document.

I like to suggest a different approach. You can either reject all XML documents with an inline DTD (SOAP does it) or you can disallow entity declarations:

class ProtectedExpatParser(ExpatParser):
    def __init__(self, forbid_dtd=True, forbid_entities=True,
                 *args, **kwargs):
        # Python 2.x old style class
        ExpatParser.__init__(self, *args, **kwargs)
        self.forbid_dtd = forbid_dtd
        self.forbid_entities = forbid_entities

    def start_doctype_decl(self, name, sysid, pubid, has_internal_subset):
        raise ValueError("Inline DTD forbidden")

    def entity_decl(self, entityName, is_parameter_entity, value, base, systemId, publicId, notationName):
        raise ValueError("<!ENTITY> forbidden")

    def unparsed_entity_decl(self, name, base, sysid, pubid, notation_name):
        # expat 1.2
        raise ValueError("<!ENTITY> forbidden")

    def reset(self):
        ExpatParser.reset(self)
        if self.forbid_dtd:
            self._parser.StartDoctypeDeclHandler = self.start_doctype_decl
        if self.forbid_entities:
            self._parser.EntityDeclHandler = self.entity_decl
            self._parser.UnparsedEntityDeclHandler = self.unparsed_entity_decl

With this parser errors and attacks aren't ignored silently.

Your fix for LXML may contain a flaw, too. LXML used to have an issue when parser is shared by multiple threads. I don't know if your are using threads nor if LXML still has a problem with shared parser instances. At work we are using a thread local storage to have a parser context for each thread.

Revision history for this message
Dan Prince (dan-prince) wrote :

Christian:

Thanks for having a look at this. ProtectedExpatParser looks good to me, is self documenting, and I like that it doesn't silently ignore things as well. Thanks for this.

ttx: The incoming Nova patches use the new ProtectedExpatParser implementation...

Revision history for this message
Dan Prince (dan-prince) wrote :
Revision history for this message
Dan Prince (dan-prince) wrote :
Revision history for this message
Dan Prince (dan-prince) wrote :
Revision history for this message
Dan Prince (dan-prince) wrote :
Revision history for this message
Dan Prince (dan-prince) wrote :
Revision history for this message
Dan Prince (dan-prince) wrote :

ttx: Okay. Cinder patches are attached.

Also, I looked again and although Quantum calls minidom.parse... once in wsgi.py it doesn't appear to be used. Note: We will certainly need the etree fix to be added once this hits: https://review.openstack.org/#/c/19998/13/quantum/wsgi.py (upstream branch under review to add XML support for the Quantum V2 API).

I think olso can wait till after we disclose this right?

Changed in quantum:
status: Confirmed → Incomplete
Revision history for this message
Thierry Carrez (ttx) wrote :

@Dan: Thx for the updated patches. Agree that Oslo can be fixed after disclosure.

@Christian: Thx for reaching out, still busy traveling back from FOSDEM and will be fully up to speed again starting Wednesday. I'm handling the disclosure process for this bug and I was wondering if we (OpenStack) can proceed in responsible disclosure or if you wanted to coordinate the python stdlib security fix(es) with ours. I see some urgency in the fact that this issue was independently reported to us 4 times over the last two weeks, which means there is research/pentest activity around this. How far are you from public disclosure on your side ?

Revision history for this message
Christian Heimes (heimes) wrote :

LXML still suggests a parser pool for threaded applications.

http://lxml.de/element_classes.html
To avoid interfering with other modules, however, it is usually a better idea to use a dedicated parser for each module (or a parser pool when using threads) and then register the required lookup scheme only for this parser.

Here is some example code from our code at work. We are using a custom Element class and thread local storage for parser instances.

import threading
from lxml import etree

class RestrictedElement(etree.ElementBase):
    __slots__ = ()
    # blacklist = (etree._Element, etree._ProcessingInstruction, etree._Comment)
    blacklist = etree._Element

    def __iter__(self):
        blacklist = self.blacklist
        for child in super(RestrictedElement, self).__iter__():
            if isinstance(child, blacklist):
                continue
            yield child

    def iterchildren(self, tag=None, reversed=False):
        blacklist = self.blacklist
        children = super(RestrictedElement, self).iterchildren(tag=tag,
                                                               reversed=reversed)
        for child in children:
            if isinstance(child, blacklist):
                continue
            yield child

    # you may need to overwrite getchildren, find, findall and more if you use them

class ParserTLS(threading.local):
    parser_cfg = {
        'resolve_entities': False,
        'remove_comments': True,
        'remove_pis': True,
    }

    @property
    def parser(self):
        parser = getattr(self, "_parser", None)
        if parser is None:
            parser = etree.XMLParser(**self.parser_cfg)
            lookup = etree.ElementDefaultClassLookup(element=RestrictedElement)
            parser.set_element_class_lookup(lookup)
            self._parser = parser
        return parser

if __name__ == "__main__":
    tls = ParserTLS()
    tree = etree.parse("test.xml", parser=tls.parser)
    print tree.getroot().text
    print list(tree.getroot().iterchildren())

@Thierry:
I'll ask on the PSRT list. My patch for expat won't be ready until Wednesday but we can release the restricted expat parsers classes for etree, sax and minidom as hotfixes. I'm waiting for some code review now. I also need to get back to the libxml2 guys ASAP.

Revision history for this message
Thierry Carrez (ttx) wrote :

@Christian: let me explain our disclosure system to see how we can align.

We push the patches to a set of downstream stakeholders (think distros) so that they can coordinate their own updates with ours. That restricted disclosure window usually lasts 3-5 business days, and then we push the patches publicly. So it would be great to come up with a common "public date" and then we can align our responsible disclosure process to that. For example if we agree we can go public Tuesday next week, I can send patches to the distros wednesday. Let me know what public date sounds doable for the Python side.

Revision history for this message
Thierry Carrez (ttx) wrote :

Vish/Russell/Mikal: could you review the proposed Nova patches ?
Cinder-core: same for Cinder patches.

Please +1 on the bug as a comment and don't disclose those patches publicly.

Revision history for this message
Vish Ishaya (vishvananda) wrote :

+1 on nova patches

Revision history for this message
John Griffith (john-griffith) wrote :

+1 for the Cinder versions

Revision history for this message
Thierry Carrez (ttx) wrote :

Please see proposed description:
-------------
Title: Information leak and Denial of Service using XML entities
Reporter: Jonathan Murray (NCC Group), Joshua Harlow (Yahoo!)
Products: Keystone, Nova, Cinder
Affects: All versions

Description:
Jonathan Murray from NCC Group and Joshua Harlow from Yahoo! independently reported a vulnerability in the parsing of XML requests in Keystone. By using entities in XML requests, an unauthenticated attacker may consume excessive resources on the Keystone, Nova or Cinder API servers, resulting in a denial of service and potentially a crash. Authenticated attackers may also leverage XML entities to read the content of a local file on the Keystone API server. This only affects servers with XML support enabled.
--------------

Revision history for this message
Thierry Carrez (ttx) wrote :

Quantum Grizzly is now affected (since yesterday) so patch should be ported to it... however it should not block security fix since it only affects version under development.

Changed in quantum:
status: Incomplete → Confirmed
Changed in nova:
status: In Progress → Triaged
Changed in cinder:
status: In Progress → Triaged
Changed in oslo:
status: New → Confirmed
Revision history for this message
Thierry Carrez (ttx) wrote :

Russell, Mikal, Cinder-core: we could use another +1 on the patch sets

Revision history for this message
Thierry Carrez (ttx) wrote :

Proposed description failed to credit Stuart Stent, see new version:
-------------
Title: Information leak and Denial of Service using XML entities
Reporter: Jonathan Murray (NCC Group), Joshua Harlow (Yahoo!), Stuart Stent
Products: Keystone, Nova, Cinder
Affects: All versions

Description:
Jonathan Murray from NCC Group, Joshua Harlow from Yahoo! and Stuart Stent independently reported a vulnerability in the parsing of XML requests in Keystone, Nova and Cinder. By using entities in XML requests, an unauthenticated attacker may consume excessive resources on the Keystone, Nova or Cinder API servers, resulting in a denial of service and potentially a crash. Authenticated attackers may also leverage XML entities to read the content of a local file on the Keystone API server. This only affects servers with XML support enabled.
--------------

Revision history for this message
Dan Prince (dan-prince) wrote :

ttx: +1 for the proposed description.

Revision history for this message
Russell Bryant (russellb) wrote :

description sounds good to me

Revision history for this message
Russell Bryant (russellb) wrote :

Nova patches look good to me

Revision history for this message
Thierry Carrez (ttx) wrote :

Sent to downstream stakeholders

Proposed public disclosure date/time:
*Tuesday February 19th, 1500UTC*

Revision history for this message
Christian Heimes (heimes) wrote :

Have you taken care of lxml, too? I just noticed that lxml always resolves and loads external entities with file:// URLs. An attacker can possibly load and retrieve all (XML) files that the service is allowed to access.

Example:

external_file.xml
==============
<!DOCTYPE external [
<!ENTITY ee SYSTEM "file:///PATH/TO/simple.xml">
]>
<root>&ee;</root>

simple.xml
=========
<!-- comment -->
<root>
   <element key='value'>text</element>
   <element>text</element>tail
   <empty-element/>
</root>

>>> from lxml import etree
>>> tree = etree.parse("external_file.xml")
>>> print(etree.tostring(tree))
<root><!-- comment -->
<root>
   <element key="value">text</element>
   <element>text</element>tail
   <empty-element/>
</root>
</root>

Revision history for this message
Dolph Mathews (dolph) wrote :

@Christian Heimes: The patches for keystone are applicable to lxml.etree, and set resolve_entities=False to address that specific issue.

Revision history for this message
Thierry Carrez (ttx) wrote :

> Please use CVE-2013-0278 for Keystone
> Please use CVE-2013-0279 for Cinder
> Please use CVE-2013-0280 for Nova

Thierry Carrez (ttx)
Changed in oslo:
importance: Undecided → High
Revision history for this message
Christian Heimes (heimes) wrote :

For reference the Python CVE numbers are

CVE-2013-1664 Unrestricted entity expansion induces DoS vulnerabilities in Python XML libraries (XML bomb)
CVE-2013-1665 External entity expansion in Python XML libraries inflicts potential security flaws and DoS vulnerabilities

Thierry Carrez (ttx)
summary: - DoS through XML entity expansion
+ DoS through XML entity expansion (CVE-2013-1664)
Dan Prince (dan-prince)
Changed in oslo:
assignee: nobody → Dan Prince (dan-prince)
Changed in quantum:
assignee: nobody → Dan Prince (dan-prince)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (master)

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

Changed in nova:
status: Triaged → In Progress
Changed in cinder:
status: Triaged → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

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

Thierry Carrez (ttx)
information type: Private Security → Public Security
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (stable/folsom)

Fix proposed to branch: stable/folsom
Review: https://review.openstack.org/22311

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (stable/folsom)

Fix proposed to branch: stable/folsom
Review: https://review.openstack.org/22312

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (stable/essex)

Fix proposed to branch: stable/essex
Review: https://review.openstack.org/22313

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (stable/folsom)

Fix proposed to branch: stable/folsom
Review: https://review.openstack.org/22314

Changed in keystone:
status: Triaged → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (stable/essex)

Fix proposed to branch: stable/essex
Review: https://review.openstack.org/22316

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to nova (master)

Reviewed: https://review.openstack.org/22309
Committed: http://github.com/openstack/nova/commit/59933249054bf71ec963585198583fe78050c9d6
Submitter: Jenkins
Branch: master

commit 59933249054bf71ec963585198583fe78050c9d6
Author: Dan Prince <email address hidden>
Date: Fri Feb 1 17:04:27 2013 -0500

    Add a safe_minidom_parse_string function.

    Adds a new utils.safe_minidom_parse_string function and
    updates external API facing Nova modules to use it.
    This ensures we have safe defaults on our incoming API XML parsing.

    Internally safe_minidom_parse_string uses a ProtectedExpatParser
    class to disable DTDs and entities from being parsed when using
    minidom.

    Fixes LP Bug #1100282.

    Change-Id: Ib90d6379320ff1d007f8a661f7ddaa286ba6918e

Changed in nova:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (stable/folsom)

Reviewed: https://review.openstack.org/22314
Committed: http://github.com/openstack/keystone/commit/8a2274595ac628b2373eab0cb14690f866b7a024
Submitter: Jenkins
Branch: stable/folsom

commit 8a2274595ac628b2373eab0cb14690f866b7a024
Author: Dolph Mathews <email address hidden>
Date: Tue Feb 19 09:04:11 2013 -0600

    Disable XML entity parsing

    Fixes bug 1100282 and bug 1100279.

    Change-Id: Ibf2d73bca17b689cfa2dfd29eb15ea6e7458a123

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.openstack.org/22310
Committed: http://github.com/openstack/cinder/commit/91ccd1501acb1316b05a0dc010601ad85a9ebd3b
Submitter: Jenkins
Branch: master

commit 91ccd1501acb1316b05a0dc010601ad85a9ebd3b
Author: Dan Prince <email address hidden>
Date: Sun Feb 3 21:54:33 2013 -0500

    Add a safe_minidom_parse_string function.

    Adds a new utils.safe_minidom_parse_string function and
    updates external API facing Cinder modules to use it.
    This ensures we have safe defaults on our incoming API XML parsing.

    Internally safe_minidom_parse_string uses a ProtectedExpatParser
    class to disable DTDs and entities from being parsed when using
    minidom.

    Fixes LP Bug #1100282.

    Change-Id: Iff8340033c8e8db58184944a1bf705e16b8b3e03

Changed in cinder:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (stable/folsom)

Reviewed: https://review.openstack.org/22311
Committed: http://github.com/openstack/cinder/commit/fcf249d1f06938280d841cb13b61556971a58e0c
Submitter: Jenkins
Branch: stable/folsom

commit fcf249d1f06938280d841cb13b61556971a58e0c
Author: Dan Prince <email address hidden>
Date: Sun Feb 3 22:25:12 2013 -0500

    Add a safe_minidom_parse_string function.

    Adds a new utils.safe_minidom_parse_string function and
    updates external API facing Cinder modules to use it.
    This ensures we have safe defaults on our incoming API XML parsing.

    Internally safe_minidom_parse_string uses a ProtectedExpatParser
    class to disable DTDs and entities from being parsed when using
    minidom.

    Fixes LP Bug #1100282 for Folsom.

    Change-Id: Ie8ae7a6e12fbf51de406d10ca21072140374abf5

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to nova (stable/folsom)

Reviewed: https://review.openstack.org/22312
Committed: http://github.com/openstack/nova/commit/2ae74f8222058e475350458ca0c820adb910582c
Submitter: Jenkins
Branch: stable/folsom

commit 2ae74f8222058e475350458ca0c820adb910582c
Author: Dan Prince <email address hidden>
Date: Sat Feb 2 11:34:25 2013 -0500

    Add a safe_minidom_parse_string function.

    Adds a new utils.safe_minidom_parse_string function and
    updates external API facing Nova modules to use it.
    This ensures we have safe defaults on our incoming API XML parsing.

    Internally safe_minidom_parse_string uses a ProtectedExpatParser
    class to disable DTDs and entities from being parsed when using
    minidom.

    Fixes LP Bug #1100282 for Folsom.

    Change-Id: I6a4051b5e66f3ce5a330b2589c42e6e9e5b9268e

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to nova (stable/essex)

Reviewed: https://review.openstack.org/22313
Committed: http://github.com/openstack/nova/commit/c0a10dbd8f903ebb2e417cc821cf8555bee8d188
Submitter: Jenkins
Branch: stable/essex

commit c0a10dbd8f903ebb2e417cc821cf8555bee8d188
Author: Dan Prince <email address hidden>
Date: Sat Feb 2 14:32:12 2013 -0500

    Add a safe_minidom_parse_string function.

    Adds a new utils.safe_minidom_parse_string function and
    updates external API facing Nova modules to use it.
    This ensures we have safe defaults on our incoming API XML parsing.

    Internally safe_minidom_parse_string uses a ProtectedExpatParser
    class to disable DTDs and entities from being parsed when using
    minidom.

    Fixes LP Bug #1100282 for Essex.

    Change-Id: I815b27ff2845293e3d6771ff8f99944ec08ccbd5

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (master)

Reviewed: https://review.openstack.org/22315
Committed: http://github.com/openstack/keystone/commit/2afe8e46893ca27ea9d61f29419d0ec23a6d8db3
Submitter: Jenkins
Branch: master

commit 2afe8e46893ca27ea9d61f29419d0ec23a6d8db3
Author: Dolph Mathews <email address hidden>
Date: Tue Feb 19 09:00:40 2013 -0600

    Disable XML entity parsing

    Fixes bug 1100282 and bug 1100279.

    Change-Id: I6a7c9e7110e1c7890205d6e4550ab46295c68906

Changed in keystone:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (stable/essex)

Reviewed: https://review.openstack.org/22316
Committed: http://github.com/openstack/keystone/commit/8945567b5ec39c7f32f27aec4eccf230cc86646c
Submitter: Jenkins
Branch: stable/essex

commit 8945567b5ec39c7f32f27aec4eccf230cc86646c
Author: Dolph Mathews <email address hidden>
Date: Tue Feb 19 09:08:41 2013 -0600

    Disable XML entity parsing

    Fixes bug 1100282 and bug 1100279.

    Change-Id: Idd3989356dfededc3d863770f0ca1661c1d45782

Thierry Carrez (ttx)
Changed in keystone:
milestone: none → grizzly-3
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in nova:
milestone: none → grizzly-3
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in cinder:
milestone: none → grizzly-3
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in quantum:
milestone: none → grizzly-rc1
Changed in oslo:
milestone: none → grizzly-rc1
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to oslo-incubator (master)

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

Changed in oslo:
assignee: Dan Prince (dan-prince) → Davanum Srinivas (DIMS) (dims-v)
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to quantum (master)

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

Changed in quantum:
assignee: Dan Prince (dan-prince) → Davanum Srinivas (DIMS) (dims-v)
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to oslo-incubator (master)

Reviewed: https://review.openstack.org/22988
Committed: http://github.com/openstack/oslo-incubator/commit/0d7417cff68e74f636d371529998e275e2765be8
Submitter: Jenkins
Branch: master

commit 0d7417cff68e74f636d371529998e275e2765be8
Author: Davanum Srinivas <email address hidden>
Date: Tue Feb 26 11:26:24 2013 -0500

    Port safe parsing with minidom patches from Nova

    Prevent attacks through xml entity expansion etc.

    Fixes LP# 1100282

    Change-Id: I391531deac122697556c282184c8f8890ea66489

Changed in oslo:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to quantum (master)

Reviewed: https://review.openstack.org/23024
Committed: http://github.com/openstack/quantum/commit/1f716e3effe1ad6eeb042a11f06a5c89498a34b8
Submitter: Jenkins
Branch: master

commit 1f716e3effe1ad6eeb042a11f06a5c89498a34b8
Author: Davanum Srinivas <email address hidden>
Date: Tue Feb 26 15:43:50 2013 -0500

    Prevent DoS through XML entity expansion

    Add a ProtectedXMLParser that overrides the
    doctype declaration handler. The handler simply
    throws an exception and prevents any further
    parsing of the incoming xml.

    Fixes LP Bug #1100282

    Change-Id: I6488e1a6a52326006e7e7927ece5b5939b72e83e

Changed in quantum:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in quantum:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in oslo:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in keystone:
milestone: grizzly-3 → 2013.1
Thierry Carrez (ttx)
Changed in quantum:
milestone: grizzly-rc1 → 2013.1
Thierry Carrez (ttx)
Changed in nova:
milestone: grizzly-3 → 2013.1
Thierry Carrez (ttx)
Changed in cinder:
milestone: grizzly-3 → 2013.1
Thierry Carrez (ttx)
summary: - DoS through XML entity expansion (CVE-2013-1664)
+ [OSSA 2013-004] DoS through XML entity expansion (CVE-2013-1664)
Changed in ossa:
assignee: nobody → Thierry Carrez (ttx)
status: New → Fix Released
Revision history for this message
Morgan Fainberg (mdrnstm) wrote :

Old bug cleanup, this was properly merged etc, looks like status was just missed.

Sean Dague (sdague)
no longer affects: nova/essex
Nassim (nassim-abbaoui)
information type: Public Security → Private Security
information type: Private Security → Private
information type: Private → Public Security
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.