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

This addresses a potential corner case where we end up marking an already
existing private key as unextractable while importing a corresponding
certificate into a hardware (unextractable slot).

BUG=chromium-os:15838
TEST=Added a new unit test


Review URL: http://codereview.chromium.org/7466006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@95486 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 57 insertions, 11 deletions

diff --git a/net/base/cert_database_nss_unittest.cc b/net/base/cert_database_nss_unittest.cc
index 63f2a43..68b449c 100644
--- a/net/base/cert_database_nss_unittest.cc
+++ b/net/base/cert_database_nss_unittest.cc
@@ -220,6 +220,35 @@ TEST_F(CertDatabaseNSSTest, ImportFromPKCS12AsUnextractableAndExportAgain) {
                                        &exported_data));
 }
 
+// Importing a PKCS#12 file with a certificate but no corresponding
+// private key should not mark an existing private key as unextractable.
+TEST_F(CertDatabaseNSSTest, ImportFromPKCS12OnlyMarkIncludedKey) {
+  std::string pkcs12_data = ReadTestFile("client.p12");
+  EXPECT_EQ(OK, cert_db_.ImportFromPKCS12(slot_,
+                                          pkcs12_data,
+                                          ASCIIToUTF16("12345"),
+                                          true));  // is_extractable
+
+  CertificateList cert_list = ListCertsInSlot(slot_->os_module_handle());
+  ASSERT_EQ(1U, cert_list.size());
+
+  // Now import a PKCS#12 file with just a certificate but no private key.
+  pkcs12_data = ReadTestFile("client-nokey.p12");
+  EXPECT_EQ(OK, cert_db_.ImportFromPKCS12(slot_,
+                                          pkcs12_data,
+                                          ASCIIToUTF16("12345"),
+                                          false));  // is_extractable
+
+  cert_list = ListCertsInSlot(slot_->os_module_handle());
+  ASSERT_EQ(1U, cert_list.size());
+
+  // Make sure the imported private key is still extractable.
+  std::string exported_data;
+  EXPECT_EQ(1, cert_db_.ExportToPKCS12(cert_list, ASCIIToUTF16("exportpw"),
+                                       &exported_data));
+  ASSERT_LT(0U, exported_data.size());
+}
+
 TEST_F(CertDatabaseNSSTest, ImportFromPKCS12InvalidFile) {
   std::string pkcs12_data = "Foobarbaz";
 
diff --git a/net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp b/net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp
index d659900..20768f0 100644
--- a/net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp
+++ b/net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp
@@ -197,28 +197,45 @@ nsPKCS12Blob_ImportHelper(const char* pkcs12_data,
     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, decoder_item->der,
+          NULL);  // wincx
+      if (!cert) {
+        LOG(ERROR) << "Could not grab a handle to the certificate in the slot "
+                   << "from the corresponding PKCS#12 DER certificate.";
+        continue;
+      }
+      SECKEYPrivateKey* privKey = PK11_FindPrivateKeyFromCert(slot, cert,
+                                                              NULL);  // wincx
+      CERT_DestroyCertificate(cert);
       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 "
+          LOG(ERROR) << "Could not set CKA_EXTRACTABLE attribute on private "
                      << "key.";
           break;
         }
       }
     }
-    CERT_DestroyCertList(cert_list);
     if (srv) goto finish;
   }