From bb9c1bb25335c9395e998cca72d99dc737714c5d Mon Sep 17 00:00:00 2001 From: Lealem Amedie Date: Fri, 3 Mar 2023 15:58:17 -0700 Subject: [PATCH 1/4] Adding support for indefinite length PKCS12 --- wolfcrypt/src/pkcs12.c | 218 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 208 insertions(+), 10 deletions(-) diff --git a/wolfcrypt/src/pkcs12.c b/wolfcrypt/src/pkcs12.c index e3a8c55b7..25ab019cd 100644 --- a/wolfcrypt/src/pkcs12.c +++ b/wolfcrypt/src/pkcs12.c @@ -110,6 +110,10 @@ struct WC_PKCS12 { AuthenticatedSafe* safe; MacData* signData; word32 oid; /* DATA / Enveloped DATA ... */ +#ifdef ASN_BER_TO_DER + byte* der; /* DER encoded version of message */ + word32 derSz; +#endif }; @@ -188,6 +192,13 @@ void wc_PKCS12_free(WC_PKCS12* pkcs12) pkcs12->signData = NULL; } +#ifdef ASN_BER_TO_DER + if (pkcs12->der != NULL) { + XFREE(pkcs12->der, pkcs12->heap, DYNAMIC_TYPE_PKCS); + pkcs12->der = NULL; + } +#endif + XFREE(pkcs12, NULL, DYNAMIC_TYPE_PKCS); } @@ -269,15 +280,39 @@ static int GetSafeContent(WC_PKCS12* pkcs12, const byte* input, XMEMCPY(safe->data, input + localIdx, size); *idx = localIdx; + localIdx = 0; + input = safe->data; + size = safe->dataSz; + +#ifdef ASN_BER_TO_DER + if (pkcs12->der) { + byte* der; + + if ((ret = wc_BerToDer(input, safe->dataSz, NULL, + (word32*)&size)) != LENGTH_ONLY_E) { + WOLFSSL_MSG("Not BER sequence"); + return ASN_PARSE_E; + } + + der = (byte*)XMALLOC(size, pkcs12->heap, DYNAMIC_TYPE_PKCS); + if (der == NULL) { + freeSafe(safe, pkcs12->heap); + return MEMORY_E; + } + + ret = wc_BerToDer(input, safe->dataSz, der, (word32*)&size); + + input = der; + } +#endif /* ASN_BER_TO_DER */ + /* an instance of AuthenticatedSafe is created from * ContentInfo's strung together in a SEQUENCE. Here we iterate * through the ContentInfo's and add them to our * AuthenticatedSafe struct */ - localIdx = 0; - input = safe->data; { int CISz; - ret = GetSequence(input, &localIdx, &CISz, safe->dataSz); + ret = GetSequence(input, &localIdx, &CISz, size); if (ret < 0) { freeSafe(safe, pkcs12->heap); return ASN_PARSE_E; @@ -292,8 +327,7 @@ static int GetSafeContent(WC_PKCS12* pkcs12, const byte* input, printf("\t\tlooking for Content Info.... "); #endif - if ((ret = GetSequence(input, &localIdx, &curSz, safe->dataSz)) - < 0) { + if ((ret = GetSequence(input, &localIdx, &curSz, size)) < 0) { freeSafe(safe, pkcs12->heap); return ret; } @@ -306,7 +340,7 @@ static int GetSafeContent(WC_PKCS12* pkcs12, const byte* input, curIdx = localIdx; if ((ret = GetObjectId(input, &localIdx, &oid, oidIgnoreType, - safe->dataSz)) < 0) { + size)) < 0) { WOLFSSL_LEAVE("Get object id failed", ret); freeSafe(safe, pkcs12->heap); return ret; @@ -651,7 +685,7 @@ int wc_d2i_PKCS12(const byte* der, word32 derSz, WC_PKCS12* pkcs12) } totalSz = derSz; - if (GetSequence(der, &idx, &size, totalSz) <= 0) { + if (GetSequence(der, &idx, &size, totalSz) < 0) { WOLFSSL_MSG("Failed to get PKCS12 sequence"); return ASN_PARSE_E; } @@ -661,6 +695,39 @@ int wc_d2i_PKCS12(const byte* der, word32 derSz, WC_PKCS12* pkcs12) return ret; } + #ifdef ASN_BER_TO_DER + if (size == 0) { + if ((ret = wc_BerToDer(der, totalSz, NULL, + (word32*)&size)) != LENGTH_ONLY_E) { + WOLFSSL_MSG("Not BER sequence"); + return ASN_PARSE_E; + } + + pkcs12->der = (byte*)XMALLOC(size, pkcs12->heap, DYNAMIC_TYPE_PKCS); + if (pkcs12->der == NULL) + return MEMORY_E; + ret = wc_BerToDer(der, derSz, pkcs12->der, (word32*)&size); + if (ret < 0) { + return ret; + } + + der = pkcs12->der; + derSz = pkcs12->derSz = size; + totalSz = size; + idx = 0; + + if ((ret = GetSequence(der, &idx, &size, totalSz)) < 0) { + WOLFSSL_MSG("Failed to get PKCS12 sequence"); + return ASN_PARSE_E; + } + + /* get version */ + if ((ret = GetMyVersion(der, &idx, &version, totalSz)) < 0) { + return ret; + } + } +#endif /* ASN_BER_TO_DER */ + #ifdef WOLFSSL_DEBUG_PKCS12 printf("\nBEGIN: PKCS12 size = %d\n", totalSz); printf("version = %d\n", version); @@ -685,6 +752,15 @@ int wc_d2i_PKCS12(const byte* der, word32 derSz, WC_PKCS12* pkcs12) return ret; } +#ifdef ASN_BER_TO_DER + /* If indef, skip EOF */ + if (pkcs12->der) { + while(der[idx] == ASN_EOC) { + idx+=1; + } + } +#endif + /* if more buffer left check for MAC data */ if (idx < totalSz) { if ((ret = GetSequence(der, &idx, &size, totalSz)) < 0) { @@ -1060,6 +1136,35 @@ static WARN_UNUSED_RESULT int freeDecCertList(WC_DerCertList** list, return 0; } +#ifdef ASN_BER_TO_DER +/* append data to encrypted content cache in PKCS12 structure + * return buffer on success, NULL on error */ +static byte* PKCS12_ConcatonateContent(byte* mergedData, word32* mergedSz, byte* in, word32 inSz) +{ + byte* oldContent; + word32 oldContentSz; + + if (mergedData == NULL || in == NULL) + return NULL; + + /* save pointer to old cache */ + oldContent = mergedData; + oldContentSz = *mergedSz; + + /* re-allocate new buffer to fit appended data */ + mergedData = (byte*)XMALLOC(oldContentSz + inSz, NULL, DYNAMIC_TYPE_PKCS); + + if (oldContent != NULL) { + XMEMCPY(mergedData, oldContent, oldContentSz); + } + XMEMCPY(mergedData + oldContentSz, in, inSz); + *mergedSz += inSz; + + XFREE(oldContent, NULL, DYNAMIC_TYPE_PKCS); + + return mergedData; +} +#endif /* return 0 on success and negative on failure. * By side effect returns private key, cert, and optionally ca. @@ -1123,11 +1228,12 @@ int wc_PKCS12_parse(WC_PKCS12* pkcs12, const char* psw, int size, totalSz; byte tag; + data = ci->data; + if (ci->type == WC_PKCS12_ENCRYPTED_DATA) { int number; WOLFSSL_MSG("Decrypting PKCS12 Content Info Container"); - data = ci->data; if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) { ERROR_OUT(ASN_PARSE_E, exit_pk12par); } @@ -1161,7 +1267,100 @@ int wc_PKCS12_parse(WC_PKCS12* pkcs12, const char* psw, ERROR_OUT(ASN_PARSE_E, exit_pk12par); } - /* decrypted content overwrites input buffer */ + +#ifdef ASN_BER_TO_DER + /* If indefinite length format, ensure it is in the ASN format + * the DecryptContent() expects */ + if (pkcs12->der) { + byte* mergedData = NULL; /* buffer for concatonated strings */ + word32 mergedSz = 0; /* total size of merged strings */ + int encryptedContentSz = 0; + int originalEncSz = 0; + int curIdx = idx; + int saveIdx; + + if ((ret = GetSequence(data, &idx, &size, ci->dataSz)) < 0) { + goto exit_pk12par; + } + + ret = GetObjectId(data, &idx, &oid, oidIgnoreType, ci->dataSz); + if (ret < 0) { + goto exit_pk12par; + } + + if ((ret = GetSequence(data, &idx, &size, ci->dataSz)) < 0) { + goto exit_pk12par; + } + + if ((ret = GetOctetString(data, &idx, &size, ci->dataSz)) < 0) { + goto exit_pk12par; + } + + idx += size; + if ((ret = GetShortInt(data, &idx, &number, ci->dataSz)) < 0) { + goto exit_pk12par; + } + + /* Check if wc_BerToDer() handled constructed [0] and octet + * strings properly, manually fix it if not. */ + if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) { + goto exit_pk12par; + } + else if (tag == 0xa0) { + data[idx-1] = ASN_LONG_LENGTH; + saveIdx = idx; + + if (GetLength(data, &idx, &originalEncSz, ci->dataSz) < 0) { + ret = ASN_PARSE_E; + } + + ret = 0; + + /* Loop through octet strings and concatonate them without + * the tags and length */ + while ((int)idx < originalEncSz + curIdx) { + if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) { + ret = ASN_PARSE_E; + } + if (ret == 0 && (tag != ASN_OCTET_STRING)) { + ret = ASN_PARSE_E; + } + if (ret == 0 && GetLength(data, &idx, + &encryptedContentSz, ci->dataSz) <= 0) { + ret = ASN_PARSE_E; + } + if (ret == 0) { + if (mergedData == NULL) { + mergedData = (byte*)XMALLOC(encryptedContentSz, + pkcs12->heap, DYNAMIC_TYPE_PKCS); + if (mergedData == NULL) { + ERROR_OUT(MEMORY_E, exit_pk12par); + } + } + mergedData = PKCS12_ConcatonateContent(mergedData, + &mergedSz, &data[idx], encryptedContentSz); + } + if (ret != 0) { + break; + } + idx += encryptedContentSz; + } + + idx = saveIdx; + + idx += SetLength(mergedSz, &data[idx]); + + /* Copy over concatonated octet strings into data buffer */ + XMEMCPY(&data[idx], mergedData, mergedSz); + + idx += 1; + } + + idx = curIdx; + } +#endif + + /* decrypted content overwrites input buffer */ size = ci->dataSz - idx; buf = (byte*)XMALLOC(size, pkcs12->heap, DYNAMIC_TYPE_PKCS); if (buf == NULL) { @@ -1189,7 +1388,6 @@ int wc_PKCS12_parse(WC_PKCS12* pkcs12, const char* psw, } else { /* type DATA */ WOLFSSL_MSG("Parsing PKCS12 DATA Content Info Container"); - data = ci->data; if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) { ERROR_OUT(ASN_PARSE_E, exit_pk12par); } From d9429185d8f77ad65279e6750e8e5113cc662319 Mon Sep 17 00:00:00 2001 From: Lealem Amedie Date: Mon, 6 Mar 2023 10:42:23 -0700 Subject: [PATCH 2/4] Addressing some review feedback --- wolfcrypt/src/pkcs12.c | 194 +++++++++++++++++++++++------------------ 1 file changed, 108 insertions(+), 86 deletions(-) diff --git a/wolfcrypt/src/pkcs12.c b/wolfcrypt/src/pkcs12.c index 25ab019cd..aeb03e382 100644 --- a/wolfcrypt/src/pkcs12.c +++ b/wolfcrypt/src/pkcs12.c @@ -110,6 +110,7 @@ struct WC_PKCS12 { AuthenticatedSafe* safe; MacData* signData; word32 oid; /* DATA / Enveloped DATA ... */ + byte indefinite; #ifdef ASN_BER_TO_DER byte* der; /* DER encoded version of message */ word32 derSz; @@ -285,7 +286,7 @@ static int GetSafeContent(WC_PKCS12* pkcs12, const byte* input, size = safe->dataSz; #ifdef ASN_BER_TO_DER - if (pkcs12->der) { + if (pkcs12->indefinite) { byte* der; if ((ret = wc_BerToDer(input, safe->dataSz, NULL, @@ -725,8 +726,15 @@ int wc_d2i_PKCS12(const byte* der, word32 derSz, WC_PKCS12* pkcs12) if ((ret = GetMyVersion(der, &idx, &version, totalSz)) < 0) { return ret; } + + pkcs12->indefinite = 1; + } + else #endif /* ASN_BER_TO_DER */ + { + pkcs12->indefinite = 0; + } #ifdef WOLFSSL_DEBUG_PKCS12 printf("\nBEGIN: PKCS12 size = %d\n", totalSz); @@ -754,7 +762,7 @@ int wc_d2i_PKCS12(const byte* der, word32 derSz, WC_PKCS12* pkcs12) #ifdef ASN_BER_TO_DER /* If indef, skip EOF */ - if (pkcs12->der) { + if (pkcs12->indefinite) { while(der[idx] == ASN_EOC) { idx+=1; } @@ -1139,7 +1147,8 @@ static WARN_UNUSED_RESULT int freeDecCertList(WC_DerCertList** list, #ifdef ASN_BER_TO_DER /* append data to encrypted content cache in PKCS12 structure * return buffer on success, NULL on error */ -static byte* PKCS12_ConcatonateContent(byte* mergedData, word32* mergedSz, byte* in, word32 inSz) +static byte* PKCS12_ConcatonateContent(byte* mergedData, word32* mergedSz, + byte* in, word32 inSz) { byte* oldContent; word32 oldContentSz; @@ -1164,6 +1173,100 @@ static byte* PKCS12_ConcatonateContent(byte* mergedData, word32* mergedSz, byte* return mergedData; } + + + +static int PKCS12_HandleIndefinite(WC_PKCS12* pkcs12, byte* data, word32 dataSz, + word32* idx) +{ + byte* mergedData = NULL; /* buffer for concatonated strings */ + word32 mergedSz = 0; /* total size of merged strings */ + word32 oid; + int encryptedContentSz = 0; + int originalEncSz = 0; + int ret = 0; + int curIdx = *idx; + int saveIdx, number, size; + byte tag; + + if (GetSequence(data, idx, &size, dataSz) < 0) { + ret = ASN_PARSE_E; + } + + if (ret == 0 && GetObjectId(data, idx, &oid, oidIgnoreType, dataSz)) { + ret = ASN_PARSE_E; + } + + if (ret == 0 && GetSequence(data, idx, &size, dataSz) < 0) { + ret = ASN_PARSE_E; + } + + if (ret == 0 && GetOctetString(data, idx, &size, dataSz) < 0) { + ret = ASN_PARSE_E; + } + + *idx += size; + if (ret == 0 && GetShortInt(data, idx, &number, dataSz) < 0) { + ret = ASN_PARSE_E; + } + + /* Check if wc_BerToDer() handled constructed [0] and octet + * strings properly, manually fix it if not. */ + if (ret == 0 && GetASNTag(data, idx, &tag, dataSz) < 0) { + ret = ASN_PARSE_E; + } + else if (ret == 0 && tag == 0xa0) { + data[*idx-1] = ASN_LONG_LENGTH; + saveIdx = *idx; + + if (GetLength(data, idx, &originalEncSz, dataSz) < 0) { + ret = ASN_PARSE_E; + } + + /* Loop through octet strings and concatonate them without + * the tags and length */ + while ((int)*idx < originalEncSz + curIdx) { + if (GetASNTag(data, idx, &tag, dataSz) < 0) { + ret = ASN_PARSE_E; + } + if (ret == 0 && (tag != ASN_OCTET_STRING)) { + ret = ASN_PARSE_E; + } + if (ret == 0 && GetLength(data, idx, + &encryptedContentSz, dataSz) <= 0) { + ret = ASN_PARSE_E; + } + if (ret == 0) { + if (mergedData == NULL) { + mergedData = (byte*)XMALLOC(encryptedContentSz, + pkcs12->heap, DYNAMIC_TYPE_PKCS); + if (mergedData == NULL) { + ret = MEMORY_E; + } + } + mergedData = PKCS12_ConcatonateContent(mergedData, + &mergedSz, &data[*idx], encryptedContentSz); + } + if (ret != 0) { + break; + } + *idx += encryptedContentSz; + } + + *idx = saveIdx; + + *idx += SetLength(mergedSz, &data[*idx]); + + /* Copy over concatonated octet strings into data buffer */ + XMEMCPY(&data[*idx], mergedData, mergedSz); + + *idx += 1; + } + + *idx = curIdx; + + return ret; +} #endif /* return 0 on success and negative on failure. @@ -1271,92 +1374,11 @@ int wc_PKCS12_parse(WC_PKCS12* pkcs12, const char* psw, #ifdef ASN_BER_TO_DER /* If indefinite length format, ensure it is in the ASN format * the DecryptContent() expects */ - if (pkcs12->der) { - byte* mergedData = NULL; /* buffer for concatonated strings */ - word32 mergedSz = 0; /* total size of merged strings */ - int encryptedContentSz = 0; - int originalEncSz = 0; - int curIdx = idx; - int saveIdx; - - if ((ret = GetSequence(data, &idx, &size, ci->dataSz)) < 0) { - goto exit_pk12par; - } - - ret = GetObjectId(data, &idx, &oid, oidIgnoreType, ci->dataSz); + if (pkcs12->indefinite) { + ret = PKCS12_HandleIndefinite(pkcs12, data, ci->dataSz, &idx); if (ret < 0) { goto exit_pk12par; } - - if ((ret = GetSequence(data, &idx, &size, ci->dataSz)) < 0) { - goto exit_pk12par; - } - - if ((ret = GetOctetString(data, &idx, &size, ci->dataSz)) < 0) { - goto exit_pk12par; - } - - idx += size; - if ((ret = GetShortInt(data, &idx, &number, ci->dataSz)) < 0) { - goto exit_pk12par; - } - - /* Check if wc_BerToDer() handled constructed [0] and octet - * strings properly, manually fix it if not. */ - if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) { - goto exit_pk12par; - } - else if (tag == 0xa0) { - data[idx-1] = ASN_LONG_LENGTH; - saveIdx = idx; - - if (GetLength(data, &idx, &originalEncSz, ci->dataSz) < 0) { - ret = ASN_PARSE_E; - } - - ret = 0; - - /* Loop through octet strings and concatonate them without - * the tags and length */ - while ((int)idx < originalEncSz + curIdx) { - if (GetASNTag(data, &idx, &tag, ci->dataSz) < 0) { - ret = ASN_PARSE_E; - } - if (ret == 0 && (tag != ASN_OCTET_STRING)) { - ret = ASN_PARSE_E; - } - if (ret == 0 && GetLength(data, &idx, - &encryptedContentSz, ci->dataSz) <= 0) { - ret = ASN_PARSE_E; - } - if (ret == 0) { - if (mergedData == NULL) { - mergedData = (byte*)XMALLOC(encryptedContentSz, - pkcs12->heap, DYNAMIC_TYPE_PKCS); - if (mergedData == NULL) { - ERROR_OUT(MEMORY_E, exit_pk12par); - } - } - mergedData = PKCS12_ConcatonateContent(mergedData, - &mergedSz, &data[idx], encryptedContentSz); - } - if (ret != 0) { - break; - } - idx += encryptedContentSz; - } - - idx = saveIdx; - - idx += SetLength(mergedSz, &data[idx]); - - /* Copy over concatonated octet strings into data buffer */ - XMEMCPY(&data[idx], mergedData, mergedSz); - - idx += 1; - } - - idx = curIdx; } #endif From 13867dab12af34043283e89c24a883f1c7c7821e Mon Sep 17 00:00:00 2001 From: Lealem Amedie Date: Wed, 8 Mar 2023 13:14:16 -0700 Subject: [PATCH 3/4] Fix leak and address more feedback --- wolfcrypt/src/pkcs12.c | 168 +++++++++++++++++++++++------------------ 1 file changed, 96 insertions(+), 72 deletions(-) diff --git a/wolfcrypt/src/pkcs12.c b/wolfcrypt/src/pkcs12.c index aeb03e382..65b1aabe7 100644 --- a/wolfcrypt/src/pkcs12.c +++ b/wolfcrypt/src/pkcs12.c @@ -112,7 +112,9 @@ struct WC_PKCS12 { word32 oid; /* DATA / Enveloped DATA ... */ byte indefinite; #ifdef ASN_BER_TO_DER - byte* der; /* DER encoded version of message */ + byte* safeDer; /* der encoded version of message */ + byte* der; /* der encoded version of message */ + word32 safeDersz; word32 derSz; #endif }; @@ -198,6 +200,10 @@ void wc_PKCS12_free(WC_PKCS12* pkcs12) XFREE(pkcs12->der, pkcs12->heap, DYNAMIC_TYPE_PKCS); pkcs12->der = NULL; } + if (pkcs12->safeDer != NULL) { + XFREE(pkcs12->safeDer, pkcs12->heap, DYNAMIC_TYPE_PKCS); + pkcs12->safeDer = NULL; + } #endif XFREE(pkcs12, NULL, DYNAMIC_TYPE_PKCS); @@ -287,23 +293,22 @@ static int GetSafeContent(WC_PKCS12* pkcs12, const byte* input, #ifdef ASN_BER_TO_DER if (pkcs12->indefinite) { - byte* der; - if ((ret = wc_BerToDer(input, safe->dataSz, NULL, - (word32*)&size)) != LENGTH_ONLY_E) { + &pkcs12->safeDersz)) != LENGTH_ONLY_E) { WOLFSSL_MSG("Not BER sequence"); return ASN_PARSE_E; } - der = (byte*)XMALLOC(size, pkcs12->heap, DYNAMIC_TYPE_PKCS); - if (der == NULL) { + pkcs12->safeDer = (byte*)XMALLOC(pkcs12->safeDersz, pkcs12->heap, + DYNAMIC_TYPE_PKCS); + if (pkcs12->safeDer == NULL) { freeSafe(safe, pkcs12->heap); return MEMORY_E; } - ret = wc_BerToDer(input, safe->dataSz, der, (word32*)&size); + ret = wc_BerToDer(input, safe->dataSz, pkcs12->safeDer, &pkcs12->safeDersz); - input = der; + input = pkcs12->safeDer; } #endif /* ASN_BER_TO_DER */ @@ -763,7 +768,7 @@ int wc_d2i_PKCS12(const byte* der, word32 derSz, WC_PKCS12* pkcs12) #ifdef ASN_BER_TO_DER /* If indef, skip EOF */ if (pkcs12->indefinite) { - while(der[idx] == ASN_EOC) { + while(der[idx] == ASN_EOC && idx < totalSz) { idx+=1; } } @@ -1147,8 +1152,8 @@ static WARN_UNUSED_RESULT int freeDecCertList(WC_DerCertList** list, #ifdef ASN_BER_TO_DER /* append data to encrypted content cache in PKCS12 structure * return buffer on success, NULL on error */ -static byte* PKCS12_ConcatonateContent(byte* mergedData, word32* mergedSz, - byte* in, word32 inSz) +static byte* PKCS12_ConcatonateContent(WC_PKCS12* pkcs12,byte* mergedData, + word32* mergedSz, byte* in, word32 inSz) { byte* oldContent; word32 oldContentSz; @@ -1161,7 +1166,8 @@ static byte* PKCS12_ConcatonateContent(byte* mergedData, word32* mergedSz, oldContentSz = *mergedSz; /* re-allocate new buffer to fit appended data */ - mergedData = (byte*)XMALLOC(oldContentSz + inSz, NULL, DYNAMIC_TYPE_PKCS); + mergedData = (byte*)XMALLOC(oldContentSz + inSz, pkcs12->heap, + DYNAMIC_TYPE_PKCS); if (oldContent != NULL) { XMEMCPY(mergedData, oldContent, oldContentSz); @@ -1169,24 +1175,18 @@ static byte* PKCS12_ConcatonateContent(byte* mergedData, word32* mergedSz, XMEMCPY(mergedData + oldContentSz, in, inSz); *mergedSz += inSz; - XFREE(oldContent, NULL, DYNAMIC_TYPE_PKCS); + XFREE(oldContent, pkcs12->heap, DYNAMIC_TYPE_PKCS); return mergedData; } - - -static int PKCS12_HandleIndefinite(WC_PKCS12* pkcs12, byte* data, word32 dataSz, - word32* idx) +/* Check if constructed [0] is seen after wc_BerToDer() or not. + * returns 1 if seen, 0 if not, ASN_PARSE_E on error */ +static int PKCS12_CheckConstructedZero(byte* data, word32 dataSz, word32* idx) { - byte* mergedData = NULL; /* buffer for concatonated strings */ - word32 mergedSz = 0; /* total size of merged strings */ word32 oid; - int encryptedContentSz = 0; - int originalEncSz = 0; int ret = 0; - int curIdx = *idx; - int saveIdx, number, size; + int number, size; byte tag; if (GetSequence(data, idx, &size, dataSz) < 0) { @@ -1216,54 +1216,70 @@ static int PKCS12_HandleIndefinite(WC_PKCS12* pkcs12, byte* data, word32 dataSz, ret = ASN_PARSE_E; } else if (ret == 0 && tag == 0xa0) { - data[*idx-1] = ASN_LONG_LENGTH; - saveIdx = *idx; - - if (GetLength(data, idx, &originalEncSz, dataSz) < 0) { - ret = ASN_PARSE_E; - } - - /* Loop through octet strings and concatonate them without - * the tags and length */ - while ((int)*idx < originalEncSz + curIdx) { - if (GetASNTag(data, idx, &tag, dataSz) < 0) { - ret = ASN_PARSE_E; - } - if (ret == 0 && (tag != ASN_OCTET_STRING)) { - ret = ASN_PARSE_E; - } - if (ret == 0 && GetLength(data, idx, - &encryptedContentSz, dataSz) <= 0) { - ret = ASN_PARSE_E; - } - if (ret == 0) { - if (mergedData == NULL) { - mergedData = (byte*)XMALLOC(encryptedContentSz, - pkcs12->heap, DYNAMIC_TYPE_PKCS); - if (mergedData == NULL) { - ret = MEMORY_E; - } - } - mergedData = PKCS12_ConcatonateContent(mergedData, - &mergedSz, &data[*idx], encryptedContentSz); - } - if (ret != 0) { - break; - } - *idx += encryptedContentSz; - } - - *idx = saveIdx; - - *idx += SetLength(mergedSz, &data[*idx]); - - /* Copy over concatonated octet strings into data buffer */ - XMEMCPY(&data[*idx], mergedData, mergedSz); - - *idx += 1; + ret = 1; } - *idx = curIdx; + return ret; +} + +/* Manually coalesce definite length octet strings into one unconstructed + * definite length octet string. + * returns 0 on success, negative on failure */ +static int PKCS12_CoalesceOctetStrings(WC_PKCS12* pkcs12, byte* data, + word32 dataSz, word32* idx, int* curIdx) +{ + byte* mergedData = NULL; /* buffer for concatonated strings */ + word32 mergedSz = 0; /* total size of merged strings */ + int encryptedContentSz = 0; + int originalEncSz = 0; + int ret = 0; + int saveIdx; + byte tag; + + saveIdx = *idx; + + if (GetLength(data, idx, &originalEncSz, dataSz) < 0) { + ret = ASN_PARSE_E; + } + + /* Loop through octet strings and concatonate them without + * the tags and length */ + while ((int)*idx < originalEncSz + *curIdx) { + if (GetASNTag(data, idx, &tag, dataSz) < 0) { + ret = ASN_PARSE_E; + } + if (ret == 0 && (tag != ASN_OCTET_STRING)) { + ret = ASN_PARSE_E; + } + if (ret == 0 && GetLength(data, idx, + &encryptedContentSz, dataSz) <= 0) { + ret = ASN_PARSE_E; + } + if (ret == 0) { + if (mergedData == NULL) { + mergedData = (byte*)XMALLOC(encryptedContentSz, + pkcs12->heap, DYNAMIC_TYPE_PKCS); + if (mergedData == NULL) { + ret = MEMORY_E; + } + } + mergedData = PKCS12_ConcatonateContent(pkcs12, mergedData, + &mergedSz, &data[*idx], encryptedContentSz); + } + if (ret != 0) { + break; + } + *idx += encryptedContentSz; + } + + *idx = saveIdx; + + *idx += SetLength(mergedSz, &data[*idx]); + + /* Copy over concatonated octet strings into data buffer */ + XMEMCPY(&data[*idx], mergedData, mergedSz); + + XFREE(mergedData, pkcs12->heap, DYNAMIC_TYPE_PKCS); return ret; } @@ -1291,8 +1307,11 @@ int wc_PKCS12_parse(WC_PKCS12* pkcs12, const char* psw, WC_DerCertList* tailList = NULL; byte* buf = NULL; word32 i, oid; - int ret, pswSz; word32 algId; + int ret, pswSz; +#ifdef ASN_BER_TO_DER + int curIdx; +#endif WOLFSSL_ENTER("wc_PKCS12_parse"); @@ -1372,14 +1391,19 @@ int wc_PKCS12_parse(WC_PKCS12* pkcs12, const char* psw, #ifdef ASN_BER_TO_DER + curIdx = idx; /* If indefinite length format, ensure it is in the ASN format * the DecryptContent() expects */ - if (pkcs12->indefinite) { - ret = PKCS12_HandleIndefinite(pkcs12, data, ci->dataSz, &idx); + if (pkcs12->indefinite && PKCS12_CheckConstructedZero(data, + ci->dataSz, &idx) == 1) { + data[idx-1] = ASN_LONG_LENGTH; + ret = PKCS12_CoalesceOctetStrings(pkcs12, data, ci->dataSz, + &idx, &curIdx); if (ret < 0) { goto exit_pk12par; } } + idx = curIdx; #endif /* decrypted content overwrites input buffer */ From 1c9fa5c5ae95af9f966f965db39878846290f45e Mon Sep 17 00:00:00 2001 From: Lealem Amedie Date: Fri, 10 Mar 2023 13:40:25 -0700 Subject: [PATCH 4/4] Set some freed data to NULL --- tests/api.c | 3 +++ wolfcrypt/src/pkcs12.c | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/tests/api.c b/tests/api.c index fea1e44c2..479cdc36b 100644 --- a/tests/api.c +++ b/tests/api.c @@ -9672,6 +9672,9 @@ static int test_wolfSSL_PKCS12(void) AssertNotNull(pkey); AssertNotNull(cert); + wolfSSL_EVP_PKEY_free(pkey); + wolfSSL_X509_free(cert); + /* check parse with extra certs kept */ ret = PKCS12_parse(pkcs12, "wolfSSL test", &pkey, &cert, &ca); AssertIntEQ(ret, WOLFSSL_SUCCESS); diff --git a/wolfcrypt/src/pkcs12.c b/wolfcrypt/src/pkcs12.c index 65b1aabe7..36f2ae0c6 100644 --- a/wolfcrypt/src/pkcs12.c +++ b/wolfcrypt/src/pkcs12.c @@ -156,11 +156,14 @@ static void freeSafe(AuthenticatedSafe* safe, void* heap) ContentInfo* ci = safe->CI; safe->CI = ci->next; XFREE(ci, heap, DYNAMIC_TYPE_PKCS); + ci = NULL; } if (safe->data != NULL) { XFREE(safe->data, heap, DYNAMIC_TYPE_PKCS); + safe->data = NULL; } XFREE(safe, heap, DYNAMIC_TYPE_PKCS); + safe = NULL; (void)heap; } @@ -207,6 +210,7 @@ void wc_PKCS12_free(WC_PKCS12* pkcs12) #endif XFREE(pkcs12, NULL, DYNAMIC_TYPE_PKCS); + pkcs12 = NULL; }