diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index dc48c277a..63b7473a7 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1943,8 +1943,9 @@ TEST(X509Test, RSASign) { EXPECT_FALSE(X509_sign_ctx(cert.get(), md_ctx.get())); } -// Test the APIs for manually signing a certificate. -TEST(X509Test, RSASignManual) { +// Test the APIs for signing a certificate, particularly whether they correctly +// handle the TBSCertificate cache. +TEST(X509Test, SignCertificate) { const int kSignatureNID = NID_sha384WithRSAEncryption; const EVP_MD *kSignatureHash = EVP_sha384(); @@ -1955,74 +1956,86 @@ TEST(X509Test, RSASignManual) { ASSERT_TRUE(X509_ALGOR_set0(algor.get(), OBJ_nid2obj(kSignatureNID), V_ASN1_NULL, nullptr)); - // Test certificates made both from other certificates and |X509_new|, in case - // there are bugs in filling in fields from different states. (Parsed - // certificates contain a TBSCertificate cache, and |X509_new| initializes - // fields based on complex ASN.1 template logic.) - for (bool new_cert : {true, false}) { - SCOPED_TRACE(new_cert); - - bssl::UniquePtr cert; - if (new_cert) { - cert.reset(X509_new()); - ASSERT_TRUE(cert); - // Fill in some fields for the certificate arbitrarily. - EXPECT_TRUE(X509_set_version(cert.get(), X509_VERSION_3)); - EXPECT_TRUE(ASN1_INTEGER_set_int64(X509_get_serialNumber(cert.get()), 1)); - EXPECT_TRUE(X509_gmtime_adj(X509_getm_notBefore(cert.get()), 0)); - EXPECT_TRUE( - X509_gmtime_adj(X509_getm_notAfter(cert.get()), 60 * 60 * 24)); - X509_NAME *subject = X509_get_subject_name(cert.get()); - X509_NAME_add_entry_by_txt(subject, "CN", MBSTRING_ASC, - reinterpret_cast("Test"), -1, - -1, 0); - EXPECT_TRUE(X509_set_issuer_name(cert.get(), subject)); - EXPECT_TRUE(X509_set_pubkey(cert.get(), pkey.get())); - } else { - // Extract fields from a parsed certificate. - cert = CertFromPEM(kLeafPEM); - ASSERT_TRUE(cert); + // Test both signing with |X509_sign| and constructing a signature manually. + for (bool sign_manual : {true, false}) { + SCOPED_TRACE(sign_manual); + + // Test certificates made both from other certificates and |X509_new|, in + // case there are bugs in filling in fields from different states. (Parsed + // certificates contain a TBSCertificate cache, and |X509_new| initializes + // fields based on complex ASN.1 template logic.) + for (bool new_cert : {true, false}) { + SCOPED_TRACE(new_cert); + + bssl::UniquePtr cert; + if (new_cert) { + cert.reset(X509_new()); + ASSERT_TRUE(cert); + // Fill in some fields for the certificate arbitrarily. + EXPECT_TRUE(X509_set_version(cert.get(), X509_VERSION_3)); + EXPECT_TRUE( + ASN1_INTEGER_set_int64(X509_get_serialNumber(cert.get()), 1)); + EXPECT_TRUE(X509_gmtime_adj(X509_getm_notBefore(cert.get()), 0)); + EXPECT_TRUE( + X509_gmtime_adj(X509_getm_notAfter(cert.get()), 60 * 60 * 24)); + X509_NAME *subject = X509_get_subject_name(cert.get()); + X509_NAME_add_entry_by_txt(subject, "CN", MBSTRING_ASC, + reinterpret_cast("Test"), + -1, -1, 0); + EXPECT_TRUE(X509_set_issuer_name(cert.get(), subject)); + EXPECT_TRUE(X509_set_pubkey(cert.get(), pkey.get())); + } else { + // Extract fields from a parsed certificate. + cert = CertFromPEM(kLeafPEM); + ASSERT_TRUE(cert); - // We should test with a different algorithm from what is already in the - // certificate. - EXPECT_NE(kSignatureNID, X509_get_signature_nid(cert.get())); - } + // We should test with a different algorithm from what is already in the + // certificate. + EXPECT_NE(kSignatureNID, X509_get_signature_nid(cert.get())); + } - // Fill in the signature algorithm. - ASSERT_TRUE(X509_set1_signature_algo(cert.get(), algor.get())); - - // Extract the TBSCertificiate. - uint8_t *tbs_cert = nullptr; - int tbs_cert_len = i2d_re_X509_tbs(cert.get(), &tbs_cert); - bssl::UniquePtr free_tbs_cert(tbs_cert); - ASSERT_GT(tbs_cert_len, 0); - - // Generate a signature externally and fill it in. - bssl::ScopedEVP_MD_CTX md_ctx; - ASSERT_TRUE(EVP_DigestSignInit(md_ctx.get(), nullptr, kSignatureHash, - nullptr, pkey.get())); - size_t sig_len; - ASSERT_TRUE(EVP_DigestSign(md_ctx.get(), nullptr, &sig_len, tbs_cert, - tbs_cert_len)); - std::vector sig(sig_len); - ASSERT_TRUE(EVP_DigestSign(md_ctx.get(), sig.data(), &sig_len, tbs_cert, - tbs_cert_len)); - sig.resize(sig_len); - ASSERT_TRUE(X509_set1_signature_value(cert.get(), sig.data(), sig.size())); - - // Check the signature. - EXPECT_TRUE(X509_verify(cert.get(), pkey.get())); + if (sign_manual) { + // Fill in the signature algorithm. + ASSERT_TRUE(X509_set1_signature_algo(cert.get(), algor.get())); + + // Extract the TBSCertificiate. + uint8_t *tbs_cert = nullptr; + int tbs_cert_len = i2d_re_X509_tbs(cert.get(), &tbs_cert); + bssl::UniquePtr free_tbs_cert(tbs_cert); + ASSERT_GT(tbs_cert_len, 0); + + // Generate a signature externally and fill it in. + bssl::ScopedEVP_MD_CTX md_ctx; + ASSERT_TRUE(EVP_DigestSignInit(md_ctx.get(), nullptr, kSignatureHash, + nullptr, pkey.get())); + size_t sig_len; + ASSERT_TRUE(EVP_DigestSign(md_ctx.get(), nullptr, &sig_len, tbs_cert, + tbs_cert_len)); + std::vector sig(sig_len); + ASSERT_TRUE(EVP_DigestSign(md_ctx.get(), sig.data(), &sig_len, tbs_cert, + tbs_cert_len)); + sig.resize(sig_len); + ASSERT_TRUE( + X509_set1_signature_value(cert.get(), sig.data(), sig.size())); + } else { + ASSERT_TRUE(X509_sign(cert.get(), pkey.get(), EVP_sha384())); + } - // Re-encode the certificate. X509 objects contain a cached TBSCertificate - // encoding and |i2d_re_X509_tbs| should have refreshed that cache. - bssl::UniquePtr copy = ReencodeCertificate(cert.get()); - ASSERT_TRUE(copy); - EXPECT_TRUE(X509_verify(copy.get(), pkey.get())); + // Check the signature. + EXPECT_TRUE(X509_verify(cert.get(), pkey.get())); + + // Re-encode the certificate. X509 objects contain a cached TBSCertificate + // encoding and |i2d_re_X509_tbs| should have refreshed that cache. + bssl::UniquePtr copy = ReencodeCertificate(cert.get()); + ASSERT_TRUE(copy); + EXPECT_TRUE(X509_verify(copy.get(), pkey.get())); + } } } -// Test the APIs for manually signing a CSR. -TEST(X509Test, RSASignCRLManual) { +// Test the APIs for signing a CRL, particularly whether they correctly handle +// the TBSCertList cache. +TEST(X509Test, SignCRL) { const int kSignatureNID = NID_sha384WithRSAEncryption; const EVP_MD *kSignatureHash = EVP_sha384(); @@ -2033,69 +2046,80 @@ TEST(X509Test, RSASignCRLManual) { ASSERT_TRUE(X509_ALGOR_set0(algor.get(), OBJ_nid2obj(kSignatureNID), V_ASN1_NULL, nullptr)); - // Test CRLs made both from other CRLs and |X509_CRL_new|, in case there are - // bugs in filling in fields from different states. (Parsed CRLs contain a - // TBSCertList cache, and |X509_CRL_new| initializes fields based on complex - // ASN.1 template logic.) - for (bool new_crl : {true, false}) { - SCOPED_TRACE(new_crl); - - bssl::UniquePtr crl; - if (new_crl) { - crl.reset(X509_CRL_new()); - ASSERT_TRUE(crl); - // Fill in some fields for the certificate arbitrarily. - ASSERT_TRUE(X509_CRL_set_version(crl.get(), X509_CRL_VERSION_2)); - bssl::UniquePtr last_update(ASN1_TIME_new()); - ASSERT_TRUE(last_update); - ASSERT_TRUE(ASN1_TIME_set(last_update.get(), kReferenceTime)); - ASSERT_TRUE(X509_CRL_set1_lastUpdate(crl.get(), last_update.get())); - bssl::UniquePtr issuer(X509_NAME_new()); - ASSERT_TRUE(issuer); - ASSERT_TRUE(X509_NAME_add_entry_by_txt( - issuer.get(), "CN", MBSTRING_ASC, - reinterpret_cast("Test"), -1, -1, 0)); - EXPECT_TRUE(X509_CRL_set_issuer_name(crl.get(), issuer.get())); - } else { - // Extract fields from a parsed CRL. - crl = CRLFromPEM(kBasicCRL); - ASSERT_TRUE(crl); + // Test both signing with |X509_CRL_sign| and constructing a signature + // manually. + for (bool sign_manual : {true, false}) { + SCOPED_TRACE(sign_manual); + + // Test CRLs made both from other CRLs and |X509_CRL_new|, in case there are + // bugs in filling in fields from different states. (Parsed CRLs contain a + // TBSCertList cache, and |X509_CRL_new| initializes fields based on complex + // ASN.1 template logic.) + for (bool new_crl : {true, false}) { + SCOPED_TRACE(new_crl); + + bssl::UniquePtr crl; + if (new_crl) { + crl.reset(X509_CRL_new()); + ASSERT_TRUE(crl); + // Fill in some fields for the certificate arbitrarily. + ASSERT_TRUE(X509_CRL_set_version(crl.get(), X509_CRL_VERSION_2)); + bssl::UniquePtr last_update(ASN1_TIME_new()); + ASSERT_TRUE(last_update); + ASSERT_TRUE(ASN1_TIME_set(last_update.get(), kReferenceTime)); + ASSERT_TRUE(X509_CRL_set1_lastUpdate(crl.get(), last_update.get())); + bssl::UniquePtr issuer(X509_NAME_new()); + ASSERT_TRUE(issuer); + ASSERT_TRUE(X509_NAME_add_entry_by_txt( + issuer.get(), "CN", MBSTRING_ASC, + reinterpret_cast("Test"), -1, -1, 0)); + EXPECT_TRUE(X509_CRL_set_issuer_name(crl.get(), issuer.get())); + } else { + // Extract fields from a parsed CRL. + crl = CRLFromPEM(kBasicCRL); + ASSERT_TRUE(crl); - // We should test with a different algorithm from what is already in the - // CRL. - EXPECT_NE(kSignatureNID, X509_CRL_get_signature_nid(crl.get())); - } + // We should test with a different algorithm from what is already in the + // CRL. + EXPECT_NE(kSignatureNID, X509_CRL_get_signature_nid(crl.get())); + } + + if (sign_manual) { + // Fill in the signature algorithm. + ASSERT_TRUE(X509_CRL_set1_signature_algo(crl.get(), algor.get())); + + // Extract the TBSCertList. + uint8_t *tbs = nullptr; + int tbs_len = i2d_re_X509_CRL_tbs(crl.get(), &tbs); + bssl::UniquePtr free_tbs(tbs); + ASSERT_GT(tbs_len, 0); + + // Generate a signature externally and fill it in. + bssl::ScopedEVP_MD_CTX md_ctx; + ASSERT_TRUE(EVP_DigestSignInit(md_ctx.get(), nullptr, kSignatureHash, + nullptr, pkey.get())); + size_t sig_len; + ASSERT_TRUE( + EVP_DigestSign(md_ctx.get(), nullptr, &sig_len, tbs, tbs_len)); + std::vector sig(sig_len); + ASSERT_TRUE( + EVP_DigestSign(md_ctx.get(), sig.data(), &sig_len, tbs, tbs_len)); + sig.resize(sig_len); + ASSERT_TRUE( + X509_CRL_set1_signature_value(crl.get(), sig.data(), sig.size())); + } else { + ASSERT_TRUE(X509_CRL_sign(crl.get(), pkey.get(), EVP_sha384())); + } + + // Check the signature. + EXPECT_TRUE(X509_CRL_verify(crl.get(), pkey.get())); - // Fill in the signature algorithm. - ASSERT_TRUE(X509_CRL_set1_signature_algo(crl.get(), algor.get())); - - // Extract the TBSCertList. - uint8_t *tbs = nullptr; - int tbs_len = i2d_re_X509_CRL_tbs(crl.get(), &tbs); - bssl::UniquePtr free_tbs(tbs); - ASSERT_GT(tbs_len, 0); - - // Generate a signature externally and fill it in. - bssl::ScopedEVP_MD_CTX md_ctx; - ASSERT_TRUE(EVP_DigestSignInit(md_ctx.get(), nullptr, kSignatureHash, - nullptr, pkey.get())); - size_t sig_len; - ASSERT_TRUE(EVP_DigestSign(md_ctx.get(), nullptr, &sig_len, tbs, tbs_len)); - std::vector sig(sig_len); - ASSERT_TRUE( - EVP_DigestSign(md_ctx.get(), sig.data(), &sig_len, tbs, tbs_len)); - sig.resize(sig_len); - ASSERT_TRUE( - X509_CRL_set1_signature_value(crl.get(), sig.data(), sig.size())); - - // Check the signature. - EXPECT_TRUE(X509_CRL_verify(crl.get(), pkey.get())); - - // Re-encode the CRL. X509_CRL objects contain a cached TBSCertList encoding - // and |i2d_re_X509_tbs| should have refreshed that cache. - bssl::UniquePtr copy = ReencodeCRL(crl.get()); - ASSERT_TRUE(copy); - EXPECT_TRUE(X509_CRL_verify(copy.get(), pkey.get())); + // Re-encode the CRL. X509_CRL objects contain a cached TBSCertList + // encoding and |i2d_re_X509_tbs| should have refreshed that cache. + bssl::UniquePtr copy = ReencodeCRL(crl.get()); + ASSERT_TRUE(copy); + EXPECT_TRUE(X509_CRL_verify(copy.get(), pkey.get())); + } } } @@ -2117,8 +2141,9 @@ rZGEJG3+X9OuhczVKGJyg+3gU7oDbecc -----END CERTIFICATE REQUEST----- )"; -// Test the APIs for manually signing a certificate. -TEST(X509Test, RSASignCSRManual) { +// Test the APIs for signing a CSR, particularly whether they correctly handle +// the CertificationRequestInfo cache. +TEST(X509Test, SignCSR) { const int kSignatureNID = NID_sha384WithRSAEncryption; const EVP_MD *kSignatureHash = EVP_sha384(); @@ -2129,71 +2154,82 @@ TEST(X509Test, RSASignCSRManual) { ASSERT_TRUE(X509_ALGOR_set0(algor.get(), OBJ_nid2obj(kSignatureNID), V_ASN1_NULL, nullptr)); - // Test CSRs made both from other CSRs and |X509_REQ_new|, in case there are - // bugs in filling in fields from different states. (Parsed CSRs contain a - // CertificationRequestInfo cache, and |X509_REQ_new| initializes fields based - // on complex ASN.1 template logic.) - for (bool new_csr : {true, false}) { - SCOPED_TRACE(new_csr); - - bssl::UniquePtr csr; - if (new_csr) { - csr.reset(X509_REQ_new()); - ASSERT_TRUE(csr); - bssl::UniquePtr subject(X509_NAME_new()); - ASSERT_TRUE(subject); - ASSERT_TRUE(X509_NAME_add_entry_by_txt( - subject.get(), "CN", MBSTRING_ASC, - reinterpret_cast("New CSR"), -1, -1, 0)); - EXPECT_TRUE(X509_REQ_set_subject_name(csr.get(), subject.get())); - } else { - // Extract fields from a parsed CSR. - csr = CSRFromPEM(kTestCSR); - ASSERT_TRUE(csr); - } + // Test both signing with |X509_REQ_sign| and constructing a signature + // manually. + for (bool sign_manual : {true, false}) { + SCOPED_TRACE(sign_manual); + + // Test CSRs made both from other CSRs and |X509_REQ_new|, in case there are + // bugs in filling in fields from different states. (Parsed CSRs contain a + // CertificationRequestInfo cache, and |X509_REQ_new| initializes fields + // based on complex ASN.1 template logic.) + for (bool new_csr : {true, false}) { + SCOPED_TRACE(new_csr); + + bssl::UniquePtr csr; + if (new_csr) { + csr.reset(X509_REQ_new()); + ASSERT_TRUE(csr); + bssl::UniquePtr subject(X509_NAME_new()); + ASSERT_TRUE(subject); + ASSERT_TRUE(X509_NAME_add_entry_by_txt( + subject.get(), "CN", MBSTRING_ASC, + reinterpret_cast("New CSR"), -1, -1, 0)); + EXPECT_TRUE(X509_REQ_set_subject_name(csr.get(), subject.get())); + } else { + // Extract fields from a parsed CSR. + csr = CSRFromPEM(kTestCSR); + ASSERT_TRUE(csr); + } + + // Override the public key from the CSR unconditionally. Unlike + // certificates and CRLs, CSRs do not contain a signed copy of the + // signature algorithm, so we use a different field to confirm + // |i2d_re_X509_REQ_tbs| clears the cache as expected. + EXPECT_TRUE(X509_REQ_set_pubkey(csr.get(), pkey.get())); + + if (sign_manual) { + // Fill in the signature algorithm. + ASSERT_TRUE(X509_REQ_set1_signature_algo(csr.get(), algor.get())); + + // Extract the CertificationRequestInfo. + uint8_t *tbs = nullptr; + int tbs_len = i2d_re_X509_REQ_tbs(csr.get(), &tbs); + bssl::UniquePtr free_tbs(tbs); + ASSERT_GT(tbs_len, 0); + + // Generate a signature externally and fill it in. + bssl::ScopedEVP_MD_CTX md_ctx; + ASSERT_TRUE(EVP_DigestSignInit(md_ctx.get(), nullptr, kSignatureHash, + nullptr, pkey.get())); + size_t sig_len; + ASSERT_TRUE( + EVP_DigestSign(md_ctx.get(), nullptr, &sig_len, tbs, tbs_len)); + std::vector sig(sig_len); + ASSERT_TRUE( + EVP_DigestSign(md_ctx.get(), sig.data(), &sig_len, tbs, tbs_len)); + sig.resize(sig_len); + ASSERT_TRUE( + X509_REQ_set1_signature_value(csr.get(), sig.data(), sig.size())); + } else { + ASSERT_TRUE(X509_REQ_sign(csr.get(), pkey.get(), EVP_sha384())); + } + + // Check the signature. + EXPECT_TRUE(X509_REQ_verify(csr.get(), pkey.get())); - // Override the public key from the CSR unconditionally. Unlike certificates - // and CRLs, CSRs do not contain a signed copy of the signature algorithm, - // so we use a different field to confirm |i2d_re_X509_REQ_tbs| clears the - // cache as expected. - EXPECT_TRUE(X509_REQ_set_pubkey(csr.get(), pkey.get())); - - // Fill in the signature algorithm. - ASSERT_TRUE(X509_REQ_set1_signature_algo(csr.get(), algor.get())); - - // Extract the CertificationRequestInfo. - uint8_t *tbs = nullptr; - int tbs_len = i2d_re_X509_REQ_tbs(csr.get(), &tbs); - bssl::UniquePtr free_tbs(tbs); - ASSERT_GT(tbs_len, 0); - - // Generate a signature externally and fill it in. - bssl::ScopedEVP_MD_CTX md_ctx; - ASSERT_TRUE(EVP_DigestSignInit(md_ctx.get(), nullptr, kSignatureHash, - nullptr, pkey.get())); - size_t sig_len; - ASSERT_TRUE(EVP_DigestSign(md_ctx.get(), nullptr, &sig_len, tbs, tbs_len)); - std::vector sig(sig_len); - ASSERT_TRUE( - EVP_DigestSign(md_ctx.get(), sig.data(), &sig_len, tbs, tbs_len)); - sig.resize(sig_len); - ASSERT_TRUE( - X509_REQ_set1_signature_value(csr.get(), sig.data(), sig.size())); - - // Check the signature. - EXPECT_TRUE(X509_REQ_verify(csr.get(), pkey.get())); - - // Re-encode the CSR. X509_REQ objects contain a cached - // CertificationRequestInfo encoding and |i2d_re_X509_REQ_tbs| should have - // refreshed that cache. - bssl::UniquePtr copy = ReencodeCSR(csr.get()); - ASSERT_TRUE(copy); - EXPECT_TRUE(X509_REQ_verify(copy.get(), pkey.get())); - - // Check the signature was over the new public key. - bssl::UniquePtr copy_pubkey(X509_REQ_get_pubkey(copy.get())); - ASSERT_TRUE(copy_pubkey); - EXPECT_EQ(1, EVP_PKEY_cmp(pkey.get(), copy_pubkey.get())); + // Re-encode the CSR. X509_REQ objects contain a cached + // CertificationRequestInfo encoding and |i2d_re_X509_REQ_tbs| should have + // refreshed that cache. + bssl::UniquePtr copy = ReencodeCSR(csr.get()); + ASSERT_TRUE(copy); + EXPECT_TRUE(X509_REQ_verify(copy.get(), pkey.get())); + + // Check the signature was over the new public key. + bssl::UniquePtr copy_pubkey(X509_REQ_get_pubkey(copy.get())); + ASSERT_TRUE(copy_pubkey); + EXPECT_EQ(1, EVP_PKEY_cmp(pkey.get(), copy_pubkey.get())); + } } } diff --git a/crypto/x509/x_all.c b/crypto/x509/x_all.c index bad9ce622..551b21672 100644 --- a/crypto/x509/x_all.c +++ b/crypto/x509/x_all.c @@ -96,11 +96,13 @@ int X509_sign_ctx(X509 *x, EVP_MD_CTX *ctx) { } int X509_REQ_sign(X509_REQ *x, EVP_PKEY *pkey, const EVP_MD *md) { + x->req_info->enc.modified = 1; return (ASN1_item_sign(ASN1_ITEM_rptr(X509_REQ_INFO), x->sig_alg, NULL, x->signature, x->req_info, pkey, md)); } int X509_REQ_sign_ctx(X509_REQ *x, EVP_MD_CTX *ctx) { + x->req_info->enc.modified = 1; return ASN1_item_sign_ctx(ASN1_ITEM_rptr(X509_REQ_INFO), x->sig_alg, NULL, x->signature, x->req_info, ctx); }