[PATCH 0/3] crypto: robustness fixes to handling of SM3 hash

[PATCH 0/3] crypto: robustness fixes to handling of SM3 hash

[Daniel P. Berrangé]

Daniel P. Berrangé (3):
  crypto: add glib initialization for SM3 hash
  crypto: unconditionally report SM3 hash length
  crypto: add preventative measures wrt uninitialized hash info

 crypto/hash-glib.c | 19 +++++++++++++++++--
 crypto/hash.c      | 16 ++++++++++++----
 crypto/hmac-glib.c | 19 +++++++++++++++++--
 3 files changed, 46 insertions(+), 8 deletions(-)

-- 
2.56.0

[PATCH 1/3] crypto: add glib initialization for SM3 hash

[Daniel P. Berrangé]

The glib backend for the hmac/hash code must have an explicit
initializer for every element, as the default static initializer
of zero maps to G_CHECKSUM_MD5 and will lead to incorrect
behaviour. This could potentially be a security through use of
the wrong hashing algorithm.

Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/hash-glib.c | 1 +
 crypto/hmac-glib.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
index 809cef98aee..39040f1856e 100644
--- a/crypto/hash-glib.c
+++ b/crypto/hash-glib.c
@@ -33,6 +33,7 @@ static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_SHA384] = G_CHECKSUM_SHA384,
     [QCRYPTO_HASH_ALGO_SHA512] = G_CHECKSUM_SHA512,
     [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
+    [QCRYPTO_HASH_ALGO_SM3] = -1,
 };
 
 gboolean qcrypto_hash_supports(QCryptoHashAlgo alg)
diff --git a/crypto/hmac-glib.c b/crypto/hmac-glib.c
index 1f17769c1c7..8f652c5064e 100644
--- a/crypto/hmac-glib.c
+++ b/crypto/hmac-glib.c
@@ -25,6 +25,7 @@ static int qcrypto_hmac_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_SHA224] = -1,
     [QCRYPTO_HASH_ALGO_SHA384] = -1,
     [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
+    [QCRYPTO_HASH_ALGO_SM3] = -1,
 };
 
 typedef struct QCryptoHmacGlib QCryptoHmacGlib;
-- 
2.56.0

[PATCH 2/3] crypto: unconditionally report SM3 hash length

[Daniel P. Berrangé]

Callers may query the hash digest length before getting to the
code path that performs hashing, which is where the lack of
support for SM3 in the backend will be reported.

Reporting a hash length of 0 is liable to cause divide by zero
errors in callers.

Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/hash.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/crypto/hash.c b/crypto/hash.c
index 6ffb88bf541..42a0a8ec56f 100644
--- a/crypto/hash.c
+++ b/crypto/hash.c
@@ -33,9 +33,7 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_SHA384]    = QCRYPTO_HASH_DIGEST_LEN_SHA384,
     [QCRYPTO_HASH_ALGO_SHA512]    = QCRYPTO_HASH_DIGEST_LEN_SHA512,
     [QCRYPTO_HASH_ALGO_RIPEMD160] = QCRYPTO_HASH_DIGEST_LEN_RIPEMD160,
-#ifdef CONFIG_CRYPTO_SM3
-    [QCRYPTO_HASH_ALGO_SM3] = QCRYPTO_HASH_DIGEST_LEN_SM3,
-#endif
+    [QCRYPTO_HASH_ALGO_SM3]       = QCRYPTO_HASH_DIGEST_LEN_SM3,
 };
 
 size_t qcrypto_hash_digest_len(QCryptoHashAlgo alg)
-- 
2.56.0

[PATCH 3/3] crypto: add preventative measures wrt uninitialized hash info

[Daniel P. Berrangé]

Several static arrays can create incorrect behaviour if a new hash
algorithm is added without them being explicitly initialized. Be
more robust against this problem by using a static assert for the
array size, instead of predeclaring the array size.

When querying the arrays also add further sanity checks for missing
initializers.

Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/hash-glib.c | 18 ++++++++++++++++--
 crypto/hash.c      | 12 +++++++++++-
 crypto/hmac-glib.c | 18 ++++++++++++++++--
 3 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
index 39040f1856e..d83caec8cab 100644
--- a/crypto/hash-glib.c
+++ b/crypto/hash-glib.c
@@ -25,7 +25,7 @@
 #include "hashpriv.h"
 
 
-static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
+static int qcrypto_hash_alg_map[] = {
     [QCRYPTO_HASH_ALGO_MD5] = G_CHECKSUM_MD5,
     [QCRYPTO_HASH_ALGO_SHA1] = G_CHECKSUM_SHA1,
     [QCRYPTO_HASH_ALGO_SHA224] = -1,
@@ -35,11 +35,25 @@ static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
     [QCRYPTO_HASH_ALGO_SM3] = -1,
 };
+/*
+ * Prefer the assert over declaring the array with
+ * size QCRYPTO_HASH_ALGO__MAX so we get better
+ * detection of uninitialize elements.
+ */
+G_STATIC_ASSERT(G_N_ELEMENTS(qcrypto_hash_alg_map) ==
+                QCRYPTO_HASH_ALGO__MAX);
 
 gboolean qcrypto_hash_supports(QCryptoHashAlgo alg)
 {
+    /*
+     * The default static initializer value of 0
+     * matches G_CHECKSUM_MD5, so protect against
+     * accidentally uninitialized fields
+     */
     if (alg < G_N_ELEMENTS(qcrypto_hash_alg_map) &&
-        qcrypto_hash_alg_map[alg] != -1) {
+        qcrypto_hash_alg_map[alg] != -1 &&
+        (alg == QCRYPTO_HASH_ALGO_MD5 ||
+         qcrypto_hash_alg_map[alg] != G_CHECKSUM_MD5)) {
         return true;
     }
     return false;
diff --git a/crypto/hash.c b/crypto/hash.c
index 42a0a8ec56f..e966fc1a633 100644
--- a/crypto/hash.c
+++ b/crypto/hash.c
@@ -25,7 +25,7 @@
 #include "crypto/hash.h"
 #include "hashpriv.h"
 
-static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALGO__MAX] = {
+static size_t qcrypto_hash_alg_size[] = {
     [QCRYPTO_HASH_ALGO_MD5]       = QCRYPTO_HASH_DIGEST_LEN_MD5,
     [QCRYPTO_HASH_ALGO_SHA1]      = QCRYPTO_HASH_DIGEST_LEN_SHA1,
     [QCRYPTO_HASH_ALGO_SHA224]    = QCRYPTO_HASH_DIGEST_LEN_SHA224,
@@ -35,10 +35,20 @@ static size_t qcrypto_hash_alg_size[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_RIPEMD160] = QCRYPTO_HASH_DIGEST_LEN_RIPEMD160,
     [QCRYPTO_HASH_ALGO_SM3]       = QCRYPTO_HASH_DIGEST_LEN_SM3,
 };
+/*
+ * Prefer the assert over declaring the array with
+ * size QCRYPTO_HASH_ALGO__MAX so we get better
+ * detection of uninitialized elements. We don't
+ * want to ever return digest lengths of zero, as
+ * that risks a div-by-zero in callers.
+ */
+G_STATIC_ASSERT(G_N_ELEMENTS(qcrypto_hash_alg_size) ==
+                QCRYPTO_HASH_ALGO__MAX);
 
 size_t qcrypto_hash_digest_len(QCryptoHashAlgo alg)
 {
     assert(alg < G_N_ELEMENTS(qcrypto_hash_alg_size));
+    assert(qcrypto_hash_alg_size[alg] != 0);
     return qcrypto_hash_alg_size[alg];
 }
 
diff --git a/crypto/hmac-glib.c b/crypto/hmac-glib.c
index 8f652c5064e..34090444083 100644
--- a/crypto/hmac-glib.c
+++ b/crypto/hmac-glib.c
@@ -17,7 +17,7 @@
 #include "crypto/hmac.h"
 #include "hmacpriv.h"
 
-static int qcrypto_hmac_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
+static int qcrypto_hmac_alg_map[] = {
     [QCRYPTO_HASH_ALGO_MD5] = G_CHECKSUM_MD5,
     [QCRYPTO_HASH_ALGO_SHA1] = G_CHECKSUM_SHA1,
     [QCRYPTO_HASH_ALGO_SHA256] = G_CHECKSUM_SHA256,
@@ -27,6 +27,13 @@ static int qcrypto_hmac_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
     [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
     [QCRYPTO_HASH_ALGO_SM3] = -1,
 };
+/*
+ * Prefer the assert over declaring the array with
+ * size QCRYPTO_HASH_ALGO__MAX so we get better
+ * detection of uninitialize elements.
+ */
+G_STATIC_ASSERT(G_N_ELEMENTS(qcrypto_hmac_alg_map) ==
+                QCRYPTO_HASH_ALGO__MAX);
 
 typedef struct QCryptoHmacGlib QCryptoHmacGlib;
 struct QCryptoHmacGlib {
@@ -35,8 +42,15 @@ struct QCryptoHmacGlib {
 
 bool qcrypto_hmac_supports(QCryptoHashAlgo alg)
 {
+    /*
+     * The default static initializer value of 0
+     * matches G_CHECKSUM_MD5, so protect against
+     * accidentally uninitialized fields
+     */
     if (alg < G_N_ELEMENTS(qcrypto_hmac_alg_map) &&
-        qcrypto_hmac_alg_map[alg] != -1) {
+        qcrypto_hmac_alg_map[alg] != -1 &&
+        (alg == QCRYPTO_HASH_ALGO_MD5 ||
+         qcrypto_hmac_alg_map[alg] != G_CHECKSUM_MD5)) {
         return true;
     }
 
-- 
2.56.0

Re: [PATCH 1/3] crypto: add glib initialization for SM3 hash

[Philippe Mathieu-Daudé]

On 30/10/26 16:04, Daniel P. Berrangé wrote:
> The glib backend for the hmac/hash code must have an explicit
> initializer for every element, as the default static initializer
> of zero maps to G_CHECKSUM_MD5 and will lead to incorrect
> behaviour. This could potentially be a security through use of
> the wrong hashing algorithm.
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   crypto/hash-glib.c | 1 +
>   crypto/hmac-glib.c | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
> index 809cef98aee..39040f1856e 100644
> --- a/crypto/hash-glib.c
> +++ b/crypto/hash-glib.c
> @@ -33,6 +33,7 @@ static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
>       [QCRYPTO_HASH_ALGO_SHA384] = G_CHECKSUM_SHA384,
>       [QCRYPTO_HASH_ALGO_SHA512] = G_CHECKSUM_SHA512,
>       [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
> +    [QCRYPTO_HASH_ALGO_SM3] = -1,
>   };

Should we initialize the array with range designator to prevent that?

   [0 ... QCRYPTO_HASH_ALGO__MAX - 1] = -1,

Re: [PATCH 1/3] crypto: add glib initialization for SM3 hash

[Daniel P. Berrangé]

On Fri, Oct 30, 2026 at 05:16:31PM +0100, Philippe Mathieu-Daudé wrote:
> On 30/10/26 16:04, Daniel P. Berrangé wrote:
> > The glib backend for the hmac/hash code must have an explicit
> > initializer for every element, as the default static initializer
> > of zero maps to G_CHECKSUM_MD5 and will lead to incorrect
> > behaviour. This could potentially be a security through use of
> > the wrong hashing algorithm.
> > 
> > Fixes: https://gitlab.com/qemu-project/qemu/-/work_items/4786
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   crypto/hash-glib.c | 1 +
> >   crypto/hmac-glib.c | 1 +
> >   2 files changed, 2 insertions(+)
> > 
> > diff --git a/crypto/hash-glib.c b/crypto/hash-glib.c
> > index 809cef98aee..39040f1856e 100644
> > --- a/crypto/hash-glib.c
> > +++ b/crypto/hash-glib.c
> > @@ -33,6 +33,7 @@ static int qcrypto_hash_alg_map[QCRYPTO_HASH_ALGO__MAX] = {
> >       [QCRYPTO_HASH_ALGO_SHA384] = G_CHECKSUM_SHA384,
> >       [QCRYPTO_HASH_ALGO_SHA512] = G_CHECKSUM_SHA512,
> >       [QCRYPTO_HASH_ALGO_RIPEMD160] = -1,
> > +    [QCRYPTO_HASH_ALGO_SM3] = -1,
> >   };
> 
> Should we initialize the array with range designator to prevent that?
> 
>   [0 ... QCRYPTO_HASH_ALGO__MAX - 1] = -1,

Oh, I didn't know that syntax exists.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|