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

[Max Reitz]

On 10.05.20 15:40, Maxim Levitsky wrote:
> Next few patches will expose that functionality to the user.
> 
> Signed-off-by: Maxim Levitsky <address@hidden>
> Reviewed-by: Daniel P. Berrangé <address@hidden>
> ---
>  crypto/block-luks.c | 395 +++++++++++++++++++++++++++++++++++++++++++-
>  qapi/crypto.json    |  61 ++++++-
>  2 files changed, 452 insertions(+), 4 deletions(-)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 4861db810c..967d382882 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c

[...]

> @@ -1069,6 +1076,119 @@ 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;

One space too many after the ==.

[...]

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

This accesses header.key_slots[slot_idx] before...

> +    size_t i;
> +    Error *local_err = NULL;
> +    int ret;
> +
> +    assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);

...we assert that slot_idx is in bounds.

I suppose that’s kind of fine, because assertions aren’t meant to fire
either, but this basically makes the assertion useless.

> +    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;
> +
> +    ret = qcrypto_block_luks_store_header(block,  writefunc,

Superfluous space after the comma.

> +                                          opaque, &local_err);
> +
> +    if (ret < 0) {
> +        error_propagate(errp, local_err);
> +    }

error_propagate() is a no-op with local_err == NULL, so you could do
without checking @ret (just calling error_propagate() unconditionally).

(But who cares, we need to set @ret anyway, so we might as well check it.)

[...]

> +static int
> +qcrypto_block_luks_amend_add_keyslot(QCryptoBlock *block,
> +                                     QCryptoBlockReadFunc readfunc,
> +                                     QCryptoBlockWriteFunc writefunc,
> +                                     void *opaque,
> +                                     QCryptoBlockAmendOptionsLUKS *opts_luks,
> +                                     bool force,
> +                                     Error **errp)
> +{
> +    QCryptoBlockLUKS *luks = block->opaque;
> +    uint64_t iter_time = opts_luks->has_iter_time ?
> +                         opts_luks->iter_time :
> +                         QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS;
> +    int keyslot;
> +    g_autofree char *old_password = NULL;
> +    g_autofree char *new_password = NULL;
> +    g_autofree uint8_t *master_key = NULL;

(I assume we don’t really care about purging secrets from memory after use)

[...]

> +static int
> +qcrypto_block_luks_amend_erase_keyslots(QCryptoBlock *block,
> +                                        QCryptoBlockReadFunc readfunc,
> +                                        QCryptoBlockWriteFunc writefunc,
> +                                        void *opaque,
> +                                        QCryptoBlockAmendOptionsLUKS 
> *opts_luks,
> +                                        bool force,
> +                                        Error **errp)
> +{

[...]

> +        for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> +            int rv = qcrypto_block_luks_load_key(block,
> +                                                 i,
> +                                                 old_password,
> +                                                 tmpkey,
> +                                                 readfunc,
> +                                                 opaque,
> +                                                 errp);
> +            if (rv == -1) {
> +                return -1;
> +            } else if (rv == 1) {
> +                bitmap_set(&slots_to_erase_bitmap, i, 1);

We should assert that QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS <=
sizeof(slots_to_erase_bitmap) * 8.  As it is, this looks like a possible
out-of-bounds access on first glance.

[...]

> +static int
> +qcrypto_block_luks_amend_options(QCryptoBlock *block,
> +                                 QCryptoBlockReadFunc readfunc,
> +                                 QCryptoBlockWriteFunc writefunc,
> +                                 void *opaque,
> +                                 QCryptoBlockAmendOptions *options,
> +                                 bool force,
> +                                 Error **errp)
> +{
> +    QCryptoBlockAmendOptionsLUKS *opts_luks = &options->u.luks;
> +
> +    if (opts_luks->state == Q_CRYPTO_BLOCKLUKS_KEYSLOT_STATE_ACTIVE) {

switch { case } might be nicer to catch unhandled cases.  Or else if +
else { g_assert_not_reached() }.

> +        return qcrypto_block_luks_amend_add_keyslot(block, readfunc,
> +                                                    writefunc, opaque,
> +                                                    opts_luks, force, errp);
> +    } else {
> +        return qcrypto_block_luks_amend_erase_keyslots(block, readfunc,
> +                                                       writefunc, opaque,
> +                                                       opts_luks, force, 
> errp);
> +    }
> +}

[...]

> diff --git a/qapi/crypto.json b/qapi/crypto.json
> index aeb6c7ef7b..29276e5e5e 100644
> --- a/qapi/crypto.json
> +++ b/qapi/crypto.json
> @@ -1,6 +1,8 @@
>  # -*- Mode: Python -*-
>  #
>  
> +{ 'include': 'common.json' }
> +

Why?  Seems to compile without it just fine.

Max

signature.asc
Description: OpenPGP digital signature