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é <[email protected]> --- 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
