Hi Daniel,
On 09/07/2016 05:09 PM, Daniel P. Berrange wrote:
> A previous commit attempted to fix the pbkdf iteration
> estimation:
>
> commit 4609fd87d7f3cbccf1ed6db257f01b18a1edcb55
> Author: Milan Broz <gmazyland@xxxxxxxxx>
> Date: Thu Oct 29 11:52:18 2015 +0100
>
> Fix PBKDF2 iteration benchmark for longer key sizes.
>
> Before this commit, it would estimate using a derived
> key size of 1. After this commit, it will estimate using
> a derived key size matching the key bytes recorded in
> the LUKS header.
>
> Unfortuntely this is still incorrect as there are two
> different places where luks estimates pbkdf2 iterations.
> For the key slots, the derived key size needs to match
> master key bytes, but for the master key digest, it must
> match LUKS_DIGESTSIZE.
For the keyslot it already matches key size, for the volume key
fingerprint (see note below).
> The relationship between the derived key size and the
> pbkdf2 hash size is directly related to the time spent
> in computation. For example, with aes, key size 256 and
> sha1, we have
.... yes, that what the original patch is about.
> A second, minor mistake is made when scaling the value
> of iterations per second, to match the requested iteration
> time. There are 1000 milliseconds in 1 second, but the
> code is scaling by 1024, making the number of iterations
> ever so slightly smaller than it should be. This isn't
> a significant issue, but for clarity it is good to use
> the correct scaling factor for ms -> secs.
Not a mistake, but just cosmetic, see below :)
> diff --git a/lib/luks1/keymanage.c b/lib/luks1/keymanage.c
> index 4a9cbd6..f1682c1 100644
> --- a/lib/luks1/keymanage.c
> +++ b/lib/luks1/keymanage.c
> @@ -707,8 +707,11 @@ int LUKS_generate_phdr(struct luks_phdr *header,
> return r;
> }
>
> - r = crypt_benchmark_kdf(ctx, "pbkdf2", header->hashSpec,
> - "foo", 3, "bar", 3, PBKDF2_per_sec);
> + r = crypt_pbkdf_check("pbkdf2", header->hashSpec,
> + vk->key, vk->keylength,
> + header->mkDigestSalt, LUKS_SALTSIZE,
> + LUKS_DIGESTSIZE,
> + PBKDF2_per_sec);
if you check crypt_benchmark_kdf() code, it contains this
// FIXME: this should be in KDF check API parameters later
if (cd)
key_length = crypt_get_volume_key_size(cd);
so the key length *is* taken into account. I intentionally want to use
libcryptsetup call here and not crypt_pbkdf_check() (but it is detail).
I just did not want to change API in stable branch, so for now it is done this way.
In next major version crypt_benchmark_kdf() will change and will contain key size.
For the benchmarking - the "foo" + "bar" (salt), yes, it is strange,
but we just benchmark to fixed password, then use baseline for real calculation.
It was this way from the beginning. In the worst case, the final iteration count
will be higher, not smaller (for reasonable long passwords).
The whole intention of dynamic iteration count for volume key fingerprint is just
countermeasure for the case that RNG was flawed (it generates just limited
keyspace) and to somehow slow down possible bruteforce using digest only.
So the iteration count here is not so important as for the keyslot.
(Original LUKS has this fixed to 10 iteration anyway.)
> if (r < 0) {
> log_err(ctx, _("Not compatible PBKDF2 options (using hash algorithm %s).\n"),
> header->hashSpec);
> @@ -717,7 +720,7 @@ int LUKS_generate_phdr(struct luks_phdr *header,
>
> /* Compute master key digest */
> iteration_time_ms /= 8;
> - header->mkDigestIterations = at_least((uint32_t)(*PBKDF2_per_sec/1024) * iteration_time_ms,
> + header->mkDigestIterations = at_least((uint32_t)(*PBKDF2_per_sec/1000) * iteration_time_ms,
> LUKS_MKD_ITERATIONS_MIN);
The 1000 vs 1024 is not in fact mistake, it was my (stupid) optimization in old code
and I just keep it in place - the effect is small, I really do not care here.
As said above, the mkDigestIterations is just additional countermeasure.
So the only real problem I see there is that for volume key fingerprint we
do not use 20 bytes output key size, so for SHA1 it generates double iteration count.
So we add cca ~120ms more for check if keyslot iteration is set to 1s.
In recent version the default hash is sha256 where the problem is not present...
Sorry, but I do not want to change this code again, this patch do not improve any security
problem, it just moves the baseline a little bit.
Or did I miss anything important there?
For new LUKS we will like to use some memory-hard KDF anyway and the whole benchmarking
will be done differently.
Milan
_______________________________________________
dm-crypt mailing list
dm-crypt@xxxxxxxx
http://www.saout.de/mailman/listinfo/dm-crypt
