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

[Erica Windisch]

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.

[Thierry Carrez (ttx)]

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 ?

[Erica Windisch (ewindisch)]

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)

[Thierry Carrez (ttx)]

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 ?

[Vish Ishaya (vishvananda)]

Yup.

[Thierry Carrez (ttx)]

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.

[Michael Still (mikal)]

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.

[Erica Windisch (ewindisch)]

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.

[Pádraig Brady (p-draigbrady)]

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.

[Erica Windisch (ewindisch)]

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.

[Erica Windisch (ewindisch)]

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

[Vish Ishaya (vishvananda)]

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

[Erica Windisch (ewindisch)]

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.

[Erica Windisch (ewindisch)]

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

Requires a new rootwrap entry...

[Russell Bryant (russellb)]

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

[Yun Mao (yunmao)]

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

[Erica Windisch (ewindisch)]

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

[Erica Windisch (ewindisch)]

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

[Thierry Carrez (ttx)]

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 ?

[Thierry Carrez (ttx)]

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

[Erica Windisch (ewindisch)]

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.

[Thierry Carrez (ttx)]

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.

[Thierry Carrez (ttx)]

More reviews and opinions wanted from nova-core here.

[Erica Windisch (ewindisch)]

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

[Michael Still (mikal)]

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

[Thierry Carrez (ttx)]

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.

[Pádraig Brady (p-draigbrady)]

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/

[Thierry Carrez (ttx)]

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

[Erica Windisch (ewindisch)]

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

[Pádraig Brady (p-draigbrady)]

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.

[Thierry Carrez (ttx)]

@nova-core: new review round

[Vish Ishaya (vishvananda)]

+2 on latest

[Mark McLoughlin (markmc)]

Nicely done Pádraig, looks good to me

[Michael Still (mikal)]

+2 as well.

[Thierry Carrez (ttx)]

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.

[Erica Windisch (ewindisch)]

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

[Pádraig Brady (p-draigbrady)]

+1 to impact description.

thanks Thierry.

[Erica Windisch (ewindisch)]

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

[Russell Bryant (russellb)]

description sounds good to me

[Thierry Carrez (ttx)]

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

[Thierry Carrez (ttx)]

CVE-2012-5625

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[Thierry Carrez (ttx)]

[OSSA 2012-020]

[Clint Byrum (clint-fewbar)]

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!