[PATCH v2 0/3] crypto: fix regression in hash result buffer handling

[PATCH v2 0/3] crypto: fix regression in hash result buffer handling

[Daniel P. Berrangé]

Daniel P. Berrangé (3):
  crypto/hash: avoid overwriting user supplied result pointer
  tests: correctly validate result buffer in hash/hmac tests
  include/crypto: clarify @result/@result_len for hash/hmac APIs

 crypto/hash-gcrypt.c          | 15 ++++++++++++---
 crypto/hash-glib.c            | 11 +++++++++--
 crypto/hash-gnutls.c          | 16 +++++++++++++---
 crypto/hash-nettle.c          | 14 +++++++++++---
 include/crypto/hash.h         | 30 +++++++++++++++++++++++-------
 include/crypto/hmac.h         | 17 ++++++++++++-----
 tests/unit/test-crypto-hash.c |  7 ++++---
 tests/unit/test-crypto-hmac.c |  6 ++++--
 8 files changed, 88 insertions(+), 28 deletions(-)

-- 
2.46.0

[PATCH v2 1/3] crypto/hash: avoid overwriting user supplied result pointer

[Daniel P. Berrangé]

If the user provides a pre-allocated buffer for the hash result,
we must use that rather than re-allocating a new buffer.

Reported-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/hash-gcrypt.c | 15 ++++++++++++---
 crypto/hash-glib.c   | 11 +++++++++--
 crypto/hash-gnutls.c | 16 +++++++++++++---
 crypto/hash-nettle.c | 14 +++++++++++---
 4 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/crypto/hash-gcrypt.c b/crypto/hash-gcrypt.c
index ccc3cce3f8..73533a4949 100644
--- a/crypto/hash-gcrypt.c
+++ b/crypto/hash-gcrypt.c
@@ -103,16 +103,25 @@ int qcrypto_gcrypt_hash_finalize(QCryptoHash *hash,
                                  size_t *result_len,
                                  Error **errp)
 {
+    int ret;
     unsigned char *digest;
     gcry_md_hd_t *ctx = hash->opaque;
 
-    *result_len = gcry_md_get_algo_dlen(qcrypto_hash_alg_map[hash->alg]);
-    if (*result_len == 0) {
+    ret = gcry_md_get_algo_dlen(qcrypto_hash_alg_map[hash->alg]);
+    if (ret == 0) {
         error_setg(errp, "Unable to get hash length");
         return -1;
     }
 
-    *result = g_new(uint8_t, *result_len);
+    if (*result_len == 0) {
+        *result_len = ret;
+        *result = g_new(uint8_t, *result_len);
+    } else if (*result_len != ret) {
+        error_setg(errp,
+                   "Result buffer size %zu is smaller than hash %d",
+                   *result_len, ret);
+        return -1;
+    }
 
     /* Digest is freed by gcry_md_close(), copy it */
     digest = gcry_md_read(*ctx, 0);
diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
index 02a6ec1edf..809cef98ae 100644
--- a/crypto/hash-glib.c
+++ b/crypto/hash-glib.c
@@ -99,8 +99,15 @@ int qcrypto_glib_hash_finalize(QCryptoHash *hash,
         return -1;
     }
 
-    *result_len = ret;
-    *result = g_new(uint8_t, *result_len);
+    if (*result_len == 0) {
+        *result_len = ret;
+        *result = g_new(uint8_t, *result_len);
+    } else if (*result_len != ret) {
+        error_setg(errp,
+                   "Result buffer size %zu is smaller than hash %d",
+                   *result_len, ret);
+        return -1;
+    }
 
     g_checksum_get_digest(ctx, *result, result_len);
     return 0;
diff --git a/crypto/hash-gnutls.c b/crypto/hash-gnutls.c
index 34a63994c9..99fbe824ea 100644
--- a/crypto/hash-gnutls.c
+++ b/crypto/hash-gnutls.c
@@ -115,14 +115,24 @@ int qcrypto_gnutls_hash_finalize(QCryptoHash *hash,
                                  Error **errp)
 {
     gnutls_hash_hd_t *ctx = hash->opaque;
+    int ret;
 
-    *result_len = gnutls_hash_get_len(qcrypto_hash_alg_map[hash->alg]);
-    if (*result_len == 0) {
+    ret = gnutls_hash_get_len(qcrypto_hash_alg_map[hash->alg]);
+    if (ret == 0) {
         error_setg(errp, "Unable to get hash length");
         return -1;
     }
 
-    *result = g_new(uint8_t, *result_len);
+    if (*result_len == 0) {
+        *result_len = ret;
+        *result = g_new(uint8_t, *result_len);
+    } else if (*result_len != ret) {
+        error_setg(errp,
+                   "Result buffer size %zu is smaller than hash %d",
+                   *result_len, ret);
+        return -1;
+    }
+
     gnutls_hash_output(*ctx, *result);
     return 0;
 }
diff --git a/crypto/hash-nettle.c b/crypto/hash-nettle.c
index 3b847aa60e..c78624b347 100644
--- a/crypto/hash-nettle.c
+++ b/crypto/hash-nettle.c
@@ -150,9 +150,17 @@ int qcrypto_nettle_hash_finalize(QCryptoHash *hash,
                                  Error **errp)
 {
     union qcrypto_hash_ctx *ctx = hash->opaque;
-
-    *result_len = qcrypto_hash_alg_map[hash->alg].len;
-    *result = g_new(uint8_t, *result_len);
+    int ret = qcrypto_hash_alg_map[hash->alg].len;
+
+    if (*result_len == 0) {
+        *result_len = ret;
+        *result = g_new(uint8_t, *result_len);
+    } else if (*result_len != ret) {
+        error_setg(errp,
+                   "Result buffer size %zu is smaller than hash %d",
+                   *result_len, ret);
+        return -1;
+    }
 
     qcrypto_hash_alg_map[hash->alg].result(ctx, *result_len, *result);
 
-- 
2.46.0

[PATCH v2 2/3] tests: correctly validate result buffer in hash/hmac tests

[Daniel P. Berrangé]

Validate that the pre-allocated buffer pointer was not overwritten
by the hash/hmac APIs.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/unit/test-crypto-hash.c | 7 ++++---
 tests/unit/test-crypto-hmac.c | 6 ++++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/unit/test-crypto-hash.c b/tests/unit/test-crypto-hash.c
index e5829ca766..76c4699c15 100644
--- a/tests/unit/test-crypto-hash.c
+++ b/tests/unit/test-crypto-hash.c
@@ -123,7 +123,7 @@ static void test_hash_prealloc(void)
     size_t i;
 
     for (i = 0; i < G_N_ELEMENTS(expected_outputs) ; i++) {
-        uint8_t *result;
+        uint8_t *result, *origresult;
         size_t resultlen;
         int ret;
         size_t j;
@@ -133,7 +133,7 @@ static void test_hash_prealloc(void)
         }
 
         resultlen = expected_lens[i];
-        result = g_new0(uint8_t, resultlen);
+        origresult = result = g_new0(uint8_t, resultlen);
 
         ret = qcrypto_hash_bytes(i,
                                  INPUT_TEXT,
@@ -142,7 +142,8 @@ static void test_hash_prealloc(void)
                                  &resultlen,
                                  &error_fatal);
         g_assert(ret == 0);
-
+        /* Validate that our pre-allocated pointer was not replaced */
+        g_assert(result == origresult);
         g_assert(resultlen == expected_lens[i]);
         for (j = 0; j < resultlen; j++) {
             g_assert(expected_outputs[i][j * 2] == hex[(result[j] >> 4) & 0xf]);
diff --git a/tests/unit/test-crypto-hmac.c b/tests/unit/test-crypto-hmac.c
index 3fa50f24bb..cdb8774443 100644
--- a/tests/unit/test-crypto-hmac.c
+++ b/tests/unit/test-crypto-hmac.c
@@ -126,7 +126,7 @@ static void test_hmac_prealloc(void)
     for (i = 0; i < G_N_ELEMENTS(test_data); i++) {
         QCryptoHmacTestData *data = &test_data[i];
         QCryptoHmac *hmac = NULL;
-        uint8_t *result = NULL;
+        uint8_t *result = NULL, *origresult = NULL;
         size_t resultlen = 0;
         const char *exp_output = NULL;
         int ret;
@@ -139,7 +139,7 @@ static void test_hmac_prealloc(void)
         exp_output = data->hex_digest;
 
         resultlen = strlen(exp_output) / 2;
-        result = g_new0(uint8_t, resultlen);
+        origresult = result = g_new0(uint8_t, resultlen);
 
         hmac = qcrypto_hmac_new(data->alg, (const uint8_t *)KEY,
                                 strlen(KEY), &error_fatal);
@@ -149,6 +149,8 @@ static void test_hmac_prealloc(void)
                                  strlen(INPUT_TEXT), &result,
                                  &resultlen, &error_fatal);
         g_assert(ret == 0);
+        /* Validate that our pre-allocated pointer was not replaced */
+        g_assert(result == origresult);
 
         exp_output = data->hex_digest;
         for (j = 0; j < resultlen; j++) {
-- 
2.46.0

[PATCH v2 3/3] include/crypto: clarify @result/@result_len for hash/hmac APIs

[Daniel P. Berrangé]

The @result parameter passed to hash/hmac APIs may either contain
a pre-allocated buffer, or a buffer can be allocated on the fly.
Clarify these two different usage models in the API docs.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/crypto/hash.h | 30 +++++++++++++++++++++++-------
 include/crypto/hmac.h | 17 ++++++++++++-----
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index b791ca92a4..317bf9ecde 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -98,11 +98,18 @@ int qcrypto_hash_bytesv(QCryptoHashAlgo alg,
  * @errp: pointer to a NULL-initialized error object
  *
  * Computes the hash across all the memory region
- * @buf of length @len. The @result pointer will be
- * filled with raw bytes representing the computed
- * hash, which will have length @resultlen. The
- * memory pointer in @result must be released
- * with a call to g_free() when no longer required.
+ * @buf of length @len.
+ *
+ * If @result_len is set to a non-zero value by the caller, then
+ * @result must hold a pointer that is @result_len in size, and
+ * @result_len match the size of the hash output. The digest will
+ * be written into @result.
+ *
+ * If @result_len is set to zero, then this function will allocate
+ * a buffer to hold the hash output digest, storing a pointer to
+ * the buffer in @result, and setting @result_len to its size.
+ * The memory referenced in @result must be released with a call
+ * to g_free() when no longer required by the caller.
  *
  * Returns: 0 on success, -1 on error
  */
@@ -215,8 +222,17 @@ int qcrypto_hash_finalize_base64(QCryptoHash *hash,
  *
  * Computes the hash from the given hash object. Hash object
  * is expected to have it's data updated from the qcrypto_hash_update function.
- * The memory pointer in @result must be released with a call to g_free()
- * when no longer required.
+ *
+ * If @result_len is set to a non-zero value by the caller, then
+ * @result must hold a pointer that is @result_len in size, and
+ * @result_len match the size of the hash output. The digest will
+ * be written into @result.
+ *
+ * If @result_len is set to zero, then this function will allocate
+ * a buffer to hold the hash output digest, storing a pointer to
+ * the buffer in @result, and setting @result_len to its size.
+ * The memory referenced in @result must be released with a call
+ * to g_free() when no longer required by the caller.
  *
  * Returns: 0 on success, -1 on error
  */
diff --git a/include/crypto/hmac.h b/include/crypto/hmac.h
index c69a0dfab3..aaff95f7db 100644
--- a/include/crypto/hmac.h
+++ b/include/crypto/hmac.h
@@ -103,11 +103,18 @@ int qcrypto_hmac_bytesv(QCryptoHmac *hmac,
  * @errp: pointer to a NULL-initialized error object
  *
  * Computes the hmac across all the memory region
- * @buf of length @len. The @result pointer will be
- * filled with raw bytes representing the computed
- * hmac, which will have length @resultlen. The
- * memory pointer in @result must be released
- * with a call to g_free() when no longer required.
+ * @buf of length @len.
+ *
+ * If @result_len is set to a non-zero value by the caller, then
+ * @result must hold a pointer that is @result_len in size, and
+ * @result_len match the size of the hash output. The digest will
+ * be written into @result.
+ *
+ * If @result_len is set to zero, then this function will allocate
+ * a buffer to hold the hash output digest, storing a pointer to
+ * the buffer in @result, and setting @result_len to its size.
+ * The memory referenced in @result must be released with a call
+ * to g_free() when no longer required by the caller.
  *
  * Returns:
  *  0 on success, -1 on error
-- 
2.46.0

Re: [PATCH v2 0/3] crypto: fix regression in hash result buffer handling

[Dorjoy Chowdhury]

On Wed, Oct 16, 2024 at 4:20 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
>
>
> Daniel P. Berrangé (3):
>   crypto/hash: avoid overwriting user supplied result pointer
>   tests: correctly validate result buffer in hash/hmac tests
>   include/crypto: clarify @result/@result_len for hash/hmac APIs
>
>  crypto/hash-gcrypt.c          | 15 ++++++++++++---
>  crypto/hash-glib.c            | 11 +++++++++--
>  crypto/hash-gnutls.c          | 16 +++++++++++++---
>  crypto/hash-nettle.c          | 14 +++++++++++---
>  include/crypto/hash.h         | 30 +++++++++++++++++++++++-------
>  include/crypto/hmac.h         | 17 ++++++++++++-----
>  tests/unit/test-crypto-hash.c |  7 ++++---
>  tests/unit/test-crypto-hmac.c |  6 ++++--
>  8 files changed, 88 insertions(+), 28 deletions(-)
>

Reviewed-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>


@Daniel the api documentation for "qcrypto_hash_bytesv" also needs to
be updated.

Regards,
Dorjoy

Re: [PATCH v2 0/3] crypto: fix regression in hash result buffer handling

[Daniel P. Berrangé]

On Wed, Oct 16, 2024 at 07:33:56PM +0600, Dorjoy Chowdhury wrote:
> On Wed, Oct 16, 2024 at 4:20 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> >
> >
> > Daniel P. Berrangé (3):
> >   crypto/hash: avoid overwriting user supplied result pointer
> >   tests: correctly validate result buffer in hash/hmac tests
> >   include/crypto: clarify @result/@result_len for hash/hmac APIs
> >
> >  crypto/hash-gcrypt.c          | 15 ++++++++++++---
> >  crypto/hash-glib.c            | 11 +++++++++--
> >  crypto/hash-gnutls.c          | 16 +++++++++++++---
> >  crypto/hash-nettle.c          | 14 +++++++++++---
> >  include/crypto/hash.h         | 30 +++++++++++++++++++++++-------
> >  include/crypto/hmac.h         | 17 ++++++++++++-----
> >  tests/unit/test-crypto-hash.c |  7 ++++---
> >  tests/unit/test-crypto-hmac.c |  6 ++++--
> >  8 files changed, 88 insertions(+), 28 deletions(-)
> >
> 
> Reviewed-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
> 
> 
> @Daniel the api documentation for "qcrypto_hash_bytesv" also needs to
> be updated.

Opps, yes, I'll make exactly the same docs change for that too, and
the equiv hmac  API.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|