[OSSA 2014-001] Insecure directory permissions with snapshot code (CVE-2013-7048)

[Daniel Berrange]

In the following commit:

commit 46de2d1e2d0abd6fdcd4da13facaf3225c721f5e
Author: Rafi Khardalian <email address hidden>
Date: Sat Jan 26 09:02:19 2013 +0000

    Libvirt: Add support for live snapshots

    blueprint libvirt-live-snapshots

There was the following chunk of code

         snapshot_directory = CONF.libvirt_snapshots_directory
         fileutils.ensure_tree(snapshot_directory)
         with utils.tempdir(dir=snapshot_directory) as tmpdir:
             try:
                 out_path = os.path.join(tmpdir, snapshot_name)
- snapshot.extract(out_path, image_format)
+ if live_snapshot:
+ # NOTE (rmk): libvirt needs to be able to write to the
+ # temp directory, which is owned nova.
+ utils.execute('chmod', '777', tmpdir, run_as_root=True)
+ self._live_snapshot(virt_dom, disk_path, out_path,
+ image_format)
+ else:
+ snapshot.extract(out_path, image_format)

Making the temporary directory 777 does indeed give QEMU and libvirt permission to write there, because it gives every user on the whole system permission to write there. Yes, the directory name is unpredictable since it uses 'tempdir', this does not eliminate the security risk of making it world writable though.

This flaw is highlighted by the following public commit which makes the mode configurable, but still defaults to insecure 777.

https://review.openstack.org/#/c/46645/

[Jeremy Stanley (fungi)]

Given that you've commented publically in that review "Setting console world readable/writable is a security flaw" I'm not sure there's much point to keeping this bug report private/embargoed. Do you concur?

[Daniel Berrange (berrange)]

Yep, unfortunately, when i commented to that effect I was thinking the code was only related to Michal's new console code, not realizing he was also touching this pre-existing code. So you're right that the cat is pretty much out of its bag by now.

[Jeremy Stanley (fungi)]

Switched to public security based on the above confirmation.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/47634

[Thierry Carrez (ttx)]

I think it makes sense to have an OSSA on this (for Grizzly/havana). Thoughts ?

[Jeremy Stanley (fungi)]

If we have hard-coded an expectation that libvirt and nova will run as separate users but both need to write to the same directory, the obvious solution is to use a group-writeable directory with a specific group to which only those two users belong (and optionally setgid if they'll be making subdirectories within it which they also need to share), though this may not be portable to non-POSIX filesystems (does libvirt run on those sorts of platforms?).

I think the approach attempted in https://review.openstack.org/46645 is on the right track, but instead of defaulting to an insecure behavior we should instead enlist the help of downstream packagers to see that an appropriate group gets created for this purpose and provide similar mitigation recommendations in an OSSA corresponding to introduction of the fixes in all affected branches.

[Daniel Berrange (berrange)]

The issue isn't sharing between libvirt & nova, it is sharing between QEMU & nova.

Having a the directory be group writable shared between Nova & QEMU is really not a good idea, because it opens up an avenue for a compromised QEMU to attack Nova.

IMHO, if Nova needs to access files that are owned by the QEMU user, then it should be using rootwrap or some other kind of helper, not try to make them writable by two different user accounts which are intended to be privilege separated.

[Jeremy Stanley (fungi)]

I'll leave it up to the nova devs to decide whether the account separation at play there is intended to guard against symlink attacks launched between these roles, but regardless I agree it deserves an advisory once addressed.

[Thierry Carrez (ttx)]

Looks like a tricky backport again: both solutions (specific group and new rootwrap commands) rely on distros handling more than just a code update... a painful upgrade, and the impact of this vulnerability might make it a bad trade-off.

Unless nova can reuse existing rootwrap filters to achieve this, so that we don't actually need to update the rootwrap filters config files !

[Xavier Queralt (xqueralt-deactivatedaccount)]

I've isolated the fix for this bug from the patch I was working on for bug 1129748 so it doesn't depend on any new rootwrap rule. With it the temporary directory is created with the correct permissions (751) and its contents (the images) are made only readable to the owner of the process by changing the file-creation mask for the whole snapshot method.

I think it should be easily backportable now.

[Kurt Seifried (kseifried)]

Does this issue need a CVE?

[Jeremy Stanley (fungi)]

Kurt: Probably. We don't usually request a CVE until we have a clear impact description and the patches which correct it in supported releases are mostly complete/ready to be reviewed, so that we don't have to go back later with revisions or retractions. If you're okay with us requesting CVEs before we're clear on the direction we're taking, we can probably see about revising our workflow ask for them earlier in the process.

[Thierry Carrez (ttx)]

@Xavier: do you have a master fix proposed somewhere already ?

[Thierry Carrez (ttx)]

Proposed impact description:

--------------------------------
Title: Nova live snapshots use an insecure local directory
Reporter: Daniel Berrange (Red Hat)
Products: Nova
Affects: Grizzly and later

Description:
Daniel Berrange from Red Hat reported that the directories used to temporarily store live snapshots on Nova compute nodes were writeable to all local users. A local attacker with shell access on compute nodes could therefore read and modify the contents of live snapshots before those are uploaded to the image service.

[Jeremy Stanley (fungi)]

This impact description looks accurate to me.

[Xavier Queralt (xqueralt-deactivatedaccount)]

@Thierry: I've the patch in gerrit, I don't know why the bug was not updated (I should have checked probably).

Upstream change is here: https://review.openstack.org/#/c/58852/

In the end is just about changing the directory's mode from 777 to 701, because libvirt only needs o+x for it to read/write on it.

[Thierry Carrez (ttx)]

Kurt: all confirmed, yes we need a CVE for this one. I suspect I should post it to oss-security since it's public already ?

[Thierry Carrez (ttx)]

Patch shall be backported once it hits the master branch.

@Xavier: feel free to propose backports yourself
See https://wiki.openstack.org/wiki/StableBranch#Proposing_Fixes

[Xavier Queralt (xqueralt-deactivatedaccount)]

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

Fix proposed to branch: grizzly
Review: https://review.openstack.org/60550

[Thierry Carrez (ttx)]

CVE requested

[Thierry Carrez (ttx)]

http://openwall.com/lists/oss-security/2013/12/11/6

[Thierry Carrez (ttx)]

CVE-2013-7048

[Thierry Carrez (ttx)]

We need to get the reviews restored, they got auto-abandoned over the holidays

[Jeremy Stanley (fungi)]

Restored:
https://review.openstack.org/58852
https://review.openstack.org/60548
https://review.openstack.org/60550

[OpenStack Infra (hudson-openstack)]

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

commit 8a34fc3d48c467aa196f65eed444ccdc7c02f19f
Author: Xavier Queralt <email address hidden>
Date: Wed Nov 27 20:44:36 2013 +0100

    Enforce permissions in snapshots temporary dir

    Live snapshots creates a temporary directory where libvirt driver
    creates a new image from the instance's disk using blockRebase.
    Currently this directory is created with 777 permissions making this
    directory accessible by all the users in the system.

    This patch changes the tempdir permissions so they have the o+x
    flag set, which is what libvirt needs to be able to write in it and

    Closes-Bug: #1227027
    Change-Id: I767ff5247b4452821727e92b668276004fc0f84d

[Thierry Carrez (ttx)]

stable/grizzly and stable/havana (grenade job running grizzly) look a bit borked right now, so I feel like we should publish advisory pointing to the reviews.

[Jeremy Stanley (fungi)]

Agreed, until someone gets to the root of the grizzly breakage in bug 1266094 we're not going to have any hope of landing backports for grizzly or havana.

[Thierry Carrez (ttx)]

OSSA 2014-001

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/60548
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=75be5abd6b3fa0f7f27fe9c805f832cd41d44a5d
Submitter: Jenkins
Branch: stable/havana

commit 75be5abd6b3fa0f7f27fe9c805f832cd41d44a5d
Author: Xavier Queralt <email address hidden>
Date: Wed Nov 27 20:44:36 2013 +0100

    Enforce permissions in snapshots temporary dir

    Live snapshots creates a temporary directory where libvirt driver
    creates a new image from the instance's disk using blockRebase.
    Currently this directory is created with 777 permissions making this
    directory accessible by all the users in the system.

    This patch changes the tempdir permissions so they have the o+x
    flag set, which is what libvirt needs to be able to write in it and

    Closes-Bug: #1227027
    Change-Id: I767ff5247b4452821727e92b668276004fc0f84d
    (cherry picked from commit 8a34fc3d48c467aa196f65eed444ccdc7c02f19f)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/60550
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=9bd7fff8c0160057643cfc37c5e2b1cd3337d6aa
Submitter: Jenkins
Branch: stable/grizzly

commit 9bd7fff8c0160057643cfc37c5e2b1cd3337d6aa
Author: Xavier Queralt <email address hidden>
Date: Wed Nov 27 20:44:36 2013 +0100

    Enforce permissions in snapshots temporary dir

    Live snapshots creates a temporary directory where libvirt driver
    creates a new image from the instance's disk using blockRebase.
    Currently this directory is created with 777 permissions making this
    directory accessible by all the users in the system.

    This patch changes the tempdir permissions so they have the o+x
    flag set, which is what libvirt needs to be able to write in it and

    Closes-Bug: #1227027
    Change-Id: I767ff5247b4452821727e92b668276004fc0f84d
    (cherry picked from commit 8a34fc3d48c467aa196f65eed444ccdc7c02f19f)