From b16bedf82a6d10d69574a16684db74dd1e66f9f2 Mon Sep 17 00:00:00 2001 From: Daniel Pouzzner Date: Fri, 10 Jan 2025 15:48:05 -0600 Subject: [PATCH] more fixes guided by clang-tidy heap analyzer using clang-20.0.0_pre20250104: wolfcrypt/src/integer.c: add additional guards against OOB access from uint wraps and null derefs of mp_int.dp, and refactor mp_grow() and mp_init_size() to use XMEMSET, for the benefit of clang-tidy. in mp_grow(), fix the condition for the realloc to assure always evaluated if a->alloc == 0. wolfcrypt/src/asn.c: fix wc_CreatePKCS8Key() so that *outSz is always assigned when LENGTH_ONLY_E is returned. wolfcrypt/src/pkcs7.c: remove redundant inner condition in wc_PKCS7_EncodeAuthEnvelopedData(), added in previous commit and caught on review by Jacob (thanks!). wolfcrypt/src/sp_int.c: in sp_mont_norm(), add another suppression for the same false positive in sp_mul() suppressed in previous commit. wolfcrypt/src/srp.c: refactor SrpHashSize() to return ALGO_ID_E rather than 0 when unknown/uncompiled alg is requested. --- wolfcrypt/src/asn.c | 2 +- wolfcrypt/src/integer.c | 31 ++++++++------- wolfcrypt/src/pkcs7.c | 6 +-- wolfcrypt/src/sp_int.c | 5 +++ wolfcrypt/src/srp.c | 85 ++++++++++++++++++++++++++++------------- 5 files changed, 82 insertions(+), 47 deletions(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 4208d4985d..917859e055 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -7429,7 +7429,7 @@ int wc_CreatePKCS8Key(byte* out, word32* outSz, byte* key, word32 keySz, /* Get the size of the DER encoding. */ ret = SizeASN_Items(pkcs8KeyASN, dataASN, pkcs8KeyASN_Length-1, &sz); } - if (ret == 0) { + if ((ret == 0) || (ret == WC_NO_ERR_TRACE(LENGTH_ONLY_E))) { /* Always return the calculated size. */ *outSz = (word32)sz; } diff --git a/wolfcrypt/src/integer.c b/wolfcrypt/src/integer.c index e40afc7213..b1707e1e46 100644 --- a/wolfcrypt/src/integer.c +++ b/wolfcrypt/src/integer.c @@ -409,11 +409,10 @@ int mp_copy (const mp_int * a, mp_int * b) /* grow as required */ int mp_grow (mp_int * a, int size) { - int i; mp_digit *tmp; /* if the alloc size is smaller alloc more ram */ - if (a->alloc < size || size == 0) { + if ((a->alloc < size) || (size == 0) || (a->alloc == 0)) { /* ensure there are always at least MP_PREC digits extra on top */ size += (MP_PREC * 2) - (size % MP_PREC); @@ -434,14 +433,11 @@ int mp_grow (mp_int * a, int size) a->dp = tmp; /* zero excess digits */ - i = a->alloc; + XMEMSET(&a->dp[a->alloc], 0, sizeof (mp_digit) * (size - a->alloc)); a->alloc = size; - for (; i < a->alloc; i++) { - a->dp[i] = 0; - } } - else if ((a->alloc > 0) && (a->dp == NULL)) { - /* opportunistic sanity check on a->dp */ + else if (a->dp == NULL) { + /* opportunistic sanity check for null a->dp with nonzero a->alloc */ return MP_VAL; } return MP_OKAY; @@ -1762,9 +1758,9 @@ int s_mp_add (mp_int * a, mp_int * b, mp_int * c) /* destination */ tmpc = c->dp; - /* sanity-check dp pointers from a and b. */ + /* sanity-check dp pointers. */ if ((min_ab > 0) && - ((tmpa == NULL) || (tmpb == NULL))) + ((tmpa == NULL) || (tmpb == NULL) || (tmpc == NULL))) { return MP_VAL; } @@ -3308,6 +3304,10 @@ int mp_div_3 (mp_int * a, mp_int *c, mp_digit * d) q.used = a->used; q.sign = a->sign; w = 0; + + if (a->used == 0) + return MP_VAL; + for (ix = a->used - 1; ix >= 0; ix--) { w = (w << ((mp_word)DIGIT_BIT)) | ((mp_word)a->dp[ix]); @@ -3350,8 +3350,6 @@ int mp_div_3 (mp_int * a, mp_int *c, mp_digit * d) /* init an mp_init for a given size */ int mp_init_size (mp_int * a, int size) { - int x; - /* pad size so there are always extra digits */ size += (MP_PREC * 2) - (size % MP_PREC); @@ -3371,9 +3369,7 @@ int mp_init_size (mp_int * a, int size) #endif /* zero the digits */ - for (x = 0; x < size; x++) { - a->dp[x] = 0; - } + XMEMSET(a->dp, 0, sizeof (mp_digit) * size); return MP_OKAY; } @@ -4699,8 +4695,11 @@ static int mp_div_d (mp_int * a, mp_digit b, mp_int * c, mp_digit * d) } } - w = 0; + + if (a->used == 0) + return MP_VAL; + for (ix = a->used - 1; ix >= 0; ix--) { w = (w << ((mp_word)DIGIT_BIT)) | ((mp_word)a->dp[ix]); diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index e617787efd..dd244d624c 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -13115,10 +13115,8 @@ int wc_PKCS7_EncodeAuthEnvelopedData(wc_PKCS7* pkcs7, byte* output, if (unauthAttribsSz > 0) { XMEMCPY(output + idx, unauthAttribSet, unauthAttribsSetSz); idx += (int)unauthAttribsSetSz; - if (unauthAttribsSz > 0) { - XMEMCPY(output + idx, flatUnauthAttribs, unauthAttribsSz); - idx += (int)unauthAttribsSz; - } + XMEMCPY(output + idx, flatUnauthAttribs, unauthAttribsSz); + idx += (int)unauthAttribsSz; } XFREE(flatUnauthAttribs, pkcs7->heap, DYNAMIC_TYPE_PKCS7); diff --git a/wolfcrypt/src/sp_int.c b/wolfcrypt/src/sp_int.c index 920a8c613e..8bf6415b54 100644 --- a/wolfcrypt/src/sp_int.c +++ b/wolfcrypt/src/sp_int.c @@ -17982,9 +17982,14 @@ int sp_mont_norm(sp_int* norm, const sp_int* m) if (err == MP_OKAY) { /* Find top bit and ensure norm has enough space. */ bits = (unsigned int)sp_count_bits(m); + /* NOLINTBEGIN(clang-analyzer-core.UndefinedBinaryOperatorResult) */ + /* clang-tidy falsely believes that norm->size was corrupted by the + * _sp_copy() to "Set real working value to base." in _sp_exptmod_ex(). + */ if (bits >= (unsigned int)norm->size * SP_WORD_SIZE) { err = MP_VAL; } + /* NOLINTEND(clang-analyzer-core.UndefinedBinaryOperatorResult) */ } if (err == MP_OKAY) { /* Round up for case when m is less than a word - no advantage in using diff --git a/wolfcrypt/src/srp.c b/wolfcrypt/src/srp.c index b06f62a808..2d4b6cff33 100644 --- a/wolfcrypt/src/srp.c +++ b/wolfcrypt/src/srp.c @@ -152,39 +152,39 @@ static int SrpHashFinal(SrpHash* hash, byte* digest) } } -static word32 SrpHashSize(SrpType type) +static int SrpHashSize(SrpType type) { switch (type) { case SRP_TYPE_SHA: #ifndef NO_SHA return WC_SHA_DIGEST_SIZE; #else - return 0; + return ALGO_ID_E; #endif case SRP_TYPE_SHA256: #ifndef NO_SHA256 return WC_SHA256_DIGEST_SIZE; #else - return 0; + return ALGO_ID_E; #endif case SRP_TYPE_SHA384: #ifdef WOLFSSL_SHA384 return WC_SHA384_DIGEST_SIZE; #else - return 0; + return ALGO_ID_E; #endif case SRP_TYPE_SHA512: #ifdef WOLFSSL_SHA512 return WC_SHA512_DIGEST_SIZE; #else - return 0; + return ALGO_ID_E; #endif default: - return 0; + return ALGO_ID_E; } } @@ -353,7 +353,8 @@ int wc_SrpSetParams(Srp* srp, const byte* N, word32 nSz, byte digest2[SRP_MAX_DIGEST_SIZE]; byte pad = 0; int r; - word32 i, j = 0; + word32 i; + int hashSize = 0; if (!srp || !N || !g || !salt || nSz < gSz) return BAD_FUNC_ARG; @@ -361,6 +362,10 @@ int wc_SrpSetParams(Srp* srp, const byte* N, word32 nSz, if (!srp->user) return SRP_CALL_ORDER_E; + hashSize = SrpHashSize(srp->type); + if (hashSize < 0) + return hashSize; + /* Set N */ if (mp_read_unsigned_bin(&srp->N, N, nSz) != MP_OKAY) return MP_READ_E; @@ -389,7 +394,7 @@ int wc_SrpSetParams(Srp* srp, const byte* N, word32 nSz, srp->saltSz = saltSz; /* Set k = H(N, g) */ - r = SrpHashInit(&hash, srp->type, srp->heap); + r = SrpHashInit(&hash, srp->type, srp->heap); if (!r) r = SrpHashUpdate(&hash, (byte*) N, nSz); for (i = 0; (word32)i < nSz - gSz; i++) { if (!r) r = SrpHashUpdate(&hash, &pad, 1); @@ -414,7 +419,7 @@ int wc_SrpSetParams(Srp* srp, const byte* N, word32 nSz, /* digest1 = H(N) ^ H(g) */ if (r == 0) { - for (i = 0, j = SrpHashSize(srp->type); i < j; i++) + for (i = 0; i < (word32)hashSize; i++) digest1[i] ^= digest2[i]; } @@ -425,8 +430,8 @@ int wc_SrpSetParams(Srp* srp, const byte* N, word32 nSz, SrpHashFree(&hash); /* client proof = H( H(N) ^ H(g) | H(user) | salt) */ - if (!r) r = SrpHashUpdate(&srp->client_proof, digest1, j); - if (!r) r = SrpHashUpdate(&srp->client_proof, digest2, j); + if (!r) r = SrpHashUpdate(&srp->client_proof, digest1, (word32)hashSize); + if (!r) r = SrpHashUpdate(&srp->client_proof, digest2, (word32)hashSize); if (!r) r = SrpHashUpdate(&srp->client_proof, salt, saltSz); return r; @@ -436,7 +441,7 @@ int wc_SrpSetPassword(Srp* srp, const byte* password, word32 size) { SrpHash hash; byte digest[SRP_MAX_DIGEST_SIZE]; - word32 digestSz; + int digestSz; int r; if (!srp || !password || srp->side != SRP_CLIENT_SIDE) @@ -446,6 +451,8 @@ int wc_SrpSetPassword(Srp* srp, const byte* password, word32 size) return SRP_CALL_ORDER_E; digestSz = SrpHashSize(srp->type); + if (digestSz < 0) + return digestSz; /* digest = H(username | ':' | password) */ r = SrpHashInit(&hash, srp->type, srp->heap); @@ -458,12 +465,12 @@ int wc_SrpSetPassword(Srp* srp, const byte* password, word32 size) /* digest = H(salt | H(username | ':' | password)) */ if (!r) r = SrpHashInit(&hash, srp->type, srp->heap); if (!r) r = SrpHashUpdate(&hash, srp->salt, srp->saltSz); - if (!r) r = SrpHashUpdate(&hash, digest, digestSz); + if (!r) r = SrpHashUpdate(&hash, digest, (word32)digestSz); if (!r) r = SrpHashFinal(&hash, digest); SrpHashFree(&hash); /* Set x (private key) */ - if (!r) r = mp_read_unsigned_bin(&srp->auth, digest, digestSz); + if (!r) r = mp_read_unsigned_bin(&srp->auth, digest, (word32)digestSz); ForceZero(digest, SRP_MAX_DIGEST_SIZE); @@ -572,10 +579,15 @@ int wc_SrpGetPublic(Srp* srp, byte* pub, word32* size) #endif word32 modulusSz; int r; + int hashSize; if (!srp || !pub || !size) return BAD_FUNC_ARG; + hashSize = SrpHashSize(srp->type); + if (hashSize < 0) + return hashSize; + if (mp_iszero(&srp->auth) == MP_YES) return SRP_CALL_ORDER_E; @@ -616,7 +628,7 @@ int wc_SrpGetPublic(Srp* srp, byte* pub, word32* size) { r = mp_init_multi(i, j, 0, 0, 0, 0); } - if (!r) r = mp_read_unsigned_bin(i, srp->k,SrpHashSize(srp->type)); + if (!r) r = mp_read_unsigned_bin(i, srp->k, (word32)hashSize); if (!r) r = mp_iszero(i) == MP_YES ? SRP_BAD_KEY_E : 0; if (!r) r = mp_exptmod(&srp->g, &srp->priv, &srp->N, pubkey); if (!r) r = mp_mulmod(i, &srp->auth, &srp->N, j); @@ -654,17 +666,22 @@ static int wc_SrpSetKey(Srp* srp, byte* secret, word32 size) { SrpHash hash; byte digest[SRP_MAX_DIGEST_SIZE]; - word32 i, j, digestSz = SrpHashSize(srp->type); + word32 i, j; + int digestSz; byte counter[4]; int r = WC_NO_ERR_TRACE(BAD_FUNC_ARG); + digestSz = SrpHashSize(srp->type); + if (digestSz < 0) + return digestSz; + XMEMSET(digest, 0, SRP_MAX_DIGEST_SIZE); - srp->key = (byte*)XMALLOC(2 * digestSz, srp->heap, DYNAMIC_TYPE_SRP); + srp->key = (byte*)XMALLOC(2 * (word32)digestSz, srp->heap, DYNAMIC_TYPE_SRP); if (srp->key == NULL) return MEMORY_E; - srp->keySz = 2 * digestSz; + srp->keySz = 2 * (word32)digestSz; for (i = j = 0; j < srp->keySz; i++) { counter[0] = (byte)(i >> 24); @@ -677,7 +694,7 @@ static int wc_SrpSetKey(Srp* srp, byte* secret, word32 size) if (!r) r = SrpHashUpdate(&hash, counter, 4); if (!r) { - if (j + digestSz > srp->keySz) { + if (j + (word32)digestSz > srp->keySz) { r = SrpHashFinal(&hash, digest); XMEMCPY(srp->key + j, digest, srp->keySz - j); j = srp->keySz; @@ -685,7 +702,7 @@ static int wc_SrpSetKey(Srp* srp, byte* secret, word32 size) else { r = SrpHashFinal(&hash, srp->key + j); - j += digestSz; + j += (word32)digestSz; } } SrpHashFree(&hash); @@ -715,7 +732,8 @@ int wc_SrpComputeKey(Srp* srp, byte* clientPubKey, word32 clientPubKeySz, mp_int u[1], s[1], temp1[1], temp2[1]; #endif byte *secret = NULL; - word32 i, secretSz, digestSz; + word32 i, secretSz; + int digestSz; byte pad = 0; int r; @@ -761,6 +779,11 @@ int wc_SrpComputeKey(Srp* srp, byte* clientPubKey, word32 clientPubKeySz, goto out; digestSz = SrpHashSize(srp->type); + if (digestSz < 0) { + r = digestSz; + goto out; + } + secretSz = (word32)mp_unsigned_bin_size(&srp->N); if ((secretSz < clientPubKeySz) || (secretSz < serverPubKeySz)) { @@ -795,7 +818,7 @@ int wc_SrpComputeKey(Srp* srp, byte* clientPubKey, word32 clientPubKeySz, /* set u */ if ((r = SrpHashFinal(hash, digest))) goto out; - if ((r = mp_read_unsigned_bin(u, digest, SrpHashSize(srp->type)))) + if ((r = mp_read_unsigned_bin(u, digest, (word32)digestSz))) goto out; SrpHashFree(hash); @@ -804,7 +827,7 @@ int wc_SrpComputeKey(Srp* srp, byte* clientPubKey, word32 clientPubKeySz, if (srp->side == SRP_CLIENT_SIDE) { /* temp1 = B - k * v; rejects k == 0, B == 0 and B >= N. */ - if ((r = mp_read_unsigned_bin(temp1, srp->k, digestSz))) + if ((r = mp_read_unsigned_bin(temp1, srp->k, (word32)digestSz))) goto out; if (mp_iszero(temp1) == MP_YES) { r = SRP_BAD_KEY_E; @@ -940,11 +963,16 @@ int wc_SrpComputeKey(Srp* srp, byte* clientPubKey, word32 clientPubKeySz, int wc_SrpGetProof(Srp* srp, byte* proof, word32* size) { int r; + int hashSize; if (!srp || !proof || !size) return BAD_FUNC_ARG; - if (*size < SrpHashSize(srp->type)) + hashSize = SrpHashSize(srp->type); + if (hashSize < 0) + return ALGO_ID_E; + + if (*size < (word32)hashSize) return BUFFER_E; if ((r = SrpHashFinal(srp->side == SRP_CLIENT_SIDE @@ -952,7 +980,7 @@ int wc_SrpGetProof(Srp* srp, byte* proof, word32* size) : &srp->server_proof, proof)) != 0) return r; - *size = SrpHashSize(srp->type); + *size = (word32)hashSize; if (srp->side == SRP_CLIENT_SIDE) { /* server proof = H( A | client proof | K) */ @@ -967,11 +995,16 @@ int wc_SrpVerifyPeersProof(Srp* srp, byte* proof, word32 size) { byte digest[SRP_MAX_DIGEST_SIZE]; int r; + int hashSize; if (!srp || !proof) return BAD_FUNC_ARG; - if (size != SrpHashSize(srp->type)) + hashSize = SrpHashSize(srp->type); + if (hashSize < 0) + return ALGO_ID_E; + + if (size != (word32)hashSize) return BUFFER_E; r = SrpHashFinal(srp->side == SRP_CLIENT_SIDE ? &srp->server_proof