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

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

2017-01-25 Thread 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 000..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

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

2017-01-24 Thread Daniel P. Berrange

On Sat, Jan 21, 2017 at 07:57:45PM +0100, Max Reitz wrote:
> On 03.01.2017 19:27, Daniel P. Berrange wrote:
> > This adds support for using LUKS as an encryption format
> > with the qcow2 file. The use of the existing 'encryption=on'
> > parameter is replaced by a new parameter 'encryption-format'
> > which takes the values 'aes' or 'luks'. e.g.
> > 
> >   # qemu-img create --object secret,data=123456,id=sec0 \
> >-f qcow2 -o encryption-format=luks,luks-key-secret=sec0 \
> >test.qcow2 10G
> > 
> > results in the creation of an image using the LUKS format.
> > Use of the legacy 'encryption=on' parameter still results
> > in creation of the old qcow2 AES format, and is equivalent
> > to the new 'encryption-format=aes'. e.g. the following are
> > equivalent:
> > 
> >   # qemu-img create --object secret,data=123456,id=sec0 \
> >-f qcow2 -o encryption=on,aes-key-secret=sec0 \
> >test.qcow2 10G
> > 
> >  # qemu-img create --object secret,data=123456,id=sec0 \
> >-f qcow2 -o encryption-format=aes,aes-key-secret=sec0 \
> >test.qcow2 10G
> > 
> > With the LUKS format it is necessary to store the LUKS
> > partition header and key material in the QCow2 file. This
> > data can be many MB in size, so cannot go into the QCow2
> > header region directly. Thus the spec defines a FDE
> > (Full Disk Encryption) header extension that specifies
> > the offset of a set of clusters to hold the FDE headers,
> > as well as the length of that region. The LUKS header is
> > thus stored in these extra allocated clusters before the
> > main image payload.
> > 
> > Aside from all the cryptographic differences implied by
> > use of the LUKS format, there is one further key difference
> > between the use of legacy AES and LUKS encryption in qcow2.
> > For LUKS, the initialiazation vectors are generated using
> > the host physical sector as the input, rather than the
> > guest virtual sector. This guarantees unique initialization
> > vectors for all sectors when qcow2 internal snapshots are
> > used, thus giving stronger protection against watermarking
> > attacks.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block/qcow2-cluster.c  |   4 +-
> >  block/qcow2-refcount.c |  10 ++
> >  block/qcow2.c  | 236 
> > ++---
> >  block/qcow2.h  |   9 ++
> >  docs/specs/qcow2.txt   |  99 +++
> 
> I'd personally rather have specification changes separate from the
> implementation.
> 
> >  qapi/block-core.json   |   6 +-
> >  tests/qemu-iotests/049 |   2 +-
> >  tests/qemu-iotests/082.out | 216 +
> >  tests/qemu-iotests/087 |  65 -
> >  tests/qemu-iotests/087.out |  22 -
> >  tests/qemu-iotests/134 |   4 +-
> >  tests/qemu-iotests/158 |   8 +-
> >  tests/qemu-iotests/174 |  76 +++
> >  tests/qemu-iotests/174.out |  19 
> >  tests/qemu-iotests/175 |  85 
> >  tests/qemu-iotests/175.out |  26 +
> >  tests/qemu-iotests/group   |   2 +
> >  17 files changed, 840 insertions(+), 49 deletions(-)
> >  create mode 100755 tests/qemu-iotests/174
> >  create mode 100644 tests/qemu-iotests/174.out
> >  create mode 100755 tests/qemu-iotests/175
> >  create mode 100644 tests/qemu-iotests/175.out
> 
> Also, in order to help reviewing by making this patch less scary (840
> insertions are kind of... Urgh.), it would make sense to add these two
> tests in one or two separate patches.

Ok, will split it in three - spec change, code change and test
additions.


> > @@ -80,6 +81,73 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, 
> > const char *filename)
> 
> [...]
> 
> > +static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t 
> > headerlen,
> > +  Error **errp, void *opaque)
> > +{
> > +BlockDriverState *bs = opaque;
> > +BDRVQcow2State *s = bs->opaque;
> > +int64_t ret;
> > +
> > +ret = qcow2_alloc_clusters(bs, headerlen);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret,
> > + "Cannot allocate cluster for LUKS header size 
> > %zu",
> > + headerlen);
> > +return -1;
> > +}
> > +
> > +s->crypto_header.length = headerlen;
> > +s->crypto_header.offset = ret;
> 
> The specification says any unused space in the last cluster has to be
> set to zero. This isn't done here.
> 
> I don't have a preference whether you write zeroes to the range in
> question here or whether you simply relax the specification to say that
> anything beyond crypto_header.length is undefined.

I'll explicitly fill it with zeros.

> > +static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t 
> > offset,
> > +   const uint8_t *buf, size_t 
> > buflen,
> > +

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

2017-01-21 Thread Max Reitz

On 03.01.2017 19:27, Daniel P. Berrange wrote:
> This adds support for using LUKS as an encryption format
> with the qcow2 file. The use of the existing 'encryption=on'
> parameter is replaced by a new parameter 'encryption-format'
> which takes the values 'aes' or 'luks'. e.g.
> 
>   # qemu-img create --object secret,data=123456,id=sec0 \
>-f qcow2 -o encryption-format=luks,luks-key-secret=sec0 \
>test.qcow2 10G
> 
> results in the creation of an image using the LUKS format.
> Use of the legacy 'encryption=on' parameter still results
> in creation of the old qcow2 AES format, and is equivalent
> to the new 'encryption-format=aes'. e.g. the following are
> equivalent:
> 
>   # qemu-img create --object secret,data=123456,id=sec0 \
>-f qcow2 -o encryption=on,aes-key-secret=sec0 \
>test.qcow2 10G
> 
>  # qemu-img create --object secret,data=123456,id=sec0 \
>-f qcow2 -o encryption-format=aes,aes-key-secret=sec0 \
>test.qcow2 10G
> 
> With the LUKS format it is necessary to store the LUKS
> partition header and key material in the QCow2 file. This
> data can be many MB in size, so cannot go into the QCow2
> header region directly. Thus the spec defines a FDE
> (Full Disk Encryption) header extension that specifies
> the offset of a set of clusters to hold the FDE headers,
> as well as the length of that region. The LUKS header is
> thus stored in these extra allocated clusters before the
> main image payload.
> 
> Aside from all the cryptographic differences implied by
> use of the LUKS format, there is one further key difference
> between the use of legacy AES and LUKS encryption in qcow2.
> For LUKS, the initialiazation vectors are generated using
> the host physical sector as the input, rather than the
> guest virtual sector. This guarantees unique initialization
> vectors for all sectors when qcow2 internal snapshots are
> used, thus giving stronger protection against watermarking
> attacks.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  block/qcow2-cluster.c  |   4 +-
>  block/qcow2-refcount.c |  10 ++
>  block/qcow2.c  | 236 
> ++---
>  block/qcow2.h  |   9 ++
>  docs/specs/qcow2.txt   |  99 +++

I'd personally rather have specification changes separate from the
implementation.

>  qapi/block-core.json   |   6 +-
>  tests/qemu-iotests/049 |   2 +-
>  tests/qemu-iotests/082.out | 216 +
>  tests/qemu-iotests/087 |  65 -
>  tests/qemu-iotests/087.out |  22 -
>  tests/qemu-iotests/134 |   4 +-
>  tests/qemu-iotests/158 |   8 +-
>  tests/qemu-iotests/174 |  76 +++
>  tests/qemu-iotests/174.out |  19 
>  tests/qemu-iotests/175 |  85 
>  tests/qemu-iotests/175.out |  26 +
>  tests/qemu-iotests/group   |   2 +
>  17 files changed, 840 insertions(+), 49 deletions(-)
>  create mode 100755 tests/qemu-iotests/174
>  create mode 100644 tests/qemu-iotests/174.out
>  create mode 100755 tests/qemu-iotests/175
>  create mode 100644 tests/qemu-iotests/175.out

Also, in order to help reviewing by making this patch less scary (840
insertions are kind of... Urgh.), it would make sense to add these two
tests in one or two separate patches.

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index a2103dc..866b122 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -383,7 +383,9 @@ static int coroutine_fn do_perform_cow(BlockDriverState 
> *bs,
>  
>  if (bs->encrypted) {
>  Error *err = NULL;
> -int64_t sector = (src_cluster_offset + offset_in_cluster)
> +int64_t sector = (s->crypt_physical_offset ?
> +  (cluster_offset + offset_in_cluster) :
> +  (src_cluster_offset + offset_in_cluster))
>   >> BDRV_SECTOR_BITS;
>  assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>  assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index cbfb3fe..afa4636 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c

[...]

> @@ -80,6 +81,73 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, 
> const char *filename)

[...]

> +static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t 
> headerlen,
> +  Error **errp, void *opaque)
> +{
> +BlockDriverState *bs = opaque;
> +BDRVQcow2State *s = bs->opaque;
> +int64_t ret;
> +
> +ret = qcow2_alloc_clusters(bs, headerlen);
> +if (ret < 0) {
> +error_setg_errno(errp, -ret,
> + "Cannot allocate cluster for LUKS header size %zu",
> + headerlen);
> +return -1;
> +}
> +
> +s->crypto_header.length = headerlen;
> +s->crypto_header.offset = ret;

The

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

2017-01-03 Thread Daniel P. Berrange

This adds support for using LUKS as an encryption format
with the qcow2 file. The use of the existing 'encryption=on'
parameter is replaced by a new parameter 'encryption-format'
which takes the values 'aes' or 'luks'. e.g.

  # qemu-img create --object secret,data=123456,id=sec0 \
   -f qcow2 -o encryption-format=luks,luks-key-secret=sec0 \
   test.qcow2 10G

results in the creation of an image using the LUKS format.
Use of the legacy 'encryption=on' parameter still results
in creation of the old qcow2 AES format, and is equivalent
to the new 'encryption-format=aes'. e.g. the following are
equivalent:

  # qemu-img create --object secret,data=123456,id=sec0 \
   -f qcow2 -o encryption=on,aes-key-secret=sec0 \
   test.qcow2 10G

 # qemu-img create --object secret,data=123456,id=sec0 \
   -f qcow2 -o encryption-format=aes,aes-key-secret=sec0 \
   test.qcow2 10G

With the LUKS format it is necessary to store the LUKS
partition header and key material in the QCow2 file. This
data can be many MB in size, so cannot go into the QCow2
header region directly. Thus the spec defines a FDE
(Full Disk Encryption) header extension that specifies
the offset of a set of clusters to hold the FDE headers,
as well as the length of that region. The LUKS header is
thus stored in these extra allocated clusters before the
main image payload.

Aside from all the cryptographic differences implied by
use of the LUKS format, there is one further key difference
between the use of legacy AES and LUKS encryption in qcow2.
For LUKS, the initialiazation vectors are generated using
the host physical sector as the input, rather than the
guest virtual sector. This guarantees unique initialization
vectors for all sectors when qcow2 internal snapshots are
used, thus giving stronger protection against watermarking
attacks.

Signed-off-by: Daniel P. Berrange 
---
 block/qcow2-cluster.c  |   4 +-
 block/qcow2-refcount.c |  10 ++
 block/qcow2.c  | 236 ++---
 block/qcow2.h  |   9 ++
 docs/specs/qcow2.txt   |  99 +++
 qapi/block-core.json   |   6 +-
 tests/qemu-iotests/049 |   2 +-
 tests/qemu-iotests/082.out | 216 +
 tests/qemu-iotests/087 |  65 -
 tests/qemu-iotests/087.out |  22 -
 tests/qemu-iotests/134 |   4 +-
 tests/qemu-iotests/158 |   8 +-
 tests/qemu-iotests/174 |  76 +++
 tests/qemu-iotests/174.out |  19 
 tests/qemu-iotests/175 |  85 
 tests/qemu-iotests/175.out |  26 +
 tests/qemu-iotests/group   |   2 +
 17 files changed, 840 insertions(+), 49 deletions(-)
 create mode 100755 tests/qemu-iotests/174
 create mode 100644 tests/qemu-iotests/174.out
 create mode 100755 tests/qemu-iotests/175
 create mode 100644 tests/qemu-iotests/175.out

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index a2103dc..866b122 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -383,7 +383,9 @@ static int coroutine_fn do_perform_cow(BlockDriverState *bs,
 
 if (bs->encrypted) {
 Error *err = NULL;
-int64_t sector = (src_cluster_offset + offset_in_cluster)
+int64_t sector = (s->crypt_physical_offset ?
+  (cluster_offset + offset_in_cluster) :
+  (src_cluster_offset + offset_in_cluster))
  >> BDRV_SECTOR_BITS;
 assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
 assert((bytes & ~BDRV_SECTOR_MASK) == 0);
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index cbfb3fe..afa4636 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1843,6 +1843,16 @@ static int calculate_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
 return ret;
 }
 
+/* encryption */
+if (s->crypto_header.length) {
+ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
+s->crypto_header.offset,
+s->crypto_header.length);
+if (ret < 0) {
+return ret;
+}
+}
+
 return check_refblocks(bs, res, fix, rebuild, refcount_table, nb_clusters);
 }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 5c9e196..b354914 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -66,6 +66,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_END 0
 #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
 
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -80,6 +81,73 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, 
const char *filename)
 }
 
 
+static ssize_t qcow2_crypto_hdr_read_func(QCryptoBlock *block, size_t offset,
+  uint8_t *buf, size_t buflen,
+

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information