From fea7a89b8688a03ccea3ae5b80070010305d88c6 Mon Sep 17 00:00:00 2001 From: Sean Parkinson Date: Wed, 10 Jul 2024 11:07:46 +1000 Subject: [PATCH] Coverity fixes pk.c: EncryptDerKey - setting wrong ret value on allocation failure. wolfssl_rsa_generate_key_native - now checks e is a valid long before passing in. Fix formatting. ssl_load.c: ProcessBufferPrivPkcs8Dec - now checking password is not NULL before zeroizing. Allocation may fail and ForceZero doesn't check for NULL. Fix formatting. tests/api.c: test_RsaSigFailure_cm - Check cert_sz is greater than zero before use. send_new_session_ticket - assert that building the message doesn't return error or 0. test_ticket_nonce_malloc - fix setting of medium and big to use preprocessor. Fix big to be medium + 20. asn.c: GetLength_ex - Fix type of bytes so that it can go negative. sp_int.h: sp_clamp - add one to ii while it is a signed. Fix formatting. --- src/pk.c | 12 ++++++++---- src/ssl_load.c | 11 ++++++++--- tests/api.c | 15 ++++++++++++--- wolfcrypt/src/asn.c | 7 ++++--- wolfssl/wolfcrypt/sp_int.h | 14 +++++++------- 5 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/pk.c b/src/pk.c index db281f600..1ec9a00ce 100644 --- a/src/pk.c +++ b/src/pk.c @@ -376,7 +376,7 @@ int EncryptDerKey(byte *der, int *derSz, const EVP_CIPHER* cipher, DYNAMIC_TYPE_ENCRYPTEDINFO); if (info == NULL) { WOLFSSL_MSG("malloc failed"); - ret = 0; + ret = MEMORY_E; } } #endif @@ -417,7 +417,8 @@ int EncryptDerKey(byte *der, int *derSz, const EVP_CIPHER* cipher, (*derSz) += (int)paddingSz; /* Encrypt DER buffer. */ - ret = wc_BufferKeyEncrypt(info, der, (word32)*derSz, passwd, passwdSz, WC_MD5); + ret = wc_BufferKeyEncrypt(info, der, (word32)*derSz, passwd, passwdSz, + WC_MD5); if (ret != 0) { WOLFSSL_MSG("encrypt key failed"); } @@ -3273,6 +3274,7 @@ static int wolfssl_rsa_generate_key_native(WOLFSSL_RSA* rsa, int bits, #endif int initTmpRng = 0; WC_RNG* rng = NULL; + long en; #endif (void)cb; @@ -3286,10 +3288,12 @@ static int wolfssl_rsa_generate_key_native(WOLFSSL_RSA* rsa, int bits, /* Something went wrong so return memory error. */ ret = MEMORY_E; } + if ((ret == 0) && ((en = (long)wolfSSL_BN_get_word(e)) <= 0)) { + ret = BAD_FUNC_ARG; + } if (ret == 0) { /* Generate an RSA key. */ - ret = wc_MakeRsaKey((RsaKey*)rsa->internal, bits, - (long)wolfSSL_BN_get_word(e), rng); + ret = wc_MakeRsaKey((RsaKey*)rsa->internal, bits, en, rng); if (ret != MP_OKAY) { WOLFSSL_ERROR_MSG("wc_MakeRsaKey failed"); } diff --git a/src/ssl_load.c b/src/ssl_load.c index 2441d485e..60eb72167 100644 --- a/src/ssl_load.c +++ b/src/ssl_load.c @@ -1227,8 +1227,13 @@ static int ProcessBufferPrivPkcs8Dec(EncryptedInfo* info, DerBuffer* der, der->length = (word32)ret; } - /* Ensure password is zeroized. */ - ForceZero(password, (word32)passwordSz); +#ifdef WOLFSSL_SMALL_STACK + if (password != NULL) +#endif + { + /* Ensure password is zeroized. */ + ForceZero(password, (word32)passwordSz); + } #ifdef WOLFSSL_SMALL_STACK /* Dispose of password memory. */ XFREE(password, heap, DYNAMIC_TYPE_STRING); @@ -5563,7 +5568,7 @@ long wolfSSL_CTX_set_tmp_dh(WOLFSSL_CTX* ctx, WOLFSSL_DH* dh) ret = wolfssl_ctx_set_tmp_dh(ctx, p, pSz, g, gSz); } - if (ret != 1 && ctx != NULL) { + if ((ret != 1) && (ctx != NULL)) { /* Free the allocated buffers if not assigned into SSL. */ XFREE(p, ctx->heap, DYNAMIC_TYPE_PUBLIC_KEY); XFREE(g, ctx->heap, DYNAMIC_TYPE_PUBLIC_KEY); diff --git a/tests/api.c b/tests/api.c index 2e23ea1fb..c6ec19da9 100644 --- a/tests/api.c +++ b/tests/api.c @@ -65624,7 +65624,7 @@ static int test_RsaSigFailure_cm(void) size_t cert_sz = 0; ExpectIntEQ(load_file(server_cert, &cert_buf, &cert_sz), 0); - if (cert_buf != NULL) { + if ((cert_buf != NULL) && (cert_sz > 0)) { /* corrupt DER - invert last byte, which is signature */ cert_buf[cert_sz-1] = ~cert_buf[cert_sz-1]; /* test bad cert */ @@ -77618,6 +77618,7 @@ static int send_new_session_ticket(WOLFSSL *ssl, byte nonceLength, byte filler) sz = BuildTls13Message(ssl, buf, 2048, buf+5, idx - 5, handshake, 0, 0, 0); + AssertIntGT(sz, 0); test_ctx = (struct test_memio_ctx*)wolfSSL_GetIOWriteCtx(ssl); AssertNotNull(test_ctx); ret = test_memio_write_cb(ssl, (char*)buf, sz, test_ctx); @@ -77716,8 +77717,16 @@ static int test_ticket_nonce_malloc(void) } small = TLS13_TICKET_NONCE_STATIC_SZ; - medium = small + 20 <= 255 ? small + 20 : 255; - big = medium + 20 <= 255 ? small + 20 : 255; +#if TLS13_TICKET_NONCE_STATIC_SZ + 20 <= 255 + medium = small + 20; +#else + medium = 255; +#endif +#if TLS13_TICKET_NONCE_STATIC_SZ + 20 + 20 <= 255 + big = small + 20; +#else + big = 255; +#endif ExpectIntEQ(test_ticket_nonce_malloc_do(ssl_s, ssl_c, small), TEST_SUCCESS); ExpectPtrEq(ssl_c->session->ticketNonce.data, diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 778d3e70f..6a557b069 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -2285,7 +2285,7 @@ int GetLength_ex(const byte* input, word32* inOutIdx, int* len, word32 maxIdx, /* Bottom 7 bits are the number of bytes to calculate length with. * Note: 0 indicates indefinite length encoding *not* 0 bytes of length. */ - word32 bytes = (word32)b & 0x7FU; + int bytes = (int)(b & 0x7F); int minLen; /* Calculate minimum length to be encoded with bytes. */ @@ -2297,10 +2297,11 @@ int GetLength_ex(const byte* input, word32* inOutIdx, int* len, word32 maxIdx, minLen = 0x80; } /* Only support up to the number of bytes that fit into return var. */ - else if (bytes > sizeof(length)) { + else if (bytes > (int)sizeof(length)) { WOLFSSL_MSG("GetLength - overlong data length spec"); return ASN_PARSE_E; - } else { + } + else { minLen = 1 << ((bytes - 1) * 8); } diff --git a/wolfssl/wolfcrypt/sp_int.h b/wolfssl/wolfcrypt/sp_int.h index ba1689561..75e735f62 100644 --- a/wolfssl/wolfcrypt/sp_int.h +++ b/wolfssl/wolfcrypt/sp_int.h @@ -692,14 +692,14 @@ typedef struct sp_ecc_ctx { * * @param [in] a SP integer to update. */ -#define sp_clamp(a) \ - do { \ - int ii; \ - if ((a)->used > 0) { \ +#define sp_clamp(a) \ + do { \ + int ii; \ + if ((a)->used > 0) { \ for (ii = (int)(a)->used - 1; ii >= 0 && (a)->dp[ii] == 0; ii--) { \ - } \ - (a)->used = (unsigned int)ii + 1; \ - } \ + } \ + (a)->used = (unsigned int)(ii + 1); \ + } \ } while (0) /* Check the compiled and linked math implementation are the same.