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

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 ?

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)

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 ?

Yup.

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.

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.

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.

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.

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.

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

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

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.

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

Requires a new rootwrap entry...

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

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

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

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

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 ?

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

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.

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.

More reviews and opinions wanted from nova-core here.

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

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

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.

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/

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

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

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.

@nova-core: new review round

+2 on latest

Nicely done Pádraig, looks good to me

+2 as well.

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.

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

+1 to impact description.

thanks Thierry.

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

description sounds good to me

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

CVE-2012-5625

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

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

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

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

[OSSA 2012-020]

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!