Re: [PATCH v2 02/14] qcrypto/luks: implement encryption key management

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

On Sun, Mar 08, 2020 at 05:18:51PM +0200, Maxim Levitsky wrote:
> Next few patches will expose that functionality
> to the user.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  crypto/block-luks.c | 398 +++++++++++++++++++++++++++++++++++++++++++-
>  qapi/crypto.json    |  61 ++++++-
>  2 files changed, 455 insertions(+), 4 deletions(-)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 4861db810c..b11ee08c6d 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c

> +/*
> + * 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);

This may set  errp and we don't return immediately, so....

> +    /*
> +     * 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) {

...this may then set errp a second time, which is not permitted.

This call needs to use a "local_err", and error_propagate(errp, local_err).
The latter is a no-op if errp is already set.

> +            /*
> +             * 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) {

same issue with errp here too.

> +            return -1;
> +        }
> +    }
> +    return 0;
> +}


> +/*
> + * Given LUKSKeyslotUpdate command, set @slots_bitmap with all slots
> + * that will be updated with new password (or erased)
> + * returns 0 on success, and -1 on failure
> + */
> +static int
> +qcrypto_block_luks_get_update_bitmap(QCryptoBlock *block,
> +                                     QCryptoBlockReadFunc readfunc,
> +                                     void *opaque,
> +                                     const QCryptoBlockAmendOptionsLUKS *opts,
> +                                     unsigned long *slots_bitmap,
> +                                     Error **errp)
> +{
> +    const QCryptoBlockLUKS *luks = block->opaque;
> +    size_t i;
> +
> +    bitmap_zero(slots_bitmap, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> +
> +    if (opts->has_keyslot) {
> +        /* keyslot set, select only this keyslot */
> +        int keyslot = opts->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);
> +            return -1;
> +        }
> +        bitmap_set(slots_bitmap, keyslot, 1);
> +
> +    } else if (opts->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);
> +            }
> +        }
> +    } 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");
> +            return -1;
> +        }
> +        bitmap_set(slots_bitmap, slot, 1);
> +    }
> +
> +    if (opts->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(opts->old_secret,
> +                                                         errp);
> +            if (!old_password) {
> +                return -1;
> +            }
> +
> +            rv = qcrypto_block_luks_load_key(block,
> +                                             i,
> +                                             old_password,
> +                                             tmpkey,
> +                                             readfunc,
> +                                             opaque,
> +                                             errp);
> +            if (rv == -1) {
> +                return -1;
> +            } else if (rv == 0) {
> +                bitmap_clear(slots_bitmap, i, 1);
> +            }
> +        }
> +    }
> +    return 0;
> +}

I'm not really liking this function as a concept. Some of the code
only applies to the "add key" code path, while some of it only
applies to the "erase key" code path.

I'd prefer it if qcrypto_block_luks_erase_keys directly had the
required logic, likewise qcrypto_block_luks_set_keys, and thus
get rid of the bitmap concept entirely. I thin kit'd make the
logic easier to understand.

> +
> +/*
> + * Erase a set of keyslots given in @slots_bitmap
> + */
> +static int qcrypto_block_luks_erase_keys(QCryptoBlock *block,
> +                                         QCryptoBlockReadFunc readfunc,
> +                                         QCryptoBlockWriteFunc writefunc,
> +                                         void *opaque,
> +                                         unsigned long *slots_bitmap,
> +                                         bool force,
> +                                         Error **errp)
> +{
> +    QCryptoBlockLUKS *luks = block->opaque;
> +    long slot_count = bitmap_count_one(slots_bitmap,
> +                                       QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> +    size_t i;
> +
> +    /* safety checks */
> +    if (!force && 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");
> +        return -EINVAL;
> +    }
> +
> +    /* new apply the update */
> +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> +        if (!test_bit(i, slots_bitmap)) {
> +            continue;
> +        }
> +        if (qcrypto_block_luks_erase_key(block, i, writefunc, opaque, errp)) {
> +            error_append_hint(errp, "Failed to erase keyslot %zu", i);
> +            return -EINVAL;
> +        }
> +    }
> +    return 0;
> +}
> +
> +/*
> + * Set a set of keyslots to @master_key encrypted by @new_secret
> + */
> +static int qcrypto_block_luks_set_keys(QCryptoBlock *block,
> +                                       QCryptoBlockReadFunc readfunc,
> +                                       QCryptoBlockWriteFunc writefunc,
> +                                       void *opaque,
> +                                       unsigned long *slots_bitmap,
> +                                       uint8_t *master_key,
> +                                       uint64_t iter_time,
> +                                       char *new_secret,
> +                                       bool force,
> +                                       Error **errp)

I'd call this  "add_key" instead of "set_keys".  I'm also unclear why
we need to support setting a range of keyslots. AFAIK, adding a key
should only ever affect a single keyslot.

> +{
> +    QCryptoBlockLUKS *luks = block->opaque;
> +    g_autofree char *new_password = NULL;
> +    size_t i;
> +
> +    /* safety checks */
> +    if (!force) {
> +        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);
> +                return -EINVAL;
> +            }
> +        }
> +    }
> +
> +    /* Load the new password */
> +    new_password = qcrypto_secret_lookup_as_utf8(new_secret, errp);
> +    if (!new_password) {
> +        return -EINVAL;
> +    }
> +
> +    /* Apply the update */
> +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> +        if (!test_bit(i, slots_bitmap)) {
> +            continue;
> +        }
> +        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);
> +            return -EINVAL;
> +        }
> +    }
> +    return 0;
> +}
> +
> +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 *opts_luks = &options->u.luks;
> +    g_autofree uint8_t *master_key = NULL;
> +    g_autofree unsigned long *update_bitmap = NULL;
> +    char *unlock_secret = NULL;
> +    long slot_count;
> +
> +    unlock_secret = opts_luks->has_unlock_secret ? opts_luks->unlock_secret :
> +                                                   luks->secret;
> +
> +    /* Retrieve set of slots that we need to update */
> +    update_bitmap = bitmap_new(QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> +    if (qcrypto_block_luks_get_update_bitmap(block, readfunc, opaque, opts_luks,
> +                                             update_bitmap, errp) != 0) {
> +        return -1;
> +    }
> +
> +    slot_count = bitmap_count_one(update_bitmap,
> +                                  QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> +
> +    /* no matching slots, so nothing to do */
> +    if (slot_count == 0) {
> +        error_setg(errp, "Requested operation didn't match any slots");
> +        return -1;
> +    }
> +
> +    if (opts_luks->state == LUKS_KEYSLOT_STATE_ACTIVE) {
> +
> +        uint64_t iter_time = opts_luks->has_iter_time ?
> +                             opts_luks->iter_time :
> +                             QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS;
> +
> +        if (!opts_luks->has_new_secret) {
> +            error_setg(errp, "'new-secret' is required to activate a keyslot");
> +            return -EINVAL;

return -1,   we shouldn't return errno values in luks code in general
as we use  Error **errp.

> +        }
> +        if (opts_luks->has_old_secret) {
> +            error_setg(errp,
> +                       "'old-secret' must not be given when activating keyslots");
> +            return -EINVAL;
> +        }
> +
> +        /* Locate the password that will be used to retrieve the master key */
> +        g_autofree char *old_password;
> +        old_password = qcrypto_secret_lookup_as_utf8(unlock_secret,  errp);
> +        if (!old_password) {
> +            return -EINVAL;
> +        }
> +
> +        /* Try to retrieve the master key */
> +        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");
> +            return -EINVAL;
> +        }
> +
> +        /* Now set the new keyslots */
> +        if (qcrypto_block_luks_set_keys(block, readfunc, writefunc,
> +                                        opaque, update_bitmap, master_key,
> +                                        iter_time,
> +                                        opts_luks->new_secret,
> +                                        force, errp) != 0) {
> +            return -1;
> +        }
> +    } else {
> +        if (opts_luks->has_new_secret) {
> +            error_setg(errp,
> +                       "'new-secret' must not be given when erasing keyslots");
> +            return -EINVAL;
> +        }
> +        if (opts_luks->has_iter_time) {
> +            error_setg(errp,
> +                       "'iter-time' must not be given when erasing keyslots");
> +            return -EINVAL;
> +        }
> +        if (opts_luks->has_unlock_secret) {
> +            error_setg(errp,
> +                       "'unlock_secret' must not be given when erasing keyslots");
> +            return -EINVAL;
> +        }
> +
> +        if (qcrypto_block_luks_erase_keys(block, readfunc, writefunc,
> +                                          opaque, update_bitmap, force,
> +                                          errp) != 0) {
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
>  
>  static int qcrypto_block_luks_get_info(QCryptoBlock *block,
>                                         QCryptoBlockInfo *info,
> @@ -1523,7 +1912,11 @@ 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;
> +    if (luks) {
> +        g_free(luks->secret);
> +        g_free(luks);
> +    }
>  }
>  
>  
> @@ -1560,6 +1953,7 @@ qcrypto_block_luks_encrypt(QCryptoBlock *block,
>  const QCryptoBlockDriver qcrypto_block_driver_luks = {
>      .open = qcrypto_block_luks_open,
>      .create = qcrypto_block_luks_create,
> +    .amend = qcrypto_block_luks_amend_options,
>      .get_info = qcrypto_block_luks_get_info,
>      .cleanup = qcrypto_block_luks_cleanup,
>      .decrypt = qcrypto_block_luks_decrypt,
> diff --git a/qapi/crypto.json b/qapi/crypto.json
> index 3fd0ce177e..fe600fc608 100644
> --- a/qapi/crypto.json
> +++ b/qapi/crypto.json
> @@ -1,6 +1,8 @@
>  # -*- Mode: Python -*-
>  #
>  
> +{ 'include': 'common.json' }
> +
>  ##
>  # = Cryptography
>  ##
> @@ -297,7 +299,6 @@
>             'uuid': 'str',
>             'slots': [ 'QCryptoBlockInfoLUKSSlot' ] }}
>  
> -
>  ##
>  # @QCryptoBlockInfo:
>  #
> @@ -310,7 +311,63 @@
>    'discriminator': 'format',
>    'data': { 'luks': 'QCryptoBlockInfoLUKS' } }
>  
> +##
> +# @LUKSKeyslotState:
> +#
> +# Defines state of keyslots that are affected by the update
> +#
> +# @active:    The slots contain the given password and marked as active
> +# @inactive:  The slots are erased (contain garbage) and marked as inactive
> +#
> +# Since: 5.0
> +##
> +{ 'enum': 'LUKSKeyslotState',
> +  'data': [ 'active', 'inactive' ] }

This should be called  QCryptoBLockLUKSKeyslotState

> +##
> +# @QCryptoBlockAmendOptionsLUKS:
> +#
> +# This struct defines the update parameters that activate/de-activate set
> +# of keyslots
> +#
> +# @state: the desired state of the keyslots
> +#
> +# @new-secret:    The ID of a QCryptoSecret object providing the password to be
> +#                 written into added active keyslots
> +#
> +# @old-secret:    Optional (for deactivation only)
> +#                 If given will deactive all keyslots that
> +#                 match password located in QCryptoSecret with this ID
> +#
> +# @iter-time:     Optional (for activation only)
> +#                 Number of milliseconds to spend in
> +#                 PBKDF passphrase processing for the newly activated keyslot.
> +#                 Currently defaults to 2000.
> +#
> +# @keyslot:       Optional. ID of the keyslot to activate/deactivate.
> +#                 For keyslot activation, keyslot should not be active already
> +#                 (this is unsafe to update an active keyslot),
> +#                 but possible if 'force' parameter is given.
> +#                 If keyslot is not given, first free keyslot will be written.
> +#
> +#                 For keyslot deactivation, this parameter specifies the exact
> +#                 keyslot to deactivate
> +#
> +# @unlock-secret: Optional. The ID of a QCryptoSecret object providing the
> +#                 password to use to retrive current master key.
> +#                 Defaults to the same secret that was used to open the image

My inclination would be to just call this  "@secret", as it serves the
same purpose as the "@secret" parameter used when opening the image.

> +{ 'struct': 'QCryptoBlockAmendOptionsLUKS',
> +  'data': { 'state': 'LUKSKeyslotState',
> +            '*new-secret': 'str',
> +            '*old-secret': 'str',
> +            '*keyslot': 'int',
> +            '*iter-time': 'int',
> +            '*unlock-secret': 'str' } }
>  
>  ##
>  # @QCryptoBlockAmendOptions:
> @@ -324,4 +381,4 @@
>    'base': 'QCryptoBlockOptionsBase',
>    'discriminator': 'format',
>    'data': {
> -            } }
> +          'luks': 'QCryptoBlockAmendOptionsLUKS' } }

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