[OSSA 2013-023] Potential unsafe XML usage (CVE-2013-4179, CVE-2013-4202)

[Thierry Carrez]

Grant Murphy (<email address hidden>) conducted an audit of OpenStack and reported the following potential XML related vulnerabilities. These may well not be exploitable, we need to doublecheck them.

===================================================
Issue #1 (CWE-776) - Widespread and inconsistent usage of XML libraries that are vulnerable to various XML resource exhaustion attack vectors. This was mostly addressed in bug 1100282 however I can still find instances of minidom.parseString used directly in contributors code:

nova/nova/virt/libvirt/driver.py
1640: dom = minidom.parseString(xml)
1657: dom = minidom.parseString(xml)

nova/nova/virt/xenapi/vm_utils.py
1501: rrd = minidom.parseString(xml)
1542: doc = minidom.parseString(xml)

nova/nova/api/openstack/compute/contrib/security_groups.py
542: root = minidom.parseString(req.body)

nova/nova/api/openstack/compute/contrib/security_group_default_rules.py
76: dom = minidom.parseString(string)

cinder/cinder/api/contrib/backups.py
85: dom = minidom.parseString(string)
104: dom = minidom.parseString(string)

cinder/cinder/api/contrib/volume_transfer.py
65: dom = minidom.parseString(string)
83: dom = minidom.parseString(string)

Furthermore there are inconsistencies in the fix between components. For example in Quantum they use a ProtectedXMLParser in quantum/quantum/wsgi.py for XMLDeserialization. The implementation is as follows:

class ProtectedXMLParser(etree.XMLParser):
    def __init__(self, *args, **kwargs):
        etree.XMLParser.__init__(self, *args, **kwargs)
        self._parser.StartDoctypeDeclHandler = self.start_doctype_decl

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

    def doctype(self, name, pubid, system):
        raise ValueError(_("Inline DTD forbidden"))

Although etree.XMLParser does not automatically expand external entities by default it is vulnerable to billion laughs and quadratic blowup according to the diffusedxml documentation. (I'm not sure this is still accurate though).

For consistency alone I would recommend shifting these libraries across the board:
 - https://pypi.python.org/pypi/defusedxml/
 - https://pypi.python.org/pypi/defusedexpat/

========================================================
Issue #2 (CWE-112)- Missing schema validation. There are several instances where XML data is processed from an external source that is not validated against an XML schema. This is a best practice that should be considered.

========================================================
Issue #3 (Probably not a bug)- HTTP parameters used directly in XML output. The output appears to use sax.xhtml_escape so in theory is safe. Could use a whitelist reject unexpected input.

The case I'm specifically talking about is the BucketHandler class in nova/nova/objectstore/s3server.py. This does:

  prefix = self.request.params.get("prefix", "")
  marker = self.request.params.get("marker", "")

And uses these values directly in render_xml (results sent to end user).
This one is probably harmless but thought I'd mention it anyway.

[Thierry Carrez (ttx)]

Analysis in Nova:

nova/nova/virt/libvirt/driver.py
What's getting parsed is XML from libvirt, not user-provided XML requests. So that's safe.

nova/nova/virt/xenapi/vm_utils.py
What's getting parsed is XML from XenServer, not user-provided XML requests, so that's safe.

nova/nova/api/openstack/compute/contrib/security_groups.py
While most calls use xmlutil.safe_minidom_parse_string (which is safe), there is one call in _extend_servers that still uses minidom.parseString(req.body) and therefore looks vulnerable.

nova/nova/api/openstack/compute/contrib/security_group_default_rules.py
Uses pure minidom in its XML deserializer, so probably vulnerable.

[Thierry Carrez (ttx)]

Quantum analysis:
My understanding is that million laughs/quadratic blowup needs DTD evaluation, and the ProtectedXMLParser protects against reading an included DTD, so that sounds safe.

I agree that more consistency across the board would be welcome though.

[Thierry Carrez (ttx)]

Looks vulnerable to me (contributed backups API extension)

[Thierry Carrez (ttx)]

Adding nova-core and cinder-core for help patching

[Daniel Berrange (berrange)]

Any patches should also include test case which validates that no code in GIT uses the vulnerable APIs, so we don't regress in this yet again.

[Thierry Carrez (ttx)]

We should push a minimal patch to fix the vulnerabilities first, that we can backport to stable branches.
Then for Havana and beyond we should push defusedxml parsers across the board.

[Michael Still (mikal)]

A patch for nova is attached.

[Thierry Carrez (ttx)]

Could we have a patch for Cinder ?

[John Griffith (john-griffith)]

cinder patch here

[Thierry Carrez (ttx)]

Looks good, could we have a cinder-core pre-approval here ? Patch is trivial so one should be enough

[Thierry Carrez (ttx)]

We need nova-core pre-approval on Mikal's patch too.

[Pádraig Brady (p-draigbrady)]

+2 On nova patch

[Thierry Carrez (ttx)]

Proposed impact description:

Title: Denial of Service using XML entities in some Nova and Cinder extensions
Reporter: Grant Murphy (Red Hat)
Products: Nova, Cinder
Affects: Grizzly

Description:
Grant Murphy from Red Hat reported that vulnerabilities in XML request parsers were not fully patched in OSSA 2013-004. By leveraging XML entity expansion in specific extensions, an unauthenticated attacker may still consume excessive resources on the Nova or Cinder API servers, resulting in a denial of service and potentially a crash. Only Nova setups making use of the security group extension in Grizzly are affected. Only Cinder setups making use of the backups or volume transfer API extension in Grizzly are affected.

[Thierry Carrez (ttx)]

The affected code was present in Grizzly but not in Folsom.

[Jeremy Stanley (fungi)]

Thierry's impact description in comment #14 looks sane to me.

[Jeremy Stanley (fungi)]

Sorry, Thierry's impact description in comment #13 looks sane to me.

[Mike Perez (thingee)]

+2 on the cinder patch.

[Thierry Carrez (ttx)]

CVE requested, no answer from Kurt yet

[Thierry Carrez (ttx)]

Sent CVE request again

[Thierry Carrez (ttx)]

Refreshed nova/havana patch

[Thierry Carrez (ttx)]

nova/grizzly patch backport

[Thierry Carrez (ttx)]

cinder/grizzly patch backport

[Thierry Carrez (ttx)]

cinder/master/gate-devstack-vm-full: PASS
cinder/grizzly/gate-devstack-vm-cinder: PASS

more tests tomorrow.

[Thierry Carrez (ttx)]

Nova tests fail with:

File "/opt/stack/new/nova/nova/api/openstack/compute/contrib/security_groups.py", line 33, in <module>
    from nova.openstack.common import xmlutils
ImportError: cannot import name xmlutils

[Thierry Carrez (ttx)]

missing a file in the grizzly backport. New version attached

[Thierry Carrez (ttx)]

nova/master/gate-devstack-vm-full: PASS
nova/grizzly/gate-devstack-vm-full: PASS

[Thierry Carrez (ttx)]

Version that doesn't fail pep8

[Thierry Carrez (ttx)]

Version that doesn't fail pep8

[Thierry Carrez (ttx)]

cinder/master/py27: PASS
cinder/master/pep8: PASS
nova/grizzly/py27: PASS
nova/grizzly/pep8: PASS
nova/master/py27: PASS
nova/master/pep8: PASS

[Thierry Carrez (ttx)]

Sent to downstream stakeholders
Proposed public disclosure date/time: Thursday, August 8, 1500UTC.

[Thierry Carrez (ttx)]

From Kurt:

Please use CVE-2013-4179 for Denial of Service using XML entities in
Nova extensions

Please use CVE-2013-4202 for Denial of Service using XML entities in
Cinder extensions

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/grizzly
Review: https://review.openstack.org/40883

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/40881
Committed: http://github.com/openstack/cinder/commit/4ad95dba4fccbbc0df923dea0dc9e5c3ac9f4cc2
Submitter: Jenkins
Branch: master

commit 4ad95dba4fccbbc0df923dea0dc9e5c3ac9f4cc2
Author: Thierry Carrez <email address hidden>
Date: Thu Aug 8 12:13:52 2013 +0200

    Use utils.safe_minidom_parse_string in extensions

    Use utils.safe_minidom_parse_string in extensions that were still
    using potentially-unsafe minidom.

    Fixes bug 1190229

    Change-Id: I43afb2e188bbea99ea30fe6cb2eb1aeedc4ddfd4

[Thierry Carrez (ttx)]

OSSA 2013-023

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/40883
Committed: http://github.com/openstack/cinder/commit/2023eecc4b1a35daf42a64fa01967ed12c7d017b
Submitter: Jenkins
Branch: stable/grizzly

commit 2023eecc4b1a35daf42a64fa01967ed12c7d017b
Author: John Griffith <email address hidden>
Date: Thu Aug 8 12:10:30 2013 +0200

    Use utils.safe_minidom_parse_string in extensions

    Use utils.safe_minidom_parse_string in extensions that were still
    directly using potentially-unsafe minidom.

    Grizzly backport of John Griffith's master patch.

    Fixes bug 1190229

    Change-Id: I43afb2e188bbea99ea30fe6cb2eb1aeedc4ddfd4

[Yang Yu (yuyangbj)]

I will be out of the office starting 2013-09-05 and will not return until
2013-09-15.

I will be on my marriage leave from 9/5 to 9/15, for any urgent issue
please call me before 9/7.

For daily work, please ask my scrum master Zhu Zhu for help.
For glance issue, please ask glance SME Feilong Wang for help.
For defect report, there will be no report next week.