crypto: misc tweaks & improvements to pbkdf code
Subsystems (2)
cover crypto: misc tweaks & improvements to pbkdf code
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.
Daniel P. Berrange (6):
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 | 35 ++++++++++++++++++--------- crypto/pbkdf-gcrypt.c | 12 +++++++++- crypto/pbkdf-nettle.c | 61 ++++++++++++++++++++++++++++++++++++++++------- crypto/pbkdf.c | 23 ++++++++++++------ include/crypto/pbkdf.h | 6 ++++- qapi/crypto.json | 6 ++++- tests/test-crypto-pbkdf.c | 54 ++++++++++++++++++++++++++++++++++++++++- 8 files changed, 172 insertions(+), 31 deletions(-)
Signature · 1 line
2.7.4
PATCH 1/6 crypto: make PBKDF iterations configurable for LUKS format
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.
--- block/crypto.c | 6 ++++++ crypto/block-luks.c | 32 +++++++++++++++++++++++--------- qapi/crypto.json | 6 +++++- 3 files changed, 34 insertions(+), 10 deletions(-)
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 +23 -9
@@ -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
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 1/6] crypto: make PBKDF iterations configurable for LUKS format
Quoted 6 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 limits. This adds a new 'iter-time'
s/limits/iterations/ ?
Quoted 26 lines
property that allows the user to choose the iteration wallclock time.
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 6 ++++++ crypto/block-luks.c | 32 +++++++++++++++++++++++--------- qapi/crypto.json | 6 +++++- 3 files changed, 34 insertions(+), 10 deletions(-)+++
b/crypto/block-luks.ccrypto/block-luks.c +4
-0@@ -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 ; }
crypto/block-luks.c qcrypto_block_luks_create(QCryptoBlock *block, · line 1072
Quoted 6 lines
crypto/block-luks.c +1
-1@@ -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...
Quoted 7 lines
crypto/block-luks.c +0
-1masterkey, 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; } -
crypto/block-luks.c qcrypto_block_luks_create(QCryptoBlock *block, · line 1089
Quoted 10 lines
crypto/block-luks.c +5
-1+/* 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.
crypto/block-luks.c qcrypto_block_luks_create(QCryptoBlock *block, · line 1143
Quoted 7 lines
crypto/block-luks.c +1
-1@@ -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.
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
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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 1/6] crypto: make PBKDF iterations configurable for LUKS format
Quoted 37 lines · Eric Blake
Quoted 6 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 limits. This adds a new 'iter-time'
s/limits/iterations/ ?
Quoted 27 lines
property that allows the user to choose the iteration wallclock time.
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 6 ++++++ crypto/block-luks.c | 32 +++++++++++++++++++++++--------- qapi/crypto.json | 6 +++++- 3 files changed, 34 insertions(+), 10 deletions(-)+++
b/crypto/block-luks.ccrypto/block-luks.c +4
-0@@ -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
Quoted 8 lines
crypto/block-luks.c qcrypto_block_luks_create(QCryptoBlock *block, · line 1072
Quoted 5 lines
crypto/block-luks.c +1
-1* 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
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 2/6 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 | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
crypto/pbkdf.c +10 -5
@@ -67,13 +67,14 @@ int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash, const uint8_t *salt, size_t nsalt, Error **errp) { + int ret = -1;uint8_t out[32 ];long long int 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 @@ int 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;
@@ -103,8 +104,12 @@ int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,if (iterations >INT32_MAX ) {error_setg (errp,"Iterations %lld too large for a 32-bit int" , iterations); -return -1 ; + goto cleanup; } -return iterations; + ret = iterations; + + cleanup: +memset (out,0 ,sizeof (out)); +return ret; }
Signature · 1 line
2.7.4
Re: [Qemu-devel] [PATCH 2/6] crypto: clear out buffer after timing pbkdf algorithm
Quoted 8 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.
Signed-off-by Daniel P. Berrange <[email protected]>--- crypto/pbkdf.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
It still doesn't prevent the memory from being copied elsewhere (such as the stack being paged out), unless we go to extraordinary lengths to explicitly request volatile memory that can't be paged out. I don't know if we need to worry about that, though. Do any of our crypto libraries provide APIs for allocating local-use-only memory for sensitive data?
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 2/6] crypto: clear out buffer after timing pbkdf algorithm
Quoted 10 lines · Eric Blake
Quoted 8 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.
Signed-off-by Daniel P. Berrange <[email protected]>--- crypto/pbkdf.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
Quoted 8 lines
Reviewed-by Eric Blake <[email protected]>It still doesn't prevent the memory from being copied elsewhere (such as the stack being paged out), unless we go to extraordinary lengths to explicitly request volatile memory that can't be paged out. I don't know if we need to worry about that, though. Do any of our crypto libraries provide APIs for allocating local-use-only memory for sensitive data?
AFAICT, while gcrypt uses such APIs internally, it doesn't expose them to users. Nettle avoids malloc entirely in its API. So if we wanted that we'd basically need to roll our own. I don't think this is a big deal, but I just wanted to add the memset() as a sanity check really.
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 3/6 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
@@ -1073,6 +1073,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);
@@ -1144,6 +1145,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,int 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) {int ret =-1 ; -uint8_t out[32 ]; +uint8_t *out;long long int iterations = (1 <<15 );unsigned long long delta_ms, start_ms, end_ms; + out =g_new0 (uint8_t, nout); +while (1 ) {if (qcrypto_pbkdf2_get_thread_cpu (&start_ms, errp) <0 ) { goto cleanup;
@@ -80,7 +83,7 @@ int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash, key, nkey, salt, nsalt, iterations, - out, sizeof(out), + out, nout, errp) < 0) { goto cleanup; }
@@ -110,6 +113,7 @@ int 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 */ int 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 3/6] crypto: use correct derived key size when timing pbkdf
Quoted 23 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.
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(-)int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
constuint8_t*key,size_tnkey, constuint8_t*salt,size_tnsalt,
crypto/pbkdf.c static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms, · line 76
Quoted 10 lines
crypto/pbkdf.c +3
-1+size_t nout, Error **errp) { int ret = -1; -uint8_t out[32 ]; +uint8_t *out;long long int iterations = (1 <<15 );unsigned long long delta_ms, start_ms, end_ms; + out =g_new0 (uint8_t, nout);
Why g_new0()? Aren't we going to immediately be overwriting its contents, in which case g_new() is slightly faster?
Also, this changes from stack to heap for the sensitive memory, which changes the analysis of how likely the memory is to be paged out somewhere where we don't need spare copies floating around (if that is indeed something we want to worry about).
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 4/6 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 by cryptsetup had this division and it was recently removed too.
--- crypto/block-luks.c | 3 --- 1 file changed, 3 deletions(-)
crypto/block-luks.c +0 -3
@@ -1154,9 +1154,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,/* 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... */ - iters /=2 ;if (iters >UINT32_MAX ) {error_setg (errp,"Too many PBKDF iterations for LUKS format" ); goto error;
Signature · 1 line
2.7.4
Re: [Qemu-devel] [PATCH 4/6] crypto: remove bogus /= 2 for pbkdf iterations
Quoted 4 lines · Daniel P. Berrange
When 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 by cryptsetup had this division and it was recently
s/by/why/ ?
Quoted 1 line
removed too.
s/too/there, too/ ?
Quoted 5 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- crypto/block-luks.c | 3 --- 1 file changed, 3 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 5/6 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
Re: [Qemu-devel] [PATCH 5/6] crypto: increase default pbkdf2 time for luks to 2 seconds
Quoted 14 lines · Daniel P. Berrange
cryptsetup 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.
Signed-off-by Daniel P. Berrange <[email protected]>--- crypto/block-luks.c | 2 +- qapi/crypto.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)+++
b/qapi/crypto.json
qapi/crypto.json line 190
Quoted 5 lines
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
Possible merge conflicts if you address my comments in earlier patches, but those are trivial.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 6/6 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 | 61 ++++++++++++++++++++++++++++++++++++++++------- tests/test-crypto-pbkdf.c | 53 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 115 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,13 +49,19 @@ 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;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 +52 -9
89 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 6/6] crypto: support more hash algorithms for pbkdf
Quoted 14 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 | 61 ++++++++++++++++++++++++++++++++++++++++------- tests/test-crypto-pbkdf.c | 53 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 11 deletions(-)if (hash >= G_N_ELEMENTS(hash_map) || hash_map[hash] == GCRY_MD_NONE) {
crypto/pbkdf-gcrypt.c int qcrypto_pbkdf2(QCryptoHashAlgorithm hash, · line 64
Quoted 4 lines
crypto/pbkdf-gcrypt.c +3
-1-error_setg (errp,"Unexpected hash algorithm %d" , hash); +error_setg_errno (errp,ENOSYS , +"PBKDF does not support hash algorithm %s" , + QCryptoHashAlgorithm_lookup[hash]);
Can this access beyond bounds if hash > G_N_ELEMENTS(hash_map)?
crypto/pbkdf-nettle.c int qcrypto_pbkdf2(QCryptoHashAlgorithm hash, · line 105
Quoted 8 lines
+++
b/crypto/pbkdf-nettle.cdefault:
error_setg_errno(errp, ENOSYS,crypto/pbkdf-nettle.c +2
-1- "PBKDF does not support hash algorithm %d", hash); +"PBKDF does not support hash algorithm %s" , + QCryptoHashAlgorithm_lookup[hash]);
and again
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 6/6] crypto: support more hash algorithms for pbkdf
Quoted 13 lines · Eric Blake
Quoted 12 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 | 61 ++++++++++++++++++++++++++++++++++++++++------- tests/test-crypto-pbkdf.c | 53 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 11 deletions(-)
Quoted 8 lines
crypto/pbkdf-gcrypt.c int qcrypto_pbkdf2(QCryptoHashAlgorithm hash, · line 64
Quoted 6 lines
if (hash >= G_N_ELEMENTS(hash_map) || hash_map[hash] == GCRY_MD_NONE) {crypto/pbkdf-gcrypt.c +3
-1-error_setg (errp,"Unexpected hash algorithm %d" , hash); +error_setg_errno (errp,ENOSYS , +"PBKDF does not support hash algorithm %s" , + QCryptoHashAlgorithm_lookup[hash]);Can this access beyond bounds if hash >
G_N_ELEMENTS(hash_map)?
I'm relying on fact that 'hash' came from QAPIs string -> enum convertor - nothing in QEMU lets users provide raw integer enum values - so will be guaranteed to be a valid enum value.
Quoted 10 lines
crypto/pbkdf-nettle.c int qcrypto_pbkdf2(QCryptoHashAlgorithm hash, · line 105
Quoted 8 lines
+++
b/crypto/pbkdf-nettle.cdefault:
error_setg_errno(errp, ENOSYS,crypto/pbkdf-nettle.c +2
-1- "PBKDF does not support hash algorithm %d", hash); +"PBKDF does not support hash algorithm %s" , + QCryptoHashAlgorithm_lookup[hash]);and again
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 :|
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 0/6] 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 Switched to a new branch 'test'
15cfa9d crypto: support more hash algorithms for pbkdf eb171ab crypto: increase default pbkdf2 time for luks to 2 seconds b16d6e2 crypto: remove bogus /= 2 for pbkdf iterations b48082f crypto: use correct derived key size when timing pbkdf a61fe5a crypto: clear out buffer after timing pbkdf algorithm cce4ddf crypto: make PBKDF iterations configurable for LUKS format
=== OUTPUT BEGIN ===
Checking PATCH 1/6: crypto: make PBKDF iterations configurable for LUKS format... Checking PATCH 2/6: crypto: clear out buffer after timing pbkdf algorithm... Checking PATCH 3/6: crypto: use correct derived key size when timing pbkdf... Checking PATCH 4/6: crypto: remove bogus /= 2 for pbkdf iterations... Checking PATCH 5/6: crypto: increase default pbkdf2 time for luks to 2 seconds... Checking PATCH 6/6: crypto: support more hash algorithms for pbkdf...
ERROR: if this code is redundant consider removing it
#211: FILE: tests/test-crypto-pbkdf.c:332:
+#if 0
total: 1 errors, 0 warnings, 185 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]