[OSSA 2012-020] create_lvm_image allocates dirty blocks (CVE-2012-5625)

Bug #1070539 reported by Erica Windisch
266
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Fix Released
Medium
Thierry Carrez
Folsom
Fix Released
Medium
Thierry Carrez
OpenStack Security Advisory
Fix Released
Undecided
Thierry Carrez

Bug Description

libvirt's create_lvm_image function will create LVM images on new logical volumes.

Logical volumes are simply linear mappings on a physical volume (PV).

Previously deleted logical volumes leave their dirty blocks (containing user and image data) on the PV. They are not zero'ed.

New LVs will make basic linear mappings to these blocks, leading information disclosure as these LVs are passed to guest virtual machines. LVM's lvcreate does not zero these blocks, nor does the device-mapper configuration used by LVM create any snapshots by default.

One solution may be to use dm-zero as a base image, apply dm-snapshot to a newly-created LV, and pass the snapshot's block device to the guest.

Revision history for this message
Thierry Carrez (ttx) wrote :

Hmm, thought we had that covered. Pulling in Vish to discuss.

Eric: I suspect that would affect Folsom as well, how about Essex or Diablo ?

Revision history for this message
Erica Windisch (ewindisch) wrote :

Yes, this affects Folsom. I believe this was a new feature for Folsom, so it would not affect Essex or Diablo. The code was added on 2012-05-16 (May).

There are similar concerns in nova-volumes / cinder, but this is slightly better handled there. They wipe the device on deletion, but there are edge-cases where that is insufficient. (That warrants another bug report, actually)

Revision history for this message
Thierry Carrez (ttx) wrote :

Looking at the code, there definitely seems to be room for leakage.

@Vish: do you confirm that there is a case where those bits are not fully overwritten before being communicated back to another user ?

Revision history for this message
Vish Ishaya (vishvananda) wrote :

Yup.

Thierry Carrez (ttx)
Changed in nova:
importance: Undecided → Medium
status: New → Confirmed
Revision history for this message
Thierry Carrez (ttx) wrote :

Adding nova-core to reach more potential patch writers
Anyone up for proposing patches ? Please attach them to this bug, do not push them to public Gerrit.

Revision history for this message
Michael Still (mikal) wrote :

There are two create_lvm_image methods in nova -- virt/libvirt/utils.py and virt/libvirt/imagebackend.py. I assume we're talking about the version in utils.py here because the other one dd's over the lvm.

Isn't an issue here that zeroing will be very slow for large LVMs and therefore delay instance start? Regardless, I've attached a first cut at a patch for comments.

Revision history for this message
Erica Windisch (ewindisch) wrote :

Michael: imagebackend.py only overwrites up to the size of the base image. Nothing outside of imagebackend.py currently seems to use utils.py's create_lvm_image function.

We could enforce this at utils.py's function, protective of future code, but then the dd will take unnecessary long. Or, we can just do this from imagebackend.py, where it is used, somewhat better optimized.

Revision history for this message
Pádraig Brady (p-draigbrady) wrote :

Eric your patch seems closest to correct.
You'll probably want to keep the resize2fs() call after the zeroing?
Also there is the caveat that if size is not a multiple of 4MiB the
skip value may be too small and overwrite FS structures.
So you may need another dd invocation to deal with the slop at the start.

Revision history for this message
Erica Windisch (ewindisch) wrote :

Padraig: I think that can be addressed by adding the remainder of bytes to zero_skip. I'll clarify that.

You're right, the resize2fs() would still be needed. I'll bring it back.

Revision history for this message
Erica Windisch (ewindisch) wrote :

Try #2 - also, I haven't /tested/ this code yet.

Revision history for this message
Vish Ishaya (vishvananda) wrote :

we switched to zero on delete for cinder because it is a better ux. I think i would prefer the same here.

Revision history for this message
Erica Windisch (ewindisch) wrote :

True. I can make a version of this patch that wipes on delete. However, I would like to better understand how delete is triggered, because it isn't clear to me where this happens outside of being an intentional administrative action. Ideally, on startup, a greenthread could spawn to wipe all LVs. Is that happening now? (I haven't found the code that does this, but perhaps I'm not looking hard enough)

I'm still concerned about edge-cases around zero on delete. Perhaps we don't need to care about all these edge cases, but it isn't well documented currently, so users are prone to follow practices that are not secure with wipe-on-delete. I don't think these edge-cases are obvious to deployers.

Specific problems:
 * Admins manually creating and deleting LVs
 * Admins using pvmove will find that the old physical volume has extents that have NOT been zero'ed.
 * Data on the disk existing at time of installation / configuration. (Disks that have been recycled/repurposed, etc)

Ultimately, I'd like to see this code support dm-zero and dm-crypt, but neither are necessarily appropriate for stable-backports.

Revision history for this message
Erica Windisch (ewindisch) wrote :

Zero-on-delete patch. Still will need some testing.

Requires a new rootwrap entry...

Revision history for this message
Russell Bryant (russellb) wrote :

Can you clarify if you are going to work on testing this?

Revision history for this message
Yun Mao (yunmao) wrote :

@Vish: In Cinder if zeroing on delete gets interrupted for some reason, would that be a problem? Or put it in another way, how to make sure the zeroing job is finished?

Revision history for this message
Erica Windisch (ewindisch) wrote :

@Russell: yes, I will... but first, are we happy with the direction of the wipe-on-delete patch? (keeping in mind things such as we'll need a new rootwrap filter to land into the stable branch, etc...)

Revision history for this message
Erica Windisch (ewindisch) wrote :

@Yun: Ideally, the actual removal of the LV only happens on successful execution of the zero-wipe. This is why, at least in my patch, I calculate the size of the volume so that DD won't error at the end of the device (this will need to be tested yet, once we have quorum this is the correct approach)

Revision history for this message
Thierry Carrez (ttx) wrote :

zero-on-delete sounds preferable to me... though I'd really prefer if we didn't add a new rootwrapped command in a security bugfix. Any other way we could fill this with junk without needing to add blockdev ?

Revision history for this message
Thierry Carrez (ttx) wrote :

...like using lvs instead to get information on the LV size ?

Revision history for this message
Erica Windisch (ewindisch) wrote :

Thierry, uglier and LVM-only, but yes. I'd personally prefer blockdev moving forward, if only because it could work against other future options such as ZFS or Btrfs volumes. I agree there could be some value in lvs for the backport.

As a not-so-clean example on the shell:
    $ sudo lvs --noheadings --separator , --units b stack-volumes/volume | cut -d, -f4 | sed 's/B$//'
versus:
    $ sudo blockdev --getsize64 stack-volumes/volume

I've confirmed that the byte sizes reported are identical.

Revision history for this message
Thierry Carrez (ttx) wrote :

Sure, blockdev is perfectly ok in grizzly. I would use lvs in the backport just so that we don't touch the rootwrap config file on a security fix.

Revision history for this message
Thierry Carrez (ttx) wrote :

More reviews and opinions wanted from nova-core here.

Revision history for this message
Erica Windisch (ewindisch) wrote :

I can update this, but I'm out of pocket until at least Monday.

Revision history for this message
Michael Still (mikal) wrote :

I agree that zeroing on delete is the way to go. The patch looks good to me.

Revision history for this message
Thierry Carrez (ttx) wrote :

My recommendation would be to address the security bug with lvs, then replace lvs with blockdev in grizzly.

@Eric: if you provide the lvs-based patch for Grizzly, I can backport it for Folsom. Next week is ok.

Revision history for this message
Pádraig Brady (p-draigbrady) wrote :

tl;dr...

I suggest adding iflag=direct oflag=direct to the first dd, and
changing the second dd to: shred -n0 -z -s$zero_remainder $path

Details...

I still don't think the second dd invocation is correct,
as the skip= parameter is a block count, whereas you intend it
to be a byte count. Also you want a seek= anyway, not a skip.

You could replace the second dd invocation with something like:

 sh -c "(dd seek=$zero_remainder_start bs=1 count=0 &&
         dd bs=$zero_remainder count=1) </dev/zero >$path"

Note newer versions of dd support iflag=count_bytes,
so this could all be done in a single invocation like
the following, but we can't rely on this being available:

 dd iflag=count_bytes if=/dev/zero count=vol_size bs=4MiB of=$path

Now the sh -c "dd ..." above is quite awkward, so since this
is only for the slop at the end of the volume, it's probably
better just to do this (though it might need a new rootwrap :()

  "shred -n0 -z -s$zero_remainder"

Performance notes...

I suppose these can be looked at later, I'm just noting
them now when it's a bit fresh in my mind.

1. Since we're usually dd'in on an image after LV creation,
it's probably sufficient to only zero the difference between
image size and the volume size.

2. Related to the previous, that only applies for non
generated images, but the point holds that we may be
better able to optimize (avoid some) zeroing, if done
in the create path, rather than the delete path?

3. TBH I don't understand the sparse option to create_lvm_image()
but the underlying lvcreate --virtualsize might be leveraged
to automatically provide zeros for unwritten parts?

4. I would add iflag=direct oflag=direct to the initial
dd invocation, to minimize kernel processing and at least
be consistent with what's done in cinder for volume clearing.

5. In many situations you wouldn't want the overhead involved
(i.e. you dn't need the security), and so would also like to
configure this away along the lines of:
https://review.openstack.org/#/c/12521/

Revision history for this message
Thierry Carrez (ttx) wrote :

@Eric: back on track to work on the patch ? Or would you prefer if we asked someone else to have a shot at it ?

Revision history for this message
Erica Windisch (ewindisch) wrote :

@Thierry: I'm on paternity leave until Friday. I thought I would find time this week, but haven't. I'd have no problems with someone else working on this. I'll be back on the 26th.

Revision history for this message
Pádraig Brady (p-draigbrady) wrote :
Revision history for this message
Pádraig Brady (p-draigbrady) wrote :
Revision history for this message
Pádraig Brady (p-draigbrady) wrote :

On consideration I've decided to be more conservative with the Folsom patch
and not use O_DIRECT. John Griffith has seen that writing to the end of
snapshot volumes (the last 6MiB) can give EIO when O_DIRECT is in effect.
There were no issues seen when writing to standard volumes, as is the case here.
But since this is only a performance consideration, I'm thinking it's better
to be a bit conservative here, lest we trigger an issue on some kernels.

Revision history for this message
Thierry Carrez (ttx) wrote :

@nova-core: new review round

Revision history for this message
Vish Ishaya (vishvananda) wrote :

+2 on latest

Revision history for this message
Mark McLoughlin (markmc) wrote :

Nicely done Pádraig, looks good to me

Revision history for this message
Michael Still (mikal) wrote :

+2 as well.

Revision history for this message
Thierry Carrez (ttx) wrote :

Please validate the impact description:

Title: Information leak in libvirt LVM-backed instances
Reporter: Eric Windisch (Cloudscaling)
Products: Nova
Affects: Folsom, Grizzly

Description:
Eric Windisch from Cloudscaling reported a vulnerability in libvirt LVM-backed instances. The physical volume content was not wiped out before being reallocated and passed to an instance, which may result in the disclosure of information from previously-allocated logical volumes. Only setups using libvirt and LVM-backed instances (libvirt_images_type=lvm) are affected.

Revision history for this message
Erica Windisch (ewindisch) wrote : Re: [Bug 1070539] Re: create_lvm_image allocates dirty blocks

This is great. Thank you.

Regards,
Eric Windisch

On Wednesday, December 5, 2012 at 08:46 AM, Thierry Carrez wrote:

> Please validate the impact description:
>
> Title: Information leak in libvirt LVM-backed instances
> Reporter: Eric Windisch (Cloudscaling)
> Products: Nova
> Affects: Folsom, Grizzly
>
> Description:
> Eric Windisch from Cloudscaling reported a vulnerability in libvirt LVM-backed instances. The physical volume content was not wiped out before being reallocated and passed to an instance, which may result in the disclosure of information from previously-allocated logical volumes. Only setups using libvirt and LVM-backed instances (libvirt_images_type=lvm) are affected.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1070539
>
> Title:
> create_lvm_image allocates dirty blocks
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/nova/+bug/1070539/+subscriptions
>
>

Revision history for this message
Pádraig Brady (p-draigbrady) wrote : Re: create_lvm_image allocates dirty blocks

+1 to impact description.

thanks Thierry.

Revision history for this message
Erica Windisch (ewindisch) wrote :

Paraig - thank you and +1 for pulling this through while I was out-of-pocket.

Revision history for this message
Russell Bryant (russellb) wrote :

description sounds good to me

Revision history for this message
Thierry Carrez (ttx) wrote :

Sent to downstream stakeholders - Proposed public disclosure date/time: Tuesday December 11th, 1500UTC

Revision history for this message
Thierry Carrez (ttx) wrote :

CVE-2012-5625

Thierry Carrez (ttx)
information type: Private Security → Public Security
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (master)

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

Changed in nova:
assignee: nobody → Thierry Carrez (ttx)
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (stable/folsom)

Fix proposed to branch: stable/folsom
Review: https://review.openstack.org/17857

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to nova (master)

Reviewed: https://review.openstack.org/17856
Committed: http://github.com/openstack/nova/commit/9d2ea970422591f8cdc394001be9a2deca499a5f
Submitter: Jenkins
Branch: master

commit 9d2ea970422591f8cdc394001be9a2deca499a5f
Author: Pádraig Brady <email address hidden>
Date: Fri Nov 23 14:59:13 2012 +0000

    Don't leak info from libvirt LVM backed instances

    * nova/virt/libvirt/utils.py (remove_logical_volumes):
    Overwrite each logical volume with zero
    (clear_logical_volume): LV obfuscation implementation.
    (logical_volume_size): A utility function used by
    clear_logical_volume()

    Fixes bug: 1070539
    Change-Id: I4e1024de8dfe9b0be3b0d6437c836d2042862f85

Changed in nova:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to nova (stable/folsom)

Reviewed: https://review.openstack.org/17857
Committed: http://github.com/openstack/nova/commit/a99a802e008eed18e39fc1d98170edc495cbd354
Submitter: Jenkins
Branch: stable/folsom

commit a99a802e008eed18e39fc1d98170edc495cbd354
Author: Pádraig Brady <email address hidden>
Date: Fri Nov 23 14:59:13 2012 +0000

    Don't leak info from libvirt LVM backed instances

    * nova/virt/libvirt/utils.py (remove_logical_volumes):
    Overwrite each logical volume with zero
    (clear_logical_volume): LV obfuscation implementation.
    (logical_volume_size): A utility function used by
    clear_logical_volume()

    Fixes bug: 1070539
    Change-Id: I4e1024de8dfe9b0be3b0d6437c836d2042862f85

Revision history for this message
Thierry Carrez (ttx) wrote : Re: create_lvm_image allocates dirty blocks

[OSSA 2012-020]

Revision history for this message
Clint Byrum (clint-fewbar) wrote : Please test proposed package

Hello Eric, or anyone else affected,

Accepted nova into quantal-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/nova/2012.2.1+stable-20121212-a99a802e-0ubuntu1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

tags: added: verification-needed
Thierry Carrez (ttx)
Changed in nova:
milestone: none → grizzly-2
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in nova:
milestone: grizzly-2 → 2013.1
Thierry Carrez (ttx)
summary: - create_lvm_image allocates dirty blocks
+ [OSSA 2012-020] create_lvm_image allocates dirty blocks (CVE-2012-5625)
Changed in ossa:
status: New → Fix Released
assignee: nobody → Thierry Carrez (ttx)
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Bug attachments