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

Bug #1227027 reported by Daniel Berrange on 2013-09-18
270
This bug affects 2 people
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
High
Unassigned
Grizzly
High
Unassigned
Havana
High
Unassigned
OpenStack Security Advisory
Medium
Thierry Carrez

Bug Description

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/

CVE References

Jeremy Stanley (fungi) wrote :

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?

Changed in nova:
milestone: none → havana-rc1
status: New → Confirmed
importance: Undecided → High
Daniel Berrange (berrange) wrote :

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

Switched to public security based on the above confirmation.

information type: Private Security → Public Security

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

Changed in nova:
assignee: nobody → Michael Still (mikalstill)
status: Confirmed → In Progress
Changed in nova:
assignee: Michael Still (mikalstill) → nobody
status: In Progress → Triaged

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

Changed in ossa:
status: New → Incomplete
tags: added: havana-rc-potential
Changed in nova:
milestone: havana-rc1 → none
tags: added: libvirt
Jeremy Stanley (fungi) wrote :

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

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

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

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 !

Thierry Carrez (ttx) on 2013-10-14
tags: added: havana-backport-potential
removed: havana-rc-potential
Changed in nova:
assignee: nobody → Xavier Queralt (xqueralt)
Changed in nova:
status: Triaged → In Progress

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

Does this issue need a CVE?

Jeremy Stanley (fungi) wrote :

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

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

Changed in ossa:
importance: Undecided → Medium
status: Incomplete → Confirmed
Thierry Carrez (ttx) wrote :

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.

Changed in ossa:
assignee: nobody → Thierry Carrez (ttx)
status: Confirmed → Triaged
Jeremy Stanley (fungi) wrote :

This impact description looks accurate to me.

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

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

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

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

CVE requested

Changed in ossa:
status: Triaged → In Progress
Thierry Carrez (ttx) wrote :

CVE-2013-7048

summary: - Insecure directory permissions with snapshot code
+ Insecure directory permissions with snapshot code (CVE-2013-7048)

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

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

Changed in nova:
status: In Progress → Fix Committed

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

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

OSSA 2014-001

Changed in ossa:
status: In Progress → Fix Committed
summary: - Insecure directory permissions with snapshot code (CVE-2013-7048)
+ [OSSA 2014-001] Insecure directory permissions with snapshot code
+ (CVE-2013-7048)
Changed in nova:
milestone: none → icehouse-2
Thierry Carrez (ttx) on 2014-01-22
Changed in nova:
status: Fix Committed → Fix Released

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)

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)

Thierry Carrez (ttx) on 2014-01-27
Changed in ossa:
status: Fix Committed → Fix Released
Alan Pevec (apevec) on 2014-02-13
tags: removed: havana-backport-potential
Thierry Carrez (ttx) on 2014-04-17
Changed in nova:
milestone: icehouse-2 → 2014.1
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Duplicates of this bug

Other bug subscribers