[Qemu-devel] [PULL v2 1/8] crypto: use uint64_t for pbkdf iteration count parameters

["Daniel P. Berrange" <berrange@redhat.com>]

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 iteration 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'.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 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(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index aba4455..bc086ac 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -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
diff --git a/crypto/pbkdf-gcrypt.c b/crypto/pbkdf-gcrypt.c
index 34af3a9..44cf31a 100644
--- a/crypto/pbkdf-gcrypt.c
+++ b/crypto/pbkdf-gcrypt.c
@@ -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 %lu",
+                         (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);
diff --git a/crypto/pbkdf-nettle.c b/crypto/pbkdf-nettle.c
index d681a60..db81517 100644
--- a/crypto/pbkdf-nettle.c
+++ b/crypto/pbkdf-nettle.c
@@ -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,
diff --git a/crypto/pbkdf-stub.c b/crypto/pbkdf-stub.c
index 266a505..a15044d 100644
--- a/crypto/pbkdf-stub.c
+++ b/crypto/pbkdf-stub.c
@@ -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)
diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c
index 695cc35..929458b 100644
--- a/crypto/pbkdf.c
+++ b/crypto/pbkdf.c
@@ -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;
 }
diff --git a/include/crypto/pbkdf.h b/include/crypto/pbkdf.h
index e9e4cec..6f4ac85 100644
--- a/include/crypto/pbkdf.h
+++ b/include/crypto/pbkdf.h
@@ -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 */
-- 
2.7.4