Support PBES2 schemes in PKCS12_create

We were able to decrypt them, but not create them. This adds support for
creating them. They are a little goofy because OpenSSL has two different
conventions for specifying PBES schemes. Decryption just requires
parsing out the AlgorithmIdentifier structure embedded in the input,
while encryption requires some way for the caller to specify the
algorithms.

PKCS#12 files could be encrypted with two different PBES (password-based
encryption scheme) schemes from PKCS#5: PBES1 and PBES2. Where PBES1 was
fully described by a single OID, PBES2 is itself parameterized by a KDF
and a cipher[*].

OpenSSL first added PBES2 encrypting support in
openssl/openssl@8eb57af,
but only added it to PKCS8_encrypt, for encrypting PKCS#8 blobs. This
was early so they were willing to break their public APIs. They chose a
convention where you passed both a NID and an EVP_CIPHER. If the NID was
-1, the cipher would be used instead with PBES2, hardcoding PBKDF2 with,
at the time, HMAC-SHA1. We implement this API.

Later, OpenSSL added PBES2 encrypting to PKCS12_create in
openssl/openssl@b0e69a0,
but now backwards compatibility does not allow adding an EVP_CIPHER
parameter to PKCS12_create. OpenSSL instead decided that if the NID
matched a known EVP_CIPHER (e.g. NID_aes_256_cbc), it would implicitly
pick PBES2, the way -1 and an EVP_CIPHER specified it before.

This CL implements that behavior. I've opted to keep the PKCS8_encrypt
calling convention and just translate to it for now. But we really
should pick a less chaotic calling convention (we're C++ now, so we
could even use std::variant...) at least for the internals.

For now, the defaults are still at the values they were in older
OpenSSL. I've filed https://crbug.com/396434682 to track that. (Only
reason to do it separately is to have something easily revertible and
decide when to do the low-but-nonzero-risk change.)

As an aside, OpenSSL later further complicated this (still not
documenting anything) with
openssl/openssl@5693a30

There was no way to specify the PBKDF2 PRF function. So they overloaded
the NID parameter, to specify that. Now the decision tree is:

- If the NID is -1, PBES2 with the cipher and the default PBKDF2
- If the NID is a PRF algorithm, PBES2 with the cipher and that PRF as
  the PBKDF2 PRF
- Otherwise, PBES1 with the NID specifying the PBES1 scheme

This CL does not implement that extra feature. This calling convention
is a mess.

[*] To be honest, I'm not sure what PBES2 even specifies. The operation
in RFC 8018, Section 6.2.1 is pretty much meaningless. Use the selected
KDF to derive a key, then use the selected cipher to encrypt it. PBES2
does not actually come with any choices of KDF or cipher, you have to
pick them when formulating a PBES2 scheme. PBES2 appears to just
describe sticking the two together generically.

Fixed: 394337104
Change-Id: I1380b339d398783ee65cde3d9f526b9d8fc1c2d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/76307
Commit-Queue: David Benjamin <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>

crypto/pkcs8/internal.h

@@ -51,11 +51,13 @@ int pkcs12_key_gen(const char *pass, size_t pass_len, const uint8_t *salt,
                    size_t out_len, uint8_t *out, const EVP_MD *md);
 
 // pkcs12_pbe_encrypt_init configures |ctx| for encrypting with a PBES1 scheme
-// defined in PKCS#12. It writes the corresponding AlgorithmIdentifier to |out|.
-int pkcs12_pbe_encrypt_init(CBB *out, EVP_CIPHER_CTX *ctx, int alg,
-                            uint32_t iterations, const char *pass,
-                            size_t pass_len, const uint8_t *salt,
-                            size_t salt_len);
+// defined in PKCS#12, or a PBES2 scheme defined in PKCS#5. The algorithm is
+// determined as in |PKCS8_encrypt|. It writes the corresponding
+// AlgorithmIdentifier to |out|.
+int pkcs12_pbe_encrypt_init(CBB *out, EVP_CIPHER_CTX *ctx, int alg_nid,
+                            const EVP_CIPHER *alg_cipher, uint32_t iterations,
+                            const char *pass, size_t pass_len,
+                            const uint8_t *salt, size_t salt_len);
 
 struct pbe_suite {
   int pbe_nid;
@@ -74,6 +76,10 @@ struct pbe_suite {
 
 #define PKCS5_SALT_LEN 8
 
+// pkcs5_pbe2_nid_to_cipher returns the |EVP_CIPHER| for |nid| if |nid| is
+// supported with PKCS#5 PBES2, and nullptr otherwise.
+const EVP_CIPHER *pkcs5_pbe2_nid_to_cipher(int nid);
+
 int PKCS5_pbe2_decrypt_init(const struct pbe_suite *suite, EVP_CIPHER_CTX *ctx,
                             const char *pass, size_t pass_len, CBS *param);
 

crypto/pkcs8/p5_pbev2.cc

@@ -78,22 +78,21 @@ static const struct {
 };
 
 static const EVP_CIPHER *cbs_to_cipher(const CBS *cbs) {
-  for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kCipherOIDs); i++) {
-    if (CBS_mem_equal(cbs, kCipherOIDs[i].oid, kCipherOIDs[i].oid_len)) {
-      return kCipherOIDs[i].cipher_func();
+  for (const auto &cipher : kCipherOIDs) {
+    if (CBS_mem_equal(cbs, cipher.oid, cipher.oid_len)) {
+      return cipher.cipher_func();
     }
   }
 
-  return NULL;
+  return nullptr;
 }
 
 static int add_cipher_oid(CBB *out, int nid) {
-  for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kCipherOIDs); i++) {
-    if (kCipherOIDs[i].nid == nid) {
+  for (const auto &cipher : kCipherOIDs) {
+    if (cipher.nid == nid) {
       CBB child;
       return CBB_add_asn1(out, &child, CBS_ASN1_OBJECT) &&
-             CBB_add_bytes(&child, kCipherOIDs[i].oid,
-                           kCipherOIDs[i].oid_len) &&
+             CBB_add_bytes(&child, cipher.oid, cipher.oid_len) &&
              CBB_flush(out);
     }
   }
@@ -102,6 +101,15 @@ static int add_cipher_oid(CBB *out, int nid) {
   return 0;
 }
 
+const EVP_CIPHER *pkcs5_pbe2_nid_to_cipher(int nid) {
+  for (const auto &cipher : kCipherOIDs) {
+    if (cipher.nid == nid) {
+      return cipher.cipher_func();
+    }
+  }
+  return nullptr;
+}
+
 static int pkcs5_pbe2_cipher_init(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *cipher,
                                   const EVP_MD *pbkdf2_md, uint32_t iterations,
                                   const char *pass, size_t pass_len,
@@ -154,6 +162,7 @@ int PKCS5_pbe2_encrypt_init(CBB *out, EVP_CIPHER_CTX *ctx,
       (cipher_nid == NID_rc2_cbc &&
        !CBB_add_asn1_uint64(&kdf_param, EVP_CIPHER_key_length(cipher))) ||
       // Omit the PRF. We use the default hmacWithSHA1.
+      // TODO(crbug.com/396434682): Improve this defaults.
       !CBB_add_asn1(&param, &cipher_cbb, CBS_ASN1_SEQUENCE) ||
       !add_cipher_oid(&cipher_cbb, cipher_nid) ||
       // RFC 2898 says RC2-CBC and RC5-CBC-Pad use a SEQUENCE with version and

crypto/pkcs8/pkcs12_test.cc

@@ -424,6 +424,31 @@ TEST(PKCS12Test, RoundTrip) {
                 {kTestCert2}, NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
                 NID_pbe_WithSHA1And3_Key_TripleDES_CBC, 100, 100);
 
+  // PBES2 ciphers.
+  TestRoundTrip(kPassword, nullptr /* no name */, kTestKey, kTestCert,
+                {kTestCert2}, NID_rc2_cbc, NID_rc2_cbc, 100, 100);
+  TestRoundTrip(kPassword, nullptr /* no name */, kTestKey, kTestCert,
+                {kTestCert2}, NID_des_ede3_cbc, NID_des_ede3_cbc, 100, 100);
+  TestRoundTrip(kPassword, nullptr /* no name */, kTestKey, kTestCert,
+                {kTestCert2}, NID_aes_128_cbc, NID_aes_128_cbc, 100, 100);
+  TestRoundTrip(kPassword, nullptr /* no name */, kTestKey, kTestCert,
+                {kTestCert2}, NID_aes_192_cbc, NID_aes_192_cbc, 100, 100);
+  TestRoundTrip(kPassword, nullptr /* no name */, kTestKey, kTestCert,
+                {kTestCert2}, NID_aes_256_cbc, NID_aes_256_cbc, 100, 100);
+
+  // Mix and match.
+  TestRoundTrip(kPassword, nullptr /* no name */, kTestKey, kTestCert,
+                {kTestCert2}, NID_pbe_WithSHA1And40BitRC2_CBC,
+                NID_pbe_WithSHA1And3_Key_TripleDES_CBC, 100, 100);
+  TestRoundTrip(kPassword, nullptr /* no name */, kTestKey, kTestCert,
+                {kTestCert2}, NID_pbe_WithSHA1And3_Key_TripleDES_CBC,
+                NID_aes_256_cbc, 100, 100);
+  TestRoundTrip(kPassword, nullptr /* no name */, kTestKey, kTestCert,
+                {kTestCert2}, NID_aes_256_cbc,
+                NID_pbe_WithSHA1And3_Key_TripleDES_CBC, 100, 100);
+  TestRoundTrip(kPassword, nullptr /* no name */, kTestKey, kTestCert,
+                {kTestCert2}, NID_aes_128_cbc, NID_aes_256_cbc, 100, 100);
+
   // Test unencrypted and partially unencrypted PKCS#12 files.
   TestRoundTrip(kPassword, /*name=*/nullptr, kTestKey, kTestCert, {kTestCert2},
                 /*key_nid=*/-1,

crypto/pkcs8/pkcs8.cc

@@ -286,11 +286,20 @@ static const struct pbe_suite *get_pkcs12_pbe_suite(int pbe_nid) {
   return NULL;
 }
 
-int pkcs12_pbe_encrypt_init(CBB *out, EVP_CIPHER_CTX *ctx, int alg,
-                            uint32_t iterations, const char *pass,
-                            size_t pass_len, const uint8_t *salt,
-                            size_t salt_len) {
-  const struct pbe_suite *suite = get_pkcs12_pbe_suite(alg);
+int pkcs12_pbe_encrypt_init(CBB *out, EVP_CIPHER_CTX *ctx, int alg_nid,
+                            const EVP_CIPHER *alg_cipher, uint32_t iterations,
+                            const char *pass, size_t pass_len,
+                            const uint8_t *salt, size_t salt_len) {
+  // TODO(davidben): OpenSSL has since extended |pbe_nid| to control either
+  // the PBES1 scheme or the PBES2 PRF. E.g. passing |NID_hmacWithSHA256| will
+  // select PBES2 with HMAC-SHA256 as the PRF. Implement this if anything uses
+  // it. See 5693a30813a031d3921a016a870420e7eb93ec90 in OpenSSL.
+  if (alg_nid == -1) {
+    return PKCS5_pbe2_encrypt_init(out, ctx, alg_cipher, iterations, pass,
+                                   pass_len, salt, salt_len);
+  }
+
+  const struct pbe_suite *suite = get_pkcs12_pbe_suite(alg_nid);
   if (suite == NULL) {
     OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_UNKNOWN_ALGORITHM);
     return 0;
@@ -437,25 +446,10 @@ int PKCS8_marshal_encrypted_private_key(CBB *out, int pbe_nid,
     }
 
     CBB epki;
-    if (!CBB_add_asn1(out, &epki, CBS_ASN1_SEQUENCE)) {
-      goto err;
-    }
-
-    // TODO(davidben): OpenSSL has since extended |pbe_nid| to control either
-    // the PBES1 scheme or the PBES2 PRF. E.g. passing |NID_hmacWithSHA256| will
-    // select PBES2 with HMAC-SHA256 as the PRF. Implement this if anything uses
-    // it. See 5693a30813a031d3921a016a870420e7eb93ec90 in OpenSSL.
-    int alg_ok;
-    if (pbe_nid == -1) {
-      alg_ok =
-          PKCS5_pbe2_encrypt_init(&epki, &ctx, cipher, (uint32_t)iterations,
-                                  pass, pass_len, salt, salt_len);
-    } else {
-      alg_ok =
-          pkcs12_pbe_encrypt_init(&epki, &ctx, pbe_nid, (uint32_t)iterations,
-                                  pass, pass_len, salt, salt_len);
-    }
-    if (!alg_ok) {
+    if (!CBB_add_asn1(out, &epki, CBS_ASN1_SEQUENCE) ||
+        !pkcs12_pbe_encrypt_init(&epki, &ctx, pbe_nid, cipher,
+                                 (uint32_t)iterations, pass, pass_len, salt,
+                                 salt_len)) {
       goto err;
     }
 

crypto/pkcs8/pkcs8_x509.cc

@@ -1005,17 +1005,20 @@ static int add_cert_safe_contents(CBB *cbb, X509 *cert,
   return CBB_flush(cbb);
 }
 
-static int add_encrypted_data(CBB *out, int pbe_nid, const char *password,
-                              size_t password_len, uint32_t iterations,
-                              const uint8_t *in, size_t in_len) {
+// add_encrypted_data encrypts |in| with |pbe_nid| and |pbe_cipher|, writing the
+// result to |out|. It returns one on success and zero on error. |pbe_nid| and
+// |pbe_cipher| are interpreted as in |PKCS8_encrypt|.
+static int add_encrypted_data(CBB *out, int pbe_nid,
+                              const EVP_CIPHER *pbe_cipher,
+                              const char *password, size_t password_len,
+                              uint32_t iterations, const uint8_t *in,
+                              size_t in_len) {
   uint8_t salt[PKCS5_SALT_LEN];
   if (!RAND_bytes(salt, sizeof(salt))) {
     return 0;
   }
 
-  int ret = 0;
-  EVP_CIPHER_CTX ctx;
-  EVP_CIPHER_CTX_init(&ctx);
+  bssl::ScopedEVP_CIPHER_CTX ctx;
   CBB content_info, type, wrapper, encrypted_data, encrypted_content_info,
       inner_type, encrypted_content;
   if (  // Add the ContentInfo wrapping.
@@ -1033,44 +1036,39 @@ static int add_encrypted_data(CBB *out, int pbe_nid, const char *password,
       !CBB_add_asn1(&encrypted_content_info, &inner_type, CBS_ASN1_OBJECT) ||
       !CBB_add_bytes(&inner_type, kPKCS7Data, sizeof(kPKCS7Data)) ||
       // Set up encryption and fill in contentEncryptionAlgorithm.
-      !pkcs12_pbe_encrypt_init(&encrypted_content_info, &ctx, pbe_nid,
-                               iterations, password, password_len, salt,
-                               sizeof(salt)) ||
+      !pkcs12_pbe_encrypt_init(&encrypted_content_info, ctx.get(), pbe_nid,
+                               pbe_cipher, iterations, password, password_len,
+                               salt, sizeof(salt)) ||
       // Note this tag is primitive. It is an implicitly-tagged OCTET_STRING, so
       // it inherits the inner tag's constructed bit.
       !CBB_add_asn1(&encrypted_content_info, &encrypted_content,
                     CBS_ASN1_CONTEXT_SPECIFIC | 0)) {
-    goto err;
+    return 0;
   }
 
-  {
-    size_t max_out = in_len + EVP_CIPHER_CTX_block_size(&ctx);
-    if (max_out < in_len) {
-      OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_TOO_LONG);
-      goto err;
-    }
-
-    uint8_t *ptr;
-    int n1, n2;
-    if (!CBB_reserve(&encrypted_content, &ptr, max_out) ||
-        !EVP_CipherUpdate(&ctx, ptr, &n1, in, in_len) ||
-        !EVP_CipherFinal_ex(&ctx, ptr + n1, &n2) ||
-        !CBB_did_write(&encrypted_content, n1 + n2) || !CBB_flush(out)) {
-      goto err;
-    }
+  size_t max_out = in_len + EVP_CIPHER_CTX_block_size(ctx.get());
+  if (max_out < in_len) {
+    OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_TOO_LONG);
+    return 0;
   }
 
-  ret = 1;
+  uint8_t *ptr;
+  int n1, n2;
+  if (!CBB_reserve(&encrypted_content, &ptr, max_out) ||
+      !EVP_CipherUpdate(ctx.get(), ptr, &n1, in, in_len) ||
+      !EVP_CipherFinal_ex(ctx.get(), ptr + n1, &n2) ||
+      !CBB_did_write(&encrypted_content, n1 + n2) || !CBB_flush(out)) {
+    return 0;
+  }
 
-err:
-  EVP_CIPHER_CTX_cleanup(&ctx);
-  return ret;
+  return 1;
 }
 
 PKCS12 *PKCS12_create(const char *password, const char *name,
                       const EVP_PKEY *pkey, X509 *cert,
                       const STACK_OF(X509) *chain, int key_nid, int cert_nid,
                       int iterations, int mac_iterations, int key_type) {
+  // TODO(crbug.com/396434682): Improve these defaults.
   if (key_nid == 0) {
     key_nid = NID_pbe_WithSHA1And3_Key_TripleDES_CBC;
   }
@@ -1186,13 +1184,21 @@ PKCS12 *PKCS12_create(const char *password, const char *name,
         goto err;
       }
     } else {
+      // This function differs from other OpenSSL functions in how PBES1 and
+      // PBES2 schemes are selected. If the NID matches a cipher, treat this as
+      // PBES2 instead. Convert to the other convention.
+      const EVP_CIPHER *cipher = pkcs5_pbe2_nid_to_cipher(cert_nid);
+      if (cipher != nullptr) {
+        cert_nid = -1;
+      }
       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));
+      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, cipher, password,
+                             password_len, iterations, CBB_data(&plaintext_cbb),
+                             CBB_len(&plaintext_cbb));
       CBB_cleanup(&plaintext_cbb);
       if (!ok) {
         goto err;
@@ -1228,12 +1234,19 @@ PKCS12 *PKCS12_create(const char *password, const char *name,
         goto err;
       }
     } else {
+      // This function differs from other OpenSSL functions in how PBES1 and
+      // PBES2 schemes are selected. If the NID matches a cipher, treat this as
+      // PBES2 instead. Convert to the other convention.
+      const EVP_CIPHER *cipher = pkcs5_pbe2_nid_to_cipher(key_nid);
+      if (cipher != nullptr) {
+        key_nid = -1;
+      }
       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,
+              &bag_contents, key_nid, cipher, password, password_len,
               NULL /* generate a random salt */,
               0 /* use default salt length */, iterations, pkey)) {
         goto err;

include/openssl/pkcs8.h

@@ -174,11 +174,24 @@ OPENSSL_EXPORT int PKCS12_verify_mac(const PKCS12 *p12, const char *password,
 // |NID_pbe_WithSHA1And40BitRC2_CBC|, |PKCS12_DEFAULT_ITER|, 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.
+// |key_nid| and |cert_nid| are then interpreted as follows:
+//
+// * If the NID is a cipher that is supported with PBES2, e.g.
+//   |NID_aes_256_cbc|, this function will use it with PBES2 and a default KDF
+//   (currently PBKDF2 with HMAC-SHA1). There is no way to specify the KDF in
+//   this function.
+//
+// * If the NID is a PBES1 suite, e.g. |NID_pbe_WithSHA1And3_Key_TripleDES_CBC|,
+//   this function will use the specified suite.
+//
+// * If the NID is -1, this function will disable encryption for the key or
+//   certificate. 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.
+//
+// WARNING: This differs from other functions in this module, which use a pair
+// of NID and |EVP_CIPHER| parameters to pick between PBES1 and PBES2 schemes.
 //
 // If |cert| or |chain| have associated aliases (see |X509_alias_set1|), they
 // will be included in the output as friendlyName attributes (RFC 2985). It is