From e8cf4b5ff0eab395cdfd8280476b7389faac5a86 Mon Sep 17 00:00:00 2001 From: David Garske Date: Mon, 8 May 2017 12:49:38 -0700 Subject: [PATCH] Coverity fixes for TLS 1.3, async, small stack and normal math. --- examples/client/client.c | 1 + examples/echoserver/echoserver.c | 2 +- src/internal.c | 28 +++----- src/ssl.c | 9 +-- src/tls13.c | 109 +++++++++++++++++++++---------- wolfcrypt/benchmark/benchmark.c | 3 +- wolfcrypt/src/asn.c | 7 +- wolfcrypt/src/ecc.c | 2 + wolfcrypt/src/hmac.c | 7 +- wolfcrypt/src/integer.c | 6 +- wolfcrypt/src/rsa.c | 33 ++++++---- 11 files changed, 131 insertions(+), 76 deletions(-) diff --git a/examples/client/client.c b/examples/client/client.c index d6b2b2b98..453bcf846 100644 --- a/examples/client/client.c +++ b/examples/client/client.c @@ -1056,6 +1056,7 @@ THREAD_RETURN WOLFSSL_THREAD client_test(void* args) #ifdef WOLFSSL_TLS13 updateKeysIVs = 1; #endif + break; case 'y' : #if defined(WOLFSSL_TLS13) && !defined(NO_DH) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index efbab5276..7c1d126d3 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -392,7 +392,7 @@ THREAD_RETURN CYASSL_THREAD echoserver_test(void* args) err = 0; /* reset error */ ret = CyaSSL_write(write_ssl, command, echoSz); if (ret <= 0) { - err = CyaSSL_get_error(ssl, 0); + err = CyaSSL_get_error(write_ssl, 0); #ifdef WOLFSSL_ASYNC_CRYPT if (err == WC_PENDING_E) { ret = wolfSSL_AsyncPoll(write_ssl, WOLF_POLL_FLAG_CHECK_HW); diff --git a/src/internal.c b/src/internal.c index 25fa2ac48..616ed75a8 100755 --- a/src/internal.c +++ b/src/internal.c @@ -5837,25 +5837,23 @@ static int BuildFinished(WOLFSSL* ssl, Hashes* hashes, const byte* sender) int ret = 0; #ifdef WOLFSSL_SHA384 #ifdef WOLFSSL_SMALL_STACK - Sha384* sha384 = (Sha384*)XMALLOC(sizeof(Sha384), ssl->heap, - DYNAMIC_TYPE_TMP_BUFFER); + Sha384* sha384; #else Sha384 sha384[1]; #endif /* WOLFSSL_SMALL_STACK */ #endif /* WOLFSSL_SHA384 */ + if (ssl == NULL) + return BAD_FUNC_ARG; + +#ifdef WOLFSSL_SHA384 #ifdef WOLFSSL_SMALL_STACK - if (ssl == NULL - #ifdef WOLFSSL_SHA384 - || sha384 == NULL - #endif - ) { - #ifdef WOLFSSL_SHA384 - XFREE(sha384, ssl->heap, DYNAMIC_TYPE_TMP_BUFFER); - #endif + sha384 = (Sha384*)XMALLOC(sizeof(Sha384), ssl->heap, + DYNAMIC_TYPE_TMP_BUFFER); + if (sha384 == NULL) return MEMORY_E; - } -#endif +#endif /* WOLFSSL_SMALL_STACK */ +#endif /* WOLFSSL_SHA384 */ /* store current states, building requires get_digest which resets state */ #ifdef WOLFSSL_SHA384 @@ -11280,12 +11278,6 @@ int BuildMessage(WOLFSSL* ssl, byte* output, int outSz, const byte* input, } #endif - /* catch mistaken sizeOnly parameter */ - if (sizeOnly && (output || input) ) { - WOLFSSL_MSG("BuildMessage with sizeOnly doesn't need input or output"); - return BAD_FUNC_ARG; - } - ret = WC_NOT_PENDING_E; #ifdef WOLFSSL_ASYNC_CRYPT if (asyncOkay) { diff --git a/src/ssl.c b/src/ssl.c index efe32125b..bfb41d5be 100755 --- a/src/ssl.c +++ b/src/ssl.c @@ -5052,9 +5052,10 @@ int wolfSSL_CertManagerVerifyBuffer(WOLFSSL_CERT_MANAGER* cm, const byte* buff, ret = PemToDer(buff, sz, CERT_TYPE, &der, cm->heap, info, &eccKey); if (ret != 0) { FreeDer(&der); - #ifdef WOLFSSL_SMALL_STACK - XFREE(info, cm->heap, DYNAMIC_TYPE_TMP_BUFFER); - #endif + #ifdef WOLFSSL_SMALL_STACK + XFREE(cert, cm->heap, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(info, cm->heap, DYNAMIC_TYPE_TMP_BUFFER); + #endif return ret; } InitDecodedCert(cert, der->buffer, der->length, cm->heap); @@ -5452,7 +5453,7 @@ int wolfSSL_CTX_load_verify_locations(WOLFSSL_CTX* ctx, const char* file, ReadDirCtx* readCtx = NULL; readCtx = (ReadDirCtx*)XMALLOC(sizeof(ReadDirCtx), ctx->heap, DYNAMIC_TYPE_TMP_BUFFER); - if (name == NULL) + if (readCtx == NULL) return MEMORY_E; #else ReadDirCtx readCtx[1]; diff --git a/src/tls13.c b/src/tls13.c index aa8d727c8..8fbc75aec 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -314,34 +314,44 @@ static int DeriveKeyMsg(WOLFSSL* ssl, byte* output, int outputLen, const byte* protocol; word32 protocolLen; int digestAlg; + int ret = BAD_FUNC_ARG; switch (hashAlgo) { #ifndef NO_WOLFSSL_SHA256 case sha256_mac: - wc_InitSha256(&digest.sha256); - wc_Sha256Update(&digest.sha256, msg, msgLen); - wc_Sha256Final(&digest.sha256, hash); - wc_Sha256Free(&digest.sha256); + ret = wc_InitSha256_ex(&digest.sha256, ssl->heap, INVALID_DEVID); + if (ret == 0) { + ret = wc_Sha256Update(&digest.sha256, msg, msgLen); + if (ret == 0) + ret = wc_Sha256Final(&digest.sha256, hash); + wc_Sha256Free(&digest.sha256); + } hashSz = SHA256_DIGEST_SIZE; digestAlg = SHA256; break; #endif #ifdef WOLFSSL_SHA384 case sha384_mac: - wc_InitSha384(&digest.sha384); - wc_Sha384Update(&digest.sha384, msg, msgLen); - wc_Sha384Final(&digest.sha384, hash); - wc_Sha384Free(&digest.sha384); + ret = wc_InitSha384_ex(&digest.sha384, ssl->heap, INVALID_DEVID); + if (ret == 0) { + ret = wc_Sha384Update(&digest.sha384, msg, msgLen); + if (ret == 0) + ret = wc_Sha384Final(&digest.sha384, hash); + wc_Sha384Free(&digest.sha384); + } hashSz = SHA384_DIGEST_SIZE; digestAlg = SHA384; break; #endif #ifdef WOLFSSL_SHA512 case sha512_mac: - wc_InitSha512(&digest.sha512); - wc_Sha512Update(&digest.sha512, msg, msgLen); - wc_Sha512Final(&digest.sha512, hash); - wc_Sha512Free(&digest.sha512); + ret = wc_InitSha512_ex(&digest.sha512, ssl->heap, INVALID_DEVID); + if (ret == 0) { + ret = wc_Sha512Update(&digest.sha512, msg, msgLen); + if (ret == 0) + ret = wc_Sha512Final(&digest.sha512, hash); + wc_Sha512Free(&digest.sha512); + } hashSz = SHA512_DIGEST_SIZE; digestAlg = SHA512; break; @@ -351,6 +361,9 @@ static int DeriveKeyMsg(WOLFSSL* ssl, byte* output, int outputLen, return BAD_FUNC_ARG; } + if (ret != 0) + return ret; + switch (ssl->version.minor) { case TLSv1_3_MINOR: protocol = tls13ProtocolLabel; @@ -733,6 +746,7 @@ static int BuildTls13HandshakeHmac(WOLFSSL* ssl, byte* key, byte* hash) Hmac verifyHmac; int hashType = SHA256; int hashSz = SHA256_DIGEST_SIZE; + int ret = BAD_FUNC_ARG; /* Get the hash of the previous handshake messages. */ switch (ssl->specs.mac_algorithm) { @@ -740,29 +754,39 @@ static int BuildTls13HandshakeHmac(WOLFSSL* ssl, byte* key, byte* hash) case sha256_mac: hashType = SHA256; hashSz = SHA256_DIGEST_SIZE; - wc_Sha256GetHash(&ssl->hsHashes->hashSha256, hash); + ret = wc_Sha256GetHash(&ssl->hsHashes->hashSha256, hash); break; #endif /* !NO_SHA256 */ #ifdef WOLFSSL_SHA384 case sha384_mac: hashType = SHA384; hashSz = SHA384_DIGEST_SIZE; - wc_Sha384GetHash(&ssl->hsHashes->hashSha384, hash); + ret = wc_Sha384GetHash(&ssl->hsHashes->hashSha384, hash); break; #endif /* WOLFSSL_SHA384 */ #ifdef WOLFSSL_SHA512 case sha512_mac: hashType = SHA512; hashSz = SHA512_DIGEST_SIZE; - wc_Sha512GetHash(&ssl->hsHashes->hashSha512, hash); + ret = wc_Sha512GetHash(&ssl->hsHashes->hashSha512, hash); break; #endif /* WOLFSSL_SHA512 */ } + if (ret != 0) + return ret; /* Calculate the verify data. */ - wc_HmacSetKey(&verifyHmac, hashType, key, ssl->specs.hash_size); - wc_HmacUpdate(&verifyHmac, hash, hashSz); - wc_HmacFinal(&verifyHmac, hash); + ret = wc_HmacInit(&verifyHmac, ssl->heap, INVALID_DEVID); + if (ret == 0) { + ret = wc_HmacSetKey(&verifyHmac, hashType, key, ssl->specs.hash_size); + if (ret == 0) + ret = wc_HmacUpdate(&verifyHmac, hash, hashSz); + if (ret == 0) + ret = wc_HmacFinal(&verifyHmac, hash); + wc_HmacFree(&verifyHmac); + } + if (ret != 0) + return ret; return hashSz; } @@ -1129,12 +1153,16 @@ static int HashInputRaw(WOLFSSL* ssl, const byte* input, int sz) #ifndef NO_OLD_TLS #ifndef NO_SHA - wc_ShaUpdate(&ssl->hsHashes->hashSha, input, sz); + ret = wc_ShaUpdate(&ssl->hsHashes->hashSha, input, sz); + if (ret != 0) + return ret; #endif #ifndef NO_MD5 - wc_Md5Update(&ssl->hsHashes->hashMd5, input, sz); -#endif + ret = wc_Md5Update(&ssl->hsHashes->hashMd5, input, sz); + if (ret != 0) + return ret; #endif +#endif /* !NO_OLD_TLS */ #ifndef NO_SHA256 ret = wc_Sha256Update(&ssl->hsHashes->hashSha256, input, sz); @@ -2956,41 +2984,54 @@ static int CreateRSAEncodedSig(byte* sig, byte* sigData, int sigDataSz, Digest digest; int hashSz = 0; int hashOid = 0; + int ret = BAD_FUNC_ARG; /* Digest the signature data. */ switch (hashAlgo) { #ifndef NO_WOLFSSL_SHA256 case sha256_mac: - wc_InitSha256(&digest.sha256); - wc_Sha256Update(&digest.sha256, sigData, sigDataSz); - wc_Sha256Final(&digest.sha256, sigData); - wc_Sha256Free(&digest.sha256); + ret = wc_InitSha256(&digest.sha256); + if (ret == 0) { + ret = wc_Sha256Update(&digest.sha256, sigData, sigDataSz); + if (ret == 0) + ret = wc_Sha256Final(&digest.sha256, sigData); + wc_Sha256Free(&digest.sha256); + } hashSz = SHA256_DIGEST_SIZE; hashOid = SHA256h; break; #endif #ifdef WOLFSSL_SHA384 case sha384_mac: - wc_InitSha384(&digest.sha384); - wc_Sha384Update(&digest.sha384, sigData, sigDataSz); - wc_Sha384Final(&digest.sha384, sigData); - wc_Sha384Free(&digest.sha384); + ret = wc_InitSha384(&digest.sha384); + if (ret == 0) { + ret = wc_Sha384Update(&digest.sha384, sigData, sigDataSz); + if (ret == 0) + ret = wc_Sha384Final(&digest.sha384, sigData); + wc_Sha384Free(&digest.sha384); + } hashSz = SHA384_DIGEST_SIZE; hashOid = SHA384h; break; #endif #ifdef WOLFSSL_SHA512 case sha512_mac: - wc_InitSha512(&digest.sha512); - wc_Sha512Update(&digest.sha512, sigData, sigDataSz); - wc_Sha512Final(&digest.sha512, sigData); - wc_Sha512Free(&digest.sha512); + ret = wc_InitSha512(&digest.sha512); + if (ret == 0) { + ret = wc_Sha512Update(&digest.sha512, sigData, sigDataSz); + if (ret == 0) + ret = wc_Sha512Final(&digest.sha512, sigData); + wc_Sha512Free(&digest.sha512); + } hashSz = SHA512_DIGEST_SIZE; hashOid = SHA512h; break; #endif } + if (ret != 0) + return ret; + /* Encode the signature data as per PKCS #1.5 */ return wc_EncodeSignature(sig, sigData, hashSz, hashOid); } @@ -3485,8 +3526,6 @@ int SendTls13CertificateVerify(WOLFSSL* ssl) /* Digest the signature data and encode. Used in verify too. */ sig->length = CreateRSAEncodedSig(sig->buffer, args->sigData, args->sigDataSz, ssl->suites->hashAlgo); - if (ret != 0) - goto exit_scv; /* Maximum size of RSA Signature. */ args->sigLen = args->length; diff --git a/wolfcrypt/benchmark/benchmark.c b/wolfcrypt/benchmark/benchmark.c index dd2f56cc2..229132456 100644 --- a/wolfcrypt/benchmark/benchmark.c +++ b/wolfcrypt/benchmark/benchmark.c @@ -129,7 +129,8 @@ #define BEGIN_INTEL_CYCLES total_cycles = get_intel_cycles(); #define END_INTEL_CYCLES total_cycles = get_intel_cycles() - total_cycles; #define SHOW_INTEL_CYCLES printf(" Cycles per byte = %6.2f", \ - (float)total_cycles / (count*BENCH_SIZE)); + count == 0 ? 0 : \ + (float)total_cycles / (count*BENCH_SIZE)); #elif defined(LINUX_CYCLE_COUNT) #include #include diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index ad8431444..e039b8a6e 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -9534,8 +9534,13 @@ int wc_EccPrivateKeyDecode(const byte* input, word32* inOutIdx, ecc_key* key, XMEMCPY(priv, &input[*inOutIdx], privSz); *inOutIdx += length; - if ((*inOutIdx + 1) > inSz) + if ((*inOutIdx + 1) > inSz) { + #ifdef WOLFSSL_SMALL_STACK + XFREE(priv, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(pub, NULL, DYNAMIC_TYPE_TMP_BUFFER); + #endif return BUFFER_E; + } /* prefix 0, may have */ b = input[*inOutIdx]; diff --git a/wolfcrypt/src/ecc.c b/wolfcrypt/src/ecc.c index 558380a1c..4b1e19e0c 100755 --- a/wolfcrypt/src/ecc.c +++ b/wolfcrypt/src/ecc.c @@ -4850,9 +4850,11 @@ int wc_ecc_import_x963_ex(const byte* in, word32 inLen, ecc_key* key, if (err == MP_OKAY) { int keysize; + #ifdef HAVE_COMP_KEY /* adjust inLen if compressed */ if (compressed) inLen = (inLen-1)*2 + 1; /* used uncompressed len */ + #endif /* determine key size */ keysize = ((inLen-1)>>1); diff --git a/wolfcrypt/src/hmac.c b/wolfcrypt/src/hmac.c index 2498a8add..0dc745361 100755 --- a/wolfcrypt/src/hmac.c +++ b/wolfcrypt/src/hmac.c @@ -806,8 +806,13 @@ int wolfSSL_GetHmacMaxSize(void) Hmac myHmac; int ret; const byte* localSalt; /* either points to user input or tmp */ - int hashSz = wc_HmacSizeByType(type); + int hashSz; + ret = wc_HmacSizeByType(type); + if (ret < 0) + return ret; + + hashSz = ret; localSalt = salt; if (localSalt == NULL) { XMEMSET(tmp, 0, hashSz); diff --git a/wolfcrypt/src/integer.c b/wolfcrypt/src/integer.c index 624deea29..c91d8331a 100644 --- a/wolfcrypt/src/integer.c +++ b/wolfcrypt/src/integer.c @@ -4253,14 +4253,16 @@ static int mp_div_d (mp_int * a, mp_digit b, mp_int * c, mp_digit * d) /* no easy answer [c'est la vie]. Just division */ if (c != NULL) { if ((res = mp_init_size(&q, a->used)) != MP_OKAY) { - return res; + return res; } q.used = a->used; q.sign = a->sign; } else { - mp_init(&q); /* initialize to help static analysis */ + if ((res = mp_init(&q)) != MP_OKAY) { + return res; + } } diff --git a/wolfcrypt/src/rsa.c b/wolfcrypt/src/rsa.c index afeec506d..b402cd15c 100755 --- a/wolfcrypt/src/rsa.c +++ b/wolfcrypt/src/rsa.c @@ -195,25 +195,32 @@ int wc_InitRsaKey_ex(RsaKey* key, void* heap, int devId) key->rng = NULL; #endif -#if defined(WOLFSSL_ASYNC_CRYPT) && defined(WC_ASYNC_ENABLE_RSA) - /* handle as async */ - ret = wolfAsync_DevCtxInit(&key->asyncDev, WOLFSSL_ASYNC_MARKER_RSA, - key->heap, devId); +#ifdef WOLFSSL_ASYNC_CRYPT #ifdef WOLFSSL_CERT_GEN XMEMSET(&key->certSignCtx, 0, sizeof(CertSignCtx)); #endif + + #ifdef WC_ASYNC_ENABLE_RSA + /* handle as async */ + ret = wolfAsync_DevCtxInit(&key->asyncDev, WOLFSSL_ASYNC_MARKER_RSA, + key->heap, devId); + if (ret != 0) + return ret; + #endif /* WC_ASYNC_ENABLE_RSA */ #else (void)devId; -#endif +#endif /* WOLFSSL_ASYNC_CRYPT */ - mp_init(&key->n); - mp_init(&key->e); - mp_init(&key->d); - mp_init(&key->p); - mp_init(&key->q); - mp_init(&key->dP); - mp_init(&key->dQ); - mp_init(&key->u); + ret = mp_init_multi(&key->n, &key->e, NULL, NULL, NULL, NULL); + if (ret != MP_OKAY) + return ret; + + ret = mp_init_multi(&key->d, &key->p, &key->q, &key->dP, &key->dQ, &key->u); + if (ret != MP_OKAY) { + mp_clear(&key->n); + mp_clear(&key->e); + return ret; + } return ret; }