From 582f0d82e4020003ca833a9989af40f73249e762 Mon Sep 17 00:00:00 2001 From: Chris Conlon Date: Wed, 2 Mar 2022 16:49:59 -0700 Subject: [PATCH] address review feedback for PKCS7 compat additions --- src/ssl.c | 140 ++++++++++++++++++++++++---------------- tests/api.c | 50 +++++++++----- wolfcrypt/src/asn.c | 8 +-- wolfssl/wolfcrypt/asn.h | 2 +- 4 files changed, 123 insertions(+), 77 deletions(-) diff --git a/src/ssl.c b/src/ssl.c index 86b3e56b9..359bb17a6 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -20907,7 +20907,7 @@ size_t wolfSSL_get_client_random(const WOLFSSL* ssl, unsigned char* out, #if (defined(KEEP_PEER_CERT) && defined(SESSION_CERTS)) || \ - (defined(OPENSSL_ALL) && defined(SESSION_CERTS)) + (defined(OPENSSL_EXTRA) && defined(SESSION_CERTS)) /* Decode the X509 DER encoded certificate into a WOLFSSL_X509 object. * * x509 WOLFSSL_X509 object to decode into. @@ -20949,7 +20949,7 @@ size_t wolfSSL_get_client_random(const WOLFSSL* ssl, unsigned char* out, return ret; } -#endif /* (KEEP_PEER_CERT && SESSION_CERTS) || (OPENSSL_ALL && HAVE_PKCS7) */ +#endif /* (KEEP_PEER_CERT & SESSION_CERTS) || (OPENSSL_EXTRA & SESSION_CERTS) */ #ifdef KEEP_PEER_CERT @@ -62007,7 +62007,7 @@ PKCS7* wolfSSL_d2i_PKCS7_ex(PKCS7** p7, const unsigned char** in, int len, WOLFSSL_ENTER("wolfSSL_d2i_PKCS7_ex"); - if (in == NULL || *in == NULL) + if (in == NULL || *in == NULL || len < 0) return NULL; if ((pkcs7 = (WOLFSSL_PKCS7*)wolfSSL_PKCS7_new()) == NULL) @@ -62090,6 +62090,18 @@ error: return NULL; } +/** + * Return stack of signers contained in PKCS7 cert. + * Notes: + * - Currently only PKCS#7 messages with a single signer cert is supported. + * - Returned WOLFSSL_STACK must be freed by caller. + * + * pkcs7 - PKCS7 struct to retrieve signer certs from. + * certs - currently unused + * flags - flags to control function behavior. + * + * Return WOLFSSL_STACK of signers on success, NULL on error. + */ WOLFSSL_STACK* wolfSSL_PKCS7_get0_signers(PKCS7* pkcs7, WOLFSSL_STACK* certs, int flags) { @@ -62201,7 +62213,6 @@ int wolfSSL_i2d_PKCS7(PKCS7 *p7, unsigned char **out) WOLFSSL_MSG("malloc error"); goto cleanup; } - XMEMSET(output, 0, len); localBuf = 1; } else { @@ -62260,24 +62271,24 @@ cleanup: * * Inner content type is set to DATA to match OpenSSL behavior. * - * signer - certificate to sign bundle with - * pkey - private key matching signcert + * signer - certificate to sign bundle with + * pkey - private key matching signer * certs - optional additional set of certificates to include * in - input data to be signed * flags - optional set of flags to control sign behavior * - * PKCS7_BINARY - Do not translate input data to MIME canonical - * format (\r\n line endings), thus preventing corruption of - * binary content. - * PKCS7_TEXT - Prepend MIME headers for text/plain to content. + * PKCS7_BINARY - Do not translate input data to MIME canonical + * format (\r\n line endings), thus preventing corruption of + * binary content. + * PKCS7_TEXT - Prepend MIME headers for text/plain to content. * PKCS7_DETACHED - Set signature detached, omit content from output bundle. - * PKCS7_STREAM - initialize PKCS7 struct for signing, do not read data. + * PKCS7_STREAM - initialize PKCS7 struct for signing, do not read data. * * Flags not currently supported: - * PKCS7_NOCERTS - Do not include the signer cert in the output bundle. - * PKCS7_PARTIAL - Allow for PKCS7_sign() to be only partially set up, - * then signers etc to be added separately before - * calling PKCS7_final(). + * PKCS7_NOCERTS - Do not include the signer cert in the output bundle. + * PKCS7_PARTIAL - Allow for PKCS7_sign() to be only partially set up, + * then signers etc to be added separately before + * calling PKCS7_final(). * * Returns valid PKCS7 structure pointer, or NULL if an error occurred. */ @@ -62341,18 +62352,16 @@ PKCS7* wolfSSL_PKCS7_sign(WOLFSSL_X509* signer, WOLFSSL_EVP_PKEY* pkey, } /* add additional chain certs if provided */ - if (err == 0) { - while (cert && (err == 0)) { - if (cert->data.x509 != NULL && cert->data.x509->derCert != NULL) { - if (wc_PKCS7_AddCertificate(&p7->pkcs7, - cert->data.x509->derCert->buffer, - cert->data.x509->derCert->length) != 0) { - WOLFSSL_MSG("Error in wc_PKCS7_AddCertificate"); - err = 1; - } + while (cert && (err == 0)) { + if (cert->data.x509 != NULL && cert->data.x509->derCert != NULL) { + if (wc_PKCS7_AddCertificate(&p7->pkcs7, + cert->data.x509->derCert->buffer, + cert->data.x509->derCert->length) != 0) { + WOLFSSL_MSG("Error in wc_PKCS7_AddCertificate"); + err = 1; } - cert = cert->next; } + cert = cert->next; } if ((err == 0) && (flags & PKCS7_DETACHED)) { @@ -62424,9 +62433,9 @@ static int wolfSSL_BIO_to_MIME_crlf(WOLFSSL_BIO* in, WOLFSSL_BIO* out) if (line[lineLen - 1] == '\r' || line[lineLen - 1] == '\n') { canonLineLen = (word32)lineLen; - if ((canonLine = wc_MIME_canonicalize( + if ((canonLine = wc_MIME_single_canonicalize( line, &canonLineLen)) == NULL) { - ret = 1; + ret = -1; break; } @@ -62436,23 +62445,23 @@ static int wolfSSL_BIO_to_MIME_crlf(WOLFSSL_BIO* in, WOLFSSL_BIO* out) } if (wolfSSL_BIO_write(out, canonLine, (int)canonLineLen) < 0) { - ret = 1; + ret = -1; break; } - XFREE(canonLine, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(canonLine, NULL, DYNAMIC_TYPE_PKCS7); canonLine = NULL; } else { /* no line ending in current line, write direct to out */ if (wolfSSL_BIO_write(out, line, lineLen) < 0) { - ret = 1; + ret = -1; break; } } } if (canonLine != NULL) { - XFREE(canonLine, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(canonLine, NULL, DYNAMIC_TYPE_PKCS7); } #ifdef WOLFSSL_SMALL_STACK XFREE(line, in->heap, DYNAMIC_TYPE_TMP_BUFFER); @@ -62463,10 +62472,13 @@ static int wolfSSL_BIO_to_MIME_crlf(WOLFSSL_BIO* in, WOLFSSL_BIO* out) #endif /* HAVE_SMIME */ +/* Used by both PKCS7_final() and PKCS7_verify() */ +static const char contTypeText[] = "Content-Type: text/plain\r\n\r\n"; + /** * Finalize PKCS7 structure, currently supports signedData only. * - * Does not do generation of final bundle (ie: signedData), but finalizes + * Does not generate final bundle (ie: signedData), but finalizes * the PKCS7 structure in preparation for a output function to be called next. * * pkcs7 - initialized PKCS7 structure, populated with signer, etc @@ -62477,7 +62489,7 @@ static int wolfSSL_BIO_to_MIME_crlf(WOLFSSL_BIO* in, WOLFSSL_BIO* out) * PKCS7_BINARY - Do not translate input data to MIME canonical * format (\r\n line endings), thus preventing corruption of * binary content. - * PKCS7_TEXT - Prepend MIME headers for text/plain to content. + * PKCS7_TEXT - Prepend MIME headers for text/plain to content. * * Returns 1 on success, 0 on error */ @@ -62488,7 +62500,6 @@ int wolfSSL_PKCS7_final(PKCS7* pkcs7, WOLFSSL_BIO* in, int flags) unsigned char* mem = NULL; WOLFSSL_PKCS7* p7 = (WOLFSSL_PKCS7*)pkcs7; WOLFSSL_BIO* data = NULL; - static const char contTypeText[] = "Content-Type: text/plain\r\n\r\n"; WOLFSSL_ENTER("wolfSSL_PKCS7_final"); @@ -62504,11 +62515,11 @@ int wolfSSL_PKCS7_final(PKCS7* pkcs7, WOLFSSL_BIO* in, int flags) } } - /* append Content-Type header if PKCS7_TEXT */ + /* prepend Content-Type header if PKCS7_TEXT */ if ((ret == 1) && (flags & PKCS7_TEXT)) { if (wolfSSL_BIO_write(data, contTypeText, - (int)XSTRLEN(contTypeText)) < 0) { - WOLFSSL_MSG("Error appending Content-Type header"); + (int)XSTR_SIZEOF(contTypeText)) < 0) { + WOLFSSL_MSG("Error prepending Content-Type header"); ret = 0; } } @@ -62516,15 +62527,33 @@ int wolfSSL_PKCS7_final(PKCS7* pkcs7, WOLFSSL_BIO* in, int flags) /* convert line endings to CRLF if !PKCS7_BINARY */ if (ret == 1) { if (flags & PKCS7_BINARY) { + /* no CRLF conversion, direct copy content */ - if ((memSz = wolfSSL_BIO_get_mem_data(in, &mem)) < 0) { + if ((memSz = wolfSSL_BIO_get_len(in)) <= 0) { ret = 0; } - else if (wolfSSL_BIO_write(data, mem, memSz) < 0) { - ret = 0; + if (ret == 1) { + mem = (unsigned char*)XMALLOC(memSz, in->heap, + DYNAMIC_TYPE_TMP_BUFFER); + if (mem == NULL) { + WOLFSSL_MSG("Failed to allocate memory for input data"); + ret = 0; + } + } + + if (ret == 1) { + if (wolfSSL_BIO_read(in, mem, memSz) != memSz) { + WOLFSSL_MSG("Error reading from input BIO"); + ret = 0; + } + else if (wolfSSL_BIO_write(data, mem, memSz) < 0) { + ret = 0; + } + } + + if (mem != NULL) { + XFREE(mem, in->heap, DYNAMIC_TYPE_TMP_BUFFER); } - mem = NULL; - memSz = 0; } else { #ifdef HAVE_SMIME @@ -62582,7 +62611,6 @@ int wolfSSL_PKCS7_verify(PKCS7* pkcs7, WOLFSSL_STACK* certs, unsigned char* mem = NULL; int memSz = 0; WOLFSSL_PKCS7* p7 = (WOLFSSL_PKCS7*)pkcs7; - static const char contTypeText[] = "Content-Type: text/plain\r\n\r\n"; int contTypeLen; WOLFSSL_X509* signer = NULL; WOLFSSL_STACK* signers = NULL; @@ -62631,7 +62659,7 @@ int wolfSSL_PKCS7_verify(PKCS7* pkcs7, WOLFSSL_STACK* certs, signer->derCert->length, WOLFSSL_FILETYPE_ASN1) != WOLFSSL_SUCCESS) { WOLFSSL_MSG("Failed to verify signer certificate"); - wolfSSL_sk_X509_free(signers); + wolfSSL_sk_X509_pop_free(signers, NULL); return WOLFSSL_FAILURE; } } @@ -62654,6 +62682,8 @@ int wolfSSL_PKCS7_verify(PKCS7* pkcs7, WOLFSSL_STACK* certs, wolfSSL_BIO_write(out, p7->pkcs7.content, p7->pkcs7.contentSz); } + WOLFSSL_LEAVE("wolfSSL_PKCS7_verify", WOLFSSL_SUCCESS); + return WOLFSSL_SUCCESS; } @@ -62903,6 +62933,7 @@ WOLFSSL_API PKCS7* wolfSSL_SMIME_read_PKCS7(WOLFSSL_BIO* in, static const char kAppPkcs7Mime[] = "application/pkcs7-mime"; static const char kAppXPkcs7Mime[] = "application/x-pkcs7-mime"; + WOLFSSL_ENTER("wolfSSL_SMIME_read_PKCS7"); if (in == NULL || bcont == NULL) { goto error; @@ -63000,8 +63031,8 @@ WOLFSSL_API PKCS7* wolfSSL_SMIME_read_PKCS7(WOLFSSL_BIO* in, while (XSTRNCMP(§ion[sectionLen], boundary, boundLen) && remainLen > 0) { canonLineLen = lineLen; - canonLine = wc_MIME_canonicalize(§ion[sectionLen], - &canonLineLen); + canonLine = wc_MIME_single_canonicalize(§ion[sectionLen], + &canonLineLen); if (canonLine == NULL) { goto error; } @@ -63265,7 +63296,6 @@ int wolfSSL_SMIME_write_PKCS7(WOLFSSL_BIO* out, PKCS7* pkcs7, WOLFSSL_BIO* in, byte* p7out = NULL; int len = 0; - WC_RNG rng; char boundary[33]; /* 32 chars + \0 */ byte* sigBase64 = NULL; word32 sigBase64Len = 0; @@ -63321,22 +63351,22 @@ int wolfSSL_SMIME_write_PKCS7(WOLFSSL_BIO* out, PKCS7* pkcs7, WOLFSSL_BIO* in, if (flags & PKCS7_DETACHED) { /* generate random boundary */ - if (wc_InitRng(&rng) != 0) { - WOLFSSL_MSG("Error in wc_InitRng"); + if (initGlobalRNG == 0 && wolfSSL_RAND_Init() != WOLFSSL_SUCCESS) { + WOLFSSL_MSG("No RNG to use"); ret = 0; } - if ((ret > 0) && (wc_RNG_GenerateBlock(&rng, (byte*)boundary, - sizeof(boundary)) != 0)) { + /* no need to generate random byte for null terminator (size-1) */ + if ((ret > 0) && (wc_RNG_GenerateBlock(&globalRNG, (byte*)boundary, + sizeof(boundary) - 1 ) != 0)) { WOLFSSL_MSG("Error in wc_RNG_GenerateBlock"); ret = 0; } - wc_FreeRng(&rng); if (ret > 0) { for (i = 0; i < (int)sizeof(boundary) - 1; i++) { boundary[i] = - alphanum[boundary[i] % (sizeof(alphanum) - 1)]; + alphanum[boundary[i] % XSTR_SIZEOF(alphanum)]; } boundary[sizeof(boundary)-1] = 0; } @@ -63411,10 +63441,10 @@ int wolfSSL_SMIME_write_PKCS7(WOLFSSL_BIO* out, PKCS7* pkcs7, WOLFSSL_BIO* in, } if (ret > 0) { - return 1; + return WOLFSSL_SUCCESS; } - return 0; + return WOLFSSL_FAILURE; } #endif /* HAVE_SMIME */ diff --git a/tests/api.c b/tests/api.c index 31cfacc38..2fdfe9234 100644 --- a/tests/api.c +++ b/tests/api.c @@ -46923,16 +46923,20 @@ static void test_wolfSSL_PKCS7_sign(void) AssertIntEQ(wc_PKCS7_VerifySignedData(p7Ver, out, outLen), 0); wc_PKCS7_Free(p7Ver); - if (out != NULL) { - XFREE(out, NULL, DYNAMIC_TYPE_TMP_BUFFER); - out = NULL; - } + AssertNotNull(out); + XFREE(out, NULL, DYNAMIC_TYPE_TMP_BUFFER); + out = NULL; PKCS7_free(p7); } /* TEST SUCCESS: Not detached, streaming, not MIME. Also bad arg * tests for PKCS7_final() while we have a PKCS7 pointer to use */ { + /* re-populate input BIO, may have been consumed */ + BIO_free(inBio); + AssertNotNull(inBio = BIO_new(BIO_s_mem())); + AssertIntGT(BIO_write(inBio, data, sizeof(data)), 0); + flags = PKCS7_BINARY | PKCS7_STREAM; AssertNotNull(p7 = PKCS7_sign(signCert, signKey, NULL, inBio, flags)); AssertIntEQ(PKCS7_final(p7, inBio, flags), 1); @@ -46948,15 +46952,19 @@ static void test_wolfSSL_PKCS7_sign(void) AssertIntEQ(PKCS7_verify(p7Ver, NULL, store, NULL, NULL, flags), 1); PKCS7_free(p7Ver); - if (out != NULL) { - XFREE(out, NULL, DYNAMIC_TYPE_TMP_BUFFER); - out = NULL; - } + AssertNotNull(out); + XFREE(out, NULL, DYNAMIC_TYPE_TMP_BUFFER); + out = NULL; PKCS7_free(p7); } /* TEST SUCCESS: Detached, not streaming, not MIME */ { + /* re-populate input BIO, may have been consumed */ + BIO_free(inBio); + AssertNotNull(inBio = BIO_new(BIO_s_mem())); + AssertIntGT(BIO_write(inBio, data, sizeof(data)), 0); + flags = PKCS7_BINARY | PKCS7_DETACHED; AssertNotNull(p7 = PKCS7_sign(signCert, signKey, NULL, inBio, flags)); AssertIntGT((outLen = i2d_PKCS7(p7, &out)), 0); @@ -46968,15 +46976,25 @@ static void test_wolfSSL_PKCS7_sign(void) AssertIntEQ(wc_PKCS7_VerifySignedData(p7Ver, out, outLen), 0); wc_PKCS7_Free(p7Ver); - if (out != NULL) { - XFREE(out, NULL, DYNAMIC_TYPE_TMP_BUFFER); - out = NULL; - } + /* verify expected failure (NULL return) from d2i_PKCS7, it does not + * yet support detached content */ + tmpPtr = out; + AssertNull(p7Ver = d2i_PKCS7(NULL, (const byte**)&tmpPtr, outLen)); + PKCS7_free(p7Ver); + + AssertNotNull(out); + XFREE(out, NULL, DYNAMIC_TYPE_TMP_BUFFER); + out = NULL; PKCS7_free(p7); } /* TEST SUCCESS: Detached, streaming, not MIME */ { + /* re-populate input BIO, may have been consumed */ + BIO_free(inBio); + AssertNotNull(inBio = BIO_new(BIO_s_mem())); + AssertIntGT(BIO_write(inBio, data, sizeof(data)), 0); + flags = PKCS7_BINARY | PKCS7_DETACHED | PKCS7_STREAM; AssertNotNull(p7 = PKCS7_sign(signCert, signKey, NULL, inBio, flags)); AssertIntEQ(PKCS7_final(p7, inBio, flags), 1); @@ -46989,10 +47007,8 @@ static void test_wolfSSL_PKCS7_sign(void) AssertIntEQ(wc_PKCS7_VerifySignedData(p7Ver, out, outLen), 0); wc_PKCS7_Free(p7Ver); - if (out != NULL) { - XFREE(out, NULL, DYNAMIC_TYPE_TMP_BUFFER); - out = NULL; - } + AssertNotNull(out); + XFREE(out, NULL, DYNAMIC_TYPE_TMP_BUFFER); PKCS7_free(p7); } @@ -47210,7 +47226,7 @@ static void test_wolfSSL_SMIME_read_PKCS7(void) AssertIntEQ(wolfSSL_PKCS7_verify(pkcs7, NULL, NULL, bcont, out, PKCS7_NOVERIFY | PKCS7_TEXT), SSL_SUCCESS); AssertIntGT((outBufLen = BIO_get_mem_data(out, &outBuf)), 0); - /* Content-Type should not show up in output buffer */ + /* Content-Type should not show up at beginning of output buffer */ AssertIntGT(outBufLen, XSTRLEN(contTypeText)); AssertIntGT(XMEMCMP(outBuf, contTypeText, XSTRLEN(contTypeText)), 0); diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index fbfda432d..4126d8c4b 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -32557,8 +32557,8 @@ MimeParam* wc_MIME_find_param_attr(const char* attribute, } /***************************************************************************** -* wc_MIME_canonicalize - Canonicalize a line by converting all line endings -* to CRLF. +* wc_MIME_single_canonicalize - Canonicalize a line by converting the trailing +* line ending to CRLF. * * line - input line to canonicalize * len - length of line in chars on input, length of output array on return @@ -32566,7 +32566,7 @@ MimeParam* wc_MIME_find_param_attr(const char* attribute, * RETURNS: * returns a pointer to a canonicalized line on success, NULL on error. */ -char* wc_MIME_canonicalize(const char* line, word32* len) +char* wc_MIME_single_canonicalize(const char* line, word32* len) { size_t end = 0; char* canonLine = NULL; @@ -32575,7 +32575,7 @@ char* wc_MIME_canonicalize(const char* line, word32* len) return NULL; } - end = *len - 1 ; + end = *len; while (end >= 1 && ((line[end-1] == '\r') || (line[end-1] == '\n'))) { end--; } diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 46039afc5..55241a32d 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -2033,7 +2033,7 @@ WOLFSSL_LOCAL int wc_MIME_parse_headers(char* in, int inLen, MimeHdr** hdrs); WOLFSSL_LOCAL int wc_MIME_header_strip(char* in, char** out, size_t start, size_t end); WOLFSSL_LOCAL MimeHdr* wc_MIME_find_header_name(const char* name, MimeHdr* hdr); WOLFSSL_LOCAL MimeParam* wc_MIME_find_param_attr(const char* attribute, MimeParam* param); -WOLFSSL_LOCAL char* wc_MIME_canonicalize(const char* line, word32* len); +WOLFSSL_LOCAL char* wc_MIME_single_canonicalize(const char* line, word32* len); WOLFSSL_LOCAL int wc_MIME_free_hdrs(MimeHdr* head); #endif /* HAVE_SMIME */