Remote arbitrary file corruption / creation flaw via injected files

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().

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.

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 "../../".

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

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.

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.

Added markmc so he can help review the patches.

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

Otherwise looks good.

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

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

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

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.

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 ?

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.

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

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)

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.

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.

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.

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

Folsom and Essex patches look good to me

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

Diablo patch.

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 :)

Diablo patch looks good too. cheers

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

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

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.

@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.

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

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

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

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

Folsom: https://review.openstack.org/#/c/9266/
Essex: https://review.openstack.org/#/c/9267/
Diablo: https://review.openstack.org/#/c/9268/

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

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

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.

Test coverage log.

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)
  ...

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.