Support creating unencrypted PKCS#12 files.

PKCS#12 is overly general and, among other things, supports disabling
encryption. In practice, the unencrypted form is not widely implemented.
Moreover, even in contexts where cleartext is fine, an unencrypted
PKCS#12 file still requires a password for the mandatory MAC component.
They're not very useful.

However, cryptography.io uses them. Previously, we added support for
parsing these. This CL adds support for creating them too, because now
cryptography.io now also depends on that.

Change-Id: Ib7c4e29615047b6c73f887fea7c80f8844999bb7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46045
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/pkcs8/pkcs12_test.cc b/crypto/pkcs8/pkcs12_test.cc
index 33fff2c..e67630d 100644
--- a/crypto/pkcs8/pkcs12_test.cc
+++ b/crypto/pkcs8/pkcs12_test.cc
@@ -428,6 +428,27 @@
                 {bssl::Span<const uint8_t>(kTestCert2)},
                 NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
                 NID_pbe_WithSHA1And3_Key_TripleDES_CBC, 100, 100);
+
+  // Test unencrypted and partially unencrypted PKCS#12 files.
+  TestRoundTrip(kPassword, /*name=*/nullptr,
+                bssl::Span<const uint8_t>(kTestKey),
+                bssl::Span<const uint8_t>(kTestCert),
+                {bssl::Span<const uint8_t>(kTestCert2)},
+                /*key_nid=*/-1,
+                /*cert_nid=*/-1, /*iterations=*/100, /*mac_iterations=*/100);
+  TestRoundTrip(kPassword, /*name=*/nullptr,
+                bssl::Span<const uint8_t>(kTestKey),
+                bssl::Span<const uint8_t>(kTestCert),
+                {bssl::Span<const uint8_t>(kTestCert2)},
+                /*key_nid=*/NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
+                /*cert_nid=*/-1, /*iterations=*/100, /*mac_iterations=*/100);
+  TestRoundTrip(kPassword, /*name=*/nullptr,
+                bssl::Span<const uint8_t>(kTestKey),
+                bssl::Span<const uint8_t>(kTestCert),
+                {bssl::Span<const uint8_t>(kTestCert2)},
+                /*key_nid=*/-1,
+                /*cert_nid=*/NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
+                /*iterations=*/100, /*mac_iterations=*/100);
 }
 
 static bssl::UniquePtr<EVP_PKEY> MakeTestKey() {
diff --git a/crypto/pkcs8/pkcs8_x509.c b/crypto/pkcs8/pkcs8_x509.c
index a2f9075..b8439a5 100644
--- a/crypto/pkcs8/pkcs8_x509.c
+++ b/crypto/pkcs8/pkcs8_x509.c
@@ -1074,31 +1074,24 @@
   return 1;
 }
 
-static int make_cert_safe_contents(uint8_t **out_data, size_t *out_len,
-                                   X509 *cert, const STACK_OF(X509) *chain,
-                                   const char *name, const uint8_t *key_id,
-                                   size_t key_id_len) {
-  int ret = 0;
-  CBB cbb, safe_contents;
-  if (!CBB_init(&cbb, 0) ||
-      !CBB_add_asn1(&cbb, &safe_contents, CBS_ASN1_SEQUENCE) ||
+static int add_cert_safe_contents(CBB *cbb, X509 *cert,
+                                  const STACK_OF(X509) *chain, const char *name,
+                                  const uint8_t *key_id, size_t key_id_len) {
+  CBB safe_contents;
+  if (!CBB_add_asn1(cbb, &safe_contents, CBS_ASN1_SEQUENCE) ||
       (cert != NULL &&
        !add_cert_bag(&safe_contents, cert, name, key_id, key_id_len))) {
-    goto err;
+    return 0;
   }
 
   for (size_t i = 0; i < sk_X509_num(chain); i++) {
     // Only the leaf certificate gets attributes.
     if (!add_cert_bag(&safe_contents, sk_X509_value(chain, i), NULL, NULL, 0)) {
-      goto err;
+      return 0;
     }
   }
 
-  ret = CBB_finish(&cbb, out_data, out_len);
-
-err:
-  CBB_cleanup(&cbb);
-  return ret;
+  return CBB_flush(cbb);
 }
 
 static int add_encrypted_data(CBB *out, int pbe_nid, const char *password,
@@ -1181,9 +1174,6 @@
   if (// In OpenSSL, this specifies a non-standard Microsoft key usage extension
       // which we do not currently support.
       key_type != 0 ||
-      // In OpenSSL, -1 here means to use no encryption, which we do not
-      // currently support.
-      key_nid < 0 || cert_nid < 0 ||
       // In OpenSSL, -1 here means to omit the MAC, which we do not
       // currently support. Omitting it is also invalid for a password-based
       // PKCS#12 file.
@@ -1194,6 +1184,36 @@
     return 0;
   }
 
+  // PKCS#12 is a very confusing recursive data format, built out of another
+  // recursive data format. Section 5.1 of RFC7292 describes the encoding
+  // algorithm, but there is no clear overview. A quick summary:
+  //
+  // PKCS#7 defines a ContentInfo structure, which is a overgeneralized typed
+  // combinator structure for applying cryptography. We care about two types. A
+  // data ContentInfo contains an OCTET STRING and is a leaf node of the
+  // combinator tree. An encrypted-data ContentInfo contains encryption
+  // parameters (key derivation and encryption) and wraps another ContentInfo,
+  // usually data.
+  //
+  // A PKCS#12 file is a PFX structure (section 4), which contains a single data
+  // ContentInfo and a MAC over it. This root ContentInfo is the
+  // AuthenticatedSafe and its payload is a SEQUENCE of other ContentInfos, so
+  // that different parts of the PKCS#12 file can by differently protected.
+  //
+  // Each ContentInfo in the AuthenticatedSafe, after undoing all the PKCS#7
+  // combinators, has SafeContents payload. A SafeContents is a SEQUENCE of
+  // SafeBag. SafeBag is PKCS#12's typed structure, with subtypes such as KeyBag
+  // and CertBag. Confusingly, there is a SafeContents bag type which itself
+  // recursively contains more SafeBags, but we do not implement this. Bags also
+  // can have attributes.
+  //
+  // The grouping of SafeBags into intermediate ContentInfos does not appear to
+  // be significant, except that all SafeBags sharing a ContentInfo have the
+  // same level of protection. Additionally, while keys may be encrypted by
+  // placing a KeyBag in an encrypted-data ContentInfo, PKCS#12 also defines a
+  // key-specific encryption container, PKCS8ShroudedKeyBag, which is used
+  // instead.
+
   // Note that |password| may be NULL to specify no password, rather than the
   // empty string. They are encoded differently in PKCS#12. (One is the empty
   // byte array and the other is NUL-terminated UCS-2.)
@@ -1236,24 +1256,43 @@
   // If there are any certificates, place them in CertBags wrapped in a single
   // encrypted ContentInfo.
   if (cert != NULL || sk_X509_num(chain) > 0) {
-    uint8_t *data;
-    size_t len;
-    if (!make_cert_safe_contents(&data, &len, cert, chain, name, key_id,
-                                 key_id_len)) {
-      goto err;
-    }
-    int ok = add_encrypted_data(&content_infos, cert_nid, password,
-                                password_len, iterations, data, len);
-    OPENSSL_free(data);
-    if (!ok) {
-      goto err;
+    if (cert_nid < 0) {
+      // Place the certificates in an unencrypted ContentInfo. This could be
+      // more compactly-encoded by reusing the same ContentInfo as the key, but
+      // OpenSSL does not do this. We keep them separate for consistency. (Keys,
+      // even when encrypted, are always placed in unencrypted ContentInfos.
+      // PKCS#12 defines bag-level encryption for keys.)
+      CBB content_info, oid, wrapper, data;
+      if (!CBB_add_asn1(&content_infos, &content_info, CBS_ASN1_SEQUENCE) ||
+          !CBB_add_asn1(&content_info, &oid, CBS_ASN1_OBJECT) ||
+          !CBB_add_bytes(&oid, kPKCS7Data, sizeof(kPKCS7Data)) ||
+          !CBB_add_asn1(&content_info, &wrapper,
+                        CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0) ||
+          !CBB_add_asn1(&wrapper, &data, CBS_ASN1_OCTETSTRING) ||
+          !add_cert_safe_contents(&data, cert, chain, name, key_id,
+                                  key_id_len) ||
+          !CBB_flush(&content_infos)) {
+        goto err;
+      }
+    } else {
+      CBB plaintext_cbb;
+      int ok = CBB_init(&plaintext_cbb, 0) &&
+               add_cert_safe_contents(&plaintext_cbb, cert, chain, name, key_id,
+                                      key_id_len) &&
+               add_encrypted_data(
+                   &content_infos, cert_nid, password, password_len, iterations,
+                   CBB_data(&plaintext_cbb), CBB_len(&plaintext_cbb));
+      CBB_cleanup(&plaintext_cbb);
+      if (!ok) {
+        goto err;
+      }
     }
   }
 
-  // If there is a key, place it in a single PKCS8ShroudedKeyBag wrapped in an
-  // unencrypted ContentInfo. (One could also place it in a KeyBag inside an
-  // encrypted ContentInfo, but OpenSSL does not do this and some PKCS#12
-  // consumers do not support KeyBags.)
+  // If there is a key, place it in a single KeyBag or PKCS8ShroudedKeyBag
+  // wrapped in an unencrypted ContentInfo. (One could also place it in a KeyBag
+  // inside an encrypted ContentInfo, but OpenSSL does not do this and some
+  // PKCS#12 consumers do not support KeyBags.)
   if (pkey != NULL) {
     CBB content_info, oid, wrapper, data, safe_contents, bag, bag_oid,
         bag_contents;
@@ -1267,16 +1306,29 @@
         !CBB_add_asn1(&data, &safe_contents, CBS_ASN1_SEQUENCE) ||
         // Add a SafeBag containing a PKCS8ShroudedKeyBag.
         !CBB_add_asn1(&safe_contents, &bag, CBS_ASN1_SEQUENCE) ||
-        !CBB_add_asn1(&bag, &bag_oid, CBS_ASN1_OBJECT) ||
-        !CBB_add_bytes(&bag_oid, kPKCS8ShroudedKeyBag,
-                       sizeof(kPKCS8ShroudedKeyBag)) ||
-        !CBB_add_asn1(&bag, &bag_contents,
-                      CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0) ||
-        !PKCS8_marshal_encrypted_private_key(
-            &bag_contents, key_nid, NULL, password, password_len,
-            NULL /* generate a random salt */, 0 /* use default salt length */,
-            iterations, pkey) ||
-        !add_bag_attributes(&bag, name, key_id, key_id_len) ||
+        !CBB_add_asn1(&bag, &bag_oid, CBS_ASN1_OBJECT)) {
+      goto err;
+    }
+    if (key_nid < 0) {
+      if (!CBB_add_bytes(&bag_oid, kKeyBag, sizeof(kKeyBag)) ||
+          !CBB_add_asn1(&bag, &bag_contents,
+                        CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0) ||
+          !EVP_marshal_private_key(&bag_contents, pkey)) {
+        goto err;
+      }
+    } else {
+      if (!CBB_add_bytes(&bag_oid, kPKCS8ShroudedKeyBag,
+                         sizeof(kPKCS8ShroudedKeyBag)) ||
+          !CBB_add_asn1(&bag, &bag_contents,
+                        CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0) ||
+          !PKCS8_marshal_encrypted_private_key(
+              &bag_contents, key_nid, NULL, password, password_len,
+              NULL /* generate a random salt */,
+              0 /* use default salt length */, iterations, pkey)) {
+        goto err;
+      }
+    }
+    if (!add_bag_attributes(&bag, name, key_id, key_id_len) ||
         !CBB_flush(&content_infos)) {
       goto err;
     }
diff --git a/include/openssl/pkcs8.h b/include/openssl/pkcs8.h
index 385b995..9da54aa 100644
--- a/include/openssl/pkcs8.h
+++ b/include/openssl/pkcs8.h
@@ -206,6 +206,12 @@
 // Each of |key_nid|, |cert_nid|, |iterations|, and |mac_iterations| may be zero
 // to use defaults, which are |NID_pbe_WithSHA1And3_Key_TripleDES_CBC|,
 // |NID_pbe_WithSHA1And40BitRC2_CBC|, 2048, and one, respectively.
+//
+// |key_nid| or |cert_nid| may also be -1 to disable encryption of the key or
+// certificate, respectively. This option is not recommended and is only
+// implemented for compatibility with external packages. Note the output still
+// requires a password for the MAC. Unencrypted keys in PKCS#12 are also not
+// widely supported and may not open in other implementations.
 OPENSSL_EXPORT PKCS12 *PKCS12_create(const char *password, const char *name,
                                      const EVP_PKEY *pkey, X509 *cert,
                                      const STACK_OF(X509) *chain, int key_nid,