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 207616 fa797212429e813a44f198c59763a44027a1cadc
parent 207615 7673b3a0749997dddcbe768371fc99a3f174491b
child 207617 dc9d168ba8fb8366f875a2556bcacd06bacf5e75
push id494
push userraliiev@mozilla.com
push dateMon, 25 Aug 2014 18:42:16 +0000
treeherdermozilla-release@a3cc3e46b571 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1020683
milestone32.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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,