crypto: misc tweaks & improvements to pbkdf code
Subsystems (2)
v2 cover crypto: misc tweaks & improvements to pbkdf code
This is an update of
v1: https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01841.html
The core goal of this series was to make the pbkdf2 iteration time configurable. In doing so a number of other improvements were identified, and some updates done to match latest cryptsetup behaviour / recommendations.
Changed in v2:
- Switch pbkdf APIs to use uint64_t for iteration count
- Fix detection of iteration count overflow (Eric)
- Misc typos (Eric)
- Use correct syntax for qapi 'since' annotation
- Use g_new instead of g_new0 (Eric)
Daniel P. Berrange (7):
crypto: use uint64_t for pbkdf iteration count parameters
crypto: make PBKDF iterations configurable for LUKS format
crypto: clear out buffer after timing pbkdf algorithm
crypto: use correct derived key size when timing pbkdf
crypto: remove bogus /= 2 for pbkdf iterations
crypto: increase default pbkdf2 time for luks to 2 seconds
crypto: support more hash algorithms for pbkdf
block/crypto.c | 6 ++++ crypto/block-luks.c | 78 +++++++++++++++++++++++++++++++++-------------- crypto/pbkdf-gcrypt.c | 21 +++++++++++-- crypto/pbkdf-nettle.c | 71 ++++++++++++++++++++++++++++++++++++------ crypto/pbkdf-stub.c | 2 +- crypto/pbkdf.c | 35 +++++++++++---------- include/crypto/pbkdf.h | 16 ++++++---- qapi/crypto.json | 6 +++- tests/test-crypto-pbkdf.c | 54 +++++++++++++++++++++++++++++++- 9 files changed, 229 insertions(+), 60 deletions(-)
Signature · 1 line
2.7.4
PATCH v2 1/7 crypto: use uint64_t for pbkdf iteration count parameters
The qcrypto_pbkdf_count_iters method uses a 64 bit int but then checks its value against INT32_MAX before returning it. This bounds check is premature, because the calling code may well scale the interation count by some value. It is thus better to return a 64-bit integer and let the caller do range checking.
For consistency the qcrypto_pbkdf method is also changed to accept a 64bit int, though this is somewhat academic since nettle is limited to taking an 'int' while gcrypt is limited to taking a 'long int'.
--- crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++-------------------- crypto/pbkdf-gcrypt.c | 9 ++++++++- crypto/pbkdf-nettle.c | 8 +++++++- crypto/pbkdf-stub.c | 2 +- crypto/pbkdf.c | 16 +++++----------- include/crypto/pbkdf.h | 10 +++++----- 6 files changed, 57 insertions(+), 40 deletions(-)
crypto/block-luks.c +31 -21
@@ -917,6 +917,7 @@ 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_cipher_alg) {
@@ -1064,12 +1065,11 @@ 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 = -qcrypto_pbkdf2_count_iters (luks_opts.hash_alg, - masterkey, luks->header.key_bytes, - luks->header.master_key_salt, -QCRYPTO_BLOCK_LUKS_SALT_LEN , - &local_err); + iters =qcrypto_pbkdf2_count_iters (luks_opts.hash_alg, + masterkey, luks->header.key_bytes, + luks->header.master_key_salt, +QCRYPTO_BLOCK_LUKS_SALT_LEN , + &local_err);if (local_err) {error_propagate (errp, local_err); goto error;
@@ -1079,11 +1079,15 @@ qcrypto_block_luks_create(QCryptoBlock *block,* 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 ; - luks->header.master_key_iterations =MAX ( - luks->header.master_key_iterations, -QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS ); - + iters /=8 ; +if (iters >UINT32_MAX ) { +error_setg_errno (errp,ERANGE , +"PBKDF iterations %llu larger than %u" , + (unsigned long long )iters,UINT32_MAX ); + goto error; + } + iters =MAX (iters,QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS ); + luks->header.master_key_iterations = iters;/* Hash the master key, saving the result in the LUKS * header. This hash is used when opening the encrypted
@@ -1131,22 +1135,28 @@ 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 = -qcrypto_pbkdf2_count_iters (luks_opts.hash_alg, - (uint8_t *)password,strlen (password), - luks->header.key_slots[0 ].salt, -QCRYPTO_BLOCK_LUKS_SALT_LEN , - &local_err); + iters =qcrypto_pbkdf2_count_iters (luks_opts.hash_alg, + (uint8_t *)password,strlen (password), + luks->header.key_slots[0 ].salt, +QCRYPTO_BLOCK_LUKS_SALT_LEN , + &local_err);if (local_err) {error_propagate (errp, local_err); goto error; }/* Why /= 2 ? That matches cryptsetup, but there's no * explanation why they chose /= 2... */ - luks->header.key_slots[0 ].iterations /=2 ; - luks->header.key_slots[0 ].iterations =MAX ( - luks->header.key_slots[0 ].iterations, -QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS ); + iters /=2 ; + +if (iters >UINT32_MAX ) { +error_setg_errno (errp,ERANGE , +"PBKDF iterations %llu larger than %u" , + (unsigned long long )iters,UINT32_MAX ); + goto error; + } + + luks->header.key_slots[0 ].iterations = +MAX (iters,QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS );/* Generate a key that we'll use to encrypt the master
crypto/pbkdf-gcrypt.c +8 -1
@@ -38,7 +38,7 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash)int qcrypto_pbkdf2 (QCryptoHashAlgorithm hash,const uint8_t *key,size_t nkey,const uint8_t *salt,size_t nsalt, -unsigned int iterations, +uint64_t iterations,uint8_t *out,size_t nout,Error **errp) {
@@ -49,6 +49,13 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash, };int ret; +if (iterations >ULONG_MAX ) { +error_setg_errno (errp,ERANGE , +"PBKDF iterations %llu must be less than %ul" , + (long long unsigned )iterations,ULONG_MAX ); +return -1 ; + } +if (hash >=G_N_ELEMENTS (hash_map) || hash_map[hash] ==GCRY_MD_NONE ) {error_setg (errp,"Unexpected hash algorithm %d" , hash);
crypto/pbkdf-nettle.c +7 -1
@@ -38,10 +38,16 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash)int qcrypto_pbkdf2 (QCryptoHashAlgorithm hash,const uint8_t *key,size_t nkey,const uint8_t *salt,size_t nsalt, -unsigned int iterations, +uint64_t iterations,uint8_t *out,size_t nout,Error **errp) { +if (iterations >UINT_MAX ) { +error_setg_errno (errp,ERANGE , +"PBKDF iterations %llu must be less than %u" , + (long long unsigned )iterations,UINT_MAX ); +return -1 ; + }switch (hash) {case QCRYPTO_HASH_ALG_SHA1 : pbkdf2_hmac_sha1(nkey, key,
crypto/pbkdf-stub.c +1 -1
@@ -32,7 +32,7 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash G_GNUC_UNUSED, size_t nkey G_GNUC_UNUSED, const uint8_t *salt G_GNUC_UNUSED, size_t nsalt G_GNUC_UNUSED, - unsigned int iterations G_GNUC_UNUSED, + uint64_t iterations G_GNUC_UNUSED, uint8_t *out G_GNUC_UNUSED, size_t nout G_GNUC_UNUSED, Error **errp)
crypto/pbkdf.c +5 -11
@@ -62,13 +62,13 @@ static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms,#endif } -int qcrypto_pbkdf2_count_iters (QCryptoHashAlgorithm hash, -const uint8_t *key,size_t nkey, -const uint8_t *salt,size_t nsalt, -Error **errp) +uint64_t qcrypto_pbkdf2_count_iters (QCryptoHashAlgorithm hash, +const uint8_t *key,size_t nkey, +const uint8_t *salt,size_t nsalt, +Error **errp) {uint8_t out[32 ]; -long long int iterations = (1 <<15 ); +uint64_t iterations = (1 <<15 );unsigned long long delta_ms, start_ms, end_ms;while (1 ) {
@@ -100,11 +100,5 @@ int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash, iterations = iterations *1000 / delta_ms; -if (iterations >INT32_MAX ) { -error_setg (errp,"Iterations %lld too large for a 32-bit int" , - iterations); -return -1 ; - } -return iterations; }
include/crypto/pbkdf.h +5 -5
@@ -122,7 +122,7 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash);int qcrypto_pbkdf2 (QCryptoHashAlgorithm hash,const uint8_t *key,size_t nkey,const uint8_t *salt,size_t nsalt, -unsigned int iterations, +uint64_t iterations,uint8_t *out,size_t nout,Error **errp);
@@ -144,9 +144,9 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,* * Returns: number of iterations in 1 second, -1 on error */ -int qcrypto_pbkdf2_count_iters (QCryptoHashAlgorithm hash, -const uint8_t *key,size_t nkey, -const uint8_t *salt,size_t nsalt, -Error **errp); +uint64_t qcrypto_pbkdf2_count_iters (QCryptoHashAlgorithm hash, +const uint8_t *key,size_t nkey, +const uint8_t *salt,size_t nsalt, +Error **errp);#endif /* QCRYPTO_PBKDF_H */
Signature · 1 line
2.7.4
Re: [Qemu-devel] [PATCH v2 1/7] crypto: use uint64_t for pbkdf iteration count parameters
Quoted 4 lines · Daniel P. Berrange
The
qcrypto_pbkdf_count_itersmethod uses a 64 bit int but then checks its value againstINT32_MAXbefore returning it. This bounds check is premature, because the calling code may well scale the interation count
s/interation/iteration/
Quoted 21 lines
by some value. It is thus better to return a 64-bit integer and let the caller do range checking.
For consistency the
qcrypto_pbkdfmethod is also changed to accept a 64bit int, though this is somewhat academic since nettle is limited to taking an 'int' while gcrypt is limited to taking a 'long int'.Signed-off-by Daniel P. Berrange <[email protected]>---
crypto/block-luks.c +1
-1+uint64_t iters;memcpy (&luks_opts, &options->u.luks,sizeof (luks_opts));if (!luks_opts.has_cipher_alg) {@@ -1079,11 +1079,15 @@ qcrypto_block_luks_create(QCryptoBlock *block,* 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 ;
crypto/block-luks.c qcrypto_block_luks_create(QCryptoBlock *block, · line 1087
Quoted 10 lines
crypto/block-luks.c +6
-4- luks->header.master_key_iterations =MAX ( - luks->header.master_key_iterations, -QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS ); - + iters /=8 ; +if (iters >UINT32_MAX ) { +error_setg_errno (errp,ERANGE , +"PBKDF iterations %llu larger than %u" , + (unsigned long long )iters,UINT32_MAX ); + goto error;
We could avoid the cast by using PRIu64 (couple of times in this patch).
But not the end of the world to leave it.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH v2 2/7 crypto: make PBKDF iterations configurable for LUKS format
In reply to Daniel P. BerrangeAs 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 iterations. This change adds a new 'iter-time' property that allows the user to choose the iteration wallclock time.
--- block/crypto.c | 6 ++++++ crypto/block-luks.c | 24 ++++++++++++++++++++++++ qapi/crypto.json | 6 +++++- 3 files changed, 35 insertions(+), 1 deletion(-)
block/crypto.c +6 -0
@@ -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 */ } }, };
crypto/block-luks.c +24 -0
@@ -920,6 +920,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,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 ; }
@@ -1075,6 +1078,16 @@ qcrypto_block_luks_create(QCryptoBlock *block, goto error; } +if (iters > (ULLONG_MAX / luks_opts.iter_time)) { +error_setg_errno (errp,ERANGE , +"PBKDF iterations %llu too large to scale" , + (unsigned long long )iters); + goto error; + } + +/* iter_time was in millis, but count_iters reported for secs */ + iters = iters * luks_opts.iter_time /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
@@ -1144,6 +1157,17 @@ qcrypto_block_luks_create(QCryptoBlock *block,error_propagate (errp, local_err); goto error; } + +if (iters > (ULLONG_MAX / luks_opts.iter_time)) { +error_setg_errno (errp,ERANGE , +"PBKDF iterations %llu too large to scale" , + (unsigned long long )iters); + goto error; + } + +/* iter_time was in millis, but count_iters reported for secs */ + iters = iters * luks_opts.iter_time /1000 ; +/* Why /= 2 ? That matches cryptsetup, but there's no * explanation why they chose /= 2... */ iters /=2 ;
qapi/crypto.json +5 -1
@@ -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'}} ##
Signature · 1 line
2.7.4
Re: [Qemu-devel] [PATCH v2 2/7] crypto: make PBKDF iterations configurable for LUKS format
Quoted 13 lines · Daniel P. Berrange
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 iterations. This change adds a new 'iter-time' property that allows the user to choose the iteration wallclock time.
Signed-off-by Daniel P. Berrange <[email protected]>---
block/crypto.c static QemuOptsList block_crypto_create_opts_luks = { · line 190
Quoted 7 lines
block/crypto.c +4
-0@@ -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",
Worth mentioning the default of 1000?
crypto/block-luks.c qcrypto_block_luks_create(QCryptoBlock *block, · line 1084
Quoted 8 lines
crypto/block-luks.c +4
-0@@ -1075,6 +1078,16 @@ qcrypto_block_luks_create(QCryptoBlock *block, goto error; } +if (iters > (ULLONG_MAX / luks_opts.iter_time)) { +error_setg_errno (errp,ERANGE , +"PBKDF iterations %llu too large to scale" , + (unsigned long long )iters);
As in 1/7, you seem to prefer casts over PRIu64 :)
qapi/crypto.json line 190
Quoted 8 lines
qapi/crypto.json +4
-0+++ 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)
Ah, you mentioned the default here.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v2 2/7] crypto: make PBKDF iterations configurable for LUKS format
Quoted 13 lines · Eric Blake
Quoted 11 lines · Daniel P. Berrange
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 iterations. This change adds a new 'iter-time' property that allows the user to choose the iteration wallclock time.
Signed-off-by Daniel P. Berrange <[email protected]>---
Quoted 10 lines
block/crypto.c static QemuOptsList block_crypto_create_opts_luks = { · line 190
Quoted 8 lines
block/crypto.c +4
-0@@ -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",Worth mentioning the default of 1000?
I tried to keep all the defaults listed just in the QAPI schema, to avoid remembering to update multiple docs.
Quoted 22 lines
crypto/block-luks.c qcrypto_block_luks_create(QCryptoBlock *block, · line 1084
Quoted 8 lines
crypto/block-luks.c +4
-0@@ -1075,6 +1078,16 @@ qcrypto_block_luks_create(QCryptoBlock *block, goto error; } +if (iters > (ULLONG_MAX / luks_opts.iter_time)) { +error_setg_errno (errp,ERANGE , +"PBKDF iterations %llu too large to scale" , + (unsigned long long )iters);As in 1/7, you seem to prefer casts over PRIu64 :)
qapi/crypto.json line 190
Quoted 8 lines
qapi/crypto.json +4
-0+++ 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)Ah, you mentioned the default here.
Quoted 6 lines
Reviewed-by Eric Blake <[email protected]>Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Regards,
Daniel
Signature · 4 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
PATCH v2 3/7 crypto: clear out buffer after timing pbkdf algorithm
In reply to Daniel P. BerrangeThe 'out' buffer will hold a key derived from master password, so it is best practice to clear this buffer when no longer required.
--- crypto/pbkdf.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
crypto/pbkdf.c +9 -4
@@ -67,13 +67,14 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash, const uint8_t *salt, size_t nsalt, Error **errp) { + uint64_t ret = -1;uint8_t out[32 ];uint64_t iterations = (1 <<15 );unsigned long long delta_ms, start_ms, end_ms;while (1 ) {if (qcrypto_pbkdf2_get_thread_cpu (&start_ms, errp) <0 ) { -return -1 ; + goto cleanup; } if (qcrypto_pbkdf2(hash, key, nkey,
@@ -81,10 +82,10 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash, iterations, out, sizeof(out), errp) < 0) { -return -1 ; + goto cleanup; }if (qcrypto_pbkdf2_get_thread_cpu (&end_ms, errp) <0 ) { -return -1 ; + goto cleanup; } delta_ms = end_ms - start_ms;
@@ -100,5 +101,9 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash, iterations = iterations *1000 / delta_ms; -return iterations; + ret = iterations; + + cleanup: +memset (out,0 ,sizeof (out)); +return ret; }
Signature · 1 line
2.7.4
Re: [Qemu-devel] [PATCH v2 3/7] crypto: clear out buffer after timing pbkdf algorithm
Quoted 3 lines · Daniel P. Berrange
The 'out' buffer will hold a key derived from master password, so it is best practice to clear this buffer when no longer required.
Still might be worth an explicit mention that we are currently not going to worry about even tougher protections against copies stored in swap.
Quoted 6 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- crypto/pbkdf.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH v2 4/7 crypto: use correct derived key size when timing pbkdf
In reply to Daniel P. BerrangeCurrently when timing the pbkdf algorithm a fixed key size of 32 bytes is used. This results in inaccurate timings for certain hashes depending on their digest size. For example when using sha1 with aes-256, this causes us to measure time for the master key digest doing 2 sha1 operations per iteration, instead of 1.
Instead we should pass in the desired key size to the timing routine that matches the key size that will be used for real later.
--- crypto/block-luks.c | 2 ++ crypto/pbkdf.c | 10 +++++++--- include/crypto/pbkdf.h | 6 +++++- tests/test-crypto-pbkdf.c | 1 + 4 files changed, 15 insertions(+), 4 deletions(-)
crypto/block-luks.c +2 -0
@@ -1072,6 +1072,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, masterkey, luks->header.key_bytes, luks->header.master_key_salt,QCRYPTO_BLOCK_LUKS_SALT_LEN , +QCRYPTO_BLOCK_LUKS_DIGEST_LEN , &local_err);if (local_err) {error_propagate (errp, local_err);
@@ -1152,6 +1153,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, (uint8_t *)password,strlen (password), luks->header.key_slots[0 ].salt,QCRYPTO_BLOCK_LUKS_SALT_LEN , + luks->header.key_bytes, &local_err);if (local_err) {error_propagate (errp, local_err);
crypto/pbkdf.c +7 -3
@@ -65,13 +65,16 @@ static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms,uint64_t qcrypto_pbkdf2_count_iters (QCryptoHashAlgorithm hash,const uint8_t *key,size_t nkey,const uint8_t *salt,size_t nsalt, +size_t nout,Error **errp) {uint64_t ret =-1 ; -uint8_t out[32 ]; +uint8_t *out;uint64_t iterations = (1 <<15 );unsigned long long delta_ms, start_ms, end_ms; + out =g_new (uint8_t, nout); +while (1 ) {if (qcrypto_pbkdf2_get_thread_cpu (&start_ms, errp) <0 ) { goto cleanup;
@@ -80,7 +83,7 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash, key, nkey, salt, nsalt, iterations, - out, sizeof(out), + out, nout, errp) < 0) { goto cleanup; }
@@ -104,6 +107,7 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash, ret = iterations; cleanup: -memset (out,0 ,sizeof (out)); +memset (out,0 , nout); +g_free (out);return ret; }
include/crypto/pbkdf.h +5 -1
@@ -133,6 +133,7 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,* @nkey: the length of @key in bytes * @salt: a random salt * @nsalt: length of @salt in bytes +* @nout: size of desired derived key * @errp: pointer to a NULL-initialized error object * * Time the PBKDF2 algorithm to determine how many
@@ -140,13 +141,16 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,* key from a user password provided in @key in 1 * second of compute time. The result of this can * be used as a the @iterations parameter of a later -* call to qcrypto_pbkdf2(). +* call to qcrypto_pbkdf2(). The value of @nout should +* match that value that will later be provided with +* a call to qcrypto_pbkdf2(). * * Returns: number of iterations in 1 second, -1 on error */ uint64_t qcrypto_pbkdf2_count_iters (QCryptoHashAlgorithm hash,const uint8_t *key,size_t nkey,const uint8_t *salt,size_t nsalt, +size_t nout,Error **errp);#endif /* QCRYPTO_PBKDF_H */
tests/test-crypto-pbkdf.c +1 -0
@@ -358,6 +358,7 @@ static void test_pbkdf_timing(void) iters =qcrypto_pbkdf2_count_iters (QCRYPTO_HASH_ALG_SHA256 , key,sizeof (key), salt,sizeof (salt), +32 , &error_abort);g_assert (iters >= (1 <<15 ));
Signature · 1 line
2.7.4
Re: [Qemu-devel] [PATCH v2 4/7] crypto: use correct derived key size when timing pbkdf
Quoted 11 lines · Daniel P. Berrange
Currently when timing the pbkdf algorithm a fixed key size of 32 bytes is used. This results in inaccurate timings for certain hashes depending on their digest size. For example when using sha1 with aes-256, this causes us to measure time for the master key digest doing 2 sha1 operations per iteration, instead of 1.
Instead we should pass in the desired key size to the timing routine that matches the key size that will be used for real later.
Quoted 8 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- crypto/block-luks.c | 2 ++ crypto/pbkdf.c | 10 +++++++--- include/crypto/pbkdf.h | 6 +++++- tests/test-crypto-pbkdf.c | 1 + 4 files changed, 15 insertions(+), 4 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH v2 5/7 crypto: remove bogus /= 2 for pbkdf iterations
In reply to Daniel P. BerrangeWhen calculating iterations for pbkdf of the key slot data, we had a /= 2, which was copied from identical code in cryptsetup. It was always unclear & undocumented why cryptsetup had this division and it was recently removed there, too.
--- crypto/block-luks.c | 4 ---- 1 file changed, 4 deletions(-)
crypto/block-luks.c +0 -4
@@ -1170,10 +1170,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,/* iter_time was in millis, but count_iters reported for secs */ iters = iters * luks_opts.iter_time /1000 ; -/* Why /= 2 ? That matches cryptsetup, but there's no -* explanation why they chose /= 2... */ - iters /=2 ; -if (iters >UINT32_MAX ) { error_setg_errno(errp, ERANGE, "PBKDF iterations %llu larger than %u",
Signature · 1 line
2.7.4
PATCH v2 6/7 crypto: increase default pbkdf2 time for luks to 2 seconds
In reply to Daniel P. Berrangecryptsetup recently increased the default pbkdf2 time to 2 seconds to partially mitigate improvements in hardware performance wrt brute-forcing the pbkdf algorithm. This updates QEMU defaults to match.
--- crypto/block-luks.c | 2 +- qapi/crypto.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
crypto/block-luks.c +1 -1
@@ -921,7 +921,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,memcpy (&luks_opts, &options->u.luks,sizeof (luks_opts));if (!luks_opts.has_iter_time) { - luks_opts.iter_time =1000 ; + luks_opts.iter_time =2000 ; }if (!luks_opts.has_cipher_alg) { luks_opts.cipher_alg =QCRYPTO_CIPHER_ALG_AES_256 ;
qapi/crypto.json +1 -1
@@ -187,7 +187,7 @@ # Currently defaults to 'sha256' # @iter-time: #optional number of milliseconds to spend in # PBKDF passphrase processing. Currently defaults -# to 1000. (since 2.8) +# to 2000. (since 2.8) # Since: 2.6 ## { 'struct': 'QCryptoBlockCreateOptionsLUKS',
Signature · 1 line
2.7.4
PATCH v2 7/7 crypto: support more hash algorithms for pbkdf
In reply to Daniel P. BerrangeCurrently pbkdf is only supported with SHA1 and SHA256. Expand this to support all algorithms known to QEMU.
--- crypto/pbkdf-gcrypt.c | 12 ++++++++- crypto/pbkdf-nettle.c | 63 ++++++++++++++++++++++++++++++++++++++++------- tests/test-crypto-pbkdf.c | 53 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 117 insertions(+), 11 deletions(-)
crypto/pbkdf-gcrypt.c +11 -1
@@ -28,7 +28,11 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash)switch (hash) {case QCRYPTO_HASH_ALG_MD5 :case QCRYPTO_HASH_ALG_SHA1 : +case QCRYPTO_HASH_ALG_SHA224 :case QCRYPTO_HASH_ALG_SHA256 : +case QCRYPTO_HASH_ALG_SHA384 : +case QCRYPTO_HASH_ALG_SHA512 : +case QCRYPTO_HASH_ALG_RIPEMD160 :return true;default :return false;
@@ -45,7 +49,11 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,static const int hash_map[QCRYPTO_HASH_ALG__MAX ] = { [QCRYPTO_HASH_ALG_MD5 ] =GCRY_MD_MD5 , [QCRYPTO_HASH_ALG_SHA1 ] =GCRY_MD_SHA1 , + [QCRYPTO_HASH_ALG_SHA224 ] =GCRY_MD_SHA224 , [QCRYPTO_HASH_ALG_SHA256 ] =GCRY_MD_SHA256 , + [QCRYPTO_HASH_ALG_SHA384 ] =GCRY_MD_SHA384 , + [QCRYPTO_HASH_ALG_SHA512 ] =GCRY_MD_SHA512 , + [QCRYPTO_HASH_ALG_RIPEMD160 ] =GCRY_MD_RMD160 , };int ret;
@@ -58,7 +66,9 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,if (hash >=G_N_ELEMENTS (hash_map) || hash_map[hash] ==GCRY_MD_NONE ) { -error_setg (errp,"Unexpected hash algorithm %d" , hash); +error_setg_errno (errp,ENOSYS , +"PBKDF does not support hash algorithm %s" , + QCryptoHashAlgorithm_lookup[hash]);return -1 ; }
crypto/pbkdf-nettle.c +54 -9
97 lines not on this page · raw message
tests/test-crypto-pbkdf.c +52 -1
65 lines not on this page · raw message
Signature · 1 line
2.7.4
Re: [Qemu-devel] [PATCH v2 7/7] crypto: support more hash algorithms for pbkdf
Quoted 10 lines · Daniel P. Berrange
Currently pbkdf is only supported with SHA1 and SHA256. Expand this to support all algorithms known to QEMU.
Signed-off-by Daniel P. Berrange <[email protected]>--- crypto/pbkdf-gcrypt.c | 12 ++++++++- crypto/pbkdf-nettle.c | 63 ++++++++++++++++++++++++++++++++++++++++------- tests/test-crypto-pbkdf.c | 53 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 117 insertions(+), 11 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Your series seems to have some coding style problems. See output below for more information:
Hi,
Your series seems to have some coding style problems. See output below for more information:
Type: series
Message-id: [email protected]
Subject: [Qemu-devel] [PATCH v2 0/7] crypto: misc tweaks & improvements to pbkdf code
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/[email protected] -> patchew/[email protected]
Switched to a new branch 'test'
c54912e crypto: support more hash algorithms for pbkdf 938fb07 crypto: increase default pbkdf2 time for luks to 2 seconds 44b7066 crypto: remove bogus /= 2 for pbkdf iterations eee0960 crypto: use correct derived key size when timing pbkdf 322de29 crypto: clear out buffer after timing pbkdf algorithm c424b75 crypto: make PBKDF iterations configurable for LUKS format 76d54b0 crypto: use uint64_t for pbkdf iteration count parameters
=== OUTPUT BEGIN ===
Checking PATCH 1/7: crypto: use uint64_t for pbkdf iteration count parameters... Checking PATCH 2/7: crypto: make PBKDF iterations configurable for LUKS format... Checking PATCH 3/7: crypto: clear out buffer after timing pbkdf algorithm... Checking PATCH 4/7: crypto: use correct derived key size when timing pbkdf... Checking PATCH 5/7: crypto: remove bogus /= 2 for pbkdf iterations... Checking PATCH 6/7: crypto: increase default pbkdf2 time for luks to 2 seconds... Checking PATCH 7/7: crypto: support more hash algorithms for pbkdf...
ERROR: if this code is redundant consider removing it
#221: FILE: tests/test-crypto-pbkdf.c:332:
+#if 0
total: 1 errors, 0 warnings, 194 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]