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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/2] crypto: fix regression in hash result buffer handling
@ 2024-10-15 12:56 Daniel P. Berrangé
  2024-10-15 12:56 ` [PATCH 1/2] crypto/hash: avoid overwriting user supplied result pointer Daniel P. Berrangé
  2024-10-15 12:56 ` [PATCH 2/2] tests: correctly validate result buffer in hash/hmac tests Daniel P. Berrangé
  0 siblings, 2 replies; 4+ messages in thread
From: Daniel P. Berrangé @ 2024-10-15 12:56 UTC (permalink / raw)
  To: qemu-devel; +Cc: 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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] crypto/hash: avoid overwriting user supplied result pointer
  2024-10-15 12:56 [PATCH 0/2] crypto: fix regression in hash result buffer handling Daniel P. Berrangé
@ 2024-10-15 12:56 ` Daniel P. Berrangé
  2024-10-15 14:13   ` Dorjoy Chowdhury
  2024-10-15 12:56 ` [PATCH 2/2] tests: correctly validate result buffer in hash/hmac tests Daniel P. Berrangé
  1 sibling, 1 reply; 4+ messages in thread
From: Daniel P. Berrangé @ 2024-10-15 12:56 UTC (permalink / raw)
  To: qemu-devel; +Cc: Daniel P. Berrangé, Dorjoy Chowdhury

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



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] crypto/hash: avoid overwriting user supplied result pointer
  2024-10-15 12:56 ` [PATCH 1/2] crypto/hash: avoid overwriting user supplied result pointer Daniel P. Berrangé
@ 2024-10-15 14:13   ` Dorjoy Chowdhury
  0 siblings, 0 replies; 4+ messages in thread
From: Dorjoy Chowdhury @ 2024-10-15 14:13 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: qemu-devel

[-- 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 --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] tests: correctly validate result buffer in hash/hmac tests
  2024-10-15 12:56 [PATCH 0/2] crypto: fix regression in hash result buffer handling Daniel P. Berrangé
  2024-10-15 12:56 ` [PATCH 1/2] crypto/hash: avoid overwriting user supplied result pointer Daniel P. Berrangé
@ 2024-10-15 12:56 ` Daniel P. Berrangé
  1 sibling, 0 replies; 4+ messages in thread
From: Daniel P. Berrangé @ 2024-10-15 12:56 UTC (permalink / raw)
  To: qemu-devel; +Cc: 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



^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-10-15 14:13 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-15 12:56 [PATCH 0/2] crypto: fix regression in hash result buffer handling Daniel P. Berrangé
2024-10-15 12:56 ` [PATCH 1/2] crypto/hash: avoid overwriting user supplied result pointer Daniel P. Berrangé
2024-10-15 14:13   ` Dorjoy Chowdhury
2024-10-15 12:56 ` [PATCH 2/2] tests: correctly validate result buffer in hash/hmac tests Daniel P. Berrangé

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.