Re: [Qemu-devel] [PATCH v2 07/17] crypto: implement the LUKS block encryption format

["Daniel P. Berrange" <berrange@redhat.com>]

On Fri, Feb 05, 2016 at 10:38:45AM -0700, Eric Blake wrote:
> On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
> > Provide a block encryption implementation that follows the
> > LUKS/dm-crypt specification.
> > 
> > This supports all combinations of hash, cipher algorithm,
> > cipher mode and iv generator that are implemented by the
> > current crypto layer.
> > 
> > The notable missing feature is support for the 'xts'
> > cipher mode, which is commonly used for disk encryption
> > instead of 'cbc'. This is because it is not provided by
> > either nettle or libgcrypt. A suitable implementation
> > will be identified & integrated later.
> > 
> > There is support for opening existing volumes formatted
> > by dm-crypt, and for formatting new volumes. In the latter
> > case it will only use key slot 0.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>


> > +
> > +/*
> > + * Reference for format is
> > + *
> > + * docs/on-disk-format.pdf
> > + *
> > + * In 'cryptsetup' package source code
> > + *
> 
> Worth calling out the document version number? I reviewed based on
> version 1.2.1, dated Oct 2011.

Yep


> > +};
> > +
> > +
> > +struct QCryptoBlockLUKSKeySlot {
> > +    /* state of keyslot, enabled/disable */
> > +    uint32_t active;
> 
> May be worth a comment that this struct is written to disk in
> big-endian, but used in native-endian for most operations for
> convenience; to make sure future developers remember to put byteswaps in
> the correct locations.  But it looks like you handled endianness correctly.

Yes, will note it.

> 
> > +    /* iterations for PBKDF2 */
> > +    uint32_t iterations;
> > +    /* salt for PBKDF2 */
> > +    uint8_t salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
> > +    /* start sector of key material */
> > +    uint32_t key_offset;
> > +    /* number of anti-forensic stripes */
> > +    uint32_t stripes;
> > +} QEMU_PACKED;
> 
> Packed makes sense since we are writing it straight to disk; although it
> looks like natural alignment prevails and this did not need to be
> packed.  Do we want some sort of BUG_ON() that we have the right size
> struct?

Yes, I'll add QEMU_BUILD_BUG_ON

> > +
> > +struct QCryptoBlockLUKSHeader {
> > +    /* 'L', 'U', 'K', 'S', '0xBA', '0xBE' */
> > +    char magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN];
> > +
> > +    /* LUKS version, currently 1 */
> > +    uint16_t version;
> > +
> > +    /* cipher name specification (aes, etc */
> 
> Missing )
> 
> > +    char cipher_name[QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN];
> > +
> > +    /* cipher mode specification () */
> 
> text in the ()?
> 
> > +    char cipher_mode[QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN];
> > +
> > +    /* hash specication () */
> 
> s/specication/specification/, text in the ()?

Will fix all these examples

> 
> > +    char hash_spec[QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN];
> > +
> > +    /* start offset of the volume data (in sectors) */
> > +    uint32_t payload_offset;
> 
> Should we say "in 512-byte sectors"?  I could not see anything in the
> LUKS specification that said whether SECTOR_SIZE can be larger than 512
> - if you have a bare metal 4k sector disk, does a payload_offset of 2
> mean 1024 or 8192 bytes into the disk?  Eww. Sounds like we should
> submit a bug against the LUKS spec to have it clarified.

cryptsetup is hardcoded to use 512 byte sectors currently. IIUC
they'll need a LUKS spec update before doing 4k sectors natively.

> 
> > +
> > +    /* Number of key bytes */
> > +    uint32_t key_bytes;
> > +
> > +    /* master key checksum after PBKDF2 */
> > +    uint8_t master_key_digest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
> > +
> > +    /* salt for master key PBKDF2 */
> > +    uint8_t master_key_salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
> > +
> > +    /* iterations for master key PBKDF2 */
> > +    uint32_t master_key_iterations;
> > +
> > +    /* UUID of the partition */
> 
> Should we say "in standard ASCII representation"?
> 
> > +    uint8_t uuid[QCRYPTO_BLOCK_LUKS_UUID_LEN];
> > +
> > +    /* key slots */
> > +    QCryptoBlockLUKSKeySlot key_slots[QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS];
> > +} QEMU_PACKED;
> 
> Again, alignment should mean that we could get away without packing; but
> the packing doesn't hurt; and a BUG_ON may be nice to ensure size.

Yep, add QEMU_BUILD_BUG_ON

> > +/* XXX doesn't QEMU already have a string -> enum val convertor ? */
> > +static int qcrypto_block_luks_name_lookup(const char *name,
> 
> Yes, spelled qapi_enum_parse().  Works if the enum was defined in a
> .json file.
> 
> 
> > +
> > +    error_setg(errp, "%s %s not supported", type, name);
> > +    return 0;
> > +}
> > +
> > +#define qcrypto_block_luks_cipher_mode_lookup(name, errp)               \
> > +    qcrypto_block_luks_name_lookup(name,                                \
> > +                                   QCryptoCipherMode_lookup,            \
> > +                                   QCRYPTO_CIPHER_MODE__MAX,            \
> > +                                   "Cipher mode",                       \
> > +                                   errp)
> > +
> > +#define qcrypto_block_luks_hash_name_lookup(name, errp)                 \
> > +    qcrypto_block_luks_name_lookup(name,                                \
> > +                                   QCryptoHashAlgorithm_lookup,         \
> > +                                   QCRYPTO_HASH_ALG__MAX,               \
> > +                                   "Hash algorithm",                    \
> > +                                   errp)
> 
> That said, your macro wrappers tweak the error message, so that it is
> perhaps more legible than the standard failure mode of
> qapi_enum_parse().  So up to you if you want to keep the wrappers.

I'll change my XXX note to mention qapi_enum_parse() as a future
todo if we can improve its error reporting in some manner.


> > +static int
> > +qcrypto_block_luks_load_key(QCryptoBlock *block,
> > +                            QCryptoBlockLUKSKeySlot *slot,
> > +                            const char *password,
> > +                            QCryptoCipherAlgorithm cipheralg,
> > +                            QCryptoCipherMode ciphermode,
> > +                            QCryptoHashAlgorithm hash,
> > +                            QCryptoIVGenAlgorithm ivalg,
> > +                            QCryptoHashAlgorithm ivhash,
> > +                            uint8_t *masterkey,
> > +                            size_t masterkeylen,
> > +                            QCryptoBlockReadFunc readfunc,
> > +                            void *opaque,
> > +                            Error **errp)
> > +{
> > +    QCryptoBlockLUKS *luks = block->opaque;
> > +    uint8_t *splitkey;
> > +    size_t splitkeylen;
> > +    uint8_t *possiblekey;
> > +    int ret = -1;
> > +    ssize_t rv;
> > +    QCryptoCipher *cipher = NULL;
> > +    uint8_t keydigest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
> > +    QCryptoIVGen *ivgen = NULL;
> > +    size_t niv;
> > +
> > +    if (slot->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {
> > +        return 0;
> > +    }
> 
> Would be worth documenting the convention of '0' (no key found, but no
> errp set), '1' (successful key), and negative (errp set).

Yes, will document that.

> > +    splitkeylen = masterkeylen * slot->stripes;
> 
> Should we do any validation that masterkeylen was in bounds and this
> doesn't overflow?  I don't want a malicious byte stream masquerading as
> a LUKS header to cause us to misbehave.

masterkeylen comes from luks->header.key_bytes. We should put some
validation in for that earlier when we first get it off disk.

> For that matter, the spec states that luks->payload_offset is a
> write-once calculation at creation time, and that it could be recomputed
> from key layout.  Should we validate that the offset listed makes
> algorithmic sense with what we know about header and key data sizes for
> the parameters listed in the header?

While it says that the offsets could be recomputed, from a technical POV
there's no reason why all impls must use the same layout / alignment for
the key slots. So I feel it safer to just rely on the header values and
not try to enforce that they match a particular offset.

> Side note: I'm a bit surprised the upstream LUKS spec does not include
> any checksum over the header contents - it does a great job at ensuring
> the master key is only going to be found by someone knowing a correct
> password and that a single byte error in a key material section
> invalidates that entire slot's usefulness, but it is not as careful on
> what happens with certain single-byte errors in the header.
> 
> > +    splitkey = g_new0(uint8_t, splitkeylen);
> > +    possiblekey = g_new0(uint8_t, masterkeylen);
> > +
> > +    /*
> > +     * The user password is used to generate a (possible)
> > +     * decryption key. This may or may not successfully
> > +     * decrypt the master key - we just blindly assume
> > +     * the key is correct and validate the results of
> > +     * decryption later.
> > +     */
> > +    if (qcrypto_pbkdf2(hash,
> > +                       (const uint8_t *)password, strlen(password),
> > +                       slot->salt, QCRYPTO_BLOCK_LUKS_SALT_LEN,
> > +                       slot->iterations,
> > +                       possiblekey, masterkeylen,
> > +                       errp) < 0) {
> > +        goto cleanup;
> > +    }
> > +
> > +    /*
> > +     * We need to read the master key material from the
> > +     * LUKS key material header. What we're reading is
> > +     * not the raw master key, but rather the data after
> > +     * it has been passed through AFSplit and the result
> > +     * then encrypted.
> > +     */
> > +    rv = readfunc(block,
> > +                  slot->key_offset * 512ll,
> 
> I tend to write LL rather than ll, to make it obvious I didn't typo '1'.
>  Looks like we're hard-coding our sector size (not the first time); but
> maybe a constant instead of a magic number will make it easier for the
> poor guy down the road that tries to switch to 4k sectors.

I'll introduce a QCRYPTO_LUKS_SECTOR_SIZE constant throughout for
now.


> > +     * it back together to get the actual master key.
> > +     */
> > +    if (qcrypto_afsplit_decode(hash,
> 
> ...
> > +
> > +    if (memcmp(keydigest, luks->header.master_key_digest,
> > +               QCRYPTO_BLOCK_LUKS_DIGEST_LEN) == 0) {
> > +        /* Success, we got the right master key */
> > +        ret = 1;
> > +        goto cleanup;
> > +    }
> > +
> > +    /* Fail, user's password was not valid for this key slot,
> > +     * tell caller to try another slot */
> > +    ret = 0;
> > +
> 
> Matches the spec.  Yay!
> 
> Side observation: The LUKS spec doesn't say anything about parallelizing
> the lookup loop across all 8 key slots, nor any randomization on which
> slot should be attempted first.  The whole point of PBKDF2 iteration
> count is to burn CPU time so that an attacker can't brute force things
> easily.  That means it is VERY easy to tell by timing how many active
> slots were attempted before a key was found, if slots are tried serially
> and we immediately break the loop on the first successful decryption.
> Is there any information leak caused by the timing observations when
> serially searching among 8 active slots, when the master key is only
> found in slot 7 vs. slot 0?  But I don't see it as your problem to
> solve, so much as something to ask the upstream LUKS design team.

FWIW, cryptsetup will short circuit the search as soon as it finds
a valid slot, so we're matching their behaviour at least.

> > +static int
> > +qcrypto_block_luks_open(QCryptoBlock *block,
> > +                        QCryptoBlockOpenOptions *options,
> > +                        QCryptoBlockReadFunc readfunc,
> > +                        void *opaque,
> > +                        unsigned int flags,
> > +                        Error **errp)
> > +{
> 
> > +    /*
> > +     * The cipher_mode header contains a string that we have
> > +     * to further parse of the format
> 
> s/parse/parse,/
> 
> > +     *
> > +     *    <cipher-mode>-<iv-generator>[:<iv-hash>]
> > +     *
> > +     * eg  cbc-essiv:sha256, cbc-plain64
> > +     */
> > +    ivgen_name = strchr(luks->header.cipher_mode, '-');
> > +    if (!ivgen_name) {
> > +        ret = -EINVAL;
> > +        error_setg(errp, "Unexpected cipher mode string format %s",
> > +                   luks->header.cipher_mode);
> > +        goto fail;
> > +    }
> > +    *ivgen_name = '\0';
> > +    ivgen_name++;
> 
> We're modifying luks->header in place; we'd better not write it back out
> to disk in the modified form.  I guess you are okay for now - your code
> only reads existing LUKS disks, and can only create a single key in slot
> 0 on conversion (that is, I don't see code for key management of an
> existing image).  Probably things we should add later, though, at which
> point we'll need to be careful that we don't overwrite too much in the
> header.

Yeah, certainly something to be careful of later.

> > +    block->payload_offset = luks->header.payload_offset;
> 
> Earlier, I argued that block->payload_offset should be in bytes.  You
> are constrained that luks->header.payload_offset is in sectors, but we
> need not propagate that craziness any higher.

Yes, I've made that change.

> > +static int
> > +qcrypto_block_luks_create(QCryptoBlock *block,
> > +                          QCryptoBlockCreateOptions *options,
> > +                          QCryptoBlockInitFunc initfunc,
> > +                          QCryptoBlockWriteFunc writefunc,
> > +                          void *opaque,
> > +                          Error **errp)
> > +{
> 
> > +
> > +    memcpy(&luks_opts, options->u.luks, sizeof(luks_opts));
> > +    if (!luks_opts.has_cipher_alg) {
> > +        luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256;
> > +    }
> > +    if (!luks_opts.has_cipher_mode) {
> > +        /* XXX switch to XTS */
> > +        luks_opts.cipher_mode = QCRYPTO_CIPHER_MODE_CBC;
> > +    }
> 
> Yeah, my (quick) reading made it seem like XTS is a slightly more secure
> default than CBC.  But as you said, XTS implementation will come later.

Yes, I still hope to post an XTS impl before 2.6, so if this code
merges before 2.6, hopefully we'll stil have time to use XTS as
the default too, matching cryptsetup

> > +    luks = g_new0(QCryptoBlockLUKS, 1);
> > +    block->opaque = luks;
> > +
> > +    memcpy(luks->header.magic, qcrypto_block_luks_magic,
> > +           QCRYPTO_BLOCK_LUKS_MAGIC_LEN);
> > +
> > +    luks->header.version = QCRYPTO_BLOCK_LUKS_VERSION;
> 
> Maybe a comment here that endianness will be addressed later, as a lot
> of code appears between here and the actual write-to-disk.

Will do

> > +    uuid_generate(uuid);
> > +    uuid_unparse(uuid, (char *)luks->header.uuid);
> > +
> > +    switch (luks_opts.cipher_alg) {
> > +    case QCRYPTO_CIPHER_ALG_AES_128:
> > +    case QCRYPTO_CIPHER_ALG_AES_192:
> > +    case QCRYPTO_CIPHER_ALG_AES_256:
> > +        cipher_alg = "aes";
> > +        break;
> > +    default:
> > +        error_setg(errp, "Cipher algorithm is not supported");
> > +        goto error;
> 
> That is, we aren't supporting "twofish", "serpent", "cast5", or "cast6".
>  Fine for now.
> 
> > +    }
> > +
> > +    cipher_mode = QCryptoCipherMode_lookup[luks_opts.cipher_mode];
> > +    ivgen_alg = QCryptoIVGenAlgorithm_lookup[luks_opts.ivgen_alg];
> > +    if (luks_opts.has_ivgen_hash_alg) {
> > +        ivgen_hash_alg = QCryptoHashAlgorithm_lookup[luks_opts.ivgen_hash_alg];
> > +        cipher_mode_spec = g_strdup_printf("%s-%s:%s", cipher_mode, ivgen_alg,
> > +                                           ivgen_hash_alg);
> > +    } else {
> > +        cipher_mode_spec = g_strdup_printf("%s-%s", cipher_mode, ivgen_alg);
> > +    }
> 
> Should we validate that the mode spec is among the set documented by the
> LUKS spec ("ecb", "cbc-plain", "cbc-essiv:hash", "xts-plain64") (well,
> we don't support xts yet), and reject combinations that aren't supported
> ("cbc-plain64" comes to mind as something the LUKS spec doesn't allow,
> even though we have the pieces to logically make it happen)?

The spec appears to be non-exhaustive, in that cryptsetup / linux allow
for cbc-plain64 and other combinations that aren't listed. So QEMU should
follow that for interoperability.

> > +    hash_alg = QCryptoHashAlgorithm_lookup[luks_opts.hash_alg];
> 
> Likewise, should we validate that the hash is one of the specified names
> ("sha1", "sha256", "sha512", "ripemd160")?

The only one we're permitting that's not listed there is 'md5' and
that should fail anyway since it provides too small a digest size
but I see we're not validating the digest size.

> > +
> > +    memcpy(luks->header.cipher_name, cipher_alg,
> > +           strlen(cipher_alg) + 1);
> > +    memcpy(luks->header.cipher_mode, cipher_mode_spec,
> > +           strlen(cipher_mode_spec) + 1);
> > +    memcpy(luks->header.hash_spec, hash_alg,
> > +           strlen(hash_alg) + 1);
> 
> Couldn't these just be strcpy()?

Yes


> > +    /* Although LUKS has multiple key slots, we're just going
> > +     * to use the first key slot */
> > +
> > +    splitkeylen = luks->header.key_bytes * QCRYPTO_BLOCK_LUKS_STRIPES;
> 
> The LUKS spec says that it is okay to allow the user to specify stripes.
>  Should that be one of our options, with a sane default?  But it can be
> added later as a followup, this patch is already quite big and I'm fine
> with hardcoding stripes for now.

Cryptsetup doesn't allow the user to specify the stripes value, just
fixing it at 4k, so I figure we're best todo the same, otherwise we'll
create images that cant be read by linux.

> 
> > +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > +        luks->header.key_slots[i].active = i == 0 ?
> > +            QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED :
> > +            QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> > +        luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> > +        luks->header.key_slots[i].key_offset =
> > +            (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512) +
> > +            (ROUND_UP(((splitkeylen + 511) / 512),
> > +                      (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512)) * i);
> 
> Eww. The spec seems buggy, saying:
> 
> > // integer divisions, result rounded down:
> > baseOffset = (size of phdr)/SECTOR SIZE + 1
> > keymaterialSectors = (stripes*masterKeyLength)/SECTOR SIZE + 1
> 
> First, look at the calculation of baseOffset.  Since size of phdr is 592
> bytes, and that is NOT a SECTOR SIZE in any disk I know of, that makes
> sense that baseOffset will be at least 2 (512-byte) sectors into the
> disk, or, if SECTOR SIZE is 4, that will result in an offset of 1
> (4096-byte) sector.  However, you've defined
> QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET as 4096, which means for our 512-byte
> sector usage, you are setting the first key material at sector 4 no
> matter what.  I guess that matches what cryptsetup does?  Maybe worth a
> comment?
> 
> Now, look at the calculation of keymaterial Sectors.  The algorithm in
> the spec mishandles the case where stripes happens to be an exact
> multiple of sector size (that is, for a keysize of 20 bytes coupled with
> 512 stripes, it would reserve 21 sectors rather than the 20 actually
> required).  I think your use of ROUND_UP() makes more sense, but it
> would be nice to file a bug against the LUKS spec to have them fix their
> math, and/or document that it is okay to use values larger than the
> absolute minimum.

Yeah, what I am doing matches the cryptsetup code. I had originally
written it to be more or less what the spec suggests, and noticed
that my images were different from those created by cryptsetup. So
I switched to use the same algorithm. I figure it is probably an
optimization for on disk alignment to stop crossing sector boundaries.


> > +     * slot headers, rounded up to the nearest sector, combined with
> > +     * the size of each master key material region, also rounded up
> > +     * to the nearest sector */
> > +    luks->header.payload_offset =
> > +        (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512) +
> > +        (ROUND_UP(((splitkeylen + 511) / 512),
> > +                  (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512)) *
> > +         QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > +
> > +    block->payload_offset = luks->header.payload_offset;
> 
> Again, I argue that block->payload_offset would be saner in bytes.

Agreed

> > +    /* Reserve header space to match payload offset */
> > +    initfunc(block, block->payload_offset * 512, &local_err, opaque);
> 
> Especially since initfunc() takes bytes.
> 


> > +
> > +static int
> > +qcrypto_block_luks_decrypt(QCryptoBlock *block,
> > +                           uint64_t startsector,
> > +                           uint8_t *buf,
> > +                           size_t len,
> > +                           Error **errp)
> > +{
> > +    return qcrypto_block_decrypt_helper(block->cipher,
> > +                                        block->niv, block->ivgen,
> > +                                        startsector, buf, len, errp);
> > +}
> 
> What happens when we try to read a LUKS device with 4k sectors?  Worth
> worrying about, maybe just as a comment that we are hard-coded to 512
> bytes at the moment?

qcrypto_block_decrypt_helper() method processes 'buf' in units of 512
so even if the underling dev is 4k, we'll also do 512 byte I/O

> >  ##
> > +# QCryptoBlockOptionsLUKS:
> > +#
> > +# The options that apply to LUKS encryption format
> > +#
> > +# @key-secret: #optional the ID of a QCryptoSecret object providing the
> > +#              decryption key
> 
> Maybe add "Mandatory except when probing the image for metadata only"

Yes


> > +# Since: 2.6
> > +##
> > +{ 'struct': 'QCryptoBlockOptionsLUKS',
> > +  'data': { '*key-secret': 'str' }}
> > +
> > +
> > +##
> > +# QCryptoBlockCreateOptionsLUKS:
> > +#
> > +# The options that apply to LUKS encryption format initialization
> > +#
> > +# @cipher-alg: #optional the cipher algorithm for data encryption
> > +# @cipher-mode: #optional the cipher mode for data encryption
> > +# @ivgen-alg: #optional the initialization vector generator
> > +# @ivgen-hash-alg: #optional the initialization vector generator hash
> > +# @hash-alg: #optional the master key hash algorithm
> 
> Would be nice to mention the defaults, particularly since we may later
> change the default from cbc to xts.

Yes, will do


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|