Index: net/cert/cert_verify_proc_unittest.cc |
diff --git a/net/cert/cert_verify_proc_unittest.cc b/net/cert/cert_verify_proc_unittest.cc |
index 18596457988892f3a1d8276f2760e09fc49a1b7f..6396d33d77f8db37f612e861f4ecf1dccc95b435 100644 |
--- a/net/cert/cert_verify_proc_unittest.cc |
+++ b/net/cert/cert_verify_proc_unittest.cc |
@@ -24,8 +24,11 @@ |
#include "net/cert/cert_verify_result.h" |
#include "net/cert/crl_set.h" |
#include "net/cert/crl_set_storage.h" |
+#include "net/cert/internal/signature_algorithm.h" |
#include "net/cert/test_root_certs.h" |
#include "net/cert/x509_certificate.h" |
+#include "net/der/input.h" |
+#include "net/der/parser.h" |
#include "net/test/cert_test_util.h" |
#include "net/test/gtest_util.h" |
#include "net/test/test_certificate_data.h" |
@@ -272,18 +275,11 @@ TEST_P(CertVerifyProcInternalTest, DISABLED_EVVerification) { |
return; |
} |
- CertificateList certs = |
- CreateCertificateListFromFile(GetTestCertsDirectory(), "comodo.chain.pem", |
- X509Certificate::FORMAT_PEM_CERT_SEQUENCE); |
- ASSERT_EQ(3U, certs.size()); |
- |
- X509Certificate::OSCertHandles intermediates; |
- intermediates.push_back(certs[1]->os_cert_handle()); |
- intermediates.push_back(certs[2]->os_cert_handle()); |
- |
- scoped_refptr<X509Certificate> comodo_chain = |
- X509Certificate::CreateFromHandle(certs[0]->os_cert_handle(), |
- intermediates); |
+ auto comodo_chain = CreateCertificateChainFromFile( |
+ GetTestCertsDirectory(), "comodo.chain.pem", |
+ X509Certificate::FORMAT_PEM_CERT_SEQUENCE); |
+ ASSERT_TRUE(comodo_chain); |
+ ASSERT_EQ(2U, comodo_chain->GetIntermediateCertificates().size()); |
scoped_refptr<CRLSet> crl_set(CRLSet::ForTesting(false, NULL, "")); |
CertVerifyResult verify_result; |
@@ -383,13 +379,9 @@ TEST_P(CertVerifyProcInternalTest, RejectExpiredCert) { |
ScopedTestRoot test_root( |
ImportCertFromFile(certs_dir, "root_ca_cert.pem").get()); |
- CertificateList certs = CreateCertificateListFromFile( |
- certs_dir, "expired_cert.pem", X509Certificate::FORMAT_AUTO); |
- ASSERT_EQ(1U, certs.size()); |
- |
- X509Certificate::OSCertHandles intermediates; |
- scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle( |
- certs[0]->os_cert_handle(), intermediates); |
+ auto cert = CreateCertificateChainFromFile(certs_dir, "expired_cert.pem", |
+ X509Certificate::FORMAT_AUTO); |
mattm
2017/03/04 02:34:26
ASSERT_TRUE(cert)
eroman
2017/03/07 23:43:00
Sorry for including these unrelated refactors -- I
|
+ ASSERT_EQ(0U, cert->GetIntermediateCertificates().size()); |
int flags = 0; |
CertVerifyResult verify_result; |
@@ -612,14 +604,10 @@ TEST_P(CertVerifyProcInternalTest, NameConstraintsOk) { |
ASSERT_EQ(1U, ca_cert_list.size()); |
ScopedTestRoot test_root(ca_cert_list[0].get()); |
- CertificateList cert_list = CreateCertificateListFromFile( |
- GetTestCertsDirectory(), "name_constraint_good.pem", |
- X509Certificate::FORMAT_AUTO); |
- ASSERT_EQ(1U, cert_list.size()); |
- |
- X509Certificate::OSCertHandles intermediates; |
- scoped_refptr<X509Certificate> leaf = X509Certificate::CreateFromHandle( |
- cert_list[0]->os_cert_handle(), intermediates); |
+ auto leaf = CreateCertificateChainFromFile(GetTestCertsDirectory(), |
+ "name_constraint_good.pem", |
+ X509Certificate::FORMAT_AUTO); |
mattm
2017/03/04 02:34:26
ASSERT_TRUE(leaf)
eroman
2017/03/07 23:43:00
Ditto.
|
+ ASSERT_EQ(0U, leaf->GetIntermediateCertificates().size()); |
int flags = 0; |
CertVerifyResult verify_result; |
@@ -634,6 +622,299 @@ TEST_P(CertVerifyProcInternalTest, NameConstraintsOk) { |
EXPECT_EQ(0U, verify_result.cert_status); |
} |
+// This fixture is for testing the verification of a certificate chain which |
+// has some sort of mismatched signature algorithm (i.e. |
+// Certificate.signatureAlgorithm and TBSCertificate.algorithm are different). |
+// |
+// X509 certificates contain two redundant descriptors for the signature |
+// algorithm. Although RFC 5280 states that they must be the same, in practice |
+// system verifiers don't check this. This can lead to confusion as the |
+// signature itself may be checked using algorithm A, but then subsequent |
+// consumers may inspect the certificate and look at signature algorithm B when |
+// making policy choices. |
+class CertVerifyProcInspectSignatureAlgorithmsTest : public ::testing::Test { |
+ protected: |
+ // In the test setup, SHA384 is given special treatment as an unkown |
mattm
2017/03/04 02:34:26
unknown
eroman
2017/03/07 23:43:00
Done.
|
+ // algorithm. |
+ static constexpr DigestAlgorithm kUnknownDigestAlgorithm = |
+ DigestAlgorithm::Sha384; |
+ |
+ struct CertParams { |
+ DigestAlgorithm cert_algorithm; |
+ DigestAlgorithm tbs_algorithm; |
+ |
+ static CertParams Sha256() { |
+ return { |
+ DigestAlgorithm::Sha256, DigestAlgorithm::Sha256, |
+ }; |
+ } |
+ }; |
+ |
+ // Shorthand for VerifyChain() where only the leaf parameters need |
+ // to be specified. |
+ WARN_UNUSED_RESULT int VerifyLeaf(DigestAlgorithm cert_algorithm, |
+ DigestAlgorithm tbs_algorithm) { |
+ return VerifyChain({{cert_algorithm, tbs_algorithm}, |
+ // Shouldn't matter what root uses. |
+ {DigestAlgorithm::Sha1, DigestAlgorithm::Sha256}}); |
+ } |
+ |
+ // Manufactures a certificate chain where each certificate has the indicated |
+ // signature algorithms, and then returns the result of verifying this chain. |
+ WARN_UNUSED_RESULT int VerifyChain( |
+ const std::vector<CertParams>& chain_params) { |
+ auto chain = CreateChain(chain_params); |
+ if (!chain) { |
+ ADD_FAILURE() << "Failed creating certificate chain"; |
+ return ERR_UNEXPECTED; |
+ } |
+ |
+ int flags = 0; |
+ CertVerifyResult verify_result; |
+ verify_result.verified_cert = chain; |
mattm
2017/03/04 02:34:27
MockCertVerifyProc already sets verified_cert
eroman
2017/03/07 23:43:00
Done -- thanks!
|
+ |
+ scoped_refptr<CertVerifyProc> verify_proc = |
+ new MockCertVerifyProc(verify_result); |
+ |
+ return verify_proc->Verify(chain.get(), "test.example.com", std::string(), |
+ flags, NULL, CertificateList(), &verify_result); |
mattm
2017/03/04 02:34:26
Using the same CertVerifyResult object as was used
eroman
2017/03/07 23:43:00
Done.
|
+ } |
+ |
+ private: |
+ // Overwrites the AlgorithmIdentifier pointed to by |algorithm_sequence| with |
+ // |algorithm|. Note this violates the constness of StringPiece. |
+ WARN_UNUSED_RESULT static bool SetAlgorithmSequence( |
+ DigestAlgorithm algorithm, |
+ base::StringPiece* algorithm_sequence) { |
+ // This string of bytes is the full SEQUENCE for an AlgorithmIdentifier. |
+ std::vector<uint8_t> replacement_sequence; |
+ switch (algorithm) { |
+ case DigestAlgorithm::Sha1: |
+ // sha1WithRSAEncryption |
+ replacement_sequence = {0x30, 0x0D, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, |
+ 0xf7, 0x0d, 0x01, 0x01, 0x05, 0x05, 0x00}; |
+ break; |
+ case DigestAlgorithm::Sha256: |
+ // sha256WithRSAEncryption |
+ replacement_sequence = {0x30, 0x0D, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, |
+ 0xf7, 0x0d, 0x01, 0x01, 0x0b, 0x05, 0x00}; |
+ break; |
+ case kUnknownDigestAlgorithm: |
+ // This shouldn't be anything meaningful (modified numbers at random). |
+ replacement_sequence = {0x30, 0x0D, 0x06, 0x09, 0x8a, 0x87, 0x18, 0x46, |
+ 0xd7, 0x0d, 0x01, 0x01, 0x0b, 0x05, 0x00}; |
+ break; |
+ default: |
+ ADD_FAILURE() << "Unsupported digest algorithm"; |
+ return false; |
+ } |
+ |
+ // For this simple replacement to work (without modifying any |
+ // other sequence lengths) the original algorithm and replacement |
+ // algorithm must have the same encoded length. |
+ if (algorithm_sequence->size() != replacement_sequence.size()) { |
+ ADD_FAILURE() << "AlgorithmIdentifier must have length " |
+ << replacement_sequence.size(); |
+ return false; |
+ } |
+ |
+ memcpy(const_cast<char*>(algorithm_sequence->data()), |
+ replacement_sequence.data(), replacement_sequence.size()); |
+ return true; |
+ } |
+ |
+ // Locate the serial number bytes. |
+ WARN_UNUSED_RESULT static bool ExtractSerialNumberFromDERCert( |
+ base::StringPiece der_cert, |
+ base::StringPiece* serial_value) { |
+ der::Parser parser((der::Input(der_cert))); |
+ der::Parser certificate; |
+ if (!parser.ReadSequence(&certificate)) |
+ return false; |
+ |
+ der::Parser tbs_certificate; |
+ if (!certificate.ReadSequence(&tbs_certificate)) |
+ return false; |
+ |
+ bool unused; |
+ if (!tbs_certificate.SkipOptionalTag( |
+ der::kTagConstructed | der::kTagContextSpecific | 0, &unused)) { |
+ return false; |
+ } |
+ |
+ // serialNumber |
+ der::Input serial_value_der; |
+ if (!tbs_certificate.ReadTag(der::kInteger, &serial_value_der)) |
+ return false; |
+ |
+ *serial_value = serial_value_der.AsStringPiece(); |
+ return true; |
+ } |
+ |
+ // Creates a certificate (based on some base certificate file) using the |
+ // specified signature algorithms. |
+ static scoped_refptr<X509Certificate> CreateCertificate( |
+ const CertParams& params) { |
+ // Dosn't really matter which base certificate is used, so long as it is |
+ // valid and uses a signature AlgorithmIdentifier with the same encoded |
+ // length as sha1WithRSASignature. |
+ const char* kLeafFilename = "name_constraint_good.pem"; |
+ |
+ auto cert = CreateCertificateChainFromFile( |
+ GetTestCertsDirectory(), kLeafFilename, X509Certificate::FORMAT_AUTO); |
+ if (!cert) { |
+ ADD_FAILURE() << "Failed to load certificate: " << kLeafFilename; |
+ return nullptr; |
+ } |
+ |
+ // Start with the DER bytes of a valid certificate. This will be the basis |
+ // for building a modified certificate. |
+ std::string cert_der; |
+ if (!X509Certificate::GetDEREncoded(cert->os_cert_handle(), &cert_der)) { |
+ ADD_FAILURE() << "Failed getting DER bytes"; |
+ return nullptr; |
+ } |
+ |
+ // Parse the certificate and identify the locations of interest within |
+ // |cert_der|. |
+ base::StringPiece cert_algorithm_sequence; |
+ base::StringPiece tbs_algorithm_sequence; |
+ if (!asn1::ExtractSignatureAlgorithmsFromDERCert( |
+ cert_der, &cert_algorithm_sequence, &tbs_algorithm_sequence)) { |
+ ADD_FAILURE() << "Failed parsing certificate algorithms"; |
+ return nullptr; |
+ } |
+ |
+ base::StringPiece serial_value; |
+ if (!ExtractSerialNumberFromDERCert(cert_der, &serial_value)) { |
+ ADD_FAILURE() << "Failed parsing certificate serial number"; |
+ return nullptr; |
+ } |
+ |
+ // Give the certificate a new serial number -- this is necessary because |
+ // otherwise importing it may fail. Changing the high order byte is good |
+ // enough for these tests. |
+ *const_cast<char*>(serial_value.data()) = next_serial_number_++; |
mattm
2017/03/04 02:34:26
what happens if you run the test under gtest_repea
eroman
2017/03/07 23:43:00
I expect that once it has created around 255 certi
mattm
2017/03/08 01:41:34
Hmm. I would say it's worth avoiding that. I guess
eroman
2017/03/08 01:59:28
Here are two possible routes forward:
(1) I can m
eroman
2017/03/08 17:09:24
Done.
I went with static serial numbers, and may
|
+ |
+ // Change the signature AlgorithmIdentifiers. |
+ if (!SetAlgorithmSequence(params.cert_algorithm, |
+ &cert_algorithm_sequence) || |
+ !SetAlgorithmSequence(params.tbs_algorithm, &tbs_algorithm_sequence)) { |
+ return nullptr; |
+ } |
+ |
+ // NOTE: The signature is NOT recomputed over TBSCertificate -- for these |
+ // tests it isn't needed. |
+ return X509Certificate::CreateFromBytes(cert_der.data(), cert_der.size()); |
+ } |
+ |
+ static scoped_refptr<X509Certificate> CreateChain( |
+ const std::vector<CertParams>& params) { |
+ // Manufacture a chain with the given combinations of signature algorithms. |
+ // This chain isn't actually a valid chain, but it is good enough for |
+ // testing the base CertVerifyProc. |
+ CertificateList certs; |
+ for (const auto& cert_params : params) { |
+ certs.push_back(CreateCertificate(cert_params)); |
+ if (!certs.back()) |
+ return nullptr; |
+ } |
+ |
+ X509Certificate::OSCertHandles intermediates; |
+ for (size_t i = 1; i < certs.size(); ++i) |
+ intermediates.push_back(certs[i]->os_cert_handle()); |
+ |
+ return X509Certificate::CreateFromHandle(certs[0]->os_cert_handle(), |
+ intermediates); |
+ } |
+ |
+ static int next_serial_number_; |
+}; |
+ |
+// Global counter to ensure that during test runs the same issuer/serialnumber |
+// isn't used when manufacturing certificates. |
+int CertVerifyProcInspectSignatureAlgorithmsTest::next_serial_number_ = 9; |
+ |
+// This is a control test to make sure that the test helper |
+// VerifyLeaf() works as expected. There is no actual mismatch in the |
+// algorithms used here. |
+// |
+// Certificate.signatureAlgorithm: sha1WithRSASignature |
+// TBSCertificate.algorithm: sha1WithRSAEncryption |
+TEST_F(CertVerifyProcInspectSignatureAlgorithmsTest, LeafSha1Sha1) { |
+ int rv = VerifyLeaf(DigestAlgorithm::Sha1, DigestAlgorithm::Sha1); |
+ ASSERT_THAT(rv, IsError(ERR_CERT_WEAK_SIGNATURE_ALGORITHM)); |
+} |
+ |
+// This is a control test to make sure that the test helper |
+// VerifyLeaf() works as expected. There is no actual mismatch in the |
+// algorithms used here. |
+// |
+// Certificate.signatureAlgorithm: sha256WithRSASignature |
+// TBSCertificate.algorithm: sha256WithRSAEncryption |
+TEST_F(CertVerifyProcInspectSignatureAlgorithmsTest, LeafSha256Sha256) { |
+ int rv = VerifyLeaf(DigestAlgorithm::Sha256, DigestAlgorithm::Sha256); |
+ ASSERT_THAT(rv, IsOk()); |
+} |
+ |
+// Mismatched signature algorithms in the leaf certificate. |
+// |
+// Certificate.signatureAlgorithm: sha1WithRSASignature |
+// TBSCertificate.algorithm: sha256WithRSAEncryption |
+TEST_F(CertVerifyProcInspectSignatureAlgorithmsTest, LeafSha1Sha256) { |
+ int rv = VerifyLeaf(DigestAlgorithm::Sha1, DigestAlgorithm::Sha256); |
+ ASSERT_THAT(rv, IsError(ERR_CERT_INVALID)); |
+} |
+ |
+// Mismatched signature algorithms in the leaf certificate. |
+// |
+// Certificate.signatureAlgorithm: sha256WithRSAEncryption |
+// TBSCertificate.algorithm: sha1WithRSASignature |
+TEST_F(CertVerifyProcInspectSignatureAlgorithmsTest, LeafSha256Sha1) { |
+ int rv = VerifyLeaf(DigestAlgorithm::Sha256, DigestAlgorithm::Sha1); |
+ ASSERT_THAT(rv, IsError(ERR_CERT_INVALID)); |
+} |
+ |
+// Mismatched signature algorithms in the intermediate certificate. |
+// |
+// Certificate.signatureAlgorithm: sha1WithRSASignature |
+// TBSCertificate.algorithm: sha256WithRSAEncryption |
+TEST_F(CertVerifyProcInspectSignatureAlgorithmsTest, IntermediateSha1Sha256) { |
+ int rv = VerifyChain({CertParams::Sha256(), |
mattm
2017/03/04 02:34:26
imo this is more confusing than just putting {Dige
eroman
2017/03/07 23:43:00
Done.
|
+ {DigestAlgorithm::Sha1, DigestAlgorithm::Sha256}, |
+ CertParams::Sha256()}); |
+ ASSERT_THAT(rv, IsError(ERR_CERT_INVALID)); |
+} |
+ |
+// Mismatched signature algorithms in the intermediate certificate. |
+// |
+// Certificate.signatureAlgorithm: sha256WithRSAEncryption |
+// TBSCertificate.algorithm: sha1WithRSASignature |
+TEST_F(CertVerifyProcInspectSignatureAlgorithmsTest, IntermediateSha256Sha1) { |
+ int rv = VerifyChain({CertParams::Sha256(), |
+ {DigestAlgorithm::Sha256, DigestAlgorithm::Sha1}, |
+ CertParams::Sha256()}); |
+ ASSERT_THAT(rv, IsError(ERR_CERT_INVALID)); |
+} |
+ |
+// Unrecognized signature algorithm in the leaf certificate. |
+// |
+// Certificate.signatureAlgorithm: sha256WithRSAEncryption |
+// TBSCertificate.algorithm: ? |
+TEST_F(CertVerifyProcInspectSignatureAlgorithmsTest, LeafSha256Unknown) { |
+ int rv = VerifyLeaf(DigestAlgorithm::Sha256, kUnknownDigestAlgorithm); |
+ ASSERT_THAT(rv, IsError(ERR_CERT_INVALID)); |
+} |
+ |
+// Unrecognized signature algorithm in the leaf certificate. |
+// |
+// Certificate.signatureAlgorithm: ? |
+// TBSCertificate.algorithm: sha256WithRSAEncryption |
+TEST_F(CertVerifyProcInspectSignatureAlgorithmsTest, LeafUnknownSha256) { |
+ int rv = VerifyLeaf(kUnknownDigestAlgorithm, DigestAlgorithm::Sha256); |
+ ASSERT_THAT(rv, IsError(ERR_CERT_INVALID)); |
+} |
+ |
TEST_P(CertVerifyProcInternalTest, NameConstraintsFailure) { |
if (!SupportsReturningVerifiedChain()) { |
LOG(INFO) << "Skipping this test in this platform."; |