Update defaults for PKCS12_create
Even the "modern" variants of the PKCS password-based private key
encryption schemes are are not very modern. We largely do not treat them
as load-bearing for security. Still, it's 2025, so we ought to upgrade
their defaults up to late 90s and early 00s cryptography.
This matches OpenSSL upstream's new behavior:
- The default MAC iteration count is 2048, not 1
- The default encryption algorithm is PBES2 with AES-256-CBC, not 3DES
  and 40-bit RC2
- The PRF function for PBKDF2 inside PBES2 is HMAC-SHA256, not
  HMAC-SHA1.
Update-Note: The defaults for PKCS#12 are changed as above. They match
upstream OpenSSL, so any systems compatible with OpenSSL will already be
compatible with this. The old defaults are still available by passing
them explicitly to PKCS12_create.
The one exception that OpenSSL's API does not have any way to change the
PBKDF2 PRF hash in PKCS#12. We don't anticipate this being a concern.
Note that this PRF change does not impact the old PBES1 schemes, only
the newer PBES2 schemes.
Fixed: 396434682
Change-Id: I949966483ff96796b0f76dd10b059fc98ecdeae9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/79527
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/pkcs8/p5_pbev2.cc b/crypto/pkcs8/p5_pbev2.cc
index 1486869..5da0cdb 100644
--- a/crypto/pkcs8/p5_pbev2.cc
+++ b/crypto/pkcs8/p5_pbev2.cc
@@ -143,7 +143,7 @@
   }
 
   // See RFC 8018, appendix A.
-  CBB algorithm, param, kdf, kdf_param, cipher_cbb;
+  CBB algorithm, param, kdf, kdf_param, prf, cipher_cbb;
   if (!CBB_add_asn1(out, &algorithm, CBS_ASN1_SEQUENCE) ||
       !CBB_add_asn1_element(&algorithm, CBS_ASN1_OBJECT, kPBES2,
                             sizeof(kPBES2)) ||
@@ -156,8 +156,11 @@
       // Specify a key length for RC2.
       (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.
+      // Use hmacWithSHA256 for the PRF.
+      !CBB_add_asn1(&kdf_param, &prf, CBS_ASN1_SEQUENCE) ||
+      !CBB_add_asn1_element(&prf, CBS_ASN1_OBJECT, kHMACWithSHA256,
+                            sizeof(kHMACWithSHA256)) ||
+      !CBB_add_asn1_element(&prf, CBS_ASN1_NULL, nullptr, 0) ||
       !CBB_add_asn1(¶m, &cipher_cbb, CBS_ASN1_SEQUENCE) ||
       !add_cipher_oid(&cipher_cbb, cipher_nid) ||
       // RFC 8018 says RC2-CBC and RC5-CBC-Pad use a SEQUENCE with version and
@@ -168,7 +171,7 @@
     return 0;
   }
 
-  return pkcs5_pbe2_cipher_init(ctx, cipher, EVP_sha1(), iterations, pass,
+  return pkcs5_pbe2_cipher_init(ctx, cipher, EVP_sha256(), iterations, pass,
                                 pass_len, salt, salt_len, iv,
                                 EVP_CIPHER_iv_length(cipher), 1 /* encrypt */);
 }
diff --git a/crypto/pkcs8/pkcs8_x509.cc b/crypto/pkcs8/pkcs8_x509.cc
index c5abc75..5f5286f 100644
--- a/crypto/pkcs8/pkcs8_x509.cc
+++ b/crypto/pkcs8/pkcs8_x509.cc
@@ -1067,18 +1067,17 @@
                       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;
+    key_nid = NID_aes_256_cbc;
   }
   if (cert_nid == 0) {
-    cert_nid = NID_pbe_WithSHA1And40BitRC2_CBC;
+    cert_nid = NID_aes_256_cbc;
   }
   if (iterations == 0) {
     iterations = PKCS12_DEFAULT_ITER;
   }
   if (mac_iterations == 0) {
-    mac_iterations = 1;
+    mac_iterations = PKCS12_DEFAULT_ITER;
   }
   if (  // In OpenSSL, this specifies a non-standard Microsoft key usage
         // extension which we do not currently support.
diff --git a/include/openssl/pkcs8.h b/include/openssl/pkcs8.h
index 8693566..47847f8 100644
--- a/include/openssl/pkcs8.h
+++ b/include/openssl/pkcs8.h
@@ -170,15 +170,14 @@
 // key. |key_type| must be zero. |pkey| and |cert| may be NULL to omit them.
 //
 // 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|, |PKCS12_DEFAULT_ITER|, and one,
-// respectively.
+// to use defaults, which are |NID_aes_256_cbc|, |NID_aes_256_cbc|,
+// |PKCS12_DEFAULT_ITER|, and |PKCS12_DEFAULT_ITER|, respectively.
 //
 // |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
+//   (currently PBKDF2 with HMAC-SHA256). 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|,