Re: [PATCH 02/13] qcrypto-luks: implement encryption key management

[Maxim Levitsky]

On Tue, 2020-01-28 at 17:21 +0000, Daniel P. Berrangé wrote:
> On Tue, Jan 14, 2020 at 09:33:39PM +0200, Maxim Levitsky wrote:
> > Next few patches will expose that functionality
> > to the user.
> > 
> > Signed-off-by: Maxim Levitsky <address@hidden>
> > ---
> >  crypto/block-luks.c | 374 +++++++++++++++++++++++++++++++++++++++++++-
> >  qapi/crypto.json    |  50 +++++-
> >  2 files changed, 421 insertions(+), 3 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index 4861db810c..349e95fed3 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -32,6 +32,7 @@
> >  #include "qemu/uuid.h"
> >  
> >  #include "qemu/coroutine.h"
> > +#include "qemu/bitmap.h"
> >  
> >  /*
> >   * Reference for the LUKS format implemented here is
> > @@ -70,6 +71,9 @@ typedef struct QCryptoBlockLUKSKeySlot 
> > QCryptoBlockLUKSKeySlot;
> >  
> >  #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
> >  
> > +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000
> > +#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40
> > +
> >  static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = 
> > {
> >      'L', 'U', 'K', 'S', 0xBA, 0xBE
> >  };
> > @@ -219,6 +223,9 @@ struct QCryptoBlockLUKS {
> >  
> >      /* Hash algorithm used in pbkdf2 function */
> >      QCryptoHashAlgorithm hash_alg;
> > +
> > +    /* Name of the secret that was used to open the image */
> > +    char *secret;
> >  };
> >  
> >  
> > @@ -1069,6 +1076,112 @@ qcrypto_block_luks_find_key(QCryptoBlock *block,
> >      return -1;
> >  }
> >  
> > +/*
> > + * Returns true if a slot i is marked as active
> > + * (contains encrypted copy of the master key)
> > + */
> > +static bool
> > +qcrypto_block_luks_slot_active(const QCryptoBlockLUKS *luks,
> > +                               unsigned int slot_idx)
> > +{
> > +    uint32_t val = luks->header.key_slots[slot_idx].active;
> > +    return val ==  QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
> > +}
> > +
> > +/*
> > + * Returns the number of slots that are marked as active
> > + * (slots that contain encrypted copy of the master key)
> > + */
> > +static unsigned int
> > +qcrypto_block_luks_count_active_slots(const QCryptoBlockLUKS *luks)
> > +{
> > +    size_t i = 0;
> > +    unsigned int ret = 0;
> > +
> > +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > +        if (qcrypto_block_luks_slot_active(luks, i)) {
> > +            ret++;
> > +        }
> > +    }
> > +    return ret;
> > +}
> > +
> > +/*
> > + * Finds first key slot which is not active
> > + * Returns the key slot index, or -1 if it doesn't exist
> > + */
> > +static int
> > +qcrypto_block_luks_find_free_keyslot(const QCryptoBlockLUKS *luks)
> > +{
> > +    size_t i;
> > +
> > +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > +        if (!qcrypto_block_luks_slot_active(luks, i)) {
> > +            return i;
> > +        }
> > +    }
> > +    return -1;
> > +
> > +}
> > +
> > +/*
> > + * Erases an keyslot given its index
> > + * Returns:
> > + *    0 if the keyslot was erased successfully
> > + *   -1 if a error occurred while erasing the keyslot
> > + *
> > + */
> > +static int
> > +qcrypto_block_luks_erase_key(QCryptoBlock *block,
> > +                             unsigned int slot_idx,
> > +                             QCryptoBlockWriteFunc writefunc,
> > +                             void *opaque,
> > +                             Error **errp)
> > +{
> > +    QCryptoBlockLUKS *luks = block->opaque;
> > +    QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
> > +    g_autofree uint8_t *garbagesplitkey = NULL;
> > +    size_t splitkeylen = luks->header.master_key_len * slot->stripes;
> > +    size_t i;
> > +
> > +    assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > +    assert(splitkeylen > 0);
> > +
> > +    garbagesplitkey = g_new0(uint8_t, splitkeylen);
> > +
> > +    /* Reset the key slot header */
> > +    memset(slot->salt, 0, QCRYPTO_BLOCK_LUKS_SALT_LEN);
> > +    slot->iterations = 0;
> > +    slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> > +
> > +    qcrypto_block_luks_store_header(block,  writefunc, opaque, errp);
> > +
> > +    /*
> > +     * Now try to erase the key material, even if the header
> > +     * update failed
> > +     */
> > +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS; i++) {
> > +        if (qcrypto_random_bytes(garbagesplitkey, splitkeylen, errp) < 0) {
> > +            /*
> > +             * If we failed to get the random data, still write
> > +             * at least zeros to the key slot at least once
> > +             */
> > +            if (i > 0) {
> > +                return -1;
> > +            }
> > +        }
> > +
> > +        if (writefunc(block,
> > +                      slot->key_offset_sector * 
> > QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
> > +                      garbagesplitkey,
> > +                      splitkeylen,
> > +                      opaque,
> > +                      errp) != splitkeylen) {
> > +            return -1;
> > +        }
> > +    }
> > +    return 0;
> > +}
> >  
> >  static int
> >  qcrypto_block_luks_open(QCryptoBlock *block,
> > @@ -1099,6 +1212,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> >  
> >      luks = g_new0(QCryptoBlockLUKS, 1);
> >      block->opaque = luks;
> > +    luks->secret = g_strdup(options->u.luks.key_secret);
> >  
> >      if (qcrypto_block_luks_load_header(block, readfunc, opaque, errp) < 0) 
> > {
> >          goto fail;
> > @@ -1164,6 +1278,7 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> >   fail:
> >      qcrypto_block_free_cipher(block);
> >      qcrypto_ivgen_free(block->ivgen);
> > +    g_free(luks->secret);
> >      g_free(luks);
> >      return -1;
> >  }
> > @@ -1204,7 +1319,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >  
> >      memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts));
> >      if (!luks_opts.has_iter_time) {
> > -        luks_opts.iter_time = 2000;
> > +        luks_opts.iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS;
> >      }
> >      if (!luks_opts.has_cipher_alg) {
> >          luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256;
> > @@ -1244,6 +1359,8 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >                     optprefix ? optprefix : "");
> >          goto error;
> >      }
> > +    luks->secret = g_strdup(options->u.luks.key_secret);
> > +
> >      password = qcrypto_secret_lookup_as_utf8(luks_opts.key_secret, errp);
> >      if (!password) {
> >          goto error;
> > @@ -1471,10 +1588,260 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >      qcrypto_block_free_cipher(block);
> >      qcrypto_ivgen_free(block->ivgen);
> >  
> > +    g_free(luks->secret);
> >      g_free(luks);
> >      return -1;
> >  }
> >  
> > +/*
> > + * Given LUKSKeyslotUpdate command, return @slots_bitmap with all slots
> > + * that will be updated with new password (or erased)
> > + * returns number of affected slots
> > + */
> > +static int qcrypto_block_luks_get_slots_bitmap(QCryptoBlock *block,
> > +                                               QCryptoBlockReadFunc 
> > readfunc,
> > +                                               void *opaque,
> > +                                               const LUKSKeyslotUpdate 
> > *command,
> > +                                               unsigned long *slots_bitmap,
> > +                                               Error **errp)
> > +{
> > +    const QCryptoBlockLUKS *luks = block->opaque;
> > +    size_t i;
> > +    int ret = 0;
> > +
> > +    if (command->has_keyslot) {
> > +        /* keyslot set, select only this keyslot */
> > +        int keyslot = command->keyslot;
> > +
> > +        if (keyslot < 0 || keyslot >= QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS) {
> > +            error_setg(errp,
> > +                       "Invalid slot %u specified, must be between 0 and 
> > %u",
> > +                       keyslot, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS - 1);
> > +            goto error;
> > +        }
> > +        bitmap_set(slots_bitmap, keyslot, 1);
> > +        ret++;
> > +
> > +    } else if (command->has_old_secret) {
> > +        /* initially select all active keyslots */
> > +        for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > +            if (qcrypto_block_luks_slot_active(luks, i)) {
> > +                bitmap_set(slots_bitmap, i, 1);
> > +                ret++;
> > +            }
> > +        }
> > +    } else {
> > +        /* find a free keyslot */
> > +        int slot = qcrypto_block_luks_find_free_keyslot(luks);
> > +
> > +        if (slot == -1) {
> > +            error_setg(errp,
> > +                       "Can't add a keyslot - all key slots are in use");
> > +            goto error;
> > +        }
> > +        bitmap_set(slots_bitmap, slot, 1);
> > +        ret++;
> > +    }
> > +
> > +    if (command->has_old_secret) {
> > +        /* now deselect all keyslots that don't contain the password */
> > +        g_autofree uint8_t *tmpkey = g_new0(uint8_t,
> > +                                            luks->header.master_key_len);
> > +
> > +        for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > +            g_autofree char *old_password = NULL;
> > +            int rv;
> > +
> > +            if (!test_bit(i, slots_bitmap)) {
> > +                continue;
> > +            }
> > +
> > +            old_password = 
> > qcrypto_secret_lookup_as_utf8(command->old_secret,
> > +                                                         errp);
> > +            if (!old_password) {
> > +                goto error;
> > +            }
> > +
> > +            rv = qcrypto_block_luks_load_key(block,
> > +                                             i,
> > +                                             old_password,
> > +                                             tmpkey,
> > +                                             readfunc,
> > +                                             opaque,
> > +                                             errp);
> > +            if (rv == -1)
> > +                goto error;
> > +            else if (rv == 0) {
> > +                bitmap_clear(slots_bitmap, i, 1);
> > +                ret--;
> > +            }
> > +        }
> > +    }
> > +    return ret;
> > +error:
> > +    return -1;
> > +}
> > +
> > +/*
> > + * Apply a single keyslot update command as described in @command
> > + * Optionally use @unlock_secret to retrieve the master key
> > + */
> > +static int
> > +qcrypto_block_luks_apply_keyslot_update(QCryptoBlock *block,
> > +                                        QCryptoBlockReadFunc readfunc,
> > +                                        QCryptoBlockWriteFunc writefunc,
> > +                                        void *opaque,
> > +                                        LUKSKeyslotUpdate *command,
> > +                                        const char *unlock_secret,
> > +                                        uint8_t **master_key,
> > +                                        bool force,
> > +                                        Error **errp)
> > +{
> > +    QCryptoBlockLUKS *luks = block->opaque;
> > +    g_autofree unsigned long *slots_bitmap = NULL;
> > +    int64_t iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS;
> > +    int slot_count;
> > +    size_t i;
> > +    char *new_password;
> > +    bool erasing;
> > +
> > +    slots_bitmap = bitmap_new(QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > +    slot_count = qcrypto_block_luks_get_slots_bitmap(block, readfunc, 
> > opaque,
> > +                                                     command, slots_bitmap,
> > +                                                     errp);
> > +    if (slot_count == -1) {
> > +        goto error;
> > +    }
> > +    /* no matching slots, so nothing to do */
> > +    if (slot_count == 0) {
> > +        error_setg(errp, "Requested operation didn't match any slots");
> > +        goto error;
> > +    }
> > +    /*
> > +     * slot is erased when the password is set to null, or empty string
> > +     * (for compatibility with command line)
> > +     */
> > +    erasing = command->new_secret->type == QTYPE_QNULL ||
> > +              strlen(command->new_secret->u.s) == 0;
> > +
> > +    /* safety checks */
> > +    if (!force) {
> > +        if (erasing) {
> > +            if (slot_count == qcrypto_block_luks_count_active_slots(luks)) 
> > {
> > +                error_setg(errp,
> > +                           "Requested operation will erase all active 
> > keyslots"
> > +                           " which will erase all the data in the image"
> > +                           " irreversibly - refusing operation");
> > +                goto error;
> > +            }
> > +        } else {
> > +            for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > +                if (!test_bit(i, slots_bitmap)) {
> > +                    continue;
> > +                }
> > +                if (qcrypto_block_luks_slot_active(luks, i)) {
> > +                    error_setg(errp,
> > +                               "Refusing to overwrite active slot %zu - "
> > +                               "please erase it first", i);
> > +                    goto error;
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    /* setup the data needed for storing the new keyslot */
> > +    if (!erasing) {
> > +        /* Load the master key if it wasn't already loaded */
> > +        if (!*master_key) {
> > +            g_autofree char *old_password;
> > +            old_password = qcrypto_secret_lookup_as_utf8(unlock_secret,  
> > errp);
> > +            if (!old_password) {
> > +                goto error;
> > +            }
> > +            *master_key = g_new0(uint8_t, luks->header.master_key_len);
> > +
> > +            if (qcrypto_block_luks_find_key(block, old_password, 
> > *master_key,
> > +                                            readfunc, opaque, errp) < 0) {
> > +                error_append_hint(errp, "Failed to retrieve the master 
> > key");
> > +                goto error;
> > +            }
> > +        }
> > +        new_password = 
> > qcrypto_secret_lookup_as_utf8(command->new_secret->u.s,
> > +                                                     errp);
> > +        if (!new_password) {
> > +            goto error;
> > +        }
> > +        if (command->has_iter_time) {
> > +            iter_time = command->iter_time;
> > +        }
> > +    }
> > +
> > +    /* new apply the update */
> > +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > +        if (!test_bit(i, slots_bitmap)) {
> > +            continue;
> > +        }
> > +        if (erasing) {
> > +            if (qcrypto_block_luks_erase_key(block, i,
> > +                                             writefunc,
> > +                                             opaque,
> > +                                             errp)) {
> > +                error_append_hint(errp, "Failed to erase keyslot %zu", i);
> > +                goto error;
> > +            }
> > +        } else {
> > +            if (qcrypto_block_luks_store_key(block, i,
> > +                                             new_password,
> > +                                             *master_key,
> > +                                             iter_time,
> > +                                             writefunc,
> > +                                             opaque,
> > +                                             errp)) {
> > +                error_append_hint(errp, "Failed to write to keyslot %zu", 
> > i);
> > +                goto error;
> > +            }
> > +        }
> > +    }
> > +    return 0;
> > +error:
> > +    return -EINVAL;
> > +}
> 
> I feel the this method is confusing from trying to handle both
> adding and erasing keyslots....
> 
> 
> > +
> > +static int
> > +qcrypto_block_luks_amend_options(QCryptoBlock *block,
> > +                                 QCryptoBlockReadFunc readfunc,
> > +                                 QCryptoBlockWriteFunc writefunc,
> > +                                 void *opaque,
> > +                                 QCryptoBlockAmendOptions *options,
> > +                                 bool force,
> > +                                 Error **errp)
> > +{
> > +    QCryptoBlockLUKS *luks = block->opaque;
> > +    QCryptoBlockAmendOptionsLUKS *options_luks = &options->u.luks;
> > +    LUKSKeyslotUpdateList *ptr;
> > +    g_autofree uint8_t *master_key = NULL;
> > +    int ret;
> > +
> > +    char *unlock_secret = options_luks->has_unlock_secret ?
> > +                          options_luks->unlock_secret :
> > +                          luks->secret;
> > +
> > +    for (ptr = options_luks->keys; ptr; ptr = ptr->next) {
> > +        ret = qcrypto_block_luks_apply_keyslot_update(block, readfunc,
> > +                                                      writefunc, opaque,
> > +                                                      ptr->value,
> > +                                                      unlock_secret,
> > +                                                      &master_key,
> > +                                                      force, errp);
> 
> .... imho we sould do
> 
>     bool  erasing = command->new_secret->type == QTYPE_QNULL;
> 
>     if (erasing) {
>          ret = qcrypto_block_luks_disable_keyslot(block, readfunc,
>                                                        writefunc, opaque,
>                                                        ptr->value,
>                                                        unlock_secret,
>                                                        &master_key,
>                                                        force, errp);
>     } else {
>         ret = qcrypto_block_luks_enable_keyslot(block, readfunc,
>                                                        writefunc, opaque,
>                                                        ptr->value,
>                                                        unlock_secret,
>                                                        &master_key,
>                                                        force, errp);
>     }

I implemented something like that now, I'll send this in next version of the 
patches.

> 
> > +
> > +        if (ret != 0) {
> > +            goto error;
> > +        }
> > +    }
> > +    return 0;
> > +error:
> > +    return -1;
> > +}
> 
> If there's no code to run in the 'error' label, then we should just
> get rid of it and 'return -1' instead of "goto error"
Old habit. Fixed now.

> 
> >  
> >  static int qcrypto_block_luks_get_info(QCryptoBlock *block,
> >                                         QCryptoBlockInfo *info,
> > @@ -1523,7 +1890,9 @@ static int qcrypto_block_luks_get_info(QCryptoBlock 
> > *block,
> >  
> >  static void qcrypto_block_luks_cleanup(QCryptoBlock *block)
> >  {
> > -    g_free(block->opaque);
> > +    QCryptoBlockLUKS *luks = block->opaque;
> > +    g_free(luks->secret);
> 
> Check if "luks" is non-NULL for robustness in early failure scenarios
100% agree. Fixed.

> 
> > +    g_free(luks);
> >  }
> >  
> 
> Regards,
> Daniel

Thanks for the review,
        Best regards,
                Maxim Levitsky