net::PathBuilder: if the leaf cert is trusted, treat as unspecified trust.

This allows path building to continue and try to build a valid path
(either to a different root, or to the same cert if it happens to be
self-signed.)

Update comments & todos that trusted leaf certs are intentionally not
supported.

Bug: 814994
Change-Id: Id8e6a5f3d00c94c96271e4c6e21860206bb71c2a
Reviewed-on: https://chromium-review.googlesource.com/933108Reviewed-by: Doug Steedman <dougsteed@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Commit-Queue: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538880}

components/cast_certificate/cast_cert_validator_unittest.cc

@@ -256,9 +256,9 @@ TEST(VerifyCastDeviceCertTest, Unchained) {
 // trust anchors after all) it fails the test as it is not a *device
 // certificate*.
 TEST(VerifyCastDeviceCertTest, CastRootCa) {
-  RunTest(CastCertError::ERR_CERTS_VERIFY_GENERIC, "",
-          CastDeviceCertPolicy::NONE, "certificates/cast_root_ca.pem",
-          AprilFirst2016(), TRUST_STORE_BUILTIN, "");
+  RunTest(CastCertError::ERR_CERTS_RESTRICTIONS, "", CastDeviceCertPolicy::NONE,
+          "certificates/cast_root_ca.pem", AprilFirst2016(),
+          TRUST_STORE_BUILTIN, "");
 }
 
 // Tests verifying a valid certificate chain of length 2:

net/cert/cert_verify_proc_unittest.cc

@@ -369,10 +369,7 @@ TEST_P(CertVerifyProcInternalTest, TrustedTargetCertWithEVPolicy) {
   int flags = CertVerifier::VERIFY_EV_CERT;
   int error = Verify(cert.get(), "policy_test.example", flags,
                      nullptr /*crl_set*/, CertificateList(), &verify_result);
-  // TODO(eroman): builtin verifier cannot verify a chain of length 1.
-  if (verify_proc_type() == CERT_VERIFY_PROC_BUILTIN) {
-    EXPECT_THAT(error, IsError(ERR_CERT_INVALID));
-  } else if (ScopedTestRootCanTrustTargetCert(verify_proc_type())) {
+  if (ScopedTestRootCanTrustTargetCert(verify_proc_type())) {
     EXPECT_THAT(error, IsOk());
     ASSERT_TRUE(verify_result.verified_cert);
     EXPECT_TRUE(verify_result.verified_cert->intermediate_buffers().empty());
@@ -409,10 +406,7 @@ TEST_P(CertVerifyProcInternalTest,
   int flags = CertVerifier::VERIFY_EV_CERT;
   int error = Verify(cert.get(), "policy_test.example", flags,
                      nullptr /*crl_set*/, CertificateList(), &verify_result);
-  // TODO(eroman): builtin verifier cannot verify a chain of length 1.
-  if (verify_proc_type() == CERT_VERIFY_PROC_BUILTIN) {
-    EXPECT_THAT(error, IsError(ERR_CERT_INVALID));
-  } else if (ScopedTestRootCanTrustTargetCert(verify_proc_type())) {
+  if (ScopedTestRootCanTrustTargetCert(verify_proc_type())) {
     EXPECT_THAT(error, IsOk());
     ASSERT_TRUE(verify_result.verified_cert);
     EXPECT_TRUE(verify_result.verified_cert->intermediate_buffers().empty());

net/cert/internal/common_cert_errors.cc

@@ -17,8 +17,7 @@ DEFINE_CERT_ERROR_ID(
     "Certificate.signatureAlgorithm != TBSCertificate.signature");
 
 DEFINE_CERT_ERROR_ID(kChainIsEmpty, "Chain is empty");
-DEFINE_CERT_ERROR_ID(kChainIsLength1,
-                     "TODO: Cannot verify a chain of length 1");
+DEFINE_CERT_ERROR_ID(kChainIsLength1, "Cannot verify a chain of length 1");
 DEFINE_CERT_ERROR_ID(kUnconsumedCriticalExtension,
                      "Unconsumed critical extension");
 DEFINE_CERT_ERROR_ID(

net/cert/internal/common_cert_errors.h

@@ -34,9 +34,8 @@ NET_EXPORT extern const CertErrorId kSignatureAlgorithmMismatch;
 NET_EXPORT extern const CertErrorId kChainIsEmpty;
 
 // Certificate verification was called with a chain of length 1, which is not
-// currently supported (i.e. the target certificate cannot also be a trusted
-// certificate).
-// TODO(eroman): Remove this.
+// supported (i.e. the target certificate cannot also be a trusted
+// certificate). See https://crbug.com/814994.
 NET_EXPORT extern const CertErrorId kChainIsLength1;
 
 // The certificate contains an unknown extension which is marked as critical.

net/cert/internal/path_builder.cc

@@ -409,6 +409,23 @@ bool CertPathIter::GetNextPath(ParsedCertificateList* out_certs,
       }
     }
 
+    // If the cert is trusted but is the leaf, treat it as having unspecified
+    // trust. This may allow a successful path to be built to a different root
+    // (or to the same cert if it's self-signed).
+    switch (next_issuer_.trust.type) {
+      case CertificateTrustType::TRUSTED_ANCHOR:
+      case CertificateTrustType::TRUSTED_ANCHOR_WITH_CONSTRAINTS:
+        if (cur_path_.Empty()) {
+          DVLOG(1) << "Leaf is a trust anchor, considering as UNSPECIFIED";
+          next_issuer_.trust = CertificateTrust::ForUnspecified();
+        }
+        break;
+      case CertificateTrustType::DISTRUSTED:
+      case CertificateTrustType::UNSPECIFIED:
+        // No override necessary.
+        break;
+    }
+
     switch (next_issuer_.trust.type) {
       // If the trust for this issuer is "known" (either becuase it is
       // distrusted, or because it is trusted) then stop building and return the

net/cert/internal/path_builder_unittest.cc

@@ -166,16 +166,11 @@ TEST_F(PathBuilderMultiRootTest, TargetHasNameAndSpkiOfTrustAnchor) {
 
   path_builder.Run();
 
-  ASSERT_FALSE(result.HasValidPath());
-
-  // TODO(eroman): This probably should have succeeded and found the path below.
-  // It fails right now because path building stops on trust anchors (and the
-  // end entity is added as a trust anchor).
-  //
-  // const auto& path = result.GetBestValidPath()->path;
-  // ASSERT_EQ(2U, path.certs.size());
-  // EXPECT_EQ(a_by_b_, path.certs[0]);
-  // EXPECT_EQ(b_by_f_, path.certs[1]);
+  ASSERT_TRUE(result.HasValidPath());
+  const auto& path = *result.GetBestValidPath();
+  ASSERT_EQ(2U, path.certs.size());
+  EXPECT_EQ(a_by_b_, path.certs[0]);
+  EXPECT_EQ(b_by_f_, path.certs[1]);
 }
 
 // If the target cert is has the same name and key as a trust anchor, however
@@ -251,16 +246,14 @@ TEST_F(PathBuilderMultiRootTest, TargetIsSelfSignedTrustAnchor) {
 
   path_builder.Run();
 
-  ASSERT_FALSE(result.HasValidPath());
+  ASSERT_TRUE(result.HasValidPath());
 
-  // TODO(eroman): This test currently fails because path building stops
-  // searching once it identifies a certificate as a trust anchor. In this case
-  // the target is a trust anchor, however could be verified using the
-  // self-signedness (or even the cert itself).
-  // const auto& path = result.GetBestValidPath()->path;
-  // ASSERT_EQ(2U, path.certs.size());
-  // EXPECT_EQ(e_by_e_, path.certs[0]);
-  // EXPECT_EQ(e_by_e_, path.certs[1]);
+  // Verifying a trusted leaf certificate is not permitted, however this
+  // certificate is self-signed, and can chain to itself.
+  const auto& path = *result.GetBestValidPath();
+  ASSERT_EQ(2U, path.certs.size());
+  EXPECT_EQ(e_by_e_, path.certs[0]);
+  EXPECT_EQ(e_by_e_, path.certs[1]);
 }
 
 // If the target cert is directly issued by a trust anchor, it should verify

net/cert/internal/trust_store_nss.cc

@@ -106,8 +106,8 @@ void TrustStoreNSS::GetTrust(const scoped_refptr<ParsedCertificate>& cert,
     return;
   }
 
-  // TODO(mattm): handle trusted server certs (CERTDB_TERMINAL_RECORD +
-  // CERTDB_TRUSTED)
+  // Trusted server certs (CERTDB_TERMINAL_RECORD + CERTDB_TRUSTED) are
+  // intentionally treated as unspecified. See https://crbug.com/814994.
 
   *out_trust = CertificateTrust::ForUnspecified();
   return;

net/cert/internal/trust_store_nss_unittest.cc

@@ -268,8 +268,9 @@ TEST_F(TrustStoreNSSTest, TrustedServer) {
   AddCertsToNSS();
   TrustServerCert(target_.get());
 
-  // TODO(mattm): server-trusted certificates are handled as UNSPECIFIED since
-  // we don't currently support the notion of explictly trusted server certs.
+  // Server-trusted certificates are handled as UNSPECIFIED since we don't
+  // support the notion of explictly trusted server certs. See
+  // https://crbug.com/814994.
   EXPECT_TRUE(HasTrust({oldroot_, newroot_, target_, oldintermediate_,
                         newintermediate_, newrootrollover_},
                        CertificateTrustType::UNSPECIFIED));

net/cert/internal/verify_certificate_chain.cc

@@ -1165,8 +1165,8 @@ void PathVerifier::Run(
     return;
   }
 
-  // TODO(eroman): Verifying a trusted leaf certificate is not currently
-  // permitted.
+  // Verifying a trusted leaf certificate is not permitted. (It isn't a
+  // well-specified operation.) See https://crbug.com/814994.
   if (certs.size() == 1) {
     errors->GetOtherErrors()->AddError(cert_errors::kChainIsLength1);
     return;