Re: [Qemu-devel] [PATCH v1 12/15] qcow2: add support for LUKS encryption format

[Max Reitz]

On 24.01.2017 14:58, Daniel P. Berrange wrote:
> On Sat, Jan 21, 2017 at 07:57:45PM +0100, Max Reitz wrote:
>> On 03.01.2017 19:27, Daniel P. Berrange wrote:

[...]

>>> diff --git a/tests/qemu-iotests/174 b/tests/qemu-iotests/174
>>> new file mode 100755
>>> index 0000000..27d4870
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/174
>>> +_supported_fmt qcow2
>>> +_supported_proto generic
>>> +_supported_os Linux
>>> +
>>> +
>>> +size=128M
>>> +
>>> +SECRET="secret,id=sec0,data=astrochicken"
>>> +SECRETALT="secret,id=sec0,data=platypus"
>>> +
>>> +_make_test_img --object $SECRET -o 
>>> "encryption-format=luks,luks-key-secret=sec0" $size
>>> +
>>> +IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,luks-key-secret=sec0"
>>> +
>>> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
>>> +
>>> +echo
>>> +echo "== reading whole image =="
>>> +$QEMU_IO --object $SECRET -c "read 0 $size" --image-opts $IMGSPEC | 
>>> _filter_qemu_io | _filter_testdir
>>
>> Shouldn't "read -P 0 0 $size" work here, too?
> 
> The underlying disk image contents will be zeros, but we'll then decrypt
> those zeros and get random garbage.

There are not disk image contents yet because you didn't use
preallocation. qcow2_co_preadv() always returns 0 for unallocated
clusters (without a backing file) and zero clusters.

While looking at that place in qcow2_co_preadv(), I also noticed that
compressed clusters are not encrypted. That looks like a flaw to me that
the user should at least be warned about when invoking qemu-img convert
with the -c option.

(You can test this by converting a disk image to an encrypted compressed
image (qemu-img convert with -c and -o encryption-format=luks; note this
doesn't actually work unless you hack into qemu-img.c, I'll write a
separate mail about this as a response to the cover letter) and then
just set the crypt_method field to 0 and overwrite the disk encryption
header extension type with 0. If all of the clusters could be compressed
(which is the case if the original image was all filled with 42-bytes or
something), then qemu-img compare will happily declare your image to be
equal to the original, without requiring any key.)

> We could only use -P 0 if we explicitly fill with encrypted-zeros.

Max

signature.asc
Description: OpenPGP digital signature