For PKCS#12 imports, only mark key as unextractable if the PKCS#12 file includes it

net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp

 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
  * http://www.mozilla.org/MPL/
  *
  * Software distributed under the License is distributed on an "AS IS" basis,
  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
@@ skipping 179 matching lines @@
   if (srv) goto finish;
   // import cert and key
   srv = SEC_PKCS12DecoderImportBags(dcx);
   if (srv) goto finish;
 
   if (!is_extractable) {
     SECItem attribute_value;
     CK_BBOOL attribute_data = CK_FALSE;
     attribute_value.data = &attribute_data;
     attribute_value.len = sizeof(attribute_data);
-    CERTCertList* cert_list = SEC_PKCS12DecoderGetCerts(dcx);
 
-    // Iterate through each certificate in the chain and mark corresponding
-    // private key as unextractable.
-    for (CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
-         !CERT_LIST_END(node, cert_list); node = CERT_LIST_NEXT(node)) {
-      SECKEYPrivateKey* privKey =  PK11_FindKeyByDERCert(slot,
-                                                         node->cert,
-                                                         NULL);  // wincx
+    srv = SEC_PKCS12DecoderIterateInit(dcx);
+    if (srv) goto finish;
+
+    const SEC_PKCS12DecoderItem* decoder_item = NULL;
+    // Iterate through all the imported PKCS12 items and mark any accompanying
+    // private keys as unextractable.
+    while (SEC_PKCS12DecoderIterateNext(dcx, &decoder_item) == SECSuccess) {
+      if (decoder_item->type != SEC_OID_PKCS12_V1_CERT_BAG_ID)
+        continue;
+      if (!decoder_item->hasKey)
+        continue;
+
+      // Once we have determined that the imported certificate has an
+      // associated private key too, only then can we mark the key as
+      // unextractable.
+      CERTCertificate* cert = PK11_FindCertFromDERCertItem(
+          slot, const_cast<SECItem*>(decoder_item->der),
+          NULL);  // wincx
+      SECKEYPrivateKey* privKey = PK11_FindPrivateKeyFromCert(slot, cert,
+                                                              NULL);  // wincx
       if (privKey) {
         // Mark the private key as unextractable.
         srv = PK11_WriteRawAttribute(PK11_TypePrivKey, privKey, CKA_EXTRACTABLE,
                                      &attribute_value);
         SECKEY_DestroyPrivateKey(privKey);
-        if (srv) {
-          LOG(ERROR) << "Couldn't set CKA_EXTRACTABLE attribute on private "
-                     << "key.";
-          break;
-        }
+      }
+      if (cert) CERT_DestroyCertificate(cert);
+      if (srv) {
+        LOG(ERROR) << "Couldn't set CKA_EXTRACTABLE attribute on private key.";
+        break;
       }

[wtc, 2011/07/29 19:08:37]

The if (srv) statement on lines 228-231 should be moved inside
the if (privKey) block, after line 225.

We should add a null pointer check for 'cert', so I suggest
this code sequence:
    CERTCertificate* cert = PK11_FindCertFromDERCertItem(...);
    if (!cert)
      continue;  // You can add a LOG(ERROR) statement
    SECKEYPrivateKey* privKey = PK11_FindPrivateKeyFromCert(...)
    CERT_DestroyCertificate(cert);
    if (privKey) {
      ...
    }

Also, do you really want to break out of the loop on
line 230 if we get an error with a private key?

[gauravsh, 2011/07/29 20:04:55]

Done.
I guess we could continue with the next PKCS#12 items although my reasoning for
the original break was that a failure to get the private key at this point means
that something failed during the earlier import.

Changed it to continue instead. We can re-visit this if we ever run into this
error case.

     }
-    CERT_DestroyCertList(cert_list);
     if (srv) goto finish;
   }
 
   import_result = net::OK;
 finish:
   // If srv != SECSuccess, NSS probably set a specific error code.
   // We should use that error code instead of inventing a new one
   // for every error possible.
   if (srv != SECSuccess) {
     int error = PORT_GetError();
@@ skipping 207 matching lines @@
 finish:
   if (srv)
     LOG(ERROR) << "PKCS#12 export failed with error " << PORT_GetError();
   if (ecx)
     SEC_PKCS12DestroyExportContext(ecx);
   SECITEM_ZfreeItem(&unicodePw, PR_FALSE);
   return return_count;
 }
 
 }  // namespace mozilla_security_manager