diff --git a/crypto/pkcs8/pkcs12_test.cc b/crypto/pkcs8/pkcs12_test.cc index 2a78a9794c..147567118a 100644 --- a/crypto/pkcs8/pkcs12_test.cc +++ b/crypto/pkcs8/pkcs12_test.cc @@ -395,6 +395,33 @@ static void TestRoundTrip(const char *password, const char *name, ASSERT_TRUE(certs2); ASSERT_TRUE(PKCS12_get_key_and_certs(&key2, certs2.get(), &cbs, password)); bssl::UniquePtr free_key2(key2); + + // Check that writing to a |BIO| does the same thing. + bssl::UniquePtr bio(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio); + ASSERT_TRUE(i2d_PKCS12_bio(bio.get(), pkcs12.get())); + const uint8_t *bio_data; + size_t bio_len; + ASSERT_TRUE(BIO_mem_contents(bio.get(), &bio_data, &bio_len)); + EXPECT_EQ(Bytes(bio_data, bio_len), Bytes(der, len)); + + // Check that the result round-trips with |PKCS12_set_mac| as well. The + // resulting bytes will be different due to the regenerated salt, but |pkcs12| + // should still be in a usable state with the same certs and keys encoded. + uint8_t *der2 = nullptr; + EXPECT_TRUE(PKCS12_set_mac(pkcs12.get(), password, strlen(password), nullptr, + 0, mac_iterations, nullptr)); + len = i2d_PKCS12(pkcs12.get(), &der2); + ASSERT_GT(len, 0); + bssl::UniquePtr free_der2(der2); + + CBS_init(&cbs, der2, len); + EVP_PKEY *key3 = nullptr; + certs2.reset(sk_X509_new_null()); + ASSERT_TRUE(certs2); + ASSERT_TRUE(PKCS12_get_key_and_certs(&key3, certs2.get(), &cbs, password)); + bssl::UniquePtr free_key3(key3); + // Note |EVP_PKEY_cmp| returns one for equality while |X509_cmp| returns zero. if (key) { EXPECT_EQ(1, EVP_PKEY_cmp(key2, key.get())); @@ -421,15 +448,6 @@ static void TestRoundTrip(const char *password, const char *name, static_cast(actual_name_len))); } } - - // Check that writing to a |BIO| does the same thing. - bssl::UniquePtr bio(BIO_new(BIO_s_mem())); - ASSERT_TRUE(bio); - ASSERT_TRUE(i2d_PKCS12_bio(bio.get(), pkcs12.get())); - const uint8_t *bio_data; - size_t bio_len; - ASSERT_TRUE(BIO_mem_contents(bio.get(), &bio_data, &bio_len)); - EXPECT_EQ(Bytes(bio_data, bio_len), Bytes(der, len)); } TEST(PKCS12Test, RoundTrip) { @@ -533,7 +551,7 @@ static bssl::UniquePtr MakeTestCert(EVP_PKEY *key) { return x509; } -static bool PKCS12CreateVector(std::vector *out, EVP_PKEY *pkey, +static bool PKCS12CreateVector(bssl::UniquePtr &p12, EVP_PKEY *pkey, const std::vector &certs) { bssl::UniquePtr chain(sk_X509_new_null()); if (!chain) { @@ -546,31 +564,17 @@ static bool PKCS12CreateVector(std::vector *out, EVP_PKEY *pkey, } } - bssl::UniquePtr p12(PKCS12_create(kPassword, nullptr /* name */, pkey, - nullptr /* cert */, chain.get(), 0, - 0, 0, 0, 0)); + p12.reset(PKCS12_create(kPassword, nullptr /* name */, pkey, + nullptr /* cert */, chain.get(), 0, 0, 0, 0, 0)); if (!p12) { return false; } - - int len = i2d_PKCS12(p12.get(), nullptr); - if (len < 0) { - return false; - } - out->resize(static_cast(len)); - uint8_t *ptr = out->data(); - return i2d_PKCS12(p12.get(), &ptr) == len; + return true; } -static void ExpectPKCS12Parse(bssl::Span in, +static void ExpectPKCS12Parse(bssl::UniquePtr &p12, EVP_PKEY *expect_key, X509 *expect_cert, const std::vector &expect_ca_certs) { - bssl::UniquePtr bio(BIO_new_mem_buf(in.data(), in.size())); - ASSERT_TRUE(bio); - - bssl::UniquePtr p12(d2i_PKCS12_bio(bio.get(), nullptr)); - ASSERT_TRUE(p12); - EVP_PKEY *key = nullptr; X509 *cert = nullptr; STACK_OF(X509) *ca_certs = nullptr; @@ -618,33 +622,32 @@ TEST(PKCS12Test, Order) { ASSERT_TRUE(cert3); // PKCS12_parse uses the key to select the main certificate. - std::vector p12; - ASSERT_TRUE(PKCS12CreateVector(&p12, key1.get(), + bssl::UniquePtr p12; + ASSERT_TRUE(PKCS12CreateVector(p12, key1.get(), {cert1.get(), cert2.get(), cert3.get()})); ExpectPKCS12Parse(p12, key1.get(), cert1.get(), {cert2.get(), cert3.get()}); - ASSERT_TRUE(PKCS12CreateVector(&p12, key1.get(), + ASSERT_TRUE(PKCS12CreateVector(p12, key1.get(), {cert3.get(), cert1.get(), cert2.get()})); ExpectPKCS12Parse(p12, key1.get(), cert1.get(), {cert3.get(), cert2.get()}); - ASSERT_TRUE(PKCS12CreateVector(&p12, key1.get(), + ASSERT_TRUE(PKCS12CreateVector(p12, key1.get(), {cert2.get(), cert3.get(), cert1.get()})); ExpectPKCS12Parse(p12, key1.get(), cert1.get(), {cert2.get(), cert3.get()}); // In case of duplicates, the last one is selected. (It is unlikely anything // depends on which is selected, but we match OpenSSL.) - ASSERT_TRUE( - PKCS12CreateVector(&p12, key1.get(), {cert1.get(), cert1b.get()})); + ASSERT_TRUE(PKCS12CreateVector(p12, key1.get(), {cert1.get(), cert1b.get()})); ExpectPKCS12Parse(p12, key1.get(), cert1b.get(), {cert1.get()}); // If there is no key, all certificates are returned as "CA" certificates. - ASSERT_TRUE(PKCS12CreateVector(&p12, nullptr, + ASSERT_TRUE(PKCS12CreateVector(p12, nullptr, {cert1.get(), cert2.get(), cert3.get()})); ExpectPKCS12Parse(p12, nullptr, nullptr, {cert1.get(), cert2.get(), cert3.get()}); // The same happens if there is a key, but it does not match any certificate. - ASSERT_TRUE(PKCS12CreateVector(&p12, key1.get(), {cert2.get(), cert3.get()})); + ASSERT_TRUE(PKCS12CreateVector(p12, key1.get(), {cert2.get(), cert3.get()})); ExpectPKCS12Parse(p12, key1.get(), nullptr, {cert2.get(), cert3.get()}); } @@ -663,13 +666,8 @@ TEST(PKCS12Test, CreateWithAlias) { ASSERT_EQ(res, 1); std::vector certs = {cert1.get(), cert2.get()}; - std::vector der; - ASSERT_TRUE(PKCS12CreateVector(&der, key.get(), certs)); - - bssl::UniquePtr bio(BIO_new_mem_buf(der.data(), der.size())); - ASSERT_TRUE(bio); - bssl::UniquePtr p12(d2i_PKCS12_bio(bio.get(), nullptr)); - ASSERT_TRUE(p12); + bssl::UniquePtr p12; + ASSERT_TRUE(PKCS12CreateVector(p12, key.get(), certs)); EVP_PKEY *parsed_key = nullptr; X509 *parsed_cert = nullptr; @@ -695,3 +693,48 @@ TEST(PKCS12Test, BasicAlloc) { bssl::UniquePtr p12(PKCS12_new()); ASSERT_TRUE(p12); } + +TEST(PKCS12Test, SetMac) { + bssl::UniquePtr key1 = MakeTestKey(); + ASSERT_TRUE(key1); + bssl::UniquePtr cert1 = MakeTestCert(key1.get()); + ASSERT_TRUE(cert1); + bssl::UniquePtr cert1b = MakeTestCert(key1.get()); + ASSERT_TRUE(cert1b); + bssl::UniquePtr key2 = MakeTestKey(); + ASSERT_TRUE(key2); + bssl::UniquePtr cert2 = MakeTestCert(key2.get()); + ASSERT_TRUE(cert2); + bssl::UniquePtr key3 = MakeTestKey(); + ASSERT_TRUE(key3); + bssl::UniquePtr cert3 = MakeTestCert(key3.get()); + ASSERT_TRUE(cert3); + + // PKCS12_parse uses the key to select the main certificate. + bssl::UniquePtr p12; + ASSERT_TRUE(PKCS12CreateVector(p12, key1.get(), + {cert1.get(), cert2.get(), cert3.get()})); + EXPECT_TRUE(PKCS12_set_mac(p12.get(), kPassword, strlen(kPassword), nullptr, + 0, 0, nullptr)); + ExpectPKCS12Parse(p12, key1.get(), cert1.get(), {cert2.get(), cert3.get()}); + + ASSERT_TRUE(PKCS12CreateVector(p12, key1.get(), + {cert3.get(), cert1.get(), cert2.get()})); + EXPECT_TRUE(PKCS12_set_mac(p12.get(), kPassword, strlen(kPassword), nullptr, + 0, 0, nullptr)); + ExpectPKCS12Parse(p12, key1.get(), cert1.get(), {cert3.get(), cert2.get()}); + + ASSERT_TRUE(PKCS12CreateVector(p12, key1.get(), + {cert2.get(), cert3.get(), cert1.get()})); + EXPECT_TRUE(PKCS12_set_mac(p12.get(), kPassword, strlen(kPassword), nullptr, + 0, 0, nullptr)); + ExpectPKCS12Parse(p12, key1.get(), cert1.get(), {cert2.get(), cert3.get()}); + + // The same password should be used with |PKCS12_create| and |PKCS12_set_mac|. + ASSERT_TRUE(PKCS12CreateVector(p12, key1.get(), + {cert1.get(), cert2.get(), cert3.get()})); + EXPECT_FALSE(PKCS12_set_mac(p12.get(), kUnicodePassword, + strlen(kUnicodePassword), nullptr, 0, 0, + nullptr)); +} + diff --git a/crypto/pkcs8/pkcs8_x509.c b/crypto/pkcs8/pkcs8_x509.c index 2e7148c45e..93ff8df459 100644 --- a/crypto/pkcs8/pkcs8_x509.c +++ b/crypto/pkcs8/pkcs8_x509.c @@ -1114,6 +1114,44 @@ static int add_encrypted_data(CBB *out, int pbe_nid, const char *password, return ret; } +static int pkcs12_gen_and_write_mac(CBB *out_pfx, const uint8_t *auth_safe_data, + size_t auth_safe_data_len, + const char *password, size_t password_len, + uint8_t *mac_salt, size_t salt_len, + int mac_iterations, const EVP_MD *md) { + int ret = 0; + uint8_t mac_key[EVP_MAX_MD_SIZE]; + uint8_t mac[EVP_MAX_MD_SIZE]; + unsigned mac_len; + if (!pkcs12_key_gen(password, password_len, mac_salt, salt_len, PKCS12_MAC_ID, + mac_iterations, EVP_MD_size(md), mac_key, md) || + !HMAC(md, mac_key, EVP_MD_size(md), auth_safe_data, auth_safe_data_len, + mac, &mac_len)) { + goto out; + } + + CBB mac_data, digest_info, mac_cbb, mac_salt_cbb; + if (!CBB_add_asn1(out_pfx, &mac_data, CBS_ASN1_SEQUENCE) || + !CBB_add_asn1(&mac_data, &digest_info, CBS_ASN1_SEQUENCE) || + !EVP_marshal_digest_algorithm(&digest_info, md) || + !CBB_add_asn1(&digest_info, &mac_cbb, CBS_ASN1_OCTETSTRING) || + !CBB_add_bytes(&mac_cbb, mac, mac_len) || + !CBB_add_asn1(&mac_data, &mac_salt_cbb, CBS_ASN1_OCTETSTRING) || + !CBB_add_bytes(&mac_salt_cbb, mac_salt, salt_len) || + // The iteration count has a DEFAULT of 1, but RFC 7292 says "The default + // is for historical reasons and its use is deprecated." Thus we + // explicitly encode the iteration count, though it is not valid DER. + !CBB_add_asn1_uint64(&mac_data, mac_iterations) || + !CBB_flush(out_pfx)) { + goto out; + } + ret = 1; + +out: + OPENSSL_cleanse(mac_key, sizeof(mac_key)); + return ret; +} + 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, @@ -1194,9 +1232,7 @@ PKCS12 *PKCS12_create(const char *password, const char *name, PKCS12 *ret = NULL; CBB cbb, pfx, auth_safe, auth_safe_oid, auth_safe_wrapper, auth_safe_data, content_infos; - uint8_t mac_key[EVP_MAX_MD_SIZE]; - if (!CBB_init(&cbb, 0) || - !CBB_add_asn1(&cbb, &pfx, CBS_ASN1_SEQUENCE) || + if (!CBB_init(&cbb, 0) || !CBB_add_asn1(&cbb, &pfx, CBS_ASN1_SEQUENCE) || !CBB_add_asn1_uint64(&pfx, 3) || // auth_safe is a data ContentInfo. !CBB_add_asn1(&pfx, &auth_safe, CBS_ASN1_SEQUENCE) || @@ -1301,30 +1337,11 @@ PKCS12 *PKCS12_create(const char *password, const char *name, // covers |auth_safe_data|. const EVP_MD *mac_md = EVP_sha1(); uint8_t mac_salt[PKCS5_SALT_LEN]; - uint8_t mac[EVP_MAX_MD_SIZE]; - unsigned mac_len; if (!CBB_flush(&auth_safe_data) || !RAND_bytes(mac_salt, sizeof(mac_salt)) || - !pkcs12_key_gen(password, password_len, mac_salt, sizeof(mac_salt), - PKCS12_MAC_ID, mac_iterations, EVP_MD_size(mac_md), - mac_key, mac_md) || - !HMAC(mac_md, mac_key, EVP_MD_size(mac_md), CBB_data(&auth_safe_data), - CBB_len(&auth_safe_data), mac, &mac_len)) { - goto err; - } - - CBB mac_data, digest_info, mac_cbb, mac_salt_cbb; - if (!CBB_add_asn1(&pfx, &mac_data, CBS_ASN1_SEQUENCE) || - !CBB_add_asn1(&mac_data, &digest_info, CBS_ASN1_SEQUENCE) || - !EVP_marshal_digest_algorithm(&digest_info, mac_md) || - !CBB_add_asn1(&digest_info, &mac_cbb, CBS_ASN1_OCTETSTRING) || - !CBB_add_bytes(&mac_cbb, mac, mac_len) || - !CBB_add_asn1(&mac_data, &mac_salt_cbb, CBS_ASN1_OCTETSTRING) || - !CBB_add_bytes(&mac_salt_cbb, mac_salt, sizeof(mac_salt)) || - // The iteration count has a DEFAULT of 1, but RFC 7292 says "The default - // is for historical reasons and its use is deprecated." Thus we - // explicitly encode the iteration count, though it is not valid DER. - !CBB_add_asn1_uint64(&mac_data, mac_iterations)) { + !pkcs12_gen_and_write_mac( + &pfx, CBB_data(&auth_safe_data), CBB_len(&auth_safe_data), password, + password_len, mac_salt, sizeof(mac_salt), mac_iterations, mac_md)) { goto err; } @@ -1337,7 +1354,6 @@ PKCS12 *PKCS12_create(const char *password, const char *name, } err: - OPENSSL_cleanse(mac_key, sizeof(mac_key)); CBB_cleanup(&cbb); return ret; } @@ -1353,3 +1369,98 @@ void PKCS12_free(PKCS12 *p12) { OPENSSL_free(p12->ber_bytes); OPENSSL_free(p12); } + +int PKCS12_set_mac(PKCS12 *p12, const char *password, int password_len, + unsigned char *salt, int salt_len, int mac_iterations, + const EVP_MD *md) { + GUARD_PTR(p12); + int ret = 0; + + if (mac_iterations == 0) { + mac_iterations = 1; + } + if (salt_len == 0) { + salt_len = PKCS5_SALT_LEN; + } + // Generate |mac_salt| if |salt| is NULL and copy if NULL. + uint8_t *mac_salt = OPENSSL_malloc(salt_len); + if (salt == NULL) { + if (!RAND_bytes(mac_salt, salt_len)) { + goto out; + } + } else { + OPENSSL_memcpy(mac_salt, salt, salt_len); + } + // Match OpenSSL in using SHA-1 as the default hash function. + if (md == NULL) { + md = EVP_sha1(); + } + + uint8_t *storage = NULL; + CBS ber_bytes, in, pfx, authsafe, content_type, wrapped_authsafes, authsafes; + uint64_t version; + // The input may be in BER format. + CBS_init(&ber_bytes, p12->ber_bytes, p12->ber_len); + if (!CBS_asn1_ber_to_der(&ber_bytes, &in, &storage)) { + OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA); + goto out; + } + // There's no use case for |storage| anymore, so we free early. + OPENSSL_free(storage); + + if (!CBS_get_asn1(&in, &pfx, CBS_ASN1_SEQUENCE) || CBS_len(&in) != 0 || + !CBS_get_asn1_uint64(&pfx, &version)) { + OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA); + goto out; + } + if (version < 3) { + OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_VERSION); + goto out; + } + + if (!CBS_get_asn1(&pfx, &authsafe, CBS_ASN1_SEQUENCE)) { + OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA); + goto out; + } + // Save contents of |authsafe| to write back before the CBS is advanced. + const uint8_t *orig_authsafe = CBS_data(&authsafe); + size_t orig_authsafe_len = CBS_len(&authsafe); + + // Parse for |authsafes| which is the data that we should be running HMAC on. + if (!CBS_get_asn1(&authsafe, &content_type, CBS_ASN1_OBJECT) || + !CBS_get_asn1(&authsafe, &wrapped_authsafes, + CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 0) || + !CBS_get_asn1(&wrapped_authsafes, &authsafes, CBS_ASN1_OCTETSTRING)) { + OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA); + goto out; + } + + // Rewrite contents of |p12| with the original contents and updated MAC. + CBB cbb, out_pfx, out_auth_safe; + if (!CBB_init(&cbb, 0) || !CBB_add_asn1(&cbb, &out_pfx, CBS_ASN1_SEQUENCE) || + !CBB_add_asn1_uint64(&out_pfx, version) || + !CBB_add_asn1(&out_pfx, &out_auth_safe, CBS_ASN1_SEQUENCE) || + !CBB_add_bytes(&out_auth_safe, orig_authsafe, orig_authsafe_len) || + !pkcs12_gen_and_write_mac(&out_pfx, CBS_data(&authsafes), + CBS_len(&authsafes), password, password_len, + mac_salt, salt_len, mac_iterations, md)) { + CBB_cleanup(&cbb); + goto out; + } + + // Verify that the new password is consistent with the original. This is + // behavior specific to AWS-LC. + OPENSSL_free(p12->ber_bytes); + if(!CBB_finish(&cbb, &p12->ber_bytes, &p12->ber_len) || + !PKCS12_verify_mac(p12, password, password_len)) { + CBB_cleanup(&cbb); + goto out; + } + + ret = 1; + +out: + OPENSSL_free(mac_salt); + return ret; +} + diff --git a/include/openssl/pkcs8.h b/include/openssl/pkcs8.h index a6fd1c4cfe..96ff8340a4 100644 --- a/include/openssl/pkcs8.h +++ b/include/openssl/pkcs8.h @@ -125,8 +125,8 @@ OPENSSL_EXPORT EVP_PKEY *PKCS8_parse_encrypted_private_key(CBS *cbs, // Any friendlyName attributes (RFC 2985) in the PKCS#12 structure will be // returned on the |X509| objects as aliases. See also |X509_alias_get0|. OPENSSL_EXPORT int PKCS12_get_key_and_certs(EVP_PKEY **out_key, - STACK_OF(X509) *out_certs, - CBS *in, const char *password); + STACK_OF(X509) *out_certs, CBS *in, + const char *password); // Deprecated functions. @@ -149,10 +149,10 @@ OPENSSL_EXPORT PKCS12 *d2i_PKCS12(PKCS12 **out_p12, const uint8_t **ber_bytes, size_t ber_len); // d2i_PKCS12_bio acts like |d2i_PKCS12| but reads from a |BIO|. -OPENSSL_EXPORT PKCS12* d2i_PKCS12_bio(BIO *bio, PKCS12 **out_p12); +OPENSSL_EXPORT PKCS12 *d2i_PKCS12_bio(BIO *bio, PKCS12 **out_p12); // d2i_PKCS12_fp acts like |d2i_PKCS12| but reads from a |FILE|. -OPENSSL_EXPORT PKCS12* d2i_PKCS12_fp(FILE *fp, PKCS12 **out_p12); +OPENSSL_EXPORT PKCS12 *d2i_PKCS12_fp(FILE *fp, PKCS12 **out_p12); // i2d_PKCS12 is a dummy function which copies the contents of |p12|. If |out| // is not NULL then the result is written to |*out| and |*out| is advanced just @@ -188,6 +188,20 @@ OPENSSL_EXPORT int PKCS12_parse(const PKCS12 *p12, const char *password, EVP_PKEY **out_pkey, X509 **out_cert, STACK_OF(X509) **out_ca_certs); +// PKCS12_set_mac generates the MAC for |p12| with the designated |password|, +// |salt|, |mac_iterations|, and |md| specified. |password| MUST be the same +// password originally used to encrypt |p12|. Although OpenSSL will allow an +// invalid state with a different |password|, AWS-LC will throw an error and +// return 0. +// +// If |salt| is NULL, a random salt of |salt_len| bytes is generated. If +// |salt_len| is zero, a default salt length is used instead. +// If |md| is NULL, the default is use SHA1 to align with OpenSSL. +OPENSSL_EXPORT int PKCS12_set_mac(PKCS12 *p12, const char *password, + int password_len, unsigned char *salt, + int salt_len, int mac_iterations, + const EVP_MD *md); + // PKCS12_verify_mac returns one if |password| is a valid password for |p12| // and zero otherwise. Since |PKCS12_parse| doesn't take a length parameter, // it's not actually possible to use a non-NUL-terminated password to actually