From 48d13bbfa590ef745930e4606312cfcdb1be060a Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Wed, 17 Mar 2021 12:24:18 +0700 Subject: [PATCH 01/11] fix for leak with wolfSSL_a2i_ASN1_INTEGER --- src/ssl.c | 9 +++++++-- tests/api.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 4fb9ebdb0..f2320d0ce 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -48230,7 +48230,7 @@ int wolfSSL_a2i_ASN1_INTEGER(WOLFSSL_BIO *bio, WOLFSSL_ASN1_INTEGER *asn1, XFREE(asn1->data, NULL, DYNAMIC_TYPE_OPENSSL); asn1->isDynamic = 0; } - XMEMSET(asn1->intData, 0, sizeof(WOLFSSL_ASN1_INTEGER)); + XMEMSET(asn1->intData, 0, WOLFSSL_ASN1_INTEGER_MAX); asn1->data = asn1->intData; asn1->length = 0; asn1->negative = 0; @@ -48259,7 +48259,7 @@ int wolfSSL_a2i_ASN1_INTEGER(WOLFSSL_BIO *bio, WOLFSSL_ASN1_INTEGER *asn1, len = asn1->length + (lineLen/2); /* Check if it will fit in static memory and * save space for the ASN tag in front */ - if (len > (int)(sizeof(asn1->intData) - extraTagSz)) { + if (len > (int)(WOLFSSL_ASN1_INTEGER_MAX - extraTagSz)) { /* Allocate mem for data */ if (asn1->isDynamic) { byte* tmp = (byte*)XREALLOC(asn1->data, len + extraTagSz, NULL, @@ -48271,12 +48271,17 @@ int wolfSSL_a2i_ASN1_INTEGER(WOLFSSL_BIO *bio, WOLFSSL_ASN1_INTEGER *asn1, asn1->data = tmp; } else { + /* Up to this point asn1->data pointed to asn1->intData. + * Now that the size has grown larger than intData can handle + * the asn1 structure moves to a dynamic type with isDynamic + * flag being set and asn1->data being malloc'd. */ asn1->data = (byte*)XMALLOC(len + extraTagSz, NULL, DYNAMIC_TYPE_OPENSSL); if (!asn1->data) { WOLFSSL_MSG("malloc error"); return WOLFSSL_FAILURE; } + asn1->isDynamic = 1; XMEMCPY(asn1->data, asn1->intData, asn1->length); } } diff --git a/tests/api.c b/tests/api.c index 37cdc2bd4..56e5efd7a 100644 --- a/tests/api.c +++ b/tests/api.c @@ -29447,6 +29447,54 @@ static void test_wolfSSL_ASN1_BIT_STRING(void) #endif } +static void test_wolfSSL_a2i_ASN1_INTEGER(void) +{ +#ifdef OPENSSL_EXTRA + BIO *bio, *out; + ASN1_INTEGER* ai; + char buf[] = "123456\n12345\n112345678912345678901234567890\n"; + char tmp[1024]; + int bufSz, tmpSz; + char* pt; + + const char expected1[] = "123456"; + const char expected2[] = "112345678912345678901234567890"; + + printf(testingFmt, "test_wolfSSL_a2i_ASN1_INTEGER()"); + pt = (char*)buf; + bufSz = sizeof(buf); + + AssertNotNull(bio = BIO_new_mem_buf(buf, -1)); + AssertNotNull(out = BIO_new(BIO_s_mem())); + AssertNotNull(ai = ASN1_INTEGER_new()); + + /* read first line */ + AssertIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, 1024), SSL_SUCCESS); + AssertIntEQ(i2a_ASN1_INTEGER(out, ai), 6); + XMEMSET(tmp, 0, 1024); + tmpSz = BIO_read(out, tmp, 1024); + AssertIntEQ(tmpSz, 6); + AssertIntEQ(XMEMCMP(tmp, expected1, tmpSz), 0); + + /* fail on second line (not % 2) */ + AssertIntNE(a2i_ASN1_INTEGER(bio, ai, tmp, 1024), SSL_SUCCESS); + + /* read 3rd long line */ + AssertIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, 1024), SSL_SUCCESS); + AssertIntEQ(i2a_ASN1_INTEGER(out, ai), 30); + XMEMSET(tmp, 0, 1024); + tmpSz = BIO_read(out, tmp, 1024); + AssertIntEQ(tmpSz, 30); + AssertIntEQ(XMEMCMP(tmp, expected2, tmpSz), 0); + + BIO_free(out); + BIO_free(bio); + ASN1_INTEGER_free(ai); + + printf(resultFmt, passed); + +#endif +} static void test_wolfSSL_DES_ecb_encrypt(void) { @@ -41480,6 +41528,7 @@ void ApiTest(void) #endif test_wolfSSL_ASN1_STRING(); test_wolfSSL_ASN1_BIT_STRING(); + test_wolfSSL_a2i_ASN1_INTEGER(); test_wolfSSL_X509(); test_wolfSSL_X509_VERIFY_PARAM(); test_wolfSSL_X509_sign(); From 6995f6dedc93c928d944a8f096b48ce2c858fb52 Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Wed, 17 Mar 2021 12:34:12 +0700 Subject: [PATCH 02/11] help out static analyizer and memset buffer created --- src/ssl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ssl.c b/src/ssl.c index f2320d0ce..3d26a38b1 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -41554,6 +41554,7 @@ cleanup: pem = (unsigned char*)XMALLOC(l, 0, DYNAMIC_TYPE_PEM); if (pem == NULL) return NULL; + XMEMSET(pem, 0, l); i = 0; if (wc_PemGetHeaderFooter(type, NULL, &footer) != 0) { From d439694eb6f826aa1147c85e1a5fc0e7c31c6e5b Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Wed, 17 Mar 2021 13:41:27 +0700 Subject: [PATCH 03/11] sanity check on length in wolfSSL_BN_rand --- src/ssl.c | 28 +++++++++++++++++++++++++--- tests/api.c | 24 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 3d26a38b1..c081086c2 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -50833,7 +50833,7 @@ int wolfSSL_mask_bits(WOLFSSL_BIGNUM* bn, int n) int wolfSSL_BN_rand(WOLFSSL_BIGNUM* bn, int bits, int top, int bottom) { int ret = 0; - int len = bits / 8; + int len; int initTmpRng = 0; WC_RNG* rng = NULL; #ifdef WOLFSSL_SMALL_STACK @@ -50848,9 +50848,19 @@ int wolfSSL_BN_rand(WOLFSSL_BIGNUM* bn, int bits, int top, int bottom) (void)bottom; WOLFSSL_MSG("wolfSSL_BN_rand"); + if (bits <= 0) { + return WOLFSSL_FAILURE; + } + + len = bits / 8; if (bits % 8) len++; + /* has to be a length of at least 1 since we set buf[0] and buf[len-1] */ + if (len < 1) { + return WOLFSSL_FAILURE; + } + #ifdef WOLFSSL_SMALL_STACK buff = (byte*)XMALLOC(1024, NULL, DYNAMIC_TYPE_TMP_BUFFER); tmpRNG = (WC_RNG*) XMALLOC(sizeof(WC_RNG), NULL, DYNAMIC_TYPE_RNG); @@ -50906,7 +50916,7 @@ int wolfSSL_BN_rand(WOLFSSL_BIGNUM* bn, int bits, int top, int bottom) int wolfSSL_BN_pseudo_rand(WOLFSSL_BIGNUM* bn, int bits, int top, int bottom) { int ret = 0; - int len = bits / 8; + int len; int initTmpRng = 0; WC_RNG* rng = NULL; #ifdef WOLFSSL_SMALL_STACK @@ -50917,11 +50927,23 @@ int wolfSSL_BN_pseudo_rand(WOLFSSL_BIGNUM* bn, int bits, int top, int bottom) byte buff[1024]; #endif - WOLFSSL_MSG("wolfSSL_BN_rand"); + WOLFSSL_MSG("wolfSSL_BN_pseudo_rand"); + if (bits <= 0) { + return WOLFSSL_FAILURE; + } + + len = bits / 8; if (bits % 8) len++; + /* has to be a length of at least 1 since we set buf[0] and buf[len-1] */ + if (top == 1 || top == 0 || bottom == 1) { + if (len < 1) { + return WOLFSSL_FAILURE; + } + } + #ifdef WOLFSSL_SMALL_STACK buff = (byte*)XMALLOC(1024, NULL, DYNAMIC_TYPE_TMP_BUFFER); tmpRNG = (WC_RNG*) XMALLOC(sizeof(WC_RNG), NULL, DYNAMIC_TYPE_TMP_BUFFER); diff --git a/tests/api.c b/tests/api.c index 56e5efd7a..a0a015387 100644 --- a/tests/api.c +++ b/tests/api.c @@ -30458,6 +30458,29 @@ static void test_wolfSSL_RAND_bytes(void) #endif } +static void test_wolfSSL_BN_rand(void) +{ + #if defined(OPENSSL_EXTRA) + BIGNUM* bn; + + printf(testingFmt, "wolfSSL_BN_rand()"); + + AssertNotNull(bn = BN_new()); + AssertIntNE(BN_rand(bn, 0, 0, 0), SSL_SUCCESS); + BN_free(bn); + + AssertNotNull(bn = BN_new()); + AssertIntEQ(BN_rand(bn, 8, 0, 0), SSL_SUCCESS); + BN_free(bn); + + AssertNotNull(bn = BN_new()); + AssertIntEQ(BN_rand(bn, 64, 0, 0), SSL_SUCCESS); + BN_free(bn); + + printf(resultFmt, passed); + #endif +} + static void test_wolfSSL_pseudo_rand(void) { #if defined(OPENSSL_EXTRA) @@ -41550,6 +41573,7 @@ void ApiTest(void) test_wolfSSL_CTX_set_ecdh_auto(); test_wolfSSL_THREADID_hash(); test_wolfSSL_RAND_bytes(); + test_wolfSSL_BN_rand(); test_wolfSSL_pseudo_rand(); test_wolfSSL_PKCS8_Compat(); test_wolfSSL_PKCS8_d2i(); From da56c33f4862a9837e0504a638b239170ef35597 Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Wed, 17 Mar 2021 16:33:37 +0700 Subject: [PATCH 04/11] add debug message on BIO write return value when printing out error nodes --- src/ssl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ssl.c b/src/ssl.c index c081086c2..6fcfba1c4 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -16459,7 +16459,9 @@ int wolfSSL_set_compression(WOLFSSL* ssl) wc_RemoveErrorNode(0); } } while (ret >= 0); - wolfSSL_BIO_write(bio, "", 1); + if (wolfSSL_BIO_write(bio, "", 1) != 1) { + WOLFSSL_MSG("Issue writing final string terminator"); + } } #endif /* !NO_BIO */ #endif /* OPENSSL_EXTRA || DEBUG_WOLFSSL_VERBOSE */ From 1ca36042121b1a9b2970e70b3f96bd41ef291b97 Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Wed, 17 Mar 2021 16:42:16 +0700 Subject: [PATCH 05/11] add check on init mutex return value --- wolfcrypt/src/evp.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/wolfcrypt/src/evp.c b/wolfcrypt/src/evp.c index 75c165c42..96e3382ea 100644 --- a/wolfcrypt/src/evp.c +++ b/wolfcrypt/src/evp.c @@ -6710,17 +6710,24 @@ WOLFSSL_EVP_PKEY* wolfSSL_EVP_PKEY_new_ex(void* heap) XMEMSET(pkey, 0, sizeof(WOLFSSL_EVP_PKEY)); pkey->heap = heap; pkey->type = WOLFSSL_EVP_PKEY_DEFAULT; + + /* init of mutex needs to come before wolfSSL_EVP_PKEY_free */ + ret = wc_InitMutex(&pkey->refMutex); + if (ret != 0){ + XFREE(pkey, heap, DYNAMIC_TYPE_PUBLIC_KEY); + WOLFSSL_MSG("Issue initializing mutex"); + return NULL; + } + #ifndef HAVE_FIPS ret = wc_InitRng_ex(&pkey->rng, heap, INVALID_DEVID); #else ret = wc_InitRng(&pkey->rng); #endif pkey->references = 1; - wc_InitMutex(&pkey->refMutex); /* init of mutex needs to come before - * wolfSSL_EVP_PKEY_free */ if (ret != 0){ wolfSSL_EVP_PKEY_free(pkey); - WOLFSSL_MSG("memory failure"); + WOLFSSL_MSG("Issue initializing RNG"); return NULL; } } From 2732ba2bbaf3642c64dbcf5f7dad4fdaf1f0cef3 Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Wed, 17 Mar 2021 16:50:53 +0700 Subject: [PATCH 06/11] check return value is not negative --- src/ssl.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ssl.c b/src/ssl.c index 6fcfba1c4..394bfdc1b 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -31994,6 +31994,10 @@ WOLFSSL_ASN1_INTEGER* wolfSSL_BN_to_ASN1_INTEGER(const WOLFSSL_BIGNUM *bn, WOLFS } else { len = wolfSSL_BN_bn2bin(bn, a->data); + if (len < 0) { + wolfSSL_ASN1_INTEGER_free(a); + return NULL; + } } a->length = len; From 6ef905c9e38979eefcfb4a80c62875579212142f Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Wed, 17 Mar 2021 17:06:03 +0700 Subject: [PATCH 07/11] use err goto for error out --- src/ssl.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 394bfdc1b..96c0f2e5f 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -9790,8 +9790,7 @@ void* wolfSSL_X509_get_ext_d2i(const WOLFSSL_X509* x509, int nid, int* c, gn = wolfSSL_GENERAL_NAME_new(); if (gn == NULL) { WOLFSSL_MSG("Error creating GENERAL_NAME"); - wolfSSL_sk_free(sk); - return NULL; + goto err; } gn->type = dns->type; @@ -9799,18 +9798,14 @@ void* wolfSSL_X509_get_ext_d2i(const WOLFSSL_X509* x509, int nid, int* c, if (wolfSSL_ASN1_STRING_set(gn->d.ia5, dns->name, gn->d.ia5->length) != WOLFSSL_SUCCESS) { WOLFSSL_MSG("ASN1_STRING_set failed"); - wolfSSL_GENERAL_NAME_free(gn); - wolfSSL_sk_free(sk); - return NULL; + goto err; } dns = dns->next; if (wolfSSL_sk_GENERAL_NAME_push(sk, gn) != WOLFSSL_SUCCESS) { WOLFSSL_MSG("Error pushing ASN1 object onto stack"); - wolfSSL_GENERAL_NAME_free(gn); - wolfSSL_sk_free(sk); - sk = NULL; + goto err; } /* null so that it doesn't get pushed again after switch */ gn = NULL; From 12b290cbaf4fe984b9ed26a519d51358f28d98e7 Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Wed, 17 Mar 2021 17:34:54 +0700 Subject: [PATCH 08/11] remove duplicate (deadcode) for clearing mp_int's --- wolfcrypt/src/dh.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/wolfcrypt/src/dh.c b/wolfcrypt/src/dh.c index c7803c33b..0ddd2603d 100644 --- a/wolfcrypt/src/dh.c +++ b/wolfcrypt/src/dh.c @@ -2177,11 +2177,6 @@ int wc_DhImportKeyPair(DhKey* key, const byte* priv, word32 privSz, WOLFSSL_MSG("DH Public Key Set"); } } - /* Free Memory if error occurred */ - if (havePriv == 0 && keyPriv != NULL) - mp_clear(keyPriv); - if (havePub == 0 && keyPub != NULL) - mp_clear(keyPub); if (havePriv == 0 && havePub == 0) { return MEMORY_E; From a64bb8aef7a2d9739e91b48b5b23ec919a023740 Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Thu, 18 Mar 2021 15:17:08 +0700 Subject: [PATCH 09/11] fix unused variable in test case from Jenkins test --- tests/api.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/api.c b/tests/api.c index a0a015387..90a3f7d3d 100644 --- a/tests/api.c +++ b/tests/api.c @@ -29454,15 +29454,12 @@ static void test_wolfSSL_a2i_ASN1_INTEGER(void) ASN1_INTEGER* ai; char buf[] = "123456\n12345\n112345678912345678901234567890\n"; char tmp[1024]; - int bufSz, tmpSz; - char* pt; + int tmpSz; const char expected1[] = "123456"; const char expected2[] = "112345678912345678901234567890"; printf(testingFmt, "test_wolfSSL_a2i_ASN1_INTEGER()"); - pt = (char*)buf; - bufSz = sizeof(buf); AssertNotNull(bio = BIO_new_mem_buf(buf, -1)); AssertNotNull(out = BIO_new(BIO_s_mem())); From 360c961b48daa7227519202b810db49c4d08ee30 Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Thu, 18 Mar 2021 20:34:38 +0700 Subject: [PATCH 10/11] fix for unused variable in dh.c from Jenkins test --- wolfcrypt/src/dh.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/wolfcrypt/src/dh.c b/wolfcrypt/src/dh.c index 0ddd2603d..79895267d 100644 --- a/wolfcrypt/src/dh.c +++ b/wolfcrypt/src/dh.c @@ -2126,7 +2126,6 @@ int wc_DhImportKeyPair(DhKey* key, const byte* priv, word32 privSz, const byte* pub, word32 pubSz) { byte havePriv, havePub; - mp_int *keyPriv = NULL, *keyPub = NULL; if (key == NULL) { return BAD_FUNC_ARG; @@ -2154,7 +2153,6 @@ int wc_DhImportKeyPair(DhKey* key, const byte* priv, word32 privSz, mp_clear(&key->priv); havePriv = 0; } else { - keyPriv = &key->priv; WOLFSSL_MSG("DH Private Key Set"); } } @@ -2172,8 +2170,11 @@ int wc_DhImportKeyPair(DhKey* key, const byte* priv, word32 privSz, if (mp_read_unsigned_bin(&key->pub, pub, pubSz) != MP_OKAY) { mp_clear(&key->pub); havePub = 0; + if (havePriv) { + mp_clear(&key->priv); + havePriv = 0; /* set to 0 to error out with failed read pub */ + } } else { - keyPub = &key->pub; WOLFSSL_MSG("DH Public Key Set"); } } From 183917f10287644791eaa6371c2ce4015aaea9fd Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Thu, 25 Mar 2021 01:16:20 +0700 Subject: [PATCH 11/11] change debug message type from review --- src/ssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ssl.c b/src/ssl.c index 96c0f2e5f..d87f44956 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -50928,7 +50928,7 @@ int wolfSSL_BN_pseudo_rand(WOLFSSL_BIGNUM* bn, int bits, int top, int bottom) byte buff[1024]; #endif - WOLFSSL_MSG("wolfSSL_BN_pseudo_rand"); + WOLFSSL_ENTER("wolfSSL_BN_pseudo_rand"); if (bits <= 0) { return WOLFSSL_FAILURE;