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

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

[Daniel P. Berrangé]

Daniel P. Berrangé (2):
  crypto/hash: avoid overwriting user supplied result pointer
  tests: correctly validate result buffer in hash/hmac tests

 crypto/hash-gcrypt.c          | 15 ++++++++++++---
 crypto/hash-glib.c            | 11 +++++++++--
 crypto/hash-gnutls.c          | 16 +++++++++++++---
 crypto/hash-nettle.c          | 14 +++++++++++---
 tests/unit/test-crypto-hash.c |  7 ++++---
 tests/unit/test-crypto-hmac.c |  6 ++++--
 6 files changed, 53 insertions(+), 16 deletions(-)

-- 
2.46.0

[PATCH 1/2] 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 2/2] 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

Re: [PATCH 1/2] crypto/hash: avoid overwriting user supplied result pointer

[Dorjoy Chowdhury]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On Tue, Oct 15, 2024 at 6:56 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> 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(-)
>

Thanks for fixing. These changes look good to me but it might make sense to
update the corresponding api documentation in include/crypto/hash.h to
explicitly mention that *result can be a user supplied buffer with proper
*result_len, if not, allocated inside the function itself. I see a few
places in that file that may be worth updating as part of this commit. What
do you think?

Regards,
Dorjoy

[-- Attachment #2: Type: text/html, Size: 1508 bytes --]