[OSSA 2013-016] Unescaped content embedded in XML (CVE-2013-2161)

[Alex Gaynor]

See the code here: https://github.com/openstack/swift/blob/master/swift/account/server.py#L255 . The ``account`` var is untrusted and should be escaped.

I'm not 100% convinced this is exploitable, however after conferring with Donald Stufft (security engineer at Nebula), neither of us were able to rule it out, so I'm filing as a security issue, better safe than sorry.

[Samuel Merritt (torgomatic)]

I don't know about exploitability, but it's certainly true that an account named AUTH_" produces this little pile of invalid XML on GET:

<?xml version="1.0" encoding="UTF-8"?>
<account name="AUTH_"">
</account>

[Alex Gaynor (alex-gaynor)]

Attached patch should resolve this issue, I did a very limited audit of other uses of XML and didn't see any other similar issues.

[Thierry Carrez (ttx)]

Agree on need to fix. Don't really see how to exploit that since you reap what you seed... and XSS-like scenarios sound very far-fetched.

Adding Rob Clark from OSSG to see if he sees a blatant way of exploiting this that I'd have missed.

[Robert Clark (robert-clark)]

The vulnerability looks real enough though I share the difficulty in identifying a clear attack vector. It's possible that this could be leveraged to attack platforms with Swift UI front ends - uploaders and such but that may be a stretch.

[Alex Gaynor (alex-gaynor)]

Let me know how you guys prefer to handle this, if it's decided to not be a security vulnerability I'll just go through the normal patch submission process.

[Robert Clark (robert-clark)]

There's no obvious way to exploit it but history is littered with 'unexploitable vulnerabilities' that were later exploited. I'd like to see this follow the normal security process.

This is more a comment on general OpenStack security policy than on this specific issue - I believe that when a security vulnerability has been identified and verified it should be treated as a sensitive issue and fixed using the same process that's used when exploitation scenarios have been identified.

[Samuel Merritt (torgomatic)]

Okay, so how do I submit code for this? I was working on bug 1183169 (another bug), and the fix for that touched the code for this bug, so I included a fix. (It's exactly the fix in Alex Gaynor's patch; there's not too many ways to skin this particular cat.)

So, can I submit my patch to Gerrit? Do I need to remove my fix for this bug from that patch? Do I submit a fix, but not reference this bug number? Something completely different?

[Jeremy Stanley (fungi)]

It's best if a security fix can be as discrete and self-contained as possible, since this helps downstream distributors backport it more easily to arbitrary releases they may be packaging/supporting. I'm in the process of drafting an attempted impact description for this and need to get a couple swift core +2 votes in this bug (subscribed them just now) for the fix proposed in comment #2 or some alternative minimal implementation.

[Jeremy Stanley (fungi)]

And to more explicitly answer your questions in comment #7, yes you'll need to remove your fix for this from your other review before you submit it to Gerrit. Otherwise that would require us to consider this issue already publically disclosed and we'd need to rush out a corresponding announcement.

[John Dickinson (notmyname)]

Seeing as the result of this bug causes invalid XML (rather than eg a remote code execution), I'd vote to open this bug publicly.

[Robert Clark (robert-clark)]

The issue is perhaps if this code is parsed anywhere else, while not strictly a direct flaw in OpenStack, allowing OpenStack flaws to be used as an attack vector to compromise deployer's services is very undesirable. I'm ok with it being dealt with openly but would still like to see a CVE.

[Robert Clark (robert-clark)]

To clarify the above 'else' means other systems consuming or integrating with Swift, 3rd party UI's, Log scanners, API metrics engines etc.

[Jeremy Stanley (fungi)]

Proposed impact description...

Title: Unchecked user input in Swift XML
Reporter: Alex Gaynor (Rackspace)
Products: Swift
Affects: All versions

Description:
Alex Gaynor withRackspace reported a vulnerability in the
AccountController class' GET method within the Swift account.server
module. By including unescaped quotes within data passed to the
account variable, unparsable or arbitrary XML can be included.

[Thierry Carrez (ttx)]

About impact, I find the description both too detailed and not sufficiently clear about impact :) My suggestion:

Title: Unchecked user input in Swift XML responses
Reporter: Alex Gaynor (Rackspace)
Products: Swift
Affects: All versions

Description:
Alex Gaynor from Rackspace reported a vulnerability in XML handling within Swift account servers.
By including malicious data into account names, an attacker could potentially generate unparsable or
arbitrary XML responses, which may be used to leverage other vulnerabilities in the calling software.

[Jeremy Stanley (fungi)]

Yes, thanks! I like yours much better. Any more suggestions from anyone before we go forward and request a CVE with that one?

[Samuel Merritt (torgomatic)]

Okay, so now what happens? If we can't submit code to Gerrit for this bug, how do we go about fixing it?

(A reply of "RTFM" with a link to TFM is an entirely reasonable response to this question. I haven't had much luck finding it on my own.)

[Jeremy Stanley (fungi)]

Our Gerrit code review infrastructure is entirely public, so any fixes pushed there would prematurely disclose this private, embargoed security issue.

OpenStack's vulnerability management process is documented at https://wiki.openstack.org/wiki/Vulnerability_Management . We are collectively in the Patch Review and Review Impact Description stages of the Embargoed Vulnerability Management Process.

From the "fixing it" side of things, Swift core developers should hopefully be reading the diff posted here in comment #2 and testing it out privately or proposing alternative fixes as diff attachments to this private security bug. Similarly the Swift PTL reads the Impact description proposed in comment #14, and if necessary suggests modifications to its wording.

Once we've got at least two Swift core reviewers in support of a patch attached this bug, have Swift PTL approval of the Impact Description, have requested and have received a CVE assignment for the bug, then we'll pick a disclosure date and provide advance warning to our registered downstream stakeholders (packagers et al). On the selected disclosure date the pre-approved patch will be published to Gerrit and approved through CI/merged concurrent with publication of an OpenStack Security Advisory to appropriate public mailing lists.

[Samuel Merritt (torgomatic)]

Thank you for the link to TFM. :)

I think the code change in the attachment "escape.diff" is a good* fix for this bug, so +2 from me.

* it could use a unit test, but we can add that on in a separate, public commit once this lands

[John Dickinson (notmyname)]

I echo everything Sam said, particularly w.r.t the quality of the proposed patch.

Here's my take on an impact description (I'm unsure about the correct tense:"were" vs. "are"):

Title: Unchecked user input in Swift XML responses
Reporter: Alex Gaynor (Rackspace)
Products: Swift
Affects: All versions

Description:
Alex Gaynor from Rackspace reported a vulnerability in XML handling within Swift account servers.
Account strings were unescaped in xml listings, and an attacker could potentially generate unparsable or
arbitrary XML responses which may be used to leverage other vulnerabilities in the calling software.

[Thierry Carrez (ttx)]

+1 for John's impact desc

[Jeremy Stanley (fungi)]

Agreed (mine was a bit of a stab in the dark). I'll go ahead and request a CVE with John's wording from comment #19, and then we can get rolling with notifications once that comes back. Thanks!

[Jeremy Stanley (fungi)]

Proposed announcement dates/times...

downstream stakeholders Monday 2013-06-10

public disclosure Thursday 2013-06-13 (15:00 UTC)

[Thierry Carrez (ttx)]

Should be FixCommitted to match embargoed disclosure step

[Jeremy Stanley (fungi)]

It looks like this moved to another file while the patch was under discussion, and so needs to be updated. I'm attaching an amended diff.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/32905
Committed: http://github.com/openstack/swift/commit/8f9b135e0a16478a628f20224ce5babe62d4aaba
Submitter: Jenkins
Branch: master

commit 8f9b135e0a16478a628f20224ce5babe62d4aaba
Author: Alex Gaynor <email address hidden>
Date: Mon May 27 02:07:39 2013 +0000

    Checked user input in XML responses.

    Fixes bug 1183884.

    * swift/account/utils.py: Escape account name in XML listings.

    Change-Id: I2392d012ddeec05a267c3dcf14748112316096f3

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/32909
Committed: http://github.com/openstack/swift/commit/6659382c4fa348e1ebbce2424968dd7267ea1db1
Submitter: Jenkins
Branch: stable/grizzly

commit 6659382c4fa348e1ebbce2424968dd7267ea1db1
Author: Alex Gaynor <email address hidden>
Date: Mon May 27 02:07:39 2013 +0000

    Check user input in XML responses.

    Fixes bug 1183884.

    * swift/account/server.py: Escape account name in XML listings.

    Change-Id: I7ba54631ed1349516132c00a53fae74f0b84ac37

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/32911
Committed: http://github.com/openstack/swift/commit/4eed6bf5b5028409f730be97ddcfb6bfa893c976
Submitter: Jenkins
Branch: stable/folsom

commit 4eed6bf5b5028409f730be97ddcfb6bfa893c976
Author: Alex Gaynor <email address hidden>
Date: Mon May 27 02:07:39 2013 +0000

    Check user input in XML responses.

    Fixes bug 1183884.

    * swift/account/server.py: Escape account name in XML listings.

    Change-Id: I33f25aa02c96a72cb54c9d7ebd916d06a8a69edf

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/32982
Committed: http://github.com/openstack/swift/commit/92d7eadd328797d392758c79e258c8455874c80e
Submitter: Jenkins
Branch: master

commit 92d7eadd328797d392758c79e258c8455874c80e
Author: Samuel Merritt <email address hidden>
Date: Thu Jun 13 11:13:36 2013 -0700

    Better escaping for GET /v1/a?format=xml.

    Commit 8f9b135 fixed a bug where an XML attribute could have arbitrary
    characters jammed into it, resulting in a document with arbitrary
    tags... and it did remove the ability to get an arbitrary XML document
    out of the object server. However, it left a couple of ways to get a
    malformed XML document, one example of which is an account named ".

    This fixes up the remaining ways and ensures you always get a
    well-formed XML document in the account-listing response. Also, it
    adds a unit test for the escaping of the container name; this was
    already working, just untested.

    If you look in the discussion for bug 1183884, you'll see that the
    review comments there are basically "seems good, but could use a unit
    test". (The astute reader will note that I am one of the guilty
    parties in that review.)

    I found this bug while writing the missing unit test.

    The moral of this story is left as an exercise for the reader.

    Fixes bug 1183884 harder.

    Change-Id: I84b24dd930ba1bb6c4f674f8d3996639dedbce15