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 <address@hidden>
> > ---
> 
> > @@ -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, &s->crypto_header, ext.len);
> > +            if (ret < 0) {
> > +                error_setg_errno(errp, -ret,
> > +                                 "Unable to read CRYPTO header extension");
> > +                return ret;
> > +            }
> > +            be64_to_cpus(&s->crypto_header.offset);
> > +            be64_to_cpus(&s->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 :|