bandit report: use defusedxml to avoid XML attack

Bug #1732155 reported by Jane Lee on 2017-11-14
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Wishlist
Eric Harney
OpenStack Security Advisory
Undecided
Unassigned

Bug Description

According to https://docs.openstack.org/bandit/latest/api/bandit.blacklists.html

Using various XLM methods to parse untrusted XML data is known to be vulnerable to XML attacks. Methods should be replaced with their defusedxml equivalents.

Jane Lee (lijing) on 2017-11-14
Changed in cinder:
assignee: nobody → Jane Lee (lijing)

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.

What's the affected code?

Changed in ossa:
status: New → Incomplete
description: updated
Eric Harney (eharney) wrote :

As far as I can tell, this is constrained to a handful of Cinder drivers and their unit tests:

<skip similar imports in unit tests>
cinder/volume/drivers/dell_emc/vmax/utils.py:from xml.dom import minidom
cinder/volume/drivers/dothill/dothill_client.py:from lxml import etree
cinder/volume/drivers/fujitsu/eternus_dx_common.py:from xml.etree.ElementTree import parse
cinder/volume/drivers/huawei/huawei_conf.py:from xml.etree import ElementTree as ET
cinder/volume/drivers/nec/volume_common.py:from lxml import etree
cinder/volume/drivers/netapp/dataontap/client/api.py:from lxml import etree
cinder/volume/drivers/zadara.py:from lxml import etree

Jane Lee (lijing) on 2017-11-15
summary: - bandit report: use defusedxml to avoid XML attach
+ bandit report: use defusedxml to avoid XML attack
Jeremy Stanley (fungi) wrote :

This bug is missing critical details: in particular, I don't see any mention of an actual attack vector. Is the implication that these drivers are handling untrusted XML data? And from where, the devices themselves? Who is the purported attacker, and how would this be used to their advantage? If there is no clear exploit scenario, we generally triage these sorts of reports as security hardening opportunities (class D in our taxonomy): https://security.openstack.org/vmt-process.html#incident-report-taxonomy

Reports like this which are simply output from running standard inspection tools against projects' source code are generally not worth keeping private anyway, in my opinion.

Agreed with the class D triage, if there are no objections then let's open this bug and close the OSSA task by next week.

Jeremy Stanley (fungi) wrote :

As no objections were raised, I'm triaging this report as a security hardening opportunity now. Thanks!

information type: Private Security → Public
description: updated
Changed in ossa:
status: Incomplete → Won't Fix
tags: added: security
Sean McGinnis (sean-mcginnis) wrote :

Doesn't look like this will be an easy drop in and replace for most of the XML usage. There are a few easy replacements, but if this is really a concern, we will probably need to work with some of the driver maintainers to see how to best replace things.

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

Changed in cinder:
assignee: Jane Lee (lijing) → Sean McGinnis (sean-mcginnis)
status: New → In Progress
Changed in cinder:
importance: Undecided → Wishlist

Reviewed: https://review.openstack.org/528516
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=4137c33922051546d45b6c9aa730433a401e3df1
Submitter: Zuul
Branch: master

commit 4137c33922051546d45b6c9aa730433a401e3df1
Author: Sean McGinnis <email address hidden>
Date: Sat Dec 16 17:38:41 2017 -0600

    Use defusedxml for XML parsing

    The built-in xml module has some vulnerabilities to several known
    XML attacks. While the chances of this are limited with the way
    it is being used by some of the volume drivers, it is still a
    security risk that has been identified and has a mostly painless
    way to be mitigated with the defusedxml package [1].

    There are still some drivers performing XML parsing that are not
    covered by this patch. They need closer analysis to see how to
    best switch to the defusedxml equivalents.

    This patch covers the instances where it was a mostly drop in and
    replace from the native xml functionality to the defusedxml
    alternatives.

    [1] https://github.com/tiran/defusedxml/blob/master/README.md

    Change-Id: I083fc23eab6f712264919a250c6fb57cc0f6a11b
    Partial-bug: #1732155

Changed in cinder:
assignee: Sean McGinnis (sean-mcginnis) → Jane Lee (lijing)
Changed in cinder:
assignee: Jane Lee (lijing) → Eric Harney (eharney)

Reviewed: https://review.openstack.org/519618
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=21362156125cadc0cddbffdc911d15a29c949902
Submitter: Zuul
Branch: master

commit 21362156125cadc0cddbffdc911d15a29c949902
Author: lijing <email address hidden>
Date: Tue Nov 14 18:59:29 2017 +0800

    use defusedxml to avoid XML attack

    According to https://docs.openstack.org/bandit/latest/api/bandit.blacklists.html

    Using various XML methods to parse untrusted XML data is known to be vulnerable
    to XML attacks. Methods should be replaced with their defusedxml equivalents.

    Change-Id: Icdd807c8fd47ce0df3e292eef910e6e6e7610686
    Partial-Bug: #1732155

Sean McGinnis (sean-mcginnis) wrote :

It doesn't cover everything, but at least the defusedxml.lxml module is deprecated and it has been suggested libxml has been updated enough to no longer need this.

Some discussion here: https://github.com/PyCQA/bandit/issues/435

Changed in cinder:
status: In Progress → Invalid
status: Invalid → Won't Fix
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

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