Bug 1020683, Part 1: Remove internal uses of CERTCertificate from mozilla::pkix::VerifyEncodedOCSPResponse, r=keeler
authorBrian Smith <brian@briansmith.org>
Thu, 05 Jun 2014 15:18:32 -0700
changeset 199770 fa797212429e813a44f198c59763a44027a1cadc
parent 199769 7673b3a0749997dddcbe768371fc99a3f174491b
child 199771 dc9d168ba8fb8366f875a2556bcacd06bacf5e75
push idunknown
push userunknown
push dateunknown
reviewerskeeler
bugs1020683
milestone32.0a1
Bug 1020683, Part 1: Remove internal uses of CERTCertificate from mozilla::pkix::VerifyEncodedOCSPResponse, r=keeler
security/apps/AppTrustDomain.cpp
security/apps/AppTrustDomain.h
security/certverifier/NSSCertDBTrustDomain.cpp
security/certverifier/NSSCertDBTrustDomain.h
security/pkix/include/pkix/pkix.h
security/pkix/include/pkix/pkixtypes.h
security/pkix/lib/pkixbuild.cpp
security/pkix/lib/pkixcheck.cpp
security/pkix/lib/pkixkey.cpp
security/pkix/lib/pkixocsp.cpp
security/pkix/lib/pkixutil.h
security/pkix/test/gtest/pkix_cert_chain_length_tests.cpp
security/pkix/test/lib/pkixtestutil.h
--- a/security/apps/AppTrustDomain.cpp
+++ b/security/apps/AppTrustDomain.cpp
@@ -158,19 +158,20 @@ AppTrustDomain::GetCertTrust(EndEntityOr
   }
 
   *trustLevel = TrustLevel::InheritsTrust;
   return SECSuccess;
 }
 
 SECStatus
 AppTrustDomain::VerifySignedData(const CERTSignedData* signedData,
-                                  const CERTCertificate* cert)
+                                 const SECItem& subjectPublicKeyInfo)
 {
-  return ::mozilla::pkix::VerifySignedData(signedData, cert, mPinArg);
+  return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
+                                           mPinArg);
 }
 
 SECStatus
 AppTrustDomain::CheckRevocation(EndEntityOrCA,
                                 const CERTCertificate*,
                                 /*const*/ CERTCertificate*,
                                 PRTime time,
                                 /*optional*/ const SECItem*)
--- a/security/apps/AppTrustDomain.h
+++ b/security/apps/AppTrustDomain.h
@@ -24,17 +24,17 @@ public:
                          const mozilla::pkix::CertPolicyId& policy,
                          const SECItem& candidateCertDER,
                  /*out*/ mozilla::pkix::TrustLevel* trustLevel) MOZ_OVERRIDE;
   SECStatus FindPotentialIssuers(const SECItem* encodedIssuerName,
                                  PRTime time,
                          /*out*/ mozilla::pkix::ScopedCERTCertList& results)
                                  MOZ_OVERRIDE;
   SECStatus VerifySignedData(const CERTSignedData* signedData,
-                             const CERTCertificate* cert) MOZ_OVERRIDE;
+                             const SECItem& subjectPublicKeyInfo) MOZ_OVERRIDE;
   SECStatus CheckRevocation(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                             const CERTCertificate* cert,
                             /*const*/ CERTCertificate* issuerCertToDup,
                             PRTime time,
                             /*optional*/ const SECItem* stapledOCSPresponse);
   SECStatus IsChainValid(const CERTCertList* certChain) { return SECSuccess; }
 
 private:
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -144,19 +144,20 @@ NSSCertDBTrustDomain::GetCertTrust(EndEn
   }
 
   *trustLevel = TrustLevel::InheritsTrust;
   return SECSuccess;
 }
 
 SECStatus
 NSSCertDBTrustDomain::VerifySignedData(const CERTSignedData* signedData,
-                                       const CERTCertificate* cert)
+                                       const SECItem& subjectPublicKeyInfo)
 {
-  return ::mozilla::pkix::VerifySignedData(signedData, cert, mPinArg);
+  return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
+                                           mPinArg);
 }
 
 static PRIntervalTime
 OCSPFetchingTypeToTimeoutTime(NSSCertDBTrustDomain::OCSPFetching ocspFetching)
 {
   switch (ocspFetching) {
     case NSSCertDBTrustDomain::FetchOCSPForDVSoftFail:
       return PR_SecondsToInterval(2);
--- a/security/certverifier/NSSCertDBTrustDomain.h
+++ b/security/certverifier/NSSCertDBTrustDomain.h
@@ -67,17 +67,17 @@ public:
                 /*out*/ mozilla::pkix::ScopedCERTCertList& results);
 
   virtual SECStatus GetCertTrust(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                                  const mozilla::pkix::CertPolicyId& policy,
                                  const SECItem& candidateCertDER,
                          /*out*/ mozilla::pkix::TrustLevel* trustLevel);
 
   virtual SECStatus VerifySignedData(const CERTSignedData* signedData,
-                                     const CERTCertificate* cert);
+                                     const SECItem& subjectPublicKeyInfo);
 
   virtual SECStatus CheckRevocation(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                                     const CERTCertificate* cert,
                           /*const*/ CERTCertificate* issuerCert,
                                     PRTime time,
                        /*optional*/ const SECItem* stapledOCSPResponse);
 
   virtual SECStatus IsChainValid(const CERTCertList* certChain);
--- a/security/pkix/include/pkix/pkix.h
+++ b/security/pkix/include/pkix/pkix.h
@@ -85,29 +85,28 @@ namespace mozilla { namespace pkix {
 //
 // SEC_ERROR_UNTRUSTED_CERT means that the end-entity certificate was actively
 //                          distrusted.
 // SEC_ERROR_UNTRUSTED_ISSUER means that path building failed because of active
 //                            distrust.
 // TODO(bug 968451): Document more of these.
 
 SECStatus BuildCertChain(TrustDomain& trustDomain,
-                         CERTCertificate* cert,
+                         const CERTCertificate* cert,
                          PRTime time,
                          EndEntityOrCA endEntityOrCA,
             /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
                          KeyPurposeId requiredEKUIfPresent,
                          const CertPolicyId& requiredPolicy,
             /*optional*/ const SECItem* stapledOCSPResponse,
                  /*out*/ ScopedCERTCertList& results);
 
-// Verify the given signed data using the public key of the given certificate.
-// (EC)DSA parameter inheritance is not supported.
+// Verify the given signed data using the given public key.
 SECStatus VerifySignedData(const CERTSignedData* sd,
-                           const CERTCertificate* cert,
+                           const SECItem& subjectPublicKeyInfo,
                            void* pkcs11PinArg);
 
 // The return value, if non-null, is owned by the arena and MUST NOT be freed.
 SECItem* CreateEncodedOCSPRequest(PLArenaPool* arena,
                                   const CERTCertificate* cert,
                                   const CERTCertificate* issuerCert);
 
 // The optional parameter thisUpdate will be the thisUpdate value of
--- a/security/pkix/include/pkix/pkixtypes.h
+++ b/security/pkix/include/pkix/pkixtypes.h
@@ -41,18 +41,16 @@ ArenaFalseCleaner(PLArenaPool *arena) {
   return PORT_FreeArena(arena, PR_FALSE);
 }
 
 typedef ScopedPtr<PLArenaPool, ArenaFalseCleaner> ScopedPLArenaPool;
 
 typedef ScopedPtr<CERTCertificate, CERT_DestroyCertificate>
         ScopedCERTCertificate;
 typedef ScopedPtr<CERTCertList, CERT_DestroyCertList> ScopedCERTCertList;
-typedef ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey>
-        ScopedSECKEYPublicKey;
 
 MOZILLA_PKIX_ENUM_CLASS EndEntityOrCA { MustBeEndEntity = 0, MustBeCA = 1 };
 
 typedef unsigned int KeyUsages;
 
 MOZILLA_PKIX_ENUM_CLASS KeyPurposeId {
   anyExtendedKeyUsage = 0,
   id_kp_serverAuth = 1,           // id-kp-serverAuth
@@ -112,25 +110,22 @@ public:
   // will be null on input. If no potential issuers are found, then this
   // function should return SECSuccess with results being either null or
   // an empty list. Otherwise, this function should construct a
   // CERTCertList and return it in |results|, transfering ownership.
   virtual SECStatus FindPotentialIssuers(const SECItem* encodedIssuerName,
                                          PRTime time,
                                  /*out*/ ScopedCERTCertList& results) = 0;
 
-  // Verify the given signature using the public key of the given certificate.
-  // The implementation should be careful to ensure that the given certificate
-  // has all the public key information needed--i.e. it should ensure that the
-  // certificate is not trying to use EC(DSA) parameter inheritance.
+  // Verify the given signature using the given public key.
   //
   // Most implementations of this function should probably forward the call
   // directly to mozilla::pkix::VerifySignedData.
   virtual SECStatus VerifySignedData(const CERTSignedData* signedData,
-                                     const CERTCertificate* cert) = 0;
+                                     const SECItem& subjectPublicKeyInfo) = 0;
 
   // issuerCertToDup is only non-const so CERT_DupCertificate can be called on
   // it.
   virtual SECStatus CheckRevocation(EndEntityOrCA endEntityOrCA,
                                     const CERTCertificate* cert,
                           /*const*/ CERTCertificate* issuerCertToDup,
                                     PRTime time,
                        /*optional*/ const SECItem* stapledOCSPresponse) = 0;
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -33,18 +33,30 @@ namespace mozilla { namespace pkix {
 
 // We assume ext has been zero-initialized by its constructor and otherwise
 // not modified.
 //
 // TODO(perf): This sorting of extensions should be be moved into the
 // certificate decoder so that the results are cached with the certificate, so
 // that the decoding doesn't have to happen more than once per cert.
 Result
-BackCert::Init()
+BackCert::Init(const SECItem& certDER)
 {
+  // XXX: Currently-known uses of mozilla::pkix create CERTCertificate objects
+  // for all certs anyway, so the overhead of CERT_NewTempCertificate will be
+  // reduced to a lookup in NSS's SECItem* -> CERTCertificate cache and
+  // a CERT_DupCertificate. Eventually, we should parse the certificate using
+  // mozilla::pkix::der and avoid the need to create a CERTCertificate at all.
+  nssCert = CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
+                                    const_cast<SECItem*>(&certDER),
+                                    nullptr, false, true);
+  if (!nssCert) {
+    return MapSECStatus(SECFailure);
+  }
+
   const CERTCertExtension* const* exts = nssCert->extensions;
   if (!exts) {
     return Success;
   }
   // We only decode v3 extensions for v3 certificates for two reasons.
   // 1. They make no sense in non-v3 certs
   // 2. An invalid cert can embed a basic constraints extension and the
   //    check basic constrains will asume that this is valid. Making it
@@ -104,16 +116,25 @@ BackCert::Init()
       }
       *out = &ext->value;
     }
   }
 
   return Success;
 }
 
+
+Result
+BackCert::VerifyOwnSignatureWithKey(TrustDomain& trustDomain,
+                                    const SECItem& subjectPublicKeyInfo) const
+{
+  return MapSECStatus(trustDomain.VerifySignedData(&nssCert->signatureWrap,
+                                                   subjectPublicKeyInfo));
+}
+
 static Result BuildForward(TrustDomain& trustDomain,
                            BackCert& subject,
                            PRTime time,
                            EndEntityOrCA endEntityOrCA,
                            KeyUsages requiredKeyUsagesIfPresent,
                            KeyPurposeId requiredEKUIfPresent,
                            const CertPolicyId& requiredPolicy,
                            /*optional*/ const SECItem* stapledOCSPResponse,
@@ -122,25 +143,22 @@ static Result BuildForward(TrustDomain& 
 
 // The code that executes in the inner loop of BuildForward
 static Result
 BuildForwardInner(TrustDomain& trustDomain,
                   BackCert& subject,
                   PRTime time,
                   KeyPurposeId requiredEKUIfPresent,
                   const CertPolicyId& requiredPolicy,
-                  CERTCertificate* potentialIssuerCertToDup,
+                  const SECItem& potentialIssuerDER,
                   unsigned int subCACount,
                   ScopedCERTCertList& results)
 {
-  PORT_Assert(potentialIssuerCertToDup);
-
-  BackCert potentialIssuer(potentialIssuerCertToDup, &subject,
-                           BackCert::IncludeCN::No);
-  Result rv = potentialIssuer.Init();
+  BackCert potentialIssuer(&subject, BackCert::IncludeCN::No);
+  Result rv = potentialIssuer.Init(potentialIssuerDER);
   if (rv != Success) {
     return rv;
   }
 
   // RFC5280 4.2.1.1. Authority Key Identifier
   // RFC5280 4.2.1.2. Subject Key Identifier
 
   // Loop prevention, done as recommended by RFC4158 Section 5.2
@@ -164,22 +182,18 @@ BuildForwardInner(TrustDomain& trustDoma
 
   rv = BuildForward(trustDomain, potentialIssuer, time, EndEntityOrCA::MustBeCA,
                     KU_KEY_CERT_SIGN, requiredEKUIfPresent, requiredPolicy,
                     nullptr, subCACount, results);
   if (rv != Success) {
     return rv;
   }
 
-  if (trustDomain.VerifySignedData(&subject.GetNSSCert()->signatureWrap,
-                                   potentialIssuer.GetNSSCert()) != SECSuccess) {
-    return MapSECStatus(SECFailure);
-  }
-
-  return Success;
+  return subject.VerifyOwnSignatureWithKey(
+                   trustDomain, potentialIssuer.GetSubjectPublicKeyInfo());
 }
 
 // Recursively build the path from the given subject certificate to the root.
 //
 // Be very careful about changing the order of checks. The order is significant
 // because it affects which error we return when a certificate or certificate
 // chain has multiple problems. See the error ranking documentation in
 // pkix/pkix.h.
@@ -274,17 +288,18 @@ BuildForward(TrustDomain& trustDomain,
     return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
   }
 
   PRErrorCode errorToReturn = 0;
 
   for (CERTCertListNode* n = CERT_LIST_HEAD(candidates);
        !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) {
     rv = BuildForwardInner(trustDomain, subject, time, requiredEKUIfPresent,
-                           requiredPolicy, n->cert, subCACount, results);
+                           requiredPolicy, n->cert->derCert, subCACount,
+                           results);
     if (rv == Success) {
       // If we found a valid chain but deferred reporting an error with the
       // end-entity certificate, report it now.
       if (deferredEndEntityError != 0) {
         return Fail(FatalError, deferredEndEntityError);
       }
 
       SECStatus srv = trustDomain.CheckRevocation(endEntityOrCA,
@@ -324,45 +339,41 @@ BuildForward(TrustDomain& trustDomain,
     errorToReturn = SEC_ERROR_UNKNOWN_ISSUER;
   }
 
   return Fail(RecoverableError, errorToReturn);
 }
 
 SECStatus
 BuildCertChain(TrustDomain& trustDomain,
-               CERTCertificate* certToDup,
+               const CERTCertificate* nssCert,
                PRTime time,
                EndEntityOrCA endEntityOrCA,
                /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
                /*optional*/ KeyPurposeId requiredEKUIfPresent,
                const CertPolicyId& requiredPolicy,
                /*optional*/ const SECItem* stapledOCSPResponse,
                /*out*/ ScopedCERTCertList& results)
 {
-  PORT_Assert(certToDup);
-
-  if (!certToDup) {
+  if (!nssCert) {
+    PR_NOT_REACHED("null cert passed to BuildCertChain");
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return SECFailure;
   }
 
-  // The only non-const operation on the cert we are allowed to do is
-  // CERT_DupCertificate.
-
   // XXX: Support the legacy use of the subject CN field for indicating the
   // domain name the certificate is valid for.
   BackCert::IncludeCN includeCN
     = endEntityOrCA == EndEntityOrCA::MustBeEndEntity &&
       requiredEKUIfPresent == KeyPurposeId::id_kp_serverAuth
     ? BackCert::IncludeCN::Yes
     : BackCert::IncludeCN::No;
 
-  BackCert cert(certToDup, nullptr, includeCN);
-  Result rv = cert.Init();
+  BackCert cert(nullptr, includeCN);
+  Result rv = cert.Init(nssCert->derCert);
   if (rv != Success) {
     return SECFailure;
   }
 
   rv = BuildForward(trustDomain, cert, time, endEntityOrCA,
                     requiredKeyUsagesIfPresent, requiredEKUIfPresent,
                     requiredPolicy, stapledOCSPResponse, 0, results);
   if (rv != Success) {
@@ -382,17 +393,17 @@ BackCert::GetArena()
   return arena.get();
 }
 
 Result
 BackCert::PrependNSSCertToList(CERTCertList* results)
 {
   PORT_Assert(results);
 
-  CERTCertificate* dup = CERT_DupCertificate(nssCert);
+  CERTCertificate* dup = CERT_DupCertificate(nssCert.get());
   if (CERT_AddCertToListHead(results, dup) != SECSuccess) { // takes ownership
     CERT_DestroyCertificate(dup);
     return FatalError;
   }
 
   return Success;
 }
 
--- a/security/pkix/lib/pkixcheck.cpp
+++ b/security/pkix/lib/pkixcheck.cpp
@@ -328,17 +328,17 @@ Result
 BackCert::GetConstrainedNames(/*out*/ const CERTGeneralName** result)
 {
   if (!constrainedNames) {
     if (!GetArena()) {
       return FatalError;
     }
 
     constrainedNames =
-      CERT_GetConstrainedCertificateNames(nssCert, arena.get(),
+      CERT_GetConstrainedCertificateNames(GetNSSCert(), arena.get(),
                                           includeCN == IncludeCN::Yes);
     if (!constrainedNames) {
       return MapSECStatus(SECFailure);
     }
   }
 
   *result = constrainedNames;
   return Success;
--- a/security/pkix/lib/pkixkey.cpp
+++ b/security/pkix/lib/pkixkey.cpp
@@ -30,42 +30,43 @@
 #include "cert.h"
 #include "cryptohi.h"
 #include "prerror.h"
 #include "secerr.h"
 
 namespace mozilla { namespace pkix {
 
 SECStatus
-VerifySignedData(const CERTSignedData* sd, const CERTCertificate* cert,
+VerifySignedData(const CERTSignedData* sd, const SECItem& subjectPublicKeyInfo,
                  void* pkcs11PinArg)
 {
   if (!sd || !sd->data.data || !sd->signatureAlgorithm.algorithm.data ||
-      !sd->signature.data || !cert) {
+      !sd->signature.data) {
     PR_NOT_REACHED("invalid args to VerifySignedData");
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return SECFailure;
   }
 
   // See bug 921585.
   if (sd->data.len > static_cast<unsigned int>(std::numeric_limits<int>::max())) {
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return SECFailure;
   }
 
   // convert sig->len from bit counts to byte count.
   SECItem sig = sd->signature;
   DER_ConvertBitString(&sig);
 
-  // Use SECKEY_ExtractPublicKey instead of CERT_ExtractPublicKey because
-  // CERT_ExtractPublicKey would try to do (EC)DSA parameter inheritance, using
-  // the classic (wrong) NSS path building logic. We intentionally do not
-  // support parameter inheritance.
-  ScopedSECKEYPublicKey
-    pubKey(SECKEY_ExtractPublicKey(&cert->subjectPublicKeyInfo));
+  ScopedPtr<CERTSubjectPublicKeyInfo, SECKEY_DestroySubjectPublicKeyInfo>
+    spki(SECKEY_DecodeDERSubjectPublicKeyInfo(&subjectPublicKeyInfo));
+  if (!spki) {
+    return SECFailure;
+  }
+  ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey>
+    pubKey(SECKEY_ExtractPublicKey(spki.get()));
   if (!pubKey) {
     return SECFailure;
   }
 
   SECOidTag hashAlg;
   if (VFY_VerifyDataWithAlgorithmID(sd->data.data, static_cast<int>(sd->data.len),
                                     pubKey.get(), &sig, &sd->signatureAlgorithm,
                                     &hashAlg, pkcs11PinArg) != SECSuccess) {
--- a/security/pkix/lib/pkixocsp.cpp
+++ b/security/pkix/lib/pkixocsp.cpp
@@ -48,42 +48,45 @@ MOZILLA_PKIX_ENUM_CLASS CertStatus : uin
   Revoked = der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
   Unknown = der::CONTEXT_SPECIFIC | 2
 };
 
 class Context
 {
 public:
   Context(TrustDomain& trustDomain,
-          const CERTCertificate& cert,
-          CERTCertificate& issuerCert,
+          const SECItem& certSerialNumber,
+          const SECItem& issuerSubject,
+          const SECItem& issuerSubjectPublicKeyInfo,
           PRTime time,
           uint16_t maxLifetimeInDays,
           PRTime* thisUpdate,
           PRTime* validThrough)
     : trustDomain(trustDomain)
-    , cert(cert)
-    , issuerCert(issuerCert)
+    , certSerialNumber(certSerialNumber)
+    , issuerSubject(issuerSubject)
+    , issuerSubjectPublicKeyInfo(issuerSubjectPublicKeyInfo)
     , time(time)
     , maxLifetimeInDays(maxLifetimeInDays)
     , certStatus(CertStatus::Unknown)
     , thisUpdate(thisUpdate)
     , validThrough(validThrough)
   {
     if (thisUpdate) {
       *thisUpdate = 0;
     }
     if (validThrough) {
       *validThrough = 0;
     }
   }
 
   TrustDomain& trustDomain;
-  const CERTCertificate& cert;
-  CERTCertificate& issuerCert;
+  const SECItem& certSerialNumber;
+  const SECItem& issuerSubject;
+  const SECItem& issuerSubjectPublicKeyInfo;
   const PRTime time;
   const uint16_t maxLifetimeInDays;
   CertStatus certStatus;
   PRTime* thisUpdate;
   PRTime* validThrough;
 
 private:
   Context(const Context&); // delete
@@ -108,33 +111,29 @@ HashBuf(const SECItem& item, /*out*/ uin
   }
   return der::Success;
 }
 
 // Verify that potentialSigner is a valid delegated OCSP response signing cert
 // according to RFC 6960 section 4.2.2.2.
 static Result
 CheckOCSPResponseSignerCert(TrustDomain& trustDomain,
-                            CERTCertificate& potentialSigner,
-                            const CERTCertificate& issuerCert, PRTime time)
+                            BackCert& potentialSigner,
+                            const SECItem& issuerSubject,
+                            const SECItem& issuerSubjectPublicKeyInfo,
+                            PRTime time)
 {
   Result rv;
 
-  BackCert cert(&potentialSigner, nullptr, BackCert::IncludeCN::No);
-  rv = cert.Init();
-  if (rv != Success) {
-    return rv;
-  }
-
   // We don't need to do a complete verification of the signer (i.e. we don't
   // have to call BuildCertChain to verify the entire chain) because we
-  // already know that the issuerCert is valid, since revocation checking is
-  // done from the root to the parent after we've built a complete chain that
-  // we know is otherwise valid. Rather, we just need to do a one-step
-  // validation from potentialSigner to issuerCert.
+  // already know that the issuer is valid, since revocation checking is done
+  // from the root to the parent after we've built a complete chain that we
+  // know is otherwise valid. Rather, we just need to do a one-step validation
+  // from potentialSigner to the issuer.
   //
   // It seems reasonable to require the KU_DIGITAL_SIGNATURE key usage on the
   // OCSP responder certificate if the OCSP responder certificate has a
   // key usage extension. However, according to bug 240456, some OCSP responder
   // certificates may have only the nonRepudiation bit set. Also, the OCSP
   // specification (RFC 6960) does not mandate any particular key usage to be
   // asserted for OCSP responde signers. Oddly, the CABForum Baseline
   // Requirements v.1.1.5 do say "If the Root CA Private Key is used for
@@ -142,46 +141,40 @@ CheckOCSPResponseSignerCert(TrustDomain&
   //
   // Note that CheckIssuerIndependentProperties processes
   // SEC_OID_OCSP_RESPONDER in the way that the OCSP specification requires us
   // to--in particular, it doesn't allow SEC_OID_OCSP_RESPONDER to be implied
   // by a missing EKU extension, unlike other EKUs.
   //
   // TODO(bug 926261): If we're validating for a policy then the policy OID we
   // are validating for should be passed to CheckIssuerIndependentProperties.
-  rv = CheckIssuerIndependentProperties(trustDomain, cert, time,
+  rv = CheckIssuerIndependentProperties(trustDomain, potentialSigner, time,
                                         EndEntityOrCA::MustBeEndEntity, 0,
                                         KeyPurposeId::id_kp_OCSPSigning,
                                         CertPolicyId::anyPolicy, 0);
   if (rv != Success) {
     return rv;
   }
 
   // It is possible that there exists a certificate with the same key as the
   // issuer but with a different name, so we need to compare names
+  // XXX(bug 926270) XXX(bug 1008133) XXX(bug 980163): Improve name
+  // comparison.
   // TODO: needs test
-  if (!SECITEM_ItemsAreEqual(&cert.GetNSSCert()->derIssuer,
-                             &issuerCert.derSubject) &&
-      CERT_CompareName(&cert.GetNSSCert()->issuer,
-                       &issuerCert.subject) != SECEqual) {
+  if (!SECITEM_ItemsAreEqual(&potentialSigner.GetIssuer(), &issuerSubject)) {
     return Fail(RecoverableError, SEC_ERROR_OCSP_RESPONDER_CERT_INVALID);
   }
 
   // TODO(bug 926260): check name constraints
-
-  if (trustDomain.VerifySignedData(&potentialSigner.signatureWrap,
-                                   &issuerCert) != SECSuccess) {
-    return MapSECStatus(SECFailure);
-  }
+  return potentialSigner.VerifyOwnSignatureWithKey(trustDomain,
+                                                   issuerSubjectPublicKeyInfo);
 
   // TODO: check for revocation of the OCSP responder certificate unless no-check
   // or the caller forcing no-check. To properly support the no-check policy, we'd
   // need to enforce policy constraints from the issuerChain.
-
-  return Success;
 }
 
 MOZILLA_PKIX_ENUM_CLASS ResponderIDType : uint8_t
 {
   byName = der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
   byKey = der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 2
 };
 
@@ -193,144 +186,127 @@ static inline der::Result ResponseData(
                               const CERTSignedData& signedResponseData,
                               /*const*/ SECItem* certs, size_t numCerts);
 static inline der::Result SingleResponse(der::Input& input,
                                           Context& context);
 static inline der::Result CheckExtensionsForCriticality(der::Input&);
 static inline der::Result CertID(der::Input& input,
                                   const Context& context,
                                   /*out*/ bool& match);
-static der::Result MatchKeyHash(const SECItem& issuerKeyHash,
-                                const CERTCertificate& issuer,
-                                /*out*/ bool& match);
+static Result MatchKeyHash(const SECItem& issuerKeyHash,
+                           const SECItem& issuerSubjectPublicKeyInfo,
+                           /*out*/ bool& match);
+
+static Result
+MatchResponderID(ResponderIDType responderIDType,
+                 const SECItem& responderIDItem,
+                 const SECItem& potentialSignerSubject,
+                 const SECItem& potentialSignerSubjectPublicKeyInfo,
+                 /*out*/ bool& match)
+{
+  match = false;
+
+  switch (responderIDType) {
+    case ResponderIDType::byName:
+      // XXX(bug 926270) XXX(bug 1008133) XXX(bug 980163): Improve name
+      // comparison.
+      match = SECITEM_ItemsAreEqual(&responderIDItem, &potentialSignerSubject);
+      return Success;
+
+    case ResponderIDType::byKey:
+    {
+      der::Input responderID;
+      if (responderID.Init(responderIDItem.data, responderIDItem.len)
+            != der::Success) {
+        return RecoverableError;
+      }
+      SECItem keyHash;
+      if (der::ExpectTagAndGetValue(responderID, der::OCTET_STRING, keyHash)
+            != der::Success) {
+        return RecoverableError;
+      }
+      return MatchKeyHash(keyHash, potentialSignerSubjectPublicKeyInfo, match);
+    }
+
+    default:
+      return Fail(RecoverableError, SEC_ERROR_OCSP_MALFORMED_RESPONSE);
+  }
+}
+
+static Result
+VerifyOCSPSignedData(TrustDomain& trustDomain,
+                     const CERTSignedData& signedResponseData,
+                     const SECItem& spki)
+{
+  SECStatus srv(trustDomain.VerifySignedData(&signedResponseData, spki));
+  if (srv != SECSuccess) {
+    if (PR_GetError() == SEC_ERROR_BAD_SIGNATURE) {
+      PR_SetError(SEC_ERROR_OCSP_BAD_SIGNATURE, 0);
+    }
+  }
+  return MapSECStatus(srv);
+}
 
 // RFC 6960 section 4.2.2.2: The OCSP responder must either be the issuer of
 // the cert or it must be a delegated OCSP response signing cert directly
 // issued by the issuer. If the OCSP responder is a delegated OCSP response
 // signer, then its certificate is (probably) embedded within the OCSP
 // response and we'll need to verify that it is a valid certificate that chains
 // *directly* to issuerCert.
-static CERTCertificate*
-GetOCSPSignerCertificate(TrustDomain& trustDomain,
-                         ResponderIDType responderIDType,
-                         const SECItem& responderIDItem,
-                         const SECItem* certs, size_t numCerts,
-                         CERTCertificate& issuerCert, PRTime time)
-{
-  bool isIssuer = true;
-  size_t i = 0;
-  for (;;) {
-    ScopedCERTCertificate potentialSigner;
-    if (isIssuer) {
-      potentialSigner = CERT_DupCertificate(&issuerCert);
-    } else if (i < numCerts) {
-      potentialSigner = CERT_NewTempCertificate(
-                          CERT_GetDefaultCertDB(),
-                          /*TODO*/const_cast<SECItem*>(&certs[i]), nullptr,
-                          false, false);
-      if (!potentialSigner) {
-        return nullptr;
-      }
-      ++i;
-    } else {
-      PR_SetError(SEC_ERROR_OCSP_INVALID_SIGNING_CERT, 0);
-      return nullptr;
-    }
-
-    bool match;
-    switch (responderIDType) {
-      case ResponderIDType::byName:
-        // The CA is very likely to have encoded the name in the OCSP response
-        // exactly the same as the name is encoded in the signing certificate.
-        // Consequently, most of the time we will avoid parsing the name
-        // completely. We're assuming here that the signer's subject name is
-        // correctly formatted.
-        // TODO: need test for exact name
-        // TODO: need test for non-exact name match
-        match = SECITEM_ItemsAreEqual(&responderIDItem,
-                                      &potentialSigner->derSubject);
-        if (!match) {
-          ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
-          if (!arena) {
-            return nullptr;
-          }
-          CERTName name;
-          if (SEC_QuickDERDecodeItem(arena.get(), &name,
-                                     SEC_ASN1_GET(CERT_NameTemplate),
-                                     &responderIDItem) != SECSuccess) {
-            return nullptr;
-          }
-          match = CERT_CompareName(&name, &potentialSigner->subject) == SECEqual;
-        }
-        break;
-
-      case ResponderIDType::byKey:
-      {
-        der::Input responderID;
-        if (responderID.Init(responderIDItem.data, responderIDItem.len)
-              != der::Success) {
-          return nullptr;
-        }
-        SECItem keyHash;
-        if (der::ExpectTagAndGetValue(responderID, der::OCTET_STRING, keyHash)
-              != der::Success) {
-          return nullptr;
-        }
-        if (MatchKeyHash(keyHash, *potentialSigner.get(), match) != der::Success) {
-          return nullptr;
-        }
-        break;
-      }
-
-      default:
-        PR_SetError(SEC_ERROR_OCSP_MALFORMED_RESPONSE, 0);
-        return nullptr;
-    }
-
-    if (match && !isIssuer) {
-      Result rv = CheckOCSPResponseSignerCert(trustDomain,
-                                              *potentialSigner.get(),
-                                              issuerCert, time);
-      if (rv == RecoverableError) {
-        match = false;
-      } else if (rv != Success) {
-        return nullptr;
-      }
-    }
-
-    if (match) {
-      return potentialSigner.release();
-    }
-
-    isIssuer = false;
-  }
-}
-
-static SECStatus
+static Result
 VerifySignature(Context& context, ResponderIDType responderIDType,
                 const SECItem& responderID, const SECItem* certs,
                 size_t numCerts, const CERTSignedData& signedResponseData)
 {
-  ScopedCERTCertificate signer(
-    GetOCSPSignerCertificate(context.trustDomain, responderIDType, responderID,
-                             certs, numCerts, context.issuerCert,
-                             context.time));
-  if (!signer) {
-    return SECFailure;
+  bool match;
+  Result rv = MatchResponderID(responderIDType, responderID,
+                               context.issuerSubject,
+                               context.issuerSubjectPublicKeyInfo, match);
+  if (rv != Success) {
+    return rv;
+  }
+  if (match) {
+    return VerifyOCSPSignedData(context.trustDomain, signedResponseData,
+                                context.issuerSubjectPublicKeyInfo);
   }
 
-  if (context.trustDomain.VerifySignedData(&signedResponseData, signer.get())
-        != SECSuccess) {
-    if (PR_GetError() == SEC_ERROR_BAD_SIGNATURE) {
-      PR_SetError(SEC_ERROR_OCSP_BAD_SIGNATURE, 0);
+  for (size_t i = 0; i < numCerts; ++i) {
+    BackCert cert(nullptr, BackCert::IncludeCN::No);
+    rv = cert.Init(certs[i]);
+    if (rv != Success) {
+      return rv;
+    }
+    rv = MatchResponderID(responderIDType, responderID,
+                          cert.GetSubject(), cert.GetSubjectPublicKeyInfo(),
+                          match);
+    if (rv == FatalError) {
+      return rv;
+    }
+    if (rv == RecoverableError) {
+      continue;
     }
-    return SECFailure;
+
+    if (match) {
+      rv = CheckOCSPResponseSignerCert(context.trustDomain, cert,
+                                       context.issuerSubject,
+                                       context.issuerSubjectPublicKeyInfo,
+                                       context.time);
+      if (rv == FatalError) {
+        return rv;
+      }
+      if (rv == RecoverableError) {
+        continue;
+      }
+
+      return VerifyOCSPSignedData(context.trustDomain, signedResponseData,
+                                  cert.GetSubjectPublicKeyInfo());
+    }
   }
 
-  return SECSuccess;
+  return Fail(RecoverableError, SEC_ERROR_OCSP_INVALID_SIGNING_CERT);
 }
 
 static inline void
 SetErrorToMalformedResponseOnBadDERError()
 {
   if (PR_GetError() == SEC_ERROR_BAD_DER) {
     PR_SetError(SEC_ERROR_OCSP_MALFORMED_RESPONSE, 0);
   }
@@ -354,17 +330,18 @@ VerifyEncodedOCSPResponse(TrustDomain& t
     return SECFailure;
   }
 
   der::Input input;
   if (input.Init(encodedResponse->data, encodedResponse->len) != der::Success) {
     SetErrorToMalformedResponseOnBadDERError();
     return SECFailure;
   }
-  Context context(trustDomain, *cert, *issuerCert, time, maxOCSPLifetimeInDays,
+  Context context(trustDomain, cert->serialNumber, issuerCert->derSubject,
+                  issuerCert->derPublicKey, time, maxOCSPLifetimeInDays,
                   thisUpdate, validThrough);
 
   if (der::Nested(input, der::SEQUENCE,
                   bind(OCSPResponse, _1, ref(context))) != der::Success) {
     SetErrorToMalformedResponseOnBadDERError();
     return SECFailure;
   }
 
@@ -536,17 +513,17 @@ ResponseData(der::Input& input, Context&
                            responderID) != der::Success) {
     return der::Failure;
   }
 
   // This is the soonest we can verify the signature. We verify the signature
   // right away to follow the principal of minimizing the processing of data
   // before verifying its signature.
   if (VerifySignature(context, responderIDType, responderID, certs, numCerts,
-                      signedResponseData) != SECSuccess) {
+                      signedResponseData) != Success) {
     return der::Failure;
   }
 
   // TODO: Do we even need to parse this? Should we just skip it?
   PRTime producedAt;
   if (der::GeneralizedTime(input, producedAt) != der::Success) {
     return der::Failure;
   }
@@ -730,20 +707,17 @@ CertID(der::Input& input, const Context&
     return der::Failure;
   }
 
   SECItem serialNumber;
   if (der::CertificateSerialNumber(input, serialNumber) != der::Success) {
     return der::Failure;
   }
 
-  const CERTCertificate& cert = context.cert;
-  const CERTCertificate& issuerCert = context.issuerCert;
-
-  if (!SECITEM_ItemsAreEqual(&serialNumber, &cert.serialNumber)) {
+  if (!SECITEM_ItemsAreEqual(&serialNumber, &context.certSerialNumber)) {
     // This does not reference the certificate we're interested in.
     // Consume the rest of the input and return successfully to
     // potentially continue processing other responses.
     input.SkipToEnd();
     return der::Success;
   }
 
   // TODO: support SHA-2 hashes.
@@ -758,54 +732,107 @@ CertID(der::Input& input, const Context&
   if (issuerNameHash.len != SHA1_LENGTH) {
     return der::Fail(SEC_ERROR_OCSP_MALFORMED_RESPONSE);
   }
 
   // From http://tools.ietf.org/html/rfc6960#section-4.1.1:
   // "The hash shall be calculated over the DER encoding of the
   // issuer's name field in the certificate being checked."
   uint8_t hashBuf[SHA1_LENGTH];
-  if (HashBuf(cert.derIssuer, hashBuf, sizeof(hashBuf)) != der::Success) {
+  if (HashBuf(context.issuerSubject, hashBuf, sizeof(hashBuf))
+        != der::Success) {
     return der::Failure;
   }
   if (memcmp(hashBuf, issuerNameHash.data, issuerNameHash.len)) {
     // Again, not interested in this response. Consume input, return success.
     input.SkipToEnd();
     return der::Success;
   }
 
-  return MatchKeyHash(issuerKeyHash, issuerCert, match);
+  if (MatchKeyHash(issuerKeyHash, context.issuerSubjectPublicKeyInfo,
+                   match) != Success) {
+    return der::Failure;
+  }
+
+  return der::Success;
 }
 
 // From http://tools.ietf.org/html/rfc6960#section-4.1.1:
 // "The hash shall be calculated over the value (excluding tag and length) of
 // the subject public key field in the issuer's certificate."
-static der::Result
-MatchKeyHash(const SECItem& keyHash, const CERTCertificate& cert,
+//
+// From http://tools.ietf.org/html/rfc6960#appendix-B.1:
+// KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key
+//                          -- (i.e., the SHA-1 hash of the value of the
+//                          -- BIT STRING subjectPublicKey [excluding
+//                          -- the tag, length, and number of unused
+//                          -- bits] in the responder's certificate)
+static Result
+MatchKeyHash(const SECItem& keyHash, const SECItem& subjectPublicKeyInfo,
              /*out*/ bool& match)
 {
   if (keyHash.len != SHA1_LENGTH)  {
-    return der::Fail(SEC_ERROR_OCSP_MALFORMED_RESPONSE);
+    return Fail(RecoverableError, SEC_ERROR_OCSP_MALFORMED_RESPONSE);
   }
 
   // TODO(bug 966856): support SHA-2 hashes
 
-  // Copy just the length and data pointer (nothing needs to be freed) of the
-  // subject public key so we can convert the length from bits to bytes, which
-  // is what the digest function expects.
-  SECItem spk = cert.subjectPublicKeyInfo.subjectPublicKey;
-  DER_ConvertBitString(&spk);
+  // RFC 5280 Section 4.1
+  //
+  // SubjectPublicKeyInfo  ::=  SEQUENCE  {
+  //    algorithm            AlgorithmIdentifier,
+  //    subjectPublicKey     BIT STRING  }
+
+  der::Input spki;
+
+  {
+    // The scope of input is limited to reduce the possibility of confusing it
+    // with spki in places we need to be using spki below.
+    der::Input input;
+    if (input.Init(subjectPublicKeyInfo.data, subjectPublicKeyInfo.len)
+          != der::Success) {
+      return MapSECStatus(SECFailure);
+    }
+
+    if (der::ExpectTagAndGetValue(input, der::SEQUENCE, spki) != der::Success) {
+      return MapSECStatus(SECFailure);
+    }
+    if (der::End(input) != der::Success) {
+      return MapSECStatus(SECFailure);
+    }
+  }
+
+  // Skip AlgorithmIdentifier
+  if (der::ExpectTagAndSkipValue(spki, der::SEQUENCE) != der::Success) {
+    return MapSECStatus(SECFailure);
+  }
+
+  SECItem subjectPublicKey;
+  if (der::ExpectTagAndGetValue(spki, der::BIT_STRING, subjectPublicKey)
+        != der::Success) {
+    return MapSECStatus(SECFailure);
+  }
+
+  if (der::End(spki) != der::Success) {
+    return MapSECStatus(SECFailure);
+  }
+
+  // Assume/require that the number of unused bits in the public key is zero.
+  if (subjectPublicKey.len == 0 || subjectPublicKey.data[0] != 0) {
+    return Fail(RecoverableError, SEC_ERROR_BAD_DER);
+  }
+  ++subjectPublicKey.data;
+  --subjectPublicKey.len;
 
   static uint8_t hashBuf[SHA1_LENGTH];
-  if (HashBuf(spk, hashBuf, sizeof(hashBuf)) != der::Success) {
-    return der::Failure;
+  if (HashBuf(subjectPublicKey, hashBuf, sizeof(hashBuf)) != der::Success) {
+    return MapSECStatus(SECFailure);
   }
-
   match = !memcmp(hashBuf, keyHash.data, keyHash.len);
-  return der::Success;
+  return Success;
 }
 
 // Extension  ::=  SEQUENCE  {
 //      extnID      OBJECT IDENTIFIER,
 //      critical    BOOLEAN DEFAULT FALSE,
 //      extnValue   OCTET STRING
 //      }
 static der::Result
--- a/security/pkix/lib/pkixutil.h
+++ b/security/pkix/lib/pkixutil.h
@@ -90,64 +90,72 @@ class BackCert
 {
 public:
   // IncludeCN::No means that GetConstrainedNames won't include the subject CN
   // in its results. IncludeCN::Yes means that GetConstrainedNames will include
   // the subject CN in its results.
   MOZILLA_PKIX_ENUM_CLASS IncludeCN { No = 0, Yes = 1 };
 
   // nssCert and childCert must be valid for the lifetime of BackCert
-  BackCert(CERTCertificate* nssCert, BackCert* childCert, IncludeCN includeCN)
+  BackCert(BackCert* childCert, IncludeCN includeCN)
     : encodedBasicConstraints(nullptr)
     , encodedCertificatePolicies(nullptr)
     , encodedExtendedKeyUsage(nullptr)
     , encodedKeyUsage(nullptr)
     , encodedNameConstraints(nullptr)
     , encodedInhibitAnyPolicy(nullptr)
     , childCert(childCert)
-    , nssCert(nssCert)
     , constrainedNames(nullptr)
     , includeCN(includeCN)
   {
   }
 
-  Result Init();
+  Result Init(const SECItem& certDER);
 
   const SECItem& GetDER() const { return nssCert->derCert; }
+  const SECItem& GetIssuer() const { return nssCert->derIssuer; }
+  const SECItem& GetSubject() const { return nssCert->derSubject; }
+  const SECItem& GetSubjectPublicKeyInfo() const
+  {
+    return nssCert->derPublicKey;
+  }
+
+  Result VerifyOwnSignatureWithKey(TrustDomain& trustDomain,
+                                   const SECItem& subjectPublicKeyInfo) const;
 
   const SECItem* encodedBasicConstraints;
   const SECItem* encodedCertificatePolicies;
   const SECItem* encodedExtendedKeyUsage;
   const SECItem* encodedKeyUsage;
   const SECItem* encodedNameConstraints;
   const SECItem* encodedInhibitAnyPolicy;
 
   BackCert* const childCert;
 
   // Only non-const so that we can pass this to TrustDomain::IsRevoked,
   // which only takes a non-const pointer because VerifyEncodedOCSPResponse
   // requires it, and that is only because the implementation of
   // VerifyEncodedOCSPResponse does a CERT_DupCertificate. TODO: get rid
   // of that CERT_DupCertificate so that we can make all these things const.
-  /*const*/ CERTCertificate* GetNSSCert() const { return nssCert; }
+  /*const*/ CERTCertificate* GetNSSCert() const { return nssCert.get(); }
 
   // Returns the names that should be considered when evaluating name
   // constraints. The list is constructed lazily and cached. The result is a
   // weak reference; do not try to free it, and do not hold long-lived
   // references to it.
   Result GetConstrainedNames(/*out*/ const CERTGeneralName** result);
 
   // This is the only place where we should be dealing with non-const
   // CERTCertificates.
   Result PrependNSSCertToList(CERTCertList* results);
 
   PLArenaPool* GetArena();
 
 private:
-  CERTCertificate* nssCert;
+  ScopedCERTCertificate nssCert;
 
   ScopedPLArenaPool arena;
   CERTGeneralName* constrainedNames;
   IncludeCN includeCN;
 
   BackCert(const BackCert&) /* = delete */;
   void operator=(const BackCert&); /* = delete */;
 };
--- a/security/pkix/test/gtest/pkix_cert_chain_length_tests.cpp
+++ b/security/pkix/test/gtest/pkix_cert_chain_length_tests.cpp
@@ -131,19 +131,20 @@ private:
                                  /*out*/ ScopedCERTCertList& results)
   {
     results = CERT_CreateSubjectCertList(nullptr, CERT_GetDefaultCertDB(),
                                          encodedIssuerName, time, true);
     return SECSuccess;
   }
 
   SECStatus VerifySignedData(const CERTSignedData* signedData,
-                             const CERTCertificate* cert)
+                             const SECItem& subjectPublicKeyInfo)
   {
-    return ::mozilla::pkix::VerifySignedData(signedData, cert, nullptr);
+    return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
+                                             nullptr);
   }
 
   SECStatus CheckRevocation(EndEntityOrCA, const CERTCertificate*,
                             /*const*/ CERTCertificate*, PRTime,
                             /*optional*/ const SECItem*)
   {
     return SECSuccess;
   }
--- a/security/pkix/test/lib/pkixtestutil.h
+++ b/security/pkix/test/lib/pkixtestutil.h
@@ -47,16 +47,18 @@ SECITEM_FreeItem_true(SECItem* item)
 {
   SECITEM_FreeItem(item, true);
 }
 
 } // unnamed namespace
 
 typedef mozilla::pkix::ScopedPtr<FILE, fclose_void> ScopedFILE;
 typedef mozilla::pkix::ScopedPtr<SECItem, SECITEM_FreeItem_true> ScopedSECItem;
+typedef mozilla::pkix::ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey>
+  ScopedSECKEYPublicKey;
 typedef mozilla::pkix::ScopedPtr<SECKEYPrivateKey, SECKEY_DestroyPrivateKey>
   ScopedSECKEYPrivateKey;
 
 FILE* OpenFile(const char* dir, const char* filename, const char* mode);
 
 extern const PRTime ONE_DAY;
 
 SECStatus GenerateKeyPair(/*out*/ ScopedSECKEYPublicKey& publicKey,