Skip to content

Commit

Permalink
more fixes guided by clang-tidy heap analyzer using clang-20.0.0_pre2…
Browse files Browse the repository at this point in the history 
…0250104:

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.
  • Loading branch information
@douzzer
douzzer committed Jan 10, 2025
1 parent 7cd2fd3 commit 72df8d8
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 34 deletions.
2 changes: 1 addition & 1 deletion wolfcrypt/src/asn.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
31 changes: 15 additions & 16 deletions wolfcrypt/src/integer.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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]);

Expand Down Expand Up @@ -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);

Expand All @@ -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;
}
Expand Down Expand Up @@ -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]);

Expand Down
6 changes: 2 additions & 4 deletions wolfcrypt/src/pkcs7.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 5 additions & 0 deletions wolfcrypt/src/sp_int.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
41 changes: 28 additions & 13 deletions wolfcrypt/src/srp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -353,14 +353,19 @@ 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;

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;
Expand Down Expand Up @@ -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);
Expand All @@ -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];
}

Expand All @@ -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;
Expand Down Expand Up @@ -940,11 +945,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
Expand All @@ -967,11 +977,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
Expand Down

0 comments on commit 72df8d8

Please sign in to comment.