bug 1437214 - if PathBuildingStep::Check fails due to a problem with the subject certificate rather than the potential issuer, set keepGoing to false r=jcj

MozReview-Commit-ID: DEr4YgXfkOL

2 files changed, 174 insertions, 2 deletions

diff --git a/lib/mozpkix/lib/pkixbuild.cpp b/lib/mozpkix/lib/pkixbuild.cpp
index fd6a9d511..0b33e3d5a 100644
--- a/lib/mozpkix/lib/pkixbuild.cpp
+++ b/lib/mozpkix/lib/pkixbuild.cpp
@@ -241,7 +241,12 @@ PathBuildingStep::Check(Input potentialIssuerDER,
                                      validityDuration, stapledOCSPResponse,
                                      subject.GetAuthorityInfoAccess());
     if (rv != Success) {
-      return RecordResult(rv, keepGoing);
+      // Since this is actually a problem with the current subject certificate
+      // (rather than the issuer), it doesn't make sense to keep going; all
+      // paths through this certificate will fail.
+      Result savedRv = RecordResult(rv, keepGoing);
+      keepGoing = false;
+      return savedRv;
     }
 
     if (subject.endEntityOrCA == EndEntityOrCA::MustBeEndEntity) {
@@ -251,7 +256,11 @@ PathBuildingStep::Check(Input potentialIssuerDER,
         rv = ExtractSignedCertificateTimestampListFromExtension(*sctExtension,
                                                                 sctList);
         if (rv != Success) {
-          return RecordResult(rv, keepGoing);
+          // Again, the problem is with this certificate, and all paths through
+          // it will fail.
+          Result savedRv = RecordResult(rv, keepGoing);
+          keepGoing = false;
+          return savedRv;
         }
         trustDomain.NoteAuxiliaryExtension(AuxiliaryExtension::EmbeddedSCTList,
                                            sctList);
diff --git a/lib/mozpkix/test/gtest/pkixbuild_tests.cpp b/lib/mozpkix/test/gtest/pkixbuild_tests.cpp
index 64fa4c307..5948df703 100644
--- a/lib/mozpkix/test/gtest/pkixbuild_tests.cpp
+++ b/lib/mozpkix/test/gtest/pkixbuild_tests.cpp
@@ -582,3 +582,166 @@ TEST_F(pkixbuild, CertificateTransparencyExtension)
   ASSERT_EQ(BytesToByteString(dummySctList),
             extTrustDomain.signedCertificateTimestamps);
 }
+
+// This TrustDomain implements a hierarchy like so:
+//
+// A   B
+// |   |
+// C   D
+//  \ /
+//   E
+//
+// where A is a trust anchor, B is not a trust anchor and has no known issuer, C
+// and D are intermediates with the same subject and subject public key, and E
+// is an end-entity (in practice, the end-entity will be generated by the test
+// functions using this trust domain).
+class MultiplePathTrustDomain: public DefaultCryptoTrustDomain
+{
+public:
+  void SetUpCerts()
+  {
+    ASSERT_FALSE(ENCODING_FAILED(CreateCert("UntrustedRoot", "UntrustedRoot",
+                                            EndEntityOrCA::MustBeCA,
+                                            &subjectDERToCertDER)));
+    // The subject DER -> cert DER mapping would be overwritten for subject
+    // "Intermediate" when we create the second "Intermediate" certificate, so
+    // we keep a copy of this "Intermediate".
+    intermediateSignedByUntrustedRootCertDER =
+      CreateCert("UntrustedRoot", "Intermediate", EndEntityOrCA::MustBeCA);
+    ASSERT_FALSE(ENCODING_FAILED(intermediateSignedByUntrustedRootCertDER));
+    rootCACertDER = CreateCert("TrustedRoot", "TrustedRoot",
+                               EndEntityOrCA::MustBeCA, &subjectDERToCertDER);
+    ASSERT_FALSE(ENCODING_FAILED(rootCACertDER));
+    ASSERT_FALSE(ENCODING_FAILED(CreateCert("TrustedRoot", "Intermediate",
+                                            EndEntityOrCA::MustBeCA,
+                                            &subjectDERToCertDER)));
+  }
+
+private:
+  Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input candidateCert,
+                      /*out*/ TrustLevel& trustLevel) override
+  {
+    trustLevel = InputEqualsByteString(candidateCert, rootCACertDER)
+               ? TrustLevel::TrustAnchor
+               : TrustLevel::InheritsTrust;
+    return Success;
+  }
+
+  Result CheckCert(ByteString& certDER, IssuerChecker& checker, bool& keepGoing)
+  {
+    Input derCert;
+    Result rv = derCert.Init(certDER.data(), certDER.length());
+    if (rv != Success) {
+      return rv;
+    }
+    return checker.Check(derCert, nullptr/*additionalNameConstraints*/,
+                         keepGoing);
+  }
+
+  Result FindIssuer(Input encodedIssuerName, IssuerChecker& checker, Time)
+                    override
+  {
+    ByteString subjectDER(InputToByteString(encodedIssuerName));
+    ByteString certDER(subjectDERToCertDER[subjectDER]);
+    assert(!ENCODING_FAILED(certDER));
+    bool keepGoing;
+    Result rv = CheckCert(certDER, checker, keepGoing);
+    if (rv != Success) {
+      return rv;
+    }
+    // Also try the other intermediate.
+    if (keepGoing) {
+      rv = CheckCert(intermediateSignedByUntrustedRootCertDER, checker,
+                     keepGoing);
+      if (rv != Success) {
+        return rv;
+      }
+    }
+    return Success;
+  }
+
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+                         /*optional*/ const Input*,
+                         /*optional*/ const Input*) override
+  {
+    return Success;
+  }
+
+  Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
+  {
+    return Success;
+  }
+
+  std::map<ByteString, ByteString> subjectDERToCertDER;
+  ByteString rootCACertDER;
+  ByteString intermediateSignedByUntrustedRootCertDER;
+};
+
+TEST_F(pkixbuild, BadEmbeddedSCTWithMultiplePaths)
+{
+  MultiplePathTrustDomain trustDomain;
+  trustDomain.SetUpCerts();
+
+  // python security/pkix/tools/DottedOIDToCode.py --tlv
+  //   id-embeddedSctList 1.3.6.1.4.1.11129.2.4.2
+  static const uint8_t tlv_id_embeddedSctList[] = {
+    0x06, 0x0a, 0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79, 0x02, 0x04, 0x02
+  };
+  static const uint8_t dummySctList[] = {
+    0x01, 0x02, 0x03, 0x04, 0x05
+  };
+  ByteString ctExtension = TLV(der::SEQUENCE,
+    BytesToByteString(tlv_id_embeddedSctList) +
+    Boolean(false) +
+    // The contents of the OCTET STRING are supposed to consist of an OCTET
+    // STRING of useful data. We're testing what happens if it isn't, so shove
+    // some bogus (non-OCTET STRING) data in there.
+    TLV(der::OCTET_STRING, BytesToByteString(dummySctList)));
+  ByteString certDER(CreateCert("Intermediate", "Cert with bogus SCT list",
+                                EndEntityOrCA::MustBeEndEntity,
+                                nullptr, /*subjectDERToCertDER*/
+                                &ctExtension));
+  ASSERT_FALSE(ENCODING_FAILED(certDER));
+  Input certDERInput;
+  ASSERT_EQ(Success, certDERInput.Init(certDER.data(), certDER.length()));
+  ASSERT_EQ(Result::ERROR_BAD_DER,
+            BuildCertChain(trustDomain, certDERInput, Now(),
+                           EndEntityOrCA::MustBeEndEntity,
+                           KeyUsage::noParticularKeyUsageRequired,
+                           KeyPurposeId::id_kp_serverAuth,
+                           CertPolicyId::anyPolicy,
+                           nullptr/*stapledOCSPResponse*/));
+}
+
+// Same as a MultiplePathTrustDomain, but the end-entity is revoked.
+class RevokedEndEntityTrustDomain final : public MultiplePathTrustDomain
+{
+public:
+  Result CheckRevocation(EndEntityOrCA endEntityOrCA, const CertID&, Time,
+                         Duration, /*optional*/ const Input*,
+                         /*optional*/ const Input*) override
+  {
+    if (endEntityOrCA == EndEntityOrCA::MustBeEndEntity) {
+      return Result::ERROR_REVOKED_CERTIFICATE;
+    }
+    return Success;
+  }
+};
+
+TEST_F(pkixbuild, RevokedEndEntityWithMultiplePaths)
+{
+  RevokedEndEntityTrustDomain trustDomain;
+  trustDomain.SetUpCerts();
+  ByteString certDER(CreateCert("Intermediate", "RevokedEndEntity",
+                                EndEntityOrCA::MustBeEndEntity));
+  ASSERT_FALSE(ENCODING_FAILED(certDER));
+  Input certDERInput;
+  ASSERT_EQ(Success, certDERInput.Init(certDER.data(), certDER.length()));
+  ASSERT_EQ(Result::ERROR_REVOKED_CERTIFICATE,
+            BuildCertChain(trustDomain, certDERInput, Now(),
+                           EndEntityOrCA::MustBeEndEntity,
+                           KeyUsage::noParticularKeyUsageRequired,
+                           KeyPurposeId::id_kp_serverAuth,
+                           CertPolicyId::anyPolicy,
+                           nullptr/*stapledOCSPResponse*/));
+}