| Message ID | 1473352047-908-2-git-send-email-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <berrange@redhat.com> > --- > 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.
On Thu, Sep 08, 2016 at 12:44:55PM -0500, Eric Blake wrote: > 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 <berrange@redhat.com> > > --- > > 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... Oh doh, there I was thinkig it was just a 32bit int... Regards, Daniel
diff --git a/block/crypto.c b/block/crypto.c index 7f61e12..7aa7eb5 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -33,6 +33,7 @@ #define BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG "ivgen-alg" #define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg" #define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg" +#define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time" typedef struct BlockCrypto BlockCrypto; @@ -183,6 +184,11 @@ static QemuOptsList block_crypto_create_opts_luks = { .type = QEMU_OPT_STRING, .help = "Name of encryption hash algorithm", }, + { + .name = BLOCK_CRYPTO_OPT_LUKS_ITER_TIME, + .type = QEMU_OPT_NUMBER, + .help = "Time to spend in PBKDF in milliseconds", + }, { /* end of list */ } }, }; diff --git a/crypto/block-luks.c b/crypto/block-luks.c index aba4455..a5d9ebc 100644 --- a/crypto/block-luks.c +++ 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, 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"); + goto error; + } luks->header.master_key_iterations = MAX( - luks->header.master_key_iterations, - QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS); + iters, QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS); /* Hash the master key, saving the result in the LUKS @@ -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, (uint8_t *)password, strlen(password), luks->header.key_slots[0].salt, @@ -1141,12 +1149,18 @@ 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 /= 2 ? That matches cryptsetup, but there's no * explanation why they chose /= 2... */ - luks->header.key_slots[0].iterations /= 2; + iters /= 2; + if (iters > UINT32_MAX) { + error_setg(errp, "Too many PBKDF iterations for LUKS format"); + goto error; + } luks->header.key_slots[0].iterations = MAX( - luks->header.key_slots[0].iterations, - QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS); + iters, QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS); /* Generate a key that we'll use to encrypt the master diff --git a/qapi/crypto.json b/qapi/crypto.json index 34d2583..1527f4b 100644 --- a/qapi/crypto.json +++ 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 # Since: 2.6 ## { 'struct': 'QCryptoBlockCreateOptionsLUKS', @@ -193,7 +196,8 @@ '*cipher-mode': 'QCryptoCipherMode', '*ivgen-alg': 'QCryptoIVGenAlgorithm', '*ivgen-hash-alg': 'QCryptoHashAlgorithm', - '*hash-alg': 'QCryptoHashAlgorithm'}} + '*hash-alg': 'QCryptoHashAlgorithm', + '*iter-time': 'int'}} ##
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' property that allows the user to choose the iteration wallclock time. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- block/crypto.c | 6 ++++++ crypto/block-luks.c | 32 +++++++++++++++++++++++--------- qapi/crypto.json | 6 +++++- 3 files changed, 34 insertions(+), 10 deletions(-)