[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 02/14] qcrypto/luks: implement encryption key management
|
From: |
Maxim Levitsky |
|
Subject: |
Re: [PATCH v2 02/14] qcrypto/luks: implement encryption key management |
|
Date: |
Sun, 03 May 2020 11:55:35 +0300 |
On Tue, 2020-04-28 at 14:16 +0100, Daniel P. Berrangé wrote:
> 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 <address@hidden>
> > ---
> > 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.
Fixed! Thanks for pointing this out!
>
> > + /*
> > + * 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.
Fixed too of course
>
> > + 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.
It used to be like that in former versions that I did send, I added the concept
of the bitmap very recently to reflect the way we defined this in the spec.
I don't mind that much coming back to older version of doing this,
but beware that it won't be that clear either.
>
> > +
> > +/*
> > + * 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.
Mostly for consistency. There is a very corner case of inline replacing
all keys that match one password with another.
If possible I would like to keep it this way though.
>
> > +{
> > + 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.
Yep, fixed.
>
> > + }
> > + 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
Roger that!
>
> > +##
> > +# @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.
Let it be 'secret' I don't mind at all.
>
> > +{ '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
Best regards and thanks for the review,
Maxim Levitsky
- Re: [PATCH v2 02/14] qcrypto/luks: implement encryption key management,
Maxim Levitsky <=