Re: [Qemu-block] [PATCH v6 13/18] qcow2: add support for LUKS encryption format

[Alberto Garcia]

On Tue 25 Apr 2017 05:38:53 PM CEST, Daniel P. Berrange wrote:
> This adds support for using LUKS as an encryption format
> with the qcow2 file, using the new encrypt.format parameter
> to request "luks" format. e.g.
>
>   # qemu-img create --object secret,data=123456,id=sec0 \
>-f qcow2 -o encrypt.-format=luks,encrypt.key-secret=sec0 \
>test.qcow2 10G
>
> 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,encrypt.key-secret=sec0 \
>test.qcow2 10G
>
>  # qemu-img create --object secret,data=123456,id=sec0 \
>-f qcow2 -o encryption-format=aes,encrypt.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 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH v6 13/18] qcow2: add support for LUKS encryption format

[Daniel P. Berrange]

On Wed, Apr 26, 2017 at 12:46:18PM -0500, Eric Blake wrote:
> On 04/25/2017 10:38 AM, Daniel P. Berrange wrote:
> > This adds support for using LUKS as an encryption format
> > with the qcow2 file, using the new encrypt.format parameter
> > to request "luks" format. e.g.
> > 
> >   # qemu-img create --object secret,data=123456,id=sec0 \
> >-f qcow2 -o encrypt.-format=luks,encrypt.key-secret=sec0 \
> 
> s/encrypt.-format/encrypt.format/
> 
> >test.qcow2 10G
> > 
> 
> > 
> > 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 
> > ---
> 
> > @@ -165,6 +246,47 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> > uint64_t start_offset,
> >  }
> >  break;
> >  
> > +case QCOW2_EXT_MAGIC_CRYPTO_HEADER: {
> > +unsigned int cflags = 0;
> > +if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
> > +error_setg(errp, "CRYPTO header extension only "
> > +   "expected with LUKS encryption method");
> > +return -EINVAL;
> > +}
> > +if (ext.len != sizeof(Qcow2CryptoHeaderExtension)) {
> > +error_setg(errp, "CRYPTO header extension size %u, "
> > +   "but expected size %zu", ext.len,
> > +   sizeof(Qcow2CryptoHeaderExtension));
> > +return -EINVAL;
> > +}
> > +
> > +ret = bdrv_pread(bs->file, offset, >crypto_header, ext.len);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret,
> > + "Unable to read CRYPTO header extension");
> > +return ret;
> > +}
> > +be64_to_cpus(>crypto_header.offset);
> > +be64_to_cpus(>crypto_header.length);
> > +
> > +if ((s->crypto_header.offset % s->cluster_size) != 0) {
> > +error_setg(errp, "Encryption header offset '%" PRIu64 "' 
> > is "
> > +   "not a multiple of cluster size '%u'",
> > +   s->crypto_header.offset, s->cluster_size);
> > +return -EINVAL;
> > +}
> 
> Do we need to sanity check that crypto_header.length is not bogus?

The only sanity check I can see would be to put an arbitrary size limit
on the length ? I'm a little loathe to do that since LUKSv2 is going to
make the header extensible, and/or future LUKSv1 changes might adjust
padding, both making the header longer than it would be today. I would
like qemu-img to be able to open such future files, even if it then
refuses support for the features.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [PATCH v6 13/18] qcow2: add support for LUKS encryption format

[Eric Blake]

On 04/25/2017 10:38 AM, Daniel P. Berrange wrote:
> This adds support for using LUKS as an encryption format
> with the qcow2 file, using the new encrypt.format parameter
> to request "luks" format. e.g.
> 
>   # qemu-img create --object secret,data=123456,id=sec0 \
>-f qcow2 -o encrypt.-format=luks,encrypt.key-secret=sec0 \

s/encrypt.-format/encrypt.format/

>test.qcow2 10G
> 

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

> @@ -165,6 +246,47 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> uint64_t start_offset,
>  }
>  break;
>  
> +case QCOW2_EXT_MAGIC_CRYPTO_HEADER: {
> +unsigned int cflags = 0;
> +if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
> +error_setg(errp, "CRYPTO header extension only "
> +   "expected with LUKS encryption method");
> +return -EINVAL;
> +}
> +if (ext.len != sizeof(Qcow2CryptoHeaderExtension)) {
> +error_setg(errp, "CRYPTO header extension size %u, "
> +   "but expected size %zu", ext.len,
> +   sizeof(Qcow2CryptoHeaderExtension));
> +return -EINVAL;
> +}
> +
> +ret = bdrv_pread(bs->file, offset, >crypto_header, ext.len);
> +if (ret < 0) {
> +error_setg_errno(errp, -ret,
> + "Unable to read CRYPTO header extension");
> +return ret;
> +}
> +be64_to_cpus(>crypto_header.offset);
> +be64_to_cpus(>crypto_header.length);
> +
> +if ((s->crypto_header.offset % s->cluster_size) != 0) {
> +error_setg(errp, "Encryption header offset '%" PRIu64 "' is "
> +   "not a multiple of cluster size '%u'",
> +   s->crypto_header.offset, s->cluster_size);
> +return -EINVAL;
> +}

Do we need to sanity check that crypto_header.length is not bogus?

> +
> +if (flags & BDRV_O_NO_IO) {
> +cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> +}
> +s->crypto = qcrypto_block_open(s->crypto_opts,
> +   qcow2_crypto_hdr_read_func,
> +   bs, cflags, errp);
> +if (!s->crypto) {
> +return -EINVAL;
> +}
> +}   break;
> +
>  default:
>  /* unknown magic - save it in case we need to rewrite the header 
> */
>  {
> @@ -464,7 +586,8 @@ static QemuOptsList qcow2_runtime_opts = {
>  .type = QEMU_OPT_NUMBER,
>  .help = "Clean unused cache entries after this time (in 
> seconds)",
>  },
> -BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET("encrypt."),
> +BLOCK_CRYPTO_OPT_DEF_KEY_SECRET("encrypt.",
> +"ID of secret providing qcow AES key or LUKS passphrase"),

s/qcow/qcow2/ ?

Minor enough that I'm okay giving:
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature