[0SSA 2014-009] Image format not enforced when using rescue (CVE-2014-0134)

Added a new OSSA task, since this looks like it will probably require an advisory once addressed.

Adding Russell and Padraig for a quick sanity-check

> - files have to be readable by libvirt/kvm
> - apparmor/selinux will limit the number of accessible files

I don't believe those points help much, if at all.

Remember while guests are running as a KVM user, libvirtd which launches them is root. If the XML config tells libvirtd to give access to a particular file, libvirtd will do that and setup the selinux/apparmour policy to allow it too.

Added Daniel Berrange as well, since this is related to the libvirt driver.

Russell, Daniel: so if I read this correctly this a a genuine flaw which can result in limited host data leakage. Likely targets would be ACL-protected files like /etc/shadow ?

Yes, at the very least, this is a serious information leakage flaw. This same issue has been given a CVE many times before in various virt projects. QEMU has the ability to merge a snapshot back into the base file - if there was a way to trigger this in openstack, the flaw would be even more serious - host data overwriting.

Anyone working on a patch for this ? Does this also affect Grizzly/Folsom ?

It goes all the way back to Diablo as far as I can tell. I'm not actively working on a patch, but will be happy to help with details / reproducing if needed.

Actually, I have not tried on the releases in between, so just to be sure, you can retest on Grizzly / Folsom. I don't have an environment to do that unfortunately.

Tested only Diablo and trunk.

I just tested trunk and didn't see the behaviour described in this report.

I have nova setup to use raw files

$ grep cow /etc/nova/nova.conf
use_cow_images = False

I then booted, stopped & rescued an instance

$ nova boot --flavor m1.small --image f16-x86_64-openstack-sda f16demo
$ nova stop f16demo
$ nova rescue f16demo

When I look at the config libvirt generated, it is correctly setting 'raw' as the disk type
# virsh dumpxml instance-00000003

    <disk type='file' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source file='/home/berrange/src/cloud/data/nova/instances/cd627e5e-928d-47c2-a24c-e1ccfb01ab6d/disk.rescue'/>
      <target dev='vda' bus='virtio'/>
      <alias name='virtio-disk0'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='raw' cache='none'/>
      <source file='/home/berrange/src/cloud/data/nova/instances/cd627e5e-928d-47c2-a24c-e1ccfb01ab6d/disk'/>
      <target dev='vdb' bus='virtio'/>
      <alias name='virtio-disk1'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
    </disk>

> Libvirt description of the rescued instance will contain the entry for the second disk with attribute type="qcow2", even though it should be "raw" - same as the original instance.

@stanislaw What version of Nova do you see this behaviour with ? Today's GIT master doesn't show it

I started from scratch right now and I'm still seeing the same, broken behaviour. Attached, is my whole test (well - I edited out devstack installation) captured via `script`.

Current nova's commit is 82a598318f5d045a2d422f257f9a770ae1f92dc8

Setting to incomplete while we confirm that current trunk is affected

I don't see this behavior either, although I see different things from Daniel.

I don't see the contents of the evil backing file in /dev/vdb from inside the rescuing guest, but I do see the path I set as the first string, which means the dd worked:

$ sudo strings /dev/vdb | head -1
/etc/libvirt/libvirt.conf

My libvirt config does show qcow2 as the disk format though:

<disk type='file' device='disk'>
      <driver name='qemu' type='qcow2' cache='none'/>
      <source file='/opt/stack/data/nova/instances/77304a99-9453-4e21-8d5b-ccf61dfa5c23/disk'/>
      <target dev='vdb' bus='virtio'/>
      <alias name='virtio-disk1'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/>
</disk>

However, if I look at the image itself:

# qemu-img info /opt/stack/data/nova/instances/77304a99-9453-4e21-8dbb-ccf61dfa5c23/disk
image: /opt/stack/data/nova/instances/77304a99-9453-4e21-8d5b-ccf61dfa5c23/disk
file format: qcow2
virtual size: 1.0G (1073741824 bytes)
disk size: 12M
cluster_size: 65536
backing file: /opt/stack/data/nova/instances/_base/c193393ae54e6028da799b4afdd8ccd8db6)

and if I follow the backing file reference:

# qemu-img info /opt/stack/data/nova/instances/_base/c193393ae54e6028da799b4afdd8ccd8db6efc07
image: /opt/stack/data/nova/instances/_base/c193393ae54e6028da799b4afdd8ccd8db6efc07
file format: raw
virtual size: 24M (25165824 bytes)
disk size: 24M

it backs up to a raw file. This was on a precise machine:

ii libvirt-bin 0.9.8-2ubuntu17 programs for the libvirt library

On a very recent commit from master:

commit 78810135d851a4db60a2cb2e2fbdb11d032a8b97
Merge: 5429048 2d13161
Author: Jenkins <email address hidden>
Date: Wed Sep 25 19:56:34 2013 +0000

Dan, I'll double-check on my machine, but I don't think what you see looks right for this scenario. In case of use_cow_images=false, I wouldn't expect the backing file to point at the _base directory at any time. That looks like the instance was created with a qcow2 disk from the start.

FYI, I have managed to reproduce the problem now & confirm it is flawed.

Looks like the flaw was introduced in this patch - it causes the libvirt imagebackend.py code to run qemu-img info to probe the disk format which is unsafe :-(

commit 494a3cb5749d52aa90daeacd980362df5f971c0d
Author: Vishvananda Ishaya <email address hidden>
Date: Mon Apr 1 14:19:49 2013 -0700

    libvirt: Get driver type from base image type.

    If we are using the raw backend (no cow), we should set the driver
    type based on the type of the source image instead of always
    using raw.

    Calls to to_xml were moved after _create_image so that the image
    type could be determined when creating the xml for libvirt.

    Fixes bug 1163009

    Change-Id: Ic8d5f0ab83d868a42f834d39c0afb64818d7e027

I think we need to just revert that patch entirely, as the approach it takes is not safe. It injects the probing into the imagebackend.py code, which means it is run in every single code path that uses the image backends.

To address the bug 1163009 safely, we would need something that only does the probing when the image is first downloaded from glance. The detected format should then be recorded (in the database?) and used for all subsequent boots of that image, whether normal start, or rescue start, or something else.

This was introduced in stable/grizzly, which complicates things a bit. Purely reverting (and introducing a small regression) might not be an option there. That said, changing db format is not an option there either... Is there a plan C ?

The key issue is that we must only ever probe the image format once and persist that data in some manner. If using the database is not possible, then perhaps we can change the naming convention used for image files, eg instead of

  $INSTANCE_DIR/disk

use

  $INSTANCE_DIR/disk.qcow2

or

  $INSTANCE_DIR/disk.raw

I'd be somewhat scared about making such a change in grizzly stable though, since hunting down all the places which assume a plain name of 'disk' may be hard.

Another option for grizzly stable would be to have something run on VM shutdown that probed the disk image and compard it to the XML nova saved in the instance directory. If it detected that the format had changed, it could do something to prevent the VM being started again - eg renamed the image from 'disk' to 'disk.compromised'

Ah, sorry, I mis-correlated the default of use_cow_images with what you said it needed to be, my bad.

I really like the file renaming, but even if we changed all of the places in openstack itself, there's bound to be a number of custom modules or supporting scripts people use that assume this name. I know we have a number of those.

To further complicate the shutdown check, you can probably find some situations where proper shutdown doesn't happen. Host reboots (crashes or forced by dos) would not run shutdown. Live migration be another edge case to check, I guess.

Just to brainstorm more ideas: what about renaming the files and leaving a symlink in place? This will require lots of renaming in libvirt driver, but hopefully it will fix the problem itself. For anything that gets accidentally left over or things that exist outside of openstack's control, the symlink from disk -> disk.{format} should be enough to keep it working.
Or maybe even better -> create disk.{format} itself as a symlink to disk and verify the format based on that file?

Is there a related case with force_raw_images=False, use_cow_images=False
where a qcow is copied directly from glance to the instance/disk ?

I.E. the disk is validly qcow2 format but not backed by an image in base_/

Even if that case isn't susceptible to the dd if=evil.qcow attach,
it could impact on the format detection.

Yet another idea for a easy hack for stable... rather than renaming disk images, we could save a little 'disks.info' file in the instance directory listing the expected formats when first booting. On subsequent boots/rescues we can consult that file instead of probing

> I.E. the disk is validly qcow2 format but not backed by an image in base_/

Glance itself validates that users don't have a nasty backing file listed when they upload qcow2 files, so that's safe enough I believe. So the issue is only with raw files being turned into qcow2 files by a malicious guest action

@Daniel: personally I like the disks.info approach, it appears to be the most foolproof (that file is only used by the verification code so all the other code paths should not be affected).

At this point in the cycle, we might apply this workaround to both grizzly and havana, and fix it "properly" in icehouse ?

Yes, I like the disk info metadata file, as well. It seems a lot less risky than renaming the files for grizzly and havana.

@Daniel: would you be up for a patch on this one ? Attach to this bug if you do.

Daniel should look at it next week.

Looks like Dan doesn't have time for this in the immediate future. Anyone else wanting to work on a patch ?

I'm actively working on finding an assignee for this. If I'm not able to, I'll take it on.

David Ripton has now been assigned to work on the fix for this.

I agree that Dan's disks.info file seems to be the most backward-compatible way.

Here's my thinking:

In imagebackend.Raw.correct_format(), we check for the existence of disks.info. And for an entry for 'path' in it. (like path~format, where format is 'raw' or 'qcow2'). If the disks.info does not exist, or does not have an entry for 'path', we probe for the image type and create the file (if needed) and entry. If it does have an entry, we use that entry, and let the instance fail to load if the image type is wrong.

In imagebackend.Qcow2, we add a parallel correct_format() that does the same thing, with the format always being 'qcow2'. (If the entry already exists and is 'raw' we raise an exception.)

The purpose of keeping this code in imagebackend rather than doing it when we download the file using glance is to simplify the timing in the upgrade case. New code plus existing data keeps working. Granted, it means there's a chance of an attack, once, right after the code is upgraded, but the chance for the attack was already there before the code was upgraded, so in practice I don't think it makes a difference.

Finally, I'm leaning toward using disks.info in Icehouse as well, to have less code and simplify upgrades from stable/havana to Icehouse. Another database column would work, but then we need a database migration script (and more chance of collision with another patch in progress), and (possibly) logic to upgrade a stable/havana box with a disks.info file.

Draft impact description #1 -

Title: Host data leak to vm instance in rescue mode through image backing file.
Reporter: Stanislaw Pitucha (HP)
Products: Nova
Affects: All supported versions

Description:
Stanislaw Pitucha from Hewlett Packard reported a vulnerability in the Nova instance rescue mode. An instance administrator can overwrite the disk from inside the instance using a malicious qcow2 image crafted to be backed by an arbitary file path. By switching the instance to rescue mode, libvirt driver will guess the new image format to be a qcow2 resulting in the compute host backing file path (controlled by the user) to be exposed to the vm as the backing device. Only setups using libvirt to spawn instance, and having "use_cow_images = False" in Nova configuration are affected.

@Tristan: nice work, but the "By... a... will... resulting in... " construct should rather be used to describe the attack vector and the impact. The goal is not to use it to get into details over what technically happens in this bug.

By doing this THING, a type of attacker can trigger THAT, resulting in THIS IMPACT for the system/normal user etc.

Your description is, I think, a bit too detailed on the defect and not detailed enough on the attack vector. We don't know what type of attacker would abuse this (local user ? unauthenticated cloud user ? authenticated cloud user ?), the complexity of the attack, and most importantly the end result impact for the rest of the world.

I would say something like...

By overwiting the disk inside an instance with a malicious image and switching the instance to rescue mode, an authenticated user would..., resulting in... (data exposure ? denial of service ?)

Many thanks for your review Thierry, I removed the technical details.

Draft impact description #2 -

Title: Host data leak to instance in rescue mode
Reporter: Stanislaw Pitucha (HP)
Products: Nova
Affects: All supported versions

Description:
Stanislaw Pitucha from Hewlett Packard reported a vulnerability in the Nova instance rescue mode. By overwriting the disk inside an instance with a malicious image and switching the instance to rescue mode an authenticated user would be able to leak an arbitrary file from the compute host to the virtual instance. Note that the host file must be readable by the libvirt/kvm context to be exposed. Only setups using libvirt to spawn instance, and having "use_cow_images = False" in Nova configuration are affected.

Attaching a patch for review. This is against current (icehouse) master.

Impact looks good. You should add a comma between "rescue mode" and "an authenticated user" for readability though.

It looks good. Just some comments about edge cases:

Is the path always guaranteed to not contain "~"? Wouldn't it be safer to do the serialisation properly? It shouldn't be a big change between:

    parts = line.strip().split('~')

and

    parts = json.loads(line)

Is there any chance for race conditions here? Maybe just os.makedirs() except OSError would be enough?

+ if not os.path.exists(CONF.instances_path):
+ os.makedirs(CONF.instances_path, 0750)

Do we need to do the mode detection in get_set_driver_format()? It looks like it could be always "a".

Since the file is created under instances_path, should it also use an external lock? (in case of shared storage)

Thanks for the review Stanislaw.

'~' characters are extremely rare in paths, but I concede that extremely rare is not the same as impossible. I'll change the serialization to json.

I will add the try/except around makedirs in case of a directory creation race.

I believe the mode detection is needed; I saw errors in testing without it.

It's probably a good idea to use the existing lock directory to serialize accesses to get_set_driver_format. I believe it's safe to read the file without the lock, but we should use the lock when we write the file.

I will make these changes (and some other improvements to the tests) and submit an updated patch.

Several more comments from my side in order of importance, and I agree with all the stuff Stanislaw said.

* We really should not be creating the instance_path dir like you are doing in get_set_driver_format. We use libvirt_utils.get_instance_path, and ensure_tree - for inspiration, check the libvirt driver's _create_image method. Also - we really should not be creating the instance directory in imagebackend classes ever, this is really not it's responsibility but this is just IMHO.

* Why not have a disks.info file per instance? The code here creates one for all the instances. Currently we can have only one image really per instance but that might change in the future (see https://blueprints.launchpad.net/nova/+spec/libvirt-image-to-local-bdm), and this seems like a more natural place to keep it, plus we need only per instance locking. There may be something I'm missing here so feel free to let me know :)

* get_set_driver_format is not a good name IMHO.

Thanks for the review Nikola.

I agree with your first point and will fix it in the next patch.

It's easy enough to add disks.info.instance_name or disks.info.instance_uuid, but I think it's premature to add it now, since we don't support multiple images per instance yet. And we definitely didn't support them in grizzly or havana, to which this security fix needs to be backported. I'd rather keep it simpler for now, but I'll be happy to add the multiple image support to disks.info later, after that blueprint goes in.

I agree that get_set_driver_format is an ugly name, but could not think of a better one. Do you have a suggestion? I thought of just calling it get_driver_format, but didn't because I wanted to make the side effect clear, as well as the reason to call it rather than just looking at self.driver_format.

Also, please consider using git format-patch next time just for the ease of applying it for review.

> It's easy enough to add disks.info.instance_name or disks.info.instance_uuid, but I think it's premature to add it now

Ok - but we do get the added benefit of not global locking needed with no real additional cost, plus if we put the file in instance dir. we get it cleaned automatically, and code wise - it's just changing how you get the path of the file all else can stay.

> I agree that get_set_driver_format is an ugly name, but could not think of a better one. Do you have a suggestion?

Maybe safe_get... that way we emphasize what was the reason for introducing it and make it less confusing?

resolve_driver_format() - doesn't imply a simple getter, but I'd expect it to cache things if possible. Or even a more explicit -
cached_driver_format() - but it would require an explanation that it's security-related permament record, rather than a pure performance improving cache.

I think either safe_get_driver_format or resolve_driver_format is reasonable. I'll use one of those.

Nikola, if we do per-instance disks.info files, is disk.info.{instance_uuid} okay?

I have the same thoughts as Nikola on this - when I proposed this idea, I was certainly thinking of disks.info being a per instance file, not global to the entire host. As Nikola says it gives us saner cleanup on VM delete if we can just rm the entire file and not worry about doing string manipulation of lines inside a shared file. It also avoids having to acquire any global lock to protect the file. If an instances disk.info file gets corrupted that's fairly harmless, but if we have a single global disks.info file and that's corrupted the impact is pretty bad. So I'd rather prefer a per-instance file

Oh one final think - I'm unclear whether things like RBD have any implications on this. eg if we're using RBD for images, do we still have a local directory where we can store this info file ? I guess all RBD volumes are assumed to be raw by nova and we don't (currently) do anything stupid like storing a qcow2 file inside an RBD volume. Nice to have confirmation that this at least doesn't cause any problems for RBD and similar drivers.

David, so like Dan said - just stick it in the instance directory (where we keep disk overlays and libvirt.xml file etc) and call it disk.info, since each instance has it's own directiry anyway. Are there some permission issues with that?

Just make sure that the disk.info file is owned by root:root, or nova:nova - we explicitly don't want QEMU to be able to write to that file

@Daniel: planning to propose a new version of that patch addressing the last comments ?

err... I mean @David: ^

@Thierry, yes, new patch should be ready later today.

Added Nova in title and the new affected version field.

Draft impact description #3 -

Title: Nova host data leak to vm instance in rescue mode.
Reporter: Stanislaw Pitucha (HP)
Products: Nova
Versions: 2013.1.1 to 2013.1.4 and 2013.2 versions up to 2013.2.1

Description:
Stanislaw Pitucha from Hewlett Packard reported a vulnerability in the Nova instance rescue mode. By overwriting the disk inside an instance with a malicious image and switching the instance to rescue mode, an authenticated user would be able to leak an arbitrary file from the compute host to the virtual instance. Note that the host file must be readable by the libvirt/kvm context to be exposed. Only setups using libvirt to spawn instance, and having "use_cow_images = False" in Nova configuration are affected.

+1 for version 3 of impact description

New patch attached, incorporating all the review comments so far.

@nova-coresec, please review on bug

The fix seems sound and incorporates the feedback from Nikola and Daniel. I have a minor issue with one part but overall the fix is good.

The resolve_driver_format method works as if there could be multiple lines within a disk.info file. Since the file is now per instance I don't see why it would need to be opened in append mode, or need to iterate over multiple lines in the file. But this is cleanup and not relevant to the security fix aspect of the patch.

Good point Andrew. I didn't change that code when we switched from a one-file multiple-lines model to a one-file-per-instance model. I will change it and submit another patch. (It's a small change.)

Here's a patch, with resolve_driver_format updated to only read one line (and catch the exception if json is invalid). Also some small test tweaks.

Looks awesome - one nit tho.

We are currently relying on umask being set properly since the permissions will be set by open() builtin, and then we are just chowning it, meaning that if umask allows for world writable files, chowning will make no difference. Not a huge deal but also really easy to fix (not realy on umask when creating see python's os.open). What do you think?

Nikola, thanks for the review. Attached is a revised patch that does os.open for mode 644, when writing a disk.info file.

thanks for addressing the comment David.

I am +2 on the patch. Will also try it later and report back.

nova-core, another +2 on that patch would be nice.
Also it will be applied as-is to havana, so let us know if the change of behavior looks acceptable to you there.

Adding Alan and Adam for the stable maint check on the proposed patch

I'm +2 on the patch as well. Looks good, thanks!

Seems this no longer cleanly applies to master :(

Would be awesome to have an updated version and the stable/havana fix (I can do the backport too). Would love to see us agree on ago public that so that this is in Icehouse rc1 if at all possible.

Here's an rebased patch. (git was able to resolve the merge conflicts without manual intervention.)

I got Russell's approval before adding this to the icehouse-rc1 milestone earlier today.

Thank you David,

Do you know if this will also be the backport for Havana ?

Tristan, that patch does not apply cleanly to stable/havana. I'm happy to backport it though. (Preferably after it has final approval for Icehouse, so I don't have to do the backport multiple times.)

The rebased patch applies cleanly to master. I'm still +2 for it.

+2 as well!

Thank you guys, this is an updated Affect line for impact description of comment #53:

Versions: 2013.2 versions up to 2013.2.2

Sending CVE requests now

@dripton Could you attach an havana backport of your patch please ? Once the backport is reviewed, we will then schedule publication.

Thanks you in advance!

@Tristan, attached is a version of the patch against stable/havana. It required a manual conflict resolution because the snapshot_delete method was removed in icehouse but is still there in havana. Other than that, the code is identical.

I confirmed that the stable/havana patch and master patch from https://bugs.launchpad.net/nova/+bug/1221190/comments/67 both apply cleanly and look ready to go.

Thank you very much guys, the pre-OSSA have been sent.

Proposed public disclosure date/time:
2014-03-25 15:00 UTC

This date seems reasonable for me - I will be sure to +A the patch once it hits the gerrit.

@ndipanov, That is very much appreciated. Thanks!

@dripton would you be available to actually push the patch to gerrit at disclosure date ? Else I can take care of that.

Tristan, I'll be available to put to Gerrit on disclosure date.

Setting task to FixCommitted since we are in pre-OSSA stage

Master patch @ https://review.openstack.org/#/c/82840
Stable/Havana patch @ https://review.openstack.org/#/c/82841

Reviewed: https://review.openstack.org/82840
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=dc8de426066969a3f0624fdc2a7b29371a2d55bf
Submitter: Jenkins
Branch: master

commit dc8de426066969a3f0624fdc2a7b29371a2d55bf
Author: David Ripton <email address hidden>
Date: Tue Jan 28 16:38:51 2014 -0500

    Persist image format to a file, to prevent attacks based on changing it

    The attack is based on creating a raw image that looks like a qcow2
    image, and taking advantage of the code that used 'qemu-img info' to
    autodetect the image format.

    Now we store the image format to a 'disk.info' file, for Qcow2 and Raw
    images, and only autodetect for images that have never been written to
    that file.

    SecurityImpact

    Co-authored-by: Nikola Dipanov <email address hidden>
    Closes-bug: #1221190
    Change-Id: I2016efdb3f49a44ec4d677ac596eacc97871f30a

Reviewed: https://review.openstack.org/82841
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=25e761acd56d4c820273fc0245ada06c500c1637
Submitter: Jenkins
Branch: stable/havana

commit 25e761acd56d4c820273fc0245ada06c500c1637
Author: David Ripton <email address hidden>
Date: Tue Jan 28 16:38:51 2014 -0500

    Persist image format to a file, to prevent attacks based on changing it

    The attack is based on creating a raw image that looks like a qcow2
    image, and taking advantage of the code that used 'qemu-img info' to
    autodetect the image format.

    Now we store the image format to a 'disk.info' file, for Qcow2 and Raw
    images, and only autodetect for images that have never been written to
    that file.

    SecurityImpact

    Conflicts:
     nova/virt/libvirt/imagebackend.py

    Manual tweaks to some mocking in test_imagebackend.py

    Change-Id: I2016efdb3f49a44ec4d677ac596eacc97871f30a
    Co-authored-by: Nikola Dipanov <email address hidden>
    Closes-bug: #1221190

Reviewed: https://review.openstack.org/86353
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=d416f4310bb946b4b127201ec3c37e530d988714
Submitter: Jenkins
Branch: master

commit d416f4310bb946b4b127201ec3c37e530d988714
Author: Nikola Dipanov <email address hidden>
Date: Wed Apr 9 15:50:20 2014 +0200

    Avoid the possibility of truncating disk info file

    Commit dc8de42 makes nova persist image format to a file to avoid
    attacks based on changing it later. However the way it was implemented
    leaves a small window of opportunity for the file to be truncated before
    it gets written back to effectively making it possible for data to get
    lost leaving us with a potential problem next time it is attempted to be
    read.

    This patch changes the way file is updated to be atomic, thus closing
    the race window (and also removes the chown that we did not really
    need).

    It is worth noting that a better solution to this would be
    to allow the code calling the imagebackend to write the file (once!)
    and make it impossible to update after the boot process is done. This
    approach would require more refactoring of the libvirt driver code, and
    may be done in the future.

    Partial-bug: #1221190
    Change-Id: Ia1b073f38e096989f34d1774a12a1b4151773fc7

Fix proposed to branch: stable/havana
Review: https://review.openstack.org/86895