Re: [Qemu-block] [Qemu-devel] [PATCH] block: Deprecate QCOW/QCOW2 encryption

[Daniel P. Berrange]

On Fri, Mar 13, 2015 at 09:09:40PM +0100, Markus Armbruster wrote:
> We've steered users away from QCOW/QCOW2 encryption for a while,
> because it's a flawed design (commit 136cd19 Describe flaws in
> qcow/qcow2 encryption in the docs).
> 
> In addition to flawed crypto, we have comically bad usability, and
> plain old bugs.  Let me show you.
> 
> = Example images =
> 
> I'm going to use a raw image as backing file, and two QCOW2 images,
> one encrypted, and one not:
> 
>     $ qemu-img create -f raw backing.img 4m
>     Formatting 'backing.img', fmt=raw size=4194304
>     $ qemu-img create -f qcow2 -o 
> encryption,backing_file=backing.img,backing_fmt=raw geheim.qcow2 4m
>     Formatting 'geheim.qcow2', fmt=qcow2 size=4194304 
> backing_file='backing.img' backing_fmt='raw' encryption=on cluster_size=65536 
> lazy_refcounts=off
>     $ qemu-img create -f qcow2 -o backing_file=backing.img,backing_fmt=raw 
> normal.qcow2 4m
>     Formatting 'normal.qcow2', fmt=qcow2 size=4194304 
> backing_file='backing.img' backing_fmt='raw' encryption=off 
> cluster_size=65536 lazy_refcounts=off
> 
> = Usability issues =
> 
> == Confusing startup ==
> 
> When no image is encrypted, and you don't give -S, QEMU starts the
> guest immediately:
> 
>     $ qemu-system-x86_64 -nodefaults -display none -monitor stdio normal.qcow2
>     QEMU 2.2.50 monitor - type 'help' for more information
>     (qemu) info status
>     VM status: running
> 
> But as soon as there's an encrypted image in play, the guest is *not*
> started, with no notification whatsoever:
> 
>     $ qemu-system-x86_64 -nodefaults -display none -monitor stdio geheim.qcow2
>     QEMU 2.2.50 monitor - type 'help' for more information
>     (qemu) info status
>     VM status: paused (prelaunch)
> 
> If the user figured out that he needs to type "cont" to enter his
> keys, the confusion enters the next level: "cont" asks for at most
> *one* key.  If more are needed, it then silently does nothing.  The
> user has to type "cont" once per encrypted image:
> 
>     $ qemu-system-x86_64 -nodefaults -display none -monitor stdio -drive 
> if=none,file=geheim.qcow2 -drive if=none,file=geheim.qcow2
>     QEMU 2.2.50 monitor - type 'help' for more information
>     (qemu) info status
>     VM status: paused (prelaunch)
>     (qemu) c
>     none0 (geheim.qcow2) is encrypted.
>     Password: ******
>     (qemu) info status
>     VM status: paused (prelaunch)
>     (qemu) c
>     none1 (geheim.qcow2) is encrypted.
>     Password: ******
>     (qemu) info status
>     VM status: running
> 
> == Incorrect passwords not caught ==
> 
> All existing encryption schemes give you the GIGO treatment: garbage
> password in, garbage data out.  Guests usually refuse to mount
> garbage, but other usage is prone to data loss.
> 
> == Need to stop the guest to add an encrypted image ==
> 
>     $ qemu-system-x86_64 -nodefaults -display none -monitor stdio
>     QEMU 2.2.50 monitor - type 'help' for more information
>     (qemu) info status
>     VM status: running
>     (qemu) drive_add "" if=none,file=geheim.qcow2
>     Guest must be stopped for opening of encrypted image
>     (qemu) stop
>     (qemu) drive_add "" if=none,file=geheim.qcow2
>     OK
> 
> Commit c3adb58 added this restriction.  Before, we could expose images
> lacking an encryption key to guests, with potentially catastrophic
> results.  See also "Use without key is not always caught".
> 
> = Bugs =
> 
> == Use without key is not always caught ==
> 
> Encrypted images can be in an intermediate state "opened, but no key".
> The weird startup behavior and the need to stop the guest are there to
> ensure the guest isn't exposed to that state.  But other things still
> are!
> 
> * drive_backup
> 
>     $ qemu-system-x86_64 -nodefaults -display none -monitor stdio geheim.qcow2
>     QEMU 2.2.50 monitor - type 'help' for more information
>     (qemu) drive_backup -f ide0-hd0 out.img raw
>     Formatting 'out.img', fmt=raw size=4194304
> 
>   I guess this writes encrypted data to raw image out.img.  Good luck
>   with figuring out how to decrypt that again.
> 
> * commit
> 
>     $ qemu-system-x86_64 -nodefaults -display none -monitor stdio geheim.qcow2
>     QEMU 2.2.50 monitor - type 'help' for more information
>     (qemu) commit ide0-hd0
> 
>   I guess this writes encrypted data into the unencrypted raw backing
>   image, effectively destroying it.
> 
> == QMP device_add of usb-storage fails when it shouldn't ==
> 
> When the image is encrypted, device_add creates the device, defers
> actually attaching it to when the key becomes available, then fails.
> This is wrong.  device_add must either create the device and succeed,
> or do nothing and fail.
> 
>     $ qemu-system-x86_64 -nodefaults -display none -usb -qmp stdio -drive 
> if=none,id=foo,file=geheim.qcow2
>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2}, 
> "package": ""}, "capabilities": []}}
>     { "execute": "qmp_capabilities" }
>     {"return": {}}
>     { "execute": "device_add", "arguments": { "driver": "usb-storage", "id": 
> "bar", "drive": "foo" } }
>     {"error": {"class": "DeviceEncrypted", "desc": "'foo' (geheim.qcow2) is 
> encrypted"}}
>     {"execute":"device_del","arguments": { "id": "bar" } }
>     {"timestamp": {"seconds": 1426003440, "microseconds": 237181}, "event": 
> "DEVICE_DELETED", "data": {"path": "/machine/peripheral/bar/bar.0/legacy[0]"}}
>     {"timestamp": {"seconds": 1426003440, "microseconds": 238231}, "event": 
> "DEVICE_DELETED", "data": {"device": "bar", "path": 
> "/machine/peripheral/bar"}}
>     {"return": {}}
> 
> This stuff is worse than useless, it's a trap for users.
> 
> If people become sufficiently interested in encrypted images to
> contribute a cryptographically sane implementation for QCOW2 (or
> whatever other format), then rewriting the necessary support around it
> from scratch will likely be easier and yield better results than
> fixing up the existing mess.
> 
> Let's deprecate the mess now, drop it after a grace period, and move
> on.
> 
> Signed-off-by: Markus Armbruster <address@hidden>

Reviewed-by: Daniel P. Berrange <address@hidden>


Looks good for this release, then we kill in next release.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|