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

[Maxim Levitsky]

On Thu, 2020-05-14 at 13:56 +0200, Max Reitz wrote:
> 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 ==.
Fixed, thanks!
> 
> [...]
> 
> > +/*
> > + * 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.
Fixed, thanks as well!
There are few more places where I didn't place this assert,
and I added it just in case.

> 
> 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.
True, fixed.

> 
> > +                                          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.)
Yep.
> 
> [...]
> 
> > +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)
Yep, I tried once fixing this but I decided to just leave it as is,
and later fix in independent patch series, which would need to touch
much that luks (we for example keep the qcrypto secrets in memory as well).


> 
> [...]
> 
> > +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.
OK, fixed.

> 
> [...]
> 
> > +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);
> > +    }
> > +}
This function used to contain the whole thing, and was converted to a wrapper 
at the
final minute, so yeh, I agree, a switch will be nice here.
Done.


> 
> [...]
> 
> > 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.
Lefover from the days of me using the StrOrNull
Fixed.

> 
> Max
> 

Thanks for the review!
Best regards,
        Maxim Levitsky