Remote arbitrary file corruption / creation flaw via injected files

Bug #1015531 reported by Thierry Carrez on 2012-06-20
278
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Critical
Pádraig Brady
Essex
Critical
Pádraig Brady
nova (Ubuntu)
Undecided
Unassigned
Precise
Undecided
Unassigned

Bug Description

Matthias Weckbecker from SUSE Security Team reported the following:

------------------
During our internal security audit efforts at SUSE for openstack, I have found
an issue in openstack-nova (compute).

Quoting from [1] (comment #1):

Vulnerable code (quoted), /usr/lib64/python2.6/site-packages/nova/utils.py:
[... snipped copy of utils.execute code ...]

It's already doing lots of things correctly, like e.g. calling Popen with
the first parameter being a list, still it is affected by traversal flaws.

Testcase (also from [1], comment #0):

mweckbecker@s3gfault:~$ cat newserver.xml
<?xml version="1.0" encoding="UTF-8"?>
<server xmlns="http://docs.openstack.org/compute/api/v1.1"
imageRef="http://anonymi.arch.suse.de:8774/985b88ae99474d6d90501870499a063f/images/2d583dfb-000a-4332-9264-ed57ce186f1d"
        flavorRef="6"
        name="new-server-test">
  <metadata>
    <meta key="My Server Name">foobar</meta>
  </metadata>
  <personality>
    <file path="../../../../../../../../../../../../../etc/hosts">
        ICAgICAgDQoiQSBjbG91ZCBkb2VzIG5vdCBrbm93IHdoeSBp
        dCBtb3ZlcyBpbiBqdXN0IHN1Y2ggYSBkaXJlY3Rpb24gYW5k
        IGF0IHN1Y2ggYSBzcGVlZC4uLkl0IGZlZWxzIGFuIGltcHVs
        c2lvbi4uLnRoaXMgaXMgdGhlIHBsYWNlIHRvIGdvIG5vdy4g
        QnV0IHRoZSBza3kga25vd3MgdGhlIHJlYXNvbnMgYW5kIHRo
        ZSBwYXR0ZXJucyBiZWhpbmQgYWxsIGNsb3VkcywgYW5kIHlv
        dSB3aWxsIGtub3csIHRvbywgd2hlbiB5b3UgbGlmdCB5b3Vy
        c2VsZiBoaWdoIGVub3VnaCB0byBzZWUgYmV5b25kIGhvcml6
        b25zLiINCg0KLVJpY2hhcmQgQmFjaA==
    </file>
  </personality>
</server>

mweckbecker@s3gfault:~$ curl -v
"http://anonymi.arch.suse.de:8774/v2/985b88ae99474d6d90501870499a063f/servers"
-H"X-Auth-Token:ef7d5faf9d864c048afce0cf6a3a3c15"
-H"Content-type:application/xml" -H"Accept:application/xml" -d @newserver.xml

Additional note: This beast is calling tee with sudo, potentially allowing
attackers to even alter files such as /etc/passwd.

[1] https://bugzilla.novell.com/show_bug.cgi?id=767687

Thanks, Matthias

Related branches

Thierry Carrez (ttx) wrote :

Ouch :)

I would not blame the utils.execute() code though, it's a low-level primitive that just does what it's told to do.

The flaw is actually in nova/virt/disk/api.py which does not check that "path" is still within the image mount_dir in inject_files() or _inject_file_into_fs().

description: updated
Changed in nova:
importance: Undecided → Critical
status: New → Confirmed
Thierry Carrez (ttx) wrote :

Added PTL to share the fun.

> I would not blame the utils.execute() code though, it's a low-level primitive that just does what it's told to do.

Well, there are probably multiple ways to address this and all of them come with their own pros and cons. Fixing it
directly at low-level means that all subsequent calls that will in turn boil down to utils.execute() code will be safe.
Otherwise we might find us ending up fixing "hundreds" of high-level occurrences.

Thierry Carrez (ttx) wrote :

The trick is that you can't decide at utils.execute() level what generic argument is or is not safe. In some cases passing "../.." is perfectly accepted use !

I see your point though... and as a strengthening low-level measure the rootwrap filter that allows to run mkdir/tee as root should also do a deeper inspection on arguments to check that it only affects nova stuff.

> The trick is that you can't decide at utils.execute() level what
> generic argument is or is not safe. In some cases passing "../.."
> is perfectly accepted use !

Just off the top of my head:

Doesn't Python offer something like Perl's caller() as well? Then you could
possibly perform whitelisting for functions that are allowed to pass "../../".

Russell Bryant (russellb) wrote :

I'm assigning this to myself. I should have a patch up shortly for review/testing.

Changed in nova:
assignee: nobody → Russell Bryant (russellb)
Russell Bryant (russellb) wrote :

Here is a patch that address the issue directly in the file injection code. There is some unit test coverage, but I haven't tested this fully yet to ensure that the failure is clean when trying to start an instance.

I'm also going to add Pádraig Brady to this bug. In addition to working on OpenStack, he's also a GNU coreutils maintainer. He helped me refine this patch and can provide valuable input here if we need to explore any additional changes.

Russell Bryant (russellb) wrote :

Here is the same patch for stable/essex.

I checked stable/diablo and the code is very different there. It looks like there is some xen specific support for file injection. I don't think it's vulnerable, but it would be nice for someone familiar with the xen code to check that.

Russell Bryant (russellb) wrote :

Added markmc so he can help review the patches.

Vish Ishaya (vishvananda) wrote :

minor issue in the patch: can you replace the two hardcoded '/' with os.sep or os.path.sep ?

Otherwise looks good.

Pádraig Brady (p-draigbrady) wrote :

Hmm, I think the _path_within_fs() check needs to be called for all injected files, as one could upload an image with symlinks in various places to get back to the host.

For example if /root/.ssh in the image was a symlink to ../../../../../root/.ssh then you'd be injecting keys to the host authorized_keys file

Mark McLoughlin (markmc) wrote :

Nice find Matthias

Russell: the way we're using commonprefix here, we may as well just use startswith() i.e.

  os.path.commonprefix([fs, absolute_path]) == fs

is equivalent to:

  absolute_path.startswith(fs)

FWIW, the xenapi driver handles injected_files via the guest agent and isn't vulnerable.

Pádraig is right:

    def _inject_key_into_fs(key, fs, execute=None):
        sshdir = os.path.join(fs, 'root', '.ssh')
        ...
        keyfile = os.path.join(sshdir, 'authorized_keys')

need to apply realpath() here too. Easiest is to add an 'append' arg to _inject_file_into_fs() and re-use that.

    def _inject_metadata_into_fs(metadata, fs, execute=None):
        metadata_path = os.path.join(fs, "meta.js")

meta.js could be a symlink ... again, we should just use _inject_file_into_fs() here

Mark McLoughlin (markmc) wrote :

Ok, here's what I've come up with

Thierry Carrez (ttx) wrote :

Last patch looks good to me, especially with all tee calls now going through a single code path.
The remark from Vish about using os.sep instead of '/' still holds.

Thierry Carrez (ttx) wrote :

Looked into stable/diablo and although it doesn't support file injection (so it's not vulnerable to this precise issue), it's still vulnerable to Padraig's variation (upload an image with symlinks in meta.js, /etc/network or /root/.ssh). The impact is slightly lower in the second case, since it's harder to inject arbitrary data, but it affects more setups.

I think we should treat those as two separate issues, two separate CVEs, though probably in the same patch:

* Matthias's is about arbitrary file injection through <personality>, affects Essex/Folsom in libvirt-based setups
* Padraig's is about file corruption through net/ssh/metadata injection, affects Diablo/Essex/Folsom, libvirt & xen setups

Thoughts ?

Mark McLoughlin (markmc) wrote :

Meant to comment on the os.sep thing - since the existing code already uses '/':

  absolute_path = os.path.join(fs, path.lstrip('/'))

and we don't support compute nodes on any platforms where '/' isn't the path separator, I don't think it's an issue.

No problem with a later general "switch to os.sep" cleanup in Nova, though.

Mark McLoughlin (markmc) wrote :

Splitting it into two separate CVEs makes sense to me since Diablo didn't have libvirt-file-injection

FYI, the blueprint for that was: https://blueprints.launchpad.net/nova/+spec/libvirt-file-injection

Pádraig Brady (p-draigbrady) wrote :

Good point Thierry on the separate CVEs.

Mark nice refactoring in the patch. It's needs this fixup though:

diff --git a/nova/virt/disk/api.py b/nova/virt/disk/api.py
index 8744dcc..730008b 100644
--- a/nova/virt/disk/api.py
+++ b/nova/virt/disk/api.py
@@ -380,7 +380,7 @@ def _inject_net_into_fs(net, fs, execute=None):
     utils.execute('chown', 'root:root', netdir, run_as_root=True)
     utils.execute('chmod', 755, netdir, run_as_root=True)

- netfile = os.path.join('etc', 'interfaces')
+ netfile = os.path.join('etc', 'network', 'interfaces')
     _inject_file_into_fs(fs, netfile, net)

Russell Bryant (russellb) wrote :

Nice work on the patch, Mark. I have attached an updated version that includes the fix from pbrady. I also added Signed-off-by lines to show who all was involved in writing the patch.

Changed in nova:
assignee: Russell Bryant (russellb) → nobody
Thierry Carrez (ttx) wrote :

Proposed Impact description:

Title: Arbitrary file injection/corruption through directory traversal issues
Impact: Critical
Reporter: Matthias Weckbecker (SUSE Security team), Pádraig Brady (RedHat)
Products: Nova
Affects: All versions

Description:
Matthias Weckbecker from SUSE Security team reported a vulnerability in Nova compute nodes handling of file injection in disk images. By requesting files to be injected in malicious paths, a remote authenticated user could inject files in arbitrary locations on the host file system, potentially resulting in full compromise of the compute node. Only Essex and later setups running the OpenStack API over libvirt-based hypervisors are affected.

Upon further inspection of the code, Pádraig Brady from RedHat found an additional vulnerability. By crafting a malicious image and requesting an instance based on it, a remote authenticated user may corrupt arbitrary files on the host filesystem, potentially resulting in a denial of service. This affects all setups.

Thierry Carrez (ttx) wrote :

Next steps here:

* Master patch: needs formal pre-approval from nova core (Vish/Markmc ?)
* Essex patch: needs to be proposed
* Diablo patch: needs to be proposed
* Impact description: VMT members please ack

Once we've all this we can push the pre-announce to downstream stakeholders, to get CVEs and propose CRD.

Thierry Carrez (ttx) wrote :

Essex patch.
Same as Russell's latest version, except Essex uses json instead of jsonutils.

Mark McLoughlin (markmc) wrote :

Folsom and Essex patches look good to me

Pádraig Brady (p-draigbrady) wrote :

+1for folsom/essex from me.
Thierry, do you want me to do the Diablo patch?

Thierry Carrez (ttx) wrote :

Diablo patch.

Thierry Carrez (ttx) wrote :

New version of the Diablo patch, with tests backported and correct paths used (thanks Padraig).
Please run through run_tests.sh if you've a run_tests.sh setup that works with stable/diablo :)

Pádraig Brady (p-draigbrady) wrote :

Diablo patch looks good too. cheers

Vish Ishaya (vishvananda) wrote :

all looks good to me. I'm happy with the .sep change going in later since its broken in a lot of other places :)

Thierry Carrez (ttx) wrote :

OK, will send notice to downstream stakeholders later today, based on the attached patches and impact description (comment 22). Please check that everything looks good before I do :)

Proposed CRD: July 3rd, 1500 UTC

Pádraig Brady (p-draigbrady) wrote :

I've only a small remark re comment 22.

It's not just corrupting arbitrary files on the host.
You could inject arbitrary keys into /root/.ssh/authorized_keys on the host.
Now that's probably not an issue as the attacker probably wouldn't have remote access to the host.
If that assumption is always valid, then comment 22 is fine as is.

#22 looks fine for me too. Thanks.

Thierry Carrez (ttx) wrote :

@Padraig: I think it's fair to assume you wouldn't run ssh on remotely-accessible compute nodes IPs. Anyway, the report is rated "critical" already, and I can't come up with a good wording that adequately covers that corner case :) Feel free to suggest if you care.

Russell Bryant (russellb) wrote :

The vulnerability description and latest patches look good. One nit pick on the description is that "RedHat" should be "Red Hat" (with a space).

Thierry Carrez (ttx) wrote :

Downstream stakeholders email sent, CRD: July 3rd, 1500 UTC

Thierry Carrez (ttx) on 2012-06-26
Changed in nova:
assignee: nobody → Thierry Carrez (ttx)
Thierry Carrez (ttx) wrote :

CVE assignment:
Mathias's is CVE-2012-3360
Padraig's is CVE-2012-3361

Thierry Carrez (ttx) on 2012-06-28
Changed in nova:
status: Confirmed → Triaged
Thierry Carrez (ttx) wrote :

Adding Dave Walker to coordinate stable/* releases, should have done that earlier.

Thierry Carrez (ttx) wrote :

Folsom/essex in, OSSA 2012-008
Diablo patch at https://review.openstack.org/#/c/9268/ still needs to be merged on stable/diablo

visibility: private → public
Changed in nova:
status: Triaged → Fix Committed
Thierry Carrez (ttx) on 2012-07-04
Changed in nova:
milestone: none → folsom-2
status: Fix Committed → Fix Released
Changed in nova:
assignee: Thierry Carrez (ttx) → Pádraig Brady (p-draigbrady)
status: Fix Released → In Progress
Thierry Carrez (ttx) on 2012-08-07
Changed in nova:
status: In Progress → Fix Released

Reviewed: https://review.openstack.org/9268
Committed: http://github.com/openstack/nova/commit/1e218105b08b1afdf944fc77af91c2cadf90b6e2
Submitter: Jenkins
Branch: stable/diablo

commit 1e218105b08b1afdf944fc77af91c2cadf90b6e2
Author: Thierry Carrez <email address hidden>
Date: Tue Jul 3 16:34:58 2012 +0200

    Prevent key/net/md injection writing to host fs

    Fix bug 1015531, CVE-2012-3361

    Checks that the final normalized path that is about to be written
    to is always within the mounted guest filesystem.

    This is a Diablo backport of the part of Russell Bryant, Pádraig Brady
    and Mark McLoughlin's Folsom patch that applies to stable/diablo.

    Change-Id: I134c40258ff2c9c225bd6092decd9c10e4e22273

Dave Walker (davewalker) on 2012-08-24
Changed in nova (Ubuntu):
status: New → Fix Released
Changed in nova (Ubuntu Precise):
status: New → Confirmed
Jamie Strandboge (jdstrand) wrote :

So, Ubuntu 12.04 LTS should be fixed between:
http://www.ubuntu.com/usn/usn-1497-1/ (CVE-2012-3361)
http://www.ubuntu.com/usn/usn-1545-1/ (CVE-2012-3447)

Please find the attached test log from the Ubuntu Server Team's CI infrastructure. As part of the verification process for this bug, Nova has been deployed and configured across multiple nodes using precise-proposed as an installation source. After successful bring-up and configuration of the cluster, a number of exercises and smoke tests have be invoked to ensure the updated package did not introduce any regressions. A number of test iterations were carried out to catch any possible transient errors.

Please Note the list of installed packages at the top and bottom of the report.

As per the provisional Micro Release Exception granted to this package by the Technical Board, we hope this contributes toward verification of this update.

Adam Gandelman (gandelman-a) wrote :

Test coverage log.

tags: added: verification-done
Launchpad Janitor (janitor) wrote :
Download full text (5.4 KiB)

This bug was fixed in the package nova - 2012.1.3+stable-20120827-4d2a4afe-0ubuntu1

---------------
nova (2012.1.3+stable-20120827-4d2a4afe-0ubuntu1) precise-proposed; urgency=low

  * New upstream snapshot, fixes FTBFS in -proposed. (LP: #1041120)
  * Resynchronize with stable/essex (4d2a4afe):
    - [5d63601] Inappropriate exception handling on kvm live/block migration
      (LP: #917615)
    - [ae280ca] Deleted floating ips can cause instance delete to fail
      (LP: #1038266)

nova (2012.1.3+stable-20120824-86fb7362-0ubuntu1) precise-proposed; urgency=low

  * New upstream snapshot. (LP: #1041120)
  * Dropped, superseded by new snapshot:
    - debian/patches/CVE-2012-3447.patch: [d9577ce]
    - debian/patches/CVE-2012-3371.patch: [25f5bd3]
    - debian/patches/CVE-2012-3360+3361.patch: [b0feaff]
  * Resynchronize with stable/essex (86fb7362):
    - [86fb736] Libvirt driver reports incorrect error when volume-detach fails
      (LP: #1029463)
    - [272b98d] nova delete lxc-instance umounts the wrong rootfs (LP: #971621)
    - [09217ab] Block storage connections are NOT restored on system reboot
      (LP: #1036902)
    - [d9577ce] CVE-2012-3361 not fully addressed (LP: #1031311)
    - [e8ef050] pycrypto is unused and the existing code is potentially insecure
      to use (LP: #1033178)
    - [3b4ac31] cannot umount guestfs (LP: #1013689)
    - [f8255f3] qpid_heartbeat setting in ineffective (LP: #1030430)
    - [413c641] Deallocation of fixed IP occurs before security group refresh
      leading to potential security issue in error / race conditions
      (LP: #1021352)
    - [219c5ca] Race condition in network/deallocate_for_instance() leads to
      security issue (LP: #1021340)
    - [f2bc403] cleanup_file_locks does not remove stale sentinel files
      (LP: #1018586)
    - [4c7d671] Deleting Flavor currently in use by instance creates error
      (LP: #994935)
    - [7e88e39] nova testsuite errors on newer versions of python-boto (e.g.
      2.5.2) (LP: #1027984)
    - [80d3026] NoMoreFloatingIps: Zero floating ips available after repeatedly
      creating and destroying instances over time (LP: #1017418)
    - [4d74631] Launching with source groups under load produces lazy load error
      (LP: #1018721)
    - [08e5128] API 'v1.1/{tenant_id}/os-hosts' does not return a list of hosts
      (LP: #1014925)
    - [801b94a] Restarting nova-compute removes ip packet filters (LP: #1027105)
    - [f6d1f55] instance live migration should create virtual_size disk image
      (LP: #977007)
    - [4b89b4f] [nova][volumes] Exceeding volumes, gigabytes and floating_ips
      quotas returns general uninformative HTTP 500 error (LP: #1021373)
    - [6e873bc] [nova][volumes] Exceeding volumes, gigabytes and floating_ips
      quotas returns general uninformative HTTP 500 error (LP: #1021373)
    - [7b215ed] Use default qemu-img cluster size in libvirt connection driver
    - [d3a87a2] Listing flavors with marker set returns 400 (LP: #956096)
    - [cf6a85a] nova-rootwrap hardcodes paths instead of using
      /sbin:/usr/sbin:/usr/bin:/bin (LP: #1013147)
    - [2efc87c] affinity filters don't work if scheduler_hints is None
      (LP: #1007573)
  ...

Read more...

Changed in nova (Ubuntu Precise):
status: Confirmed → Fix Released

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.

Thierry Carrez (ttx) on 2012-09-27
Changed in nova:
milestone: folsom-2 → 2012.2
Sean Dague (sdague) on 2014-09-15
no longer affects: nova/diablo
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers