Re: [Qemu-devel] [PATCH 1/6] crypto: make PBKDF iterations configurable for LUKS format

[Eric Blake]

On 09/08/2016 11:27 AM, Daniel P. Berrange wrote:
> As protection against bruteforcing passphrases, the PBKDF
> algorithm is tuned by counting the number of iterations
> needed to produce 1 second of running time. If the machine
> that the image will be used on is much faster than the
> machine where the image is created, it can be desirable
> to raise the number of limits. This adds a new 'iter-time'

s/limits/iterations/ ?

> property that allows the user to choose the iteration
> wallclock time.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  block/crypto.c      |  6 ++++++
>  crypto/block-luks.c | 32 +++++++++++++++++++++++---------
>  qapi/crypto.json    |  6 +++++-
>  3 files changed, 34 insertions(+), 10 deletions(-)
> 

> +++ b/crypto/block-luks.c
> @@ -917,8 +917,12 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>      const char *hash_alg;
>      char *cipher_mode_spec = NULL;
>      QCryptoCipherAlgorithm ivcipheralg = 0;
> +    uint64_t iters;
>  
>      memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts));
> +    if (!luks_opts.has_iter_time) {
> +        luks_opts.iter_time = 1000;
> +    }
>      if (!luks_opts.has_cipher_alg) {
>          luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256;
>      }
> @@ -1064,7 +1068,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>      /* Determine how many iterations we need to hash the master
>       * key, in order to have 1 second of compute time used
>       */
> -    luks->header.master_key_iterations =
> +    iters = luks_opts.iter_time *
>          qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,

luks_opts.iter_time is a user-provided 64-bit value, so this
multiplication can overflow...

>                                     masterkey, luks->header.key_bytes,
>                                     luks->header.master_key_salt,
> @@ -1074,15 +1078,19 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>          error_propagate(errp, local_err);
>          goto error;
>      }
> -
> +    /* iter_time was in millis, but count_iters reported for secs */
> +    iters /= 1000;
>      /* Why /= 8 ?  That matches cryptsetup, but there's no
>       * explanation why they chose /= 8... Probably so that
>       * if all 8 keyslots are active we only spend 1 second
>       * in total time to check all keys */
> -    luks->header.master_key_iterations /= 8;
> +    iters /= 8;
> +    if (iters > UINT32_MAX) {
> +        error_setg(errp, "Too many PBKDF iterations for LUKS format");

...and your check here is too late to catch it.

> @@ -1131,7 +1139,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>      /* Again we determine how many iterations are required to
>       * hash the user password while consuming 1 second of compute
>       * time */
> -    luks->header.key_slots[0].iterations =
> +    iters = luks_opts.iter_time *
>          qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,

And again.

> +++ b/qapi/crypto.json
> @@ -185,6 +185,9 @@
>  #                  Currently defaults to 'sha256'
>  # @hash-alg: #optional the master key hash algorithm
>  #            Currently defaults to 'sha256'
> +# @iter-time: #optional number of milliseconds to spend in
> +#             PBKDF passphrase processing. Currently defaults
> +#             to 1000. Since 2.8

I think this is usually written '(since 2.8)' rather than 'Since 2.8'.
Marc-Andre may have more input on exactly what format he is expecting
when his patches enable doc generation from .json source.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature