Re: [Qemu-devel] The unholy encrypted image key mess

[Eric Blake]

On 02/28/2014 02:01 PM, Markus Armbruster wrote:
> The encrypted images Saturday afternoon hack is a gift that keeps on
> giving.  I wish we could rip it out and bury it deep.  Or that I could
> continue to ignore it.  Unfortunately, it looks like I need to touch it
> to clean up error propagation in the monitor.  So here goes.
> 
> A naive user might expect QEMU to require the password right when he
> asks QEMU to open an encrypted image.  But that's not how it works.
> 

> 
> The user can open an image via command line, HMP commands change,
> usb_add (legacy), drive_add, and QMP command blockdev-add.  They all
> create BlockDriverStates in states NOKEY or NEEDKEY.

Uggh - so there's no current way to hot-plug a device in state GOTKEY
short of using a two-command sequence?  It would be nicer if hot-plug
had a way to fail to add encrypted devices unless the user also passes
the password at the same time, creating the device directly into the
GOTKEY state.

> 
> The user can set a key with HMP / QMP command block_passwd.  If it
> succeeds, state goes from NEEDKEY to GOTKEY.
> 
> You can't unpause a guest while a BlockDriverState is in state NEEDKEY:
> 
> * QMP command cont fails if any BlockDriverState is in state NEEDKEY.
> 
> * HMP command cont tries to be cute then: it picks a BlockDriverState in
>   state NEEDKEY and prompts for its key.  If this is the only one in
>   state NEEDKEY, it unpauses the guest as ordered.  Else it silently
>   doesn't.  The user is expected to know to rerun cont for the next key
>   prompt.

Arguably, the HMP behavior could be made smarter, to do the loop itself.

> 
> This might make you think that the guest is protected from ever having a
> block device backed by a BlockDriverState in state NEEDKEY.  Not true!
> Simply open an image while the guest runs, and hot-plug a device backed
> by it.  The guest will happily read encrypted garbage from it, and if it
> writes anything to it, it's not so encrypted anymore.  To add insult to
> injury, the "protection" kicks in next time you pause the guest.
> 
> Changing media has its wrinkles, too:
> 
> * HMP command change first closes the old image, then opens the new
>   image, and if it's encrypted, it asks for a key.  Okay.
> 
> * QMP command change first opens the image, then errors out if it's
>   encrypted and the password argument is missing, or it's not encrypted
>   and the password argument is present.  These two errors aren't really
>   failures; the change succeeds just fine.

Indeed, the 'change' command specifically documents that using 'change'
on a running guest with an encrypted image is ill-advised, because
there's no password provided along with the 'change' action, so the user
is forced to do a 2-command sequence where any window with the guest
running is hosed before the followup 'block_passwd'.

Question: when changing from one encrypted image to another, does the
second image inherit the password from the first (whether or not it
decrypts the second), or do we always clean state back to having no
known password?  The way you worded it, it sounds like the password is
inherited from the first media to the second?

> 
>   Clients can detect the former non-failure error (it's class
>   "DeviceEncrypted"), and use block_passwd to go to state GOTKEY.

This makes sense whether you go from unencrypted to encrypted media, or
if going to an encrypted media always wipes out the old password state.

> 
>   Clients can't reliably distinguish the latter non-failure error from
>   real errors, because it's class "GenericError".

It seems like this error (providing a password argument for a
non-encrypted disk) is not possible via the 'change' command.  How
exactly is this state even triggered?

> 
> In any case, callback bdrv_dev_change_media_cb() is delayed for
> encrypted images until we enter state GOTKEY.  As afar as I can tell,
> nothing stops the device model from reading or writing before it gets
> the callback, but that would be a device model bug.

And one that the qapi docs specifically warn against.

> 
> The final funny is device model usb-storage (another one that
> desperately needs to be buried deep).  Its init() callback
> usb_msd_initfn_storage() tries to be cute when it detects state NEEDKEY.
> 
> If it's running in monitor context (say in HMP/QMP command device_add),
> it attempts to ask for a key.  In HMP context, it unplugs itself when
> this fails (I think).  In QMP context, it behaves similar to change: it
> works, but you get a "DeviceEncrypted" error, and the backend remains in
> state NEEDKEY.
> 
> If it's not running in monitor context, it clears autostart.  No idea
> why it does that, and I'm not sure it has any effect.  Opening an
> encrypted image clears autostart already, in blockdev_init().
> 
> Thankfully, usb-storage is the only device model that messes with keys.
> 
> Questions:
> 
> 1. Should we protect guests from state NEEDKEY?

Ideally, yes.

> 
> 2. If yes, how?
> 
>    Pause the guest when something enters state NEEDKEY?  I'd hate that.
> 
>    Fail device_add in state NEEDKEY?  Takes care of hot-plug, and
>    cold-plug is already protected by cont.

The existing QMP 'change' command is stupid.  Add a new 'disk-change'
command that takes a password argument, and require that the new media
either be unencrypted (and error if a password was provided) or fully
reach the GOTKEY state (and error if no password was provided) for the
command to successfully change the media.  Similarly for hotplug.  That
is, enforce that hot-plug can never make a NEEDKEY block device visible
to the guest.

> Other bright ideas?

Use the fact that we are calling the next release "2.0" to actually kill
qemu disk encryption as a horribly botched feature, on the grounds that
we are doing users a favor by not letting them use broken encryption?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature