Bug 1804640 - improve handling of unknown PKCS#12 safe bag types. r=rrelyea

Differential Revision: https://phabricator.services.mozilla.com/D167443

3 files changed, 18 insertions, 10 deletions

diff --git a/lib/pkcs12/p12d.c b/lib/pkcs12/p12d.c
index dd91a4a7c..615b123db 100644
--- a/lib/pkcs12/p12d.c
+++ b/lib/pkcs12/p12d.c
@@ -337,31 +337,38 @@ sec_pkcs12_decoder_safe_bag_update(void *arg, const char *data,
     SEC_PKCS12DecoderContext *p12dcx;
     SECStatus rv;
 
-    /* make sure that we are not skipping the current safeBag,
-     * and that there are no errors.  If so, just return rather
-     * than continuing to process.
-     */
-    if (!safeContentsCtx || !safeContentsCtx->p12dcx ||
-        safeContentsCtx->p12dcx->error || safeContentsCtx->skipCurrentSafeBag) {
+    if (!safeContentsCtx || !safeContentsCtx->p12dcx || !safeContentsCtx->currentSafeBagA1Dcx) {
         return;
     }
     p12dcx = safeContentsCtx->p12dcx;
 
+    /* make sure that there are no errors and we are not skipping the current safeBag */
+    if (p12dcx->error || safeContentsCtx->skipCurrentSafeBag) {
+        goto loser;
+    }
+
     rv = SEC_ASN1DecoderUpdate(safeContentsCtx->currentSafeBagA1Dcx, data, len);
     if (rv != SECSuccess) {
         p12dcx->errorValue = PORT_GetError();
+        p12dcx->error = PR_TRUE;
+        goto loser;
+    }
+
+    /* The update may have set safeContentsCtx->skipCurrentSafeBag, and we
+     * may not get another opportunity to clean up the decoder context.
+     */
+    if (safeContentsCtx->skipCurrentSafeBag) {
         goto loser;
     }
 
     return;
 
 loser:
-    /* set the error, and finish the decoder context.  because there
+    /* Finish the decoder context. Because there
      * is not a way of returning an error message, it may be worth
      * while to do a check higher up and finish any decoding contexts
      * that are still open.
      */
-    p12dcx->error = PR_TRUE;
     SEC_ASN1DecoderFinish(safeContentsCtx->currentSafeBagA1Dcx);
     safeContentsCtx->currentSafeBagA1Dcx = NULL;
     return;
diff --git a/lib/pkcs12/p12t.h b/lib/pkcs12/p12t.h
index b22f0dd82..d449afdd8 100644
--- a/lib/pkcs12/p12t.h
+++ b/lib/pkcs12/p12t.h
@@ -73,6 +73,7 @@ struct sec_PKCS12SafeBagStr {
         sec_PKCS12CRLBag *crlBag;
         sec_PKCS12SecretBag *secretBag;
         sec_PKCS12SafeContents *safeContents;
+        SECItem *unknownBag;
     } safeBagContent;
 
     sec_PKCS12Attribute **attribs;
diff --git a/lib/pkcs12/p12tmpl.c b/lib/pkcs12/p12tmpl.c
index 7437cbcc6..b08384f66 100644
--- a/lib/pkcs12/p12tmpl.c
+++ b/lib/pkcs12/p12tmpl.c
@@ -30,12 +30,12 @@ sec_pkcs12_choose_safe_bag_type(void *src_or_dest, PRBool encoding)
 
     oiddata = SECOID_FindOID(&safeBag->safeBagType);
     if (oiddata == NULL) {
-        return SEC_ASN1_GET(SEC_AnyTemplate);
+        return SEC_ASN1_GET(SEC_PointerToAnyTemplate);
     }
 
     switch (oiddata->offset) {
         default:
-            theTemplate = SEC_ASN1_GET(SEC_AnyTemplate);
+            theTemplate = SEC_ASN1_GET(SEC_PointerToAnyTemplate);
             break;
         case SEC_OID_PKCS12_V1_KEY_BAG_ID:
             theTemplate = SEC_ASN1_GET(SECKEY_PointerToPrivateKeyInfoTemplate);