From 487e66394e1ce1f45f7f30baccb80b017248edf9 Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Fri, 16 Aug 2019 15:19:33 -0600 Subject: [PATCH 1/3] adjust wc_i2d_PKCS12 API --- src/ssl.c | 2 +- tests/api.c | 51 ++++++++++++++++++++++++++++++++++ wolfcrypt/src/pkcs12.c | 57 ++++++++++++++++++++++++++------------ wolfssl/wolfcrypt/pkcs12.h | 2 +- 4 files changed, 93 insertions(+), 19 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index a3559d681..8fbafddd9 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -19007,7 +19007,7 @@ int wolfSSL_i2d_PKCS12_bio(WOLFSSL_BIO *bio, WC_PKCS12 *pkcs12) word32 certSz = 0; byte *certDer = NULL; - certSz = wc_i2d_PKCS12(pkcs12, &certDer); + certSz = wc_i2d_PKCS12(pkcs12, &certDer, NULL); if ((certSz > 0) && (certDer != NULL)) { if (wolfSSL_BIO_write(bio, certDer, certSz) == (int)certSz) { ret = SSL_SUCCESS; diff --git a/tests/api.c b/tests/api.c index a1ade6296..a670ac3f2 100644 --- a/tests/api.c +++ b/tests/api.c @@ -285,6 +285,9 @@ #ifdef HAVE_CURVE25519 #include #endif +#ifdef HAVE_PKCS12 + #include +#endif #if (defined(OPENSSL_EXTRA) || defined(OPENSSL_EXTRA_X509_SMALL) || defined(OPENSSL_ALL)) #include @@ -17700,6 +17703,52 @@ static void test_PKCS7_signed_enveloped(void) #endif } +static void test_wc_i2d_PKCS12(void) +{ +#if !defined(NO_ASN) && !defined(NO_PWDBASED) && defined(HAVE_PKCS12) \ + && !defined(NO_FILESYSTEM) && !defined(NO_RSA) \ + && !defined(NO_AES) && !defined(NO_DES3) + WC_PKCS12* pkcs12 = NULL; + unsigned char der[FOURK_BUF * 2]; + unsigned char* pt; + int derSz; + unsigned char out[FOURK_BUF * 2]; + int outSz = FOURK_BUF * 2; + + const char p12_f[] = "./certs/test-servercert.p12"; + XFILE f; + + printf(testingFmt, "wc_i2d_PKCS12"); + + AssertNotNull(f = XFOPEN(p12_f, "rb")); + derSz = XFREAD(der, 1, sizeof(der), f); + AssertIntGT(derSz, 0); + XFCLOSE(f); + + AssertNotNull(pkcs12 = wc_PKCS12_new()); + AssertIntEQ(wc_d2i_PKCS12(der, derSz, pkcs12), 0); + AssertIntEQ(wc_i2d_PKCS12(pkcs12, NULL, &outSz), LENGTH_ONLY_E); + AssertIntEQ(outSz, derSz); + + outSz = derSz - 1; + pt = out; + AssertIntLE(wc_i2d_PKCS12(pkcs12, &pt, &outSz), 0); + + outSz = derSz; + AssertIntEQ(wc_i2d_PKCS12(pkcs12, &pt, &outSz), derSz); + AssertIntEQ((pt == out), 0); + + pt = NULL; + AssertIntEQ(wc_i2d_PKCS12(pkcs12, &pt, NULL), derSz); + XFREE(pt, NULL, DYNAMIC_TYPE_PKCS); + wc_PKCS12_free(pkcs12); + + printf(resultFmt, passed); + +#endif +} + + /* Testing wc_SignatureGetSize() for signature type ECC */ static int test_wc_SignatureGetSize_ecc(void) { @@ -25892,6 +25941,8 @@ void ApiTest(void) test_wc_PKCS7_BER(); test_PKCS7_signed_enveloped(); + test_wc_i2d_PKCS12(); + test_wolfSSL_CTX_LoadCRL(); AssertIntEQ(test_ForceZero(), 0); diff --git a/wolfcrypt/src/pkcs12.c b/wolfcrypt/src/pkcs12.c index 08c5aacb4..ada39177c 100644 --- a/wolfcrypt/src/pkcs12.c +++ b/wolfcrypt/src/pkcs12.c @@ -674,19 +674,23 @@ int wc_d2i_PKCS12(const byte* der, word32 derSz, WC_PKCS12* pkcs12) * pkcs12 : non-null pkcs12 pointer * der : pointer-pointer to der buffer. If NULL space will be * allocated for der, which must be freed by application. + * derSz : size of buffer passed in when der is not NULL. NULL arg disables + * sanity checks on buffer read/writes. Max size gets set to derSz when + * the "der" buffer passed in is NULL and LENGTH_ONLY_E is returned. * return size of DER on success and negative on failure. */ -int wc_i2d_PKCS12(WC_PKCS12* pkcs12, byte** der) +int wc_i2d_PKCS12(WC_PKCS12* pkcs12, byte** der, int* derSz) { int ret = 0; - word32 seqSz, verSz, totalSz = 0, idx = 0, sdBufSz = 0; + word32 seqSz, verSz = 0, totalSz = 0, idx = 0, sdBufSz = 0; byte *buf = NULL; byte ver[MAX_VERSION_SZ]; byte seq[MAX_SEQ_SZ]; byte *sdBuf = NULL; - if ((pkcs12 == NULL) || (pkcs12->safe == NULL) || (der == NULL)) { - ret = BAD_FUNC_ARG; + if ((pkcs12 == NULL) || (pkcs12->safe == NULL) || + (der == NULL && derSz == NULL)) { + return BAD_FUNC_ARG; } /* Create the MAC portion */ @@ -761,21 +765,40 @@ int wc_i2d_PKCS12(WC_PKCS12* pkcs12, byte** der) totalSz += 4; /* Seq */ - verSz = SetMyVersion(WC_PKCS12_VERSION_DEFAULT, ver, FALSE); - totalSz += verSz; + ret = SetMyVersion(WC_PKCS12_VERSION_DEFAULT, ver, FALSE); + if (ret > 0) { + verSz = (word32)ret; + ret = 0; /* value larger than 0 is success */ + totalSz += verSz; - seqSz = SetSequence(totalSz, seq); - totalSz += seqSz; + seqSz = SetSequence(totalSz, seq); + totalSz += seqSz; - if (*der == NULL) { - /* Allocate if requested */ - buf = (byte*)XMALLOC(totalSz, NULL, DYNAMIC_TYPE_PKCS); - if (buf == NULL) { - ret = MEMORY_E; + /* check if getting length only */ + if (der == NULL && derSz != NULL) { + *derSz = totalSz; + XFREE(sdBuf, pkcs12->heap, DYNAMIC_TYPE_PKCS); + return LENGTH_ONLY_E; + } + + if (*der == NULL) { + /* Allocate if requested */ + buf = (byte*)XMALLOC(totalSz, NULL, DYNAMIC_TYPE_PKCS); + if (buf == NULL) { + ret = MEMORY_E; + } + } + else { + buf = *der; + + /* sanity check on buffer size if passed in */ + if (derSz != NULL) { + if (*derSz < (int)totalSz) { + WOLFSSL_MSG("Buffer passed in is too small"); + ret = BUFFER_E; + } + } } - } - else { - buf = *der; } } @@ -799,7 +822,7 @@ int wc_i2d_PKCS12(WC_PKCS12* pkcs12, byte** der) idx += sizeof(WC_PKCS12_DATA_OID); /* Element */ - buf[idx++] = 0xA0; + buf[idx++] = ASN_CONSTRUCTED | ASN_CONTEXT_SPECIFIC; idx += SetLength(totalSz - sdBufSz - idx - 3, &buf[idx]); /* Octet string */ diff --git a/wolfssl/wolfcrypt/pkcs12.h b/wolfssl/wolfcrypt/pkcs12.h index 3e5750797..da9d4f02e 100644 --- a/wolfssl/wolfcrypt/pkcs12.h +++ b/wolfssl/wolfcrypt/pkcs12.h @@ -49,7 +49,7 @@ enum { WOLFSSL_API WC_PKCS12* wc_PKCS12_new(void); WOLFSSL_API void wc_PKCS12_free(WC_PKCS12* pkcs12); WOLFSSL_API int wc_d2i_PKCS12(const byte* der, word32 derSz, WC_PKCS12* pkcs12); -WOLFSSL_API int wc_i2d_PKCS12(WC_PKCS12* pkcs12, byte** der); +WOLFSSL_API int wc_i2d_PKCS12(WC_PKCS12* pkcs12, byte** der, int* derSz); WOLFSSL_API int wc_PKCS12_parse(WC_PKCS12* pkcs12, const char* psw, byte** pkey, word32* pkeySz, byte** cert, word32* certSz, WC_DerCertList** ca); From 01a3b59e2864f8cfab3639fe8989265db1c25654 Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Mon, 19 Aug 2019 14:54:53 -0600 Subject: [PATCH 2/3] fix cast and initialization of variable --- tests/api.c | 42 +++++++++++++++++++++--------------------- wolfcrypt/src/pkcs12.c | 2 +- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/tests/api.c b/tests/api.c index a670ac3f2..f1caaff8a 100644 --- a/tests/api.c +++ b/tests/api.c @@ -16029,7 +16029,7 @@ static void test_wc_PKCS7_InitWithCert (void) fp = XFOPEN("./certs/1024/client-cert.der", "rb"); AssertTrue(fp != XBADFILE); - certSz = XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); + certSz = (int)XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); XFCLOSE(fp); #endif #elif defined(HAVE_ECC) @@ -16046,7 +16046,7 @@ static void test_wc_PKCS7_InitWithCert (void) AssertTrue(fp != XBADFILE); - certSz = XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); + certSz = (int)XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); XFCLOSE(fp); #endif #else @@ -16116,12 +16116,12 @@ static void test_wc_PKCS7_EncodeData (void) fp = XFOPEN("./certs/1024/client-cert.der", "rb"); AssertTrue(fp != XBADFILE); - certSz = XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); + certSz = (int)XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); XFCLOSE(fp); fp = XFOPEN("./certs/1024/client-key.der", "rb"); AssertTrue(fp != XBADFILE); - keySz = XFREAD(key, 1, sizeof_client_key_der_1024, fp); + keySz = (int)XFREAD(key, 1, sizeof_client_key_der_1024, fp); XFCLOSE(fp); #endif #elif defined(HAVE_ECC) @@ -16142,12 +16142,12 @@ static void test_wc_PKCS7_EncodeData (void) fp = XFOPEN("./certs/client-ecc-cert.der", "rb"); AssertTrue(fp != XBADFILE); - certSz = XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); + certSz = (int)XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); XFCLOSE(fp); fp = XFOPEN("./certs/client-ecc-key.der", "rb"); AssertTrue(fp != XBADFILE); - keySz = XFREAD(key, 1, sizeof_ecc_clikey_der_256, fp); + keySz = (int)XFREAD(key, 1, sizeof_ecc_clikey_der_256, fp); XFCLOSE(fp); #endif #endif @@ -16223,12 +16223,12 @@ static void test_wc_PKCS7_EncodeSignedData(void) fp = XFOPEN("./certs/1024/client-cert.der", "rb"); AssertTrue(fp != XBADFILE); - certSz = XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); + certSz = (int)XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); XFCLOSE(fp); fp = XFOPEN("./certs/1024/client-key.der", "rb"); AssertTrue(fp != XBADFILE); - keySz = XFREAD(key, 1, sizeof_client_key_der_1024, fp); + keySz = (int)XFREAD(key, 1, sizeof_client_key_der_1024, fp); XFCLOSE(fp); #endif #elif defined(HAVE_ECC) @@ -16249,12 +16249,12 @@ static void test_wc_PKCS7_EncodeSignedData(void) fp = XOPEN("./certs/client-ecc-cert.der", "rb"); AssertTrue(fp != XBADFILE); - certSz = XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); + certSz = (int)XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); XFCLOSE(fp); fp = XFOPEN("./certs/client-ecc-key.der", "rb"); AssertTrue(fp != XBADFILE); - keySz = XFREAD(key, 1, sizeof_ecc_clikey_der_256, fp); + keySz = (int)XFREAD(key, 1, sizeof_ecc_clikey_der_256, fp); XFCLOSE(fp); #endif #endif @@ -16347,12 +16347,12 @@ static void test_wc_PKCS7_EncodeSignedData_ex(void) fp = XFOPEN("./certs/1024/client-cert.der", "rb"); AssertTrue((fp != XBADFILE)); - certSz = XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); + certSz = (int)XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); XFCLOSE(fp); fp = XFOPEN("./certs/1024/client-key.der", "rb"); AssertTrue(fp != XBADFILE); - keySz = XFREAD(key, 1, sizeof_client_key_der_1024, fp); + keySz = (int)XFREAD(key, 1, sizeof_client_key_der_1024, fp); XFCLOSE(fp); #endif #elif defined(HAVE_ECC) @@ -16373,12 +16373,12 @@ static void test_wc_PKCS7_EncodeSignedData_ex(void) fp = XFOPEN("./certs/client-ecc-cert.der", "rb"); AssertTrue(fp != XBADFILE); - certSz = XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); + certSz = (int)XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); XFCLOSE(fp); fp = XFOPEN("./certs/client-ecc-key.der", "rb"); AssertTrue(fp != XBADFILE); - keySz = XFREAD(key, 1, sizeof_ecc_clikey_der_256, fp); + keySz = (int)XFREAD(key, 1, sizeof_ecc_clikey_der_256, fp); XFCLOSE(fp); #endif #endif @@ -17721,7 +17721,7 @@ static void test_wc_i2d_PKCS12(void) printf(testingFmt, "wc_i2d_PKCS12"); AssertNotNull(f = XFOPEN(p12_f, "rb")); - derSz = XFREAD(der, 1, sizeof(der), f); + derSz = (int)XFREAD(der, 1, sizeof(der), f); AssertIntGT(derSz, 0); XFCLOSE(f); @@ -17852,7 +17852,7 @@ static int test_wc_SignatureGetSize_rsa(void) #elif !defined(NO_FILESYSTEM) file = XFOPEN(clientKey, "rb"); if (file != XBADFILE) { - bytes = XFREAD(tmp, 1, FOURK_BUF, file); + bytes = (size_t)XFREAD(tmp, 1, FOURK_BUF, file); XFCLOSE(file); } else { @@ -18827,7 +18827,7 @@ static void test_wolfSSL_PEM_PrivateKey(void) f = XFOPEN("./certs/ecc-key.der", "rb"); AssertTrue((f != XBADFILE)); - bytes = XFREAD(buf, 1, sizeof(buf), f); + bytes = (size_t)XFREAD(buf, 1, sizeof(buf), f); XFCLOSE(f); server_key = buf; @@ -24071,12 +24071,12 @@ static void test_wolfSSL_PEM_write_bio_PKCS7(void) fp = XFOPEN("./certs/1024/client-cert.der", "rb"); AssertTrue((fp != XBADFILE)); - certSz = XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); + certSz = (int)XFREAD(cert, 1, sizeof_client_cert_der_1024, fp); XFCLOSE(fp); fp = XFOPEN("./certs/1024/client-key.der", "rb"); AssertTrue(fp != XBADFILE); - keySz = XFREAD(key, 1, sizeof_client_key_der_1024, fp); + keySz = (int)XFREAD(key, 1, sizeof_client_key_der_1024, fp); XFCLOSE(fp); #endif #elif defined(HAVE_ECC) @@ -24097,12 +24097,12 @@ static void test_wolfSSL_PEM_write_bio_PKCS7(void) fp = XFOPEN("./certs/client-ecc-cert.der", "rb"); AssertTrue(fp != XBADFILE); - certSz = XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); + certSz = (int)XFREAD(cert, 1, sizeof_cliecc_cert_der_256, fp); XFCLOSE(fp); fp = XFOPEN("./certs/client-ecc-key.der", "rb"); AssertTrue(fp != XBADFILE); - keySz = XFREAD(key, 1, sizeof_ecc_clikey_der_256, fp); + keySz = (int)XFREAD(key, 1, sizeof_ecc_clikey_der_256, fp); XFCLOSE(fp); #endif #else diff --git a/wolfcrypt/src/pkcs12.c b/wolfcrypt/src/pkcs12.c index ada39177c..399b2a7ad 100644 --- a/wolfcrypt/src/pkcs12.c +++ b/wolfcrypt/src/pkcs12.c @@ -682,7 +682,7 @@ int wc_d2i_PKCS12(const byte* der, word32 derSz, WC_PKCS12* pkcs12) int wc_i2d_PKCS12(WC_PKCS12* pkcs12, byte** der, int* derSz) { int ret = 0; - word32 seqSz, verSz = 0, totalSz = 0, idx = 0, sdBufSz = 0; + word32 seqSz = 0, verSz = 0, totalSz = 0, idx = 0, sdBufSz = 0; byte *buf = NULL; byte ver[MAX_VERSION_SZ]; byte seq[MAX_SEQ_SZ]; From b83aebafb1697ca1c882cd326dd87cfbd8abfc1f Mon Sep 17 00:00:00 2001 From: Jacob Barthelmeh Date: Thu, 22 Aug 2019 11:49:10 -0600 Subject: [PATCH 3/3] help out static analysis tool --- wolfcrypt/src/pkcs12.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/wolfcrypt/src/pkcs12.c b/wolfcrypt/src/pkcs12.c index 399b2a7ad..c221f7b22 100644 --- a/wolfcrypt/src/pkcs12.c +++ b/wolfcrypt/src/pkcs12.c @@ -784,9 +784,6 @@ int wc_i2d_PKCS12(WC_PKCS12* pkcs12, byte** der, int* derSz) if (*der == NULL) { /* Allocate if requested */ buf = (byte*)XMALLOC(totalSz, NULL, DYNAMIC_TYPE_PKCS); - if (buf == NULL) { - ret = MEMORY_E; - } } else { buf = *der; @@ -802,6 +799,10 @@ int wc_i2d_PKCS12(WC_PKCS12* pkcs12, byte** der, int* derSz) } } + if (buf == NULL) { + ret = MEMORY_E; + } + if (ret == 0) { idx = 0;