Bug 1036107, Part 1: Stop using CERTSignedData in mozilla::pkix, r=keeler

--HG--
extra : rebase_source : 94c49062ae3ddf755651f151e2d648543b10e1ad
extra : histedit_source : a7377bf1d9adb62e1c584e2adeb793aa074245fb

security/apps/AppTrustDomain.cpp

@@ -189,7 +189,7 @@ AppTrustDomain::GetCertTrust(EndEntityOrCA endEntityOrCA,
 }
 
 SECStatus
-AppTrustDomain::VerifySignedData(const CERTSignedData& signedData,
+AppTrustDomain::VerifySignedData(const SignedDataWithSignature& signedData,
                                  const SECItem& subjectPublicKeyInfo)
 {
   return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,

security/apps/AppTrustDomain.h

@@ -27,8 +27,9 @@ public:
                  /*out*/ mozilla::pkix::TrustLevel* trustLevel) MOZ_OVERRIDE;
   SECStatus FindIssuer(const SECItem& encodedIssuerName,
                        IssuerChecker& checker, PRTime time) MOZ_OVERRIDE;
-  SECStatus VerifySignedData(const CERTSignedData& signedData,
-                             const SECItem& subjectPublicKeyInfo) MOZ_OVERRIDE;
+  SECStatus VerifySignedData(
+              const mozilla::pkix::SignedDataWithSignature& signedData,
+              const SECItem& subjectPublicKeyInfo) MOZ_OVERRIDE;
   SECStatus CheckRevocation(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                             const mozilla::pkix::CertID& certID, PRTime time,
                             /*optional*/ const SECItem* stapledOCSPresponse,

security/build/nss.def

@@ -652,4 +652,5 @@ VFY_CreateContext
 VFY_DestroyContext
 VFY_End
 VFY_Update
+VFY_VerifyDataDirect
 VFY_VerifyDataWithAlgorithmID

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -217,7 +217,7 @@ NSSCertDBTrustDomain::GetCertTrust(EndEntityOrCA endEntityOrCA,
 }
 
 SECStatus
-NSSCertDBTrustDomain::VerifySignedData(const CERTSignedData& signedData,
+NSSCertDBTrustDomain::VerifySignedData(const SignedDataWithSignature& signedData,
                                        const SECItem& subjectPublicKeyInfo)
 {
   return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,

security/certverifier/NSSCertDBTrustDomain.h

@@ -62,8 +62,9 @@ public:
                                  const SECItem& candidateCertDER,
                          /*out*/ mozilla::pkix::TrustLevel* trustLevel);
 
-  virtual SECStatus VerifySignedData(const CERTSignedData& signedData,
-                                     const SECItem& subjectPublicKeyInfo);
+  virtual SECStatus VerifySignedData(
+                      const mozilla::pkix::SignedDataWithSignature& signedData,
+                      const SECItem& subjectPublicKeyInfo);
 
   virtual SECStatus CheckRevocation(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                                     const mozilla::pkix::CertID& certID,

security/pkix/include/pkix/pkix.h

@@ -98,7 +98,7 @@ SECStatus BuildCertChain(TrustDomain& trustDomain, const SECItem& cert,
             /*optional*/ const SECItem* stapledOCSPResponse);
 
 // Verify the given signed data using the given public key.
-SECStatus VerifySignedData(const CERTSignedData& sd,
+SECStatus VerifySignedData(const SignedDataWithSignature& sd,
                            const SECItem& subjectPublicKeyInfo,
                            void* pkcs11PinArg);
 

security/pkix/include/pkix/pkixtypes.h

@@ -25,14 +25,74 @@
 #ifndef mozilla_pkix__pkixtypes_h
 #define mozilla_pkix__pkixtypes_h
 
-#include "cert.h"
 #include "pkix/enumclass.h"
+#include "pkix/nullptr.h"
+#include "prtime.h"
 #include "seccomon.h"
-#include "secport.h"
 #include "stdint.h"
 
 namespace mozilla { namespace pkix {
 
+MOZILLA_PKIX_ENUM_CLASS DigestAlgorithm
+{
+  sha512 = 1,
+  sha384 = 2,
+  sha256 = 3,
+  sha1 = 4,
+};
+
+// Named ECC Curves:
+//   * secp521r1 (OID 1.3.132.0.35, RFC 5480)
+//   * secp384r1 (OID 1.3.132.0.34, RFC 5480)
+//   * secp256r1 (OID 1.2.840.10045.3.17, RFC 5480)
+MOZILLA_PKIX_ENUM_CLASS SignatureAlgorithm
+{
+  // ecdsa-with-SHA512 (OID 1.2.840.10045.4.3.4, RFC 5758 Section 3.2)
+  ecdsa_with_sha512 = 1,
+
+  // ecdsa-with-SHA384 (OID 1.2.840.10045.4.3.3, RFC 5758 Section 3.2)
+  ecdsa_with_sha384 = 4,
+
+  // ecdsa-with-SHA256 (OID 1.2.840.10045.4.3.2, RFC 5758 Section 3.2)
+  ecdsa_with_sha256 = 7,
+
+  // ecdsa-with-SHA1 (OID 1.2.840.10045.4.1, RFC 3279 Section 2.2.3)
+  ecdsa_with_sha1 = 10,
+
+  // sha512WithRSAEncryption (OID 1.2.840.113549.1.1.13, RFC 4055 Section 5)
+  rsa_pkcs1_with_sha512 = 13,
+
+  // sha384WithRSAEncryption (OID 1.2.840.113549.1.1.12, RFC 4055 Section 5)
+  rsa_pkcs1_with_sha384 = 14,
+
+  // sha256WithRSAEncryption (OID 1.2.840.113549.1.1.11, RFC 4055 Section 5)
+  rsa_pkcs1_with_sha256 = 15,
+
+  // sha-1WithRSAEncryption (OID 1.2.840.113549.1.1.5, RFC 3279 Section 2.2.1)
+  rsa_pkcs1_with_sha1 = 16,
+
+  // id-dsa-with-sha256 (OID 2.16.840.1.101.3.4.3.2, RFC 5758 Section 3.1)
+  dsa_with_sha256 = 17,
+
+  // id-dsa-with-sha1 (OID 1.2.840.10040.4.3, RFC 3279 Section 2.2.2)
+  dsa_with_sha1 = 18,
+};
+
+struct SignedDataWithSignature
+{
+public:
+  SignedDataWithSignature()
+  {
+    data.data = nullptr;
+    data.len = 0;
+    signature.data = nullptr;
+    signature.len = 0;
+  }
+  SECItem data; // non-owning
+  SignatureAlgorithm algorithm;
+  SECItem signature; // non-owning
+};
+
 MOZILLA_PKIX_ENUM_CLASS EndEntityOrCA { MustBeEndEntity = 0, MustBeCA = 1 };
 
 MOZILLA_PKIX_ENUM_CLASS KeyUsage : uint8_t {
@@ -208,13 +268,6 @@ public:
   virtual SECStatus FindIssuer(const SECItem& encodedIssuerName,
                                IssuerChecker& checker, PRTime time) = 0;
 
-  // 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 SECItem& subjectPublicKeyInfo) = 0;
-
   // Called as soon as we think we have a valid chain but before revocation
   // checks are done. This function can be used to compute additional checks,
   // especilaly checks that require the entire certificate chain. This callback
@@ -245,6 +298,12 @@ public:
                        /*optional*/ const SECItem* stapledOCSPresponse,
                        /*optional*/ const SECItem* aiaExtension) = 0;
 
+  // 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 SignedDataWithSignature& signedData,
+                                     const SECItem& subjectPublicKeyInfo) = 0;
 protected:
   TrustDomain() { }
 

security/pkix/lib/pkixcert.cpp

@@ -86,8 +86,9 @@ BackCert::Init()
   // XXX: Ignored. What are we supposed to check? This seems totally redundant
   // with Certificate.signatureAlgorithm. Is it important to check that they
   // are consistent with each other? It doesn't seem to matter!
-  SECAlgorithmID signature;
-  if (der::AlgorithmIdentifier(tbsCertificate, signature) != der::Success) {
+  SignatureAlgorithm signature;
+  if (der::SignatureAlgorithmIdentifier(tbsCertificate, signature)
+        != der::Success) {
     return MapSECStatus(SECFailure);
   }
   if (der::ExpectTagAndGetTLV(tbsCertificate, der::SEQUENCE, issuer)

security/pkix/lib/pkixcheck.cpp

@@ -22,8 +22,7 @@
  * limitations under the License.
  */
 
-#include <limits>
-
+#include "cert.h"
 #include "pkix/bind.h"
 #include "pkix/pkix.h"
 #include "pkix/ScopedPtr.h"

security/pkix/lib/pkixder.cpp

@@ -92,8 +92,193 @@ ExpectTagAndGetLength(Input& input, uint8_t expectedTag, uint16_t& length)
 
 } // namespace internal
 
+static Result
+OptionalNull(Input& input)
+{
+  if (input.Peek(NULLTag)) {
+    return Null(input);
+  }
+  return Success;
+}
+
+namespace {
+
 Result
-SignedData(Input& input, /*out*/ Input& tbs, /*out*/ CERTSignedData& signedData)
+DigestAlgorithmOIDValue(Input& algorithmID, /*out*/ DigestAlgorithm& algorithm)
+{
+  // RFC 4055 Section 2.1
+  // python DottedOIDToCode.py id-sha1 1.3.14.3.2.26
+  static const uint8_t id_sha1[] = {
+    0x2b, 0x0e, 0x03, 0x02, 0x1a
+  };
+  // python DottedOIDToCode.py id-sha256 2.16.840.1.101.3.4.2.1
+  static const uint8_t id_sha256[] = {
+    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01
+  };
+  // python DottedOIDToCode.py id-sha384 2.16.840.1.101.3.4.2.2
+  static const uint8_t id_sha384[] = {
+    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x02
+  };
+  // python DottedOIDToCode.py id-sha512 2.16.840.1.101.3.4.2.3
+  static const uint8_t id_sha512[] = {
+    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03
+  };
+
+  // Matching is attempted based on a rough estimate of the commonality of the
+  // algorithm, to minimize the number of MatchRest calls.
+  if (algorithmID.MatchRest(id_sha1)) {
+    algorithm = DigestAlgorithm::sha1;
+  } else if (algorithmID.MatchRest(id_sha256)) {
+    algorithm = DigestAlgorithm::sha256;
+  } else if (algorithmID.MatchRest(id_sha384)) {
+    algorithm = DigestAlgorithm::sha384;
+  } else if (algorithmID.MatchRest(id_sha512)) {
+    algorithm = DigestAlgorithm::sha512;
+  } else {
+    return Fail(SEC_ERROR_INVALID_ALGORITHM);
+  }
+
+  return Success;
+}
+
+Result
+SignatureAlgorithmOIDValue(Input& algorithmID,
+                           /*out*/ SignatureAlgorithm& algorithm)
+{
+  // RFC 5758 Section 3.1 (id-dsa-with-sha224 is intentionally excluded)
+  // python DottedOIDToCode.py id-dsa-with-sha256 2.16.840.1.101.3.4.3.2
+  static const uint8_t id_dsa_with_sha256[] = {
+    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x03, 0x02
+  };
+
+  // RFC 5758 Section 3.2 (ecdsa-with-SHA224 is intentionally excluded)
+  // python DottedOIDToCode.py ecdsa-with-SHA256 1.2.840.10045.4.3.2
+  static const uint8_t ecdsa_with_SHA256[] = {
+    0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x03, 0x02
+  };
+  // python DottedOIDToCode.py ecdsa-with-SHA384 1.2.840.10045.4.3.3
+  static const uint8_t ecdsa_with_SHA384[] = {
+    0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x03, 0x03
+  };
+  // python DottedOIDToCode.py ecdsa-with-SHA512 1.2.840.10045.4.3.4
+  static const uint8_t ecdsa_with_SHA512[] = {
+    0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x03, 0x04
+  };
+
+  // RFC 4055 Section 5 (sha224WithRSAEncryption is intentionally excluded)
+  // python DottedOIDToCode.py sha256WithRSAEncryption 1.2.840.113549.1.1.11
+  static const uint8_t sha256WithRSAEncryption[] = {
+    0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0b
+  };
+  // python DottedOIDToCode.py sha384WithRSAEncryption 1.2.840.113549.1.1.12
+  static const uint8_t sha384WithRSAEncryption[] = {
+    0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0c
+  };
+  // python DottedOIDToCode.py sha512WithRSAEncryption 1.2.840.113549.1.1.13
+  static const uint8_t sha512WithRSAEncryption[] = {
+    0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0d
+  };
+
+  // RFC 3279 Section 2.2.1
+  // python DottedOIDToCode.py sha-1WithRSAEncryption 1.2.840.113549.1.1.5
+  static const uint8_t sha_1WithRSAEncryption[] = {
+    0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x05
+  };
+
+  // RFC 3279 Section 2.2.2
+  // python DottedOIDToCode.py id-dsa-with-sha1 1.2.840.10040.4.3
+  static const uint8_t id_dsa_with_sha1[] = {
+    0x2a, 0x86, 0x48, 0xce, 0x38, 0x04, 0x03
+  };
+
+  // RFC 3279 Section 2.2.3
+  // python DottedOIDToCode.py ecdsa-with-SHA1 1.2.840.10045.4.1
+  static const uint8_t ecdsa_with_SHA1[] = {
+    0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x01
+  };
+
+  // RFC 5758 Section 3.1 (DSA with SHA-2), RFC 3279 Section 2.2.2 (DSA with
+  // SHA-1), RFC 5758 Section 3.2 (ECDSA with SHA-2), and RFC 3279
+  // Section 2.2.3 (ECDSA with SHA-1) all say that parameters must be omitted.
+  //
+  // RFC 4055 Section 5 and RFC 3279 Section 2.2.1 both say that parameters for
+  // RSA must be encoded as NULL; we relax that requirement by allowing the
+  // NULL to be omitted, to match all the other signature algorithms we support
+  // and for compatibility.
+
+  // Matching is attempted based on a rough estimate of the commonality of the
+  // algorithm, to minimize the number of MatchRest calls.
+  if (algorithmID.MatchRest(sha256WithRSAEncryption)) {
+    algorithm = SignatureAlgorithm::rsa_pkcs1_with_sha256;
+  } else if (algorithmID.MatchRest(ecdsa_with_SHA256)) {
+    algorithm = SignatureAlgorithm::ecdsa_with_sha256;
+  } else if (algorithmID.MatchRest(sha_1WithRSAEncryption)) {
+    algorithm = SignatureAlgorithm::rsa_pkcs1_with_sha1;
+  } else if (algorithmID.MatchRest(ecdsa_with_SHA1)) {
+    algorithm = SignatureAlgorithm::ecdsa_with_sha1;
+  } else if (algorithmID.MatchRest(ecdsa_with_SHA384)) {
+    algorithm = SignatureAlgorithm::ecdsa_with_sha384;
+  } else if (algorithmID.MatchRest(ecdsa_with_SHA512)) {
+    algorithm = SignatureAlgorithm::ecdsa_with_sha512;
+  } else if (algorithmID.MatchRest(sha384WithRSAEncryption)) {
+    algorithm = SignatureAlgorithm::rsa_pkcs1_with_sha384;
+  } else if (algorithmID.MatchRest(sha512WithRSAEncryption)) {
+    algorithm = SignatureAlgorithm::rsa_pkcs1_with_sha512;
+  } else if (algorithmID.MatchRest(id_dsa_with_sha1)) {
+    algorithm = SignatureAlgorithm::dsa_with_sha1;
+  } else if (algorithmID.MatchRest(id_dsa_with_sha256)) {
+    algorithm = SignatureAlgorithm::dsa_with_sha256;
+  } else {
+    // Any MD5-based signature algorithm, or any unknown signature algorithm.
+    return Fail(SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED);
+  }
+
+  return Success;
+}
+
+template <typename OidValueParser, typename Algorithm>
+Result
+AlgorithmIdentifier(OidValueParser oidValueParser, Input& input,
+                    /*out*/ Algorithm& algorithm)
+{
+  Input value;
+  if (ExpectTagAndGetValue(input, SEQUENCE, value) != Success) {
+    return Failure;
+  }
+
+  Input algorithmID;
+  if (ExpectTagAndGetValue(value, der::OIDTag, algorithmID) != Success) {
+    return Failure;
+  }
+  if (oidValueParser(algorithmID, algorithm) != Success) {
+    return Failure;
+  }
+
+  if (OptionalNull(value) != Success) {
+    return Failure;
+  }
+
+  return End(value);
+}
+
+} // unnamed namespace
+
+Result
+SignatureAlgorithmIdentifier(Input& input,
+                             /*out*/ SignatureAlgorithm& algorithm)
+{
+  return AlgorithmIdentifier(SignatureAlgorithmOIDValue, input, algorithm);
+}
+
+Result
+DigestAlgorithmIdentifier(Input& input, /*out*/ DigestAlgorithm& algorithm)
+{
+  return AlgorithmIdentifier(DigestAlgorithmOIDValue, input, algorithm);
+}
+
+Result
+SignedData(Input& input, /*out*/ Input& tbs,
+           /*out*/ SignedDataWithSignature& signedData)
 {
   Input::Mark mark(input.GetMark());
 
@@ -105,7 +290,7 @@ SignedData(Input& input, /*out*/ Input& tbs, /*out*/ CERTSignedData& signedData)
     return Failure;
   }
 
-  if (AlgorithmIdentifier(input, signedData.signatureAlgorithm) != Success) {
+  if (SignatureAlgorithmIdentifier(input, signedData.algorithm) != Success) {
     return Failure;
   }
 
@@ -128,7 +313,6 @@ SignedData(Input& input, /*out*/ Input& tbs, /*out*/ CERTSignedData& signedData)
   }
   ++signedData.signature.data;
   --signedData.signature.len;
-  signedData.signature.len = (signedData.signature.len << 3); // Bytes to bits
 
   return Success;
 }

security/pkix/lib/pkixder.h

@@ -39,7 +39,7 @@
 
 #include "pkix/enumclass.h"
 #include "pkix/nullptr.h"
-
+#include "pkix/pkixtypes.h"
 #include "prerror.h"
 #include "prtime.h"
 #include "secerr.h"
@@ -600,29 +600,6 @@ OID(Input& input, const uint8_t (&expectedOid)[Len])
 
 // PKI-specific types
 
-// AlgorithmIdentifier  ::=  SEQUENCE  {
-//         algorithm               OBJECT IDENTIFIER,
-//         parameters              ANY DEFINED BY algorithm OPTIONAL  }
-inline Result
-AlgorithmIdentifier(Input& input, SECAlgorithmID& algorithmID)
-{
-  Input value;
-  if (ExpectTagAndGetValue(input, der::SEQUENCE, value) != Success) {
-    return Failure;
-  }
-  if (ExpectTagAndGetValue(value, OIDTag, algorithmID.algorithm) != Success) {
-    return Failure;
-  }
-  algorithmID.parameters.data = nullptr;
-  algorithmID.parameters.len = 0;
-  if (!value.AtEnd()) {
-    if (Null(value) != Success) {
-      return Failure;
-    }
-  }
-  return End(value);
-}
-
 inline Result
 CertificateSerialNumber(Input& input, /*out*/ SECItem& value)
 {
@@ -766,6 +743,12 @@ OptionalExtensions(Input& input, uint8_t tag, ExtensionHandler extensionHandler)
   return Success;
 }
 
+Result DigestAlgorithmIdentifier(Input& input,
+                                 /*out*/ DigestAlgorithm& algorithm);
+
+Result SignatureAlgorithmIdentifier(Input& input,
+                                    /*out*/ SignatureAlgorithm& algorithm);
+
 // Parses a SEQUENCE into tbs and then parses an AlgorithmIdentifier followed
 // by a BIT STRING into signedData. This handles the commonality between
 // parsing the signed/signature fields of certificates and OCSP responses. In
@@ -782,8 +765,8 @@ OptionalExtensions(Input& input, uint8_t tag, ExtensionHandler extensionHandler)
 //    signatureAlgorithm   AlgorithmIdentifier,
 //    signature            BIT STRING,
 //    certs            [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL }
-Result
-SignedData(Input& input, /*out*/ Input& tbs, /*out*/ CERTSignedData& signedData);
+Result SignedData(Input& input, /*out*/ Input& tbs,
+                  /*out*/ SignedDataWithSignature& signedDataWithSignature);
 
 } } } // namespace mozilla::pkix::der
 

security/pkix/lib/pkixkey.cpp

@@ -36,11 +36,10 @@
 namespace mozilla { namespace pkix {
 
 SECStatus
-VerifySignedData(const CERTSignedData& sd, const SECItem& subjectPublicKeyInfo,
-                 void* pkcs11PinArg)
+VerifySignedData(const SignedDataWithSignature& sd,
+                 const SECItem& subjectPublicKeyInfo, void* pkcs11PinArg)
 {
-  if (!sd.data.data || !sd.signatureAlgorithm.algorithm.data ||
-      !sd.signature.data) {
+  if (!sd.data.data || !sd.signature.data) {
     PR_NOT_REACHED("invalid args to VerifySignedData");
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return SECFailure;
@@ -52,9 +51,54 @@ VerifySignedData(const CERTSignedData& sd, const SECItem& subjectPublicKeyInfo,
     return SECFailure;
   }
 
-  // convert sig->len from bit counts to byte count.
-  SECItem sig = sd.signature;
-  DER_ConvertBitString(&sig);
+  SECOidTag pubKeyAlg;
+  SECOidTag digestAlg;
+  switch (sd.algorithm) {
+    case SignatureAlgorithm::ecdsa_with_sha512:
+      pubKeyAlg = SEC_OID_ANSIX962_EC_PUBLIC_KEY;
+      digestAlg = SEC_OID_SHA512;
+      break;
+    case SignatureAlgorithm::ecdsa_with_sha384:
+      pubKeyAlg = SEC_OID_ANSIX962_EC_PUBLIC_KEY;
+      digestAlg = SEC_OID_SHA384;
+      break;
+    case SignatureAlgorithm::ecdsa_with_sha256:
+      pubKeyAlg = SEC_OID_ANSIX962_EC_PUBLIC_KEY;
+      digestAlg = SEC_OID_SHA256;
+      break;
+    case SignatureAlgorithm::ecdsa_with_sha1:
+      pubKeyAlg = SEC_OID_ANSIX962_EC_PUBLIC_KEY;
+      digestAlg = SEC_OID_SHA1;
+      break;
+    case SignatureAlgorithm::rsa_pkcs1_with_sha512:
+      pubKeyAlg = SEC_OID_PKCS1_RSA_ENCRYPTION;
+      digestAlg = SEC_OID_SHA512;
+      break;
+    case SignatureAlgorithm::rsa_pkcs1_with_sha384:
+      pubKeyAlg = SEC_OID_PKCS1_RSA_ENCRYPTION;
+      digestAlg = SEC_OID_SHA384;
+      break;
+    case SignatureAlgorithm::rsa_pkcs1_with_sha256:
+      pubKeyAlg = SEC_OID_PKCS1_RSA_ENCRYPTION;
+      digestAlg = SEC_OID_SHA256;
+      break;
+    case SignatureAlgorithm::rsa_pkcs1_with_sha1:
+      pubKeyAlg = SEC_OID_PKCS1_RSA_ENCRYPTION;
+      digestAlg = SEC_OID_SHA1;
+      break;
+    case SignatureAlgorithm::dsa_with_sha256:
+      pubKeyAlg = SEC_OID_ANSIX9_DSA_SIGNATURE;
+      digestAlg = SEC_OID_SHA256;
+      break;
+    case SignatureAlgorithm::dsa_with_sha1:
+      pubKeyAlg = SEC_OID_ANSIX9_DSA_SIGNATURE;
+      digestAlg = SEC_OID_SHA1;
+      break;
+    default:
+      PR_NOT_REACHED("unknown signature algorithm");
+      PR_SetError(SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED, 0);
+      return SECFailure;
+  }
 
   ScopedPtr<CERTSubjectPublicKeyInfo, SECKEY_DestroySubjectPublicKeyInfo>
     spki(SECKEY_DecodeDERSubjectPublicKeyInfo(&subjectPublicKeyInfo));
@@ -67,32 +111,11 @@ VerifySignedData(const CERTSignedData& sd, const SECItem& subjectPublicKeyInfo,
     return SECFailure;
   }
 
-  SECOidTag hashAlg;
-  if (VFY_VerifyDataWithAlgorithmID(sd.data.data, static_cast<int>(sd.data.len),
-                                    pubKey.get(), &sig, &sd.signatureAlgorithm,
-                                    &hashAlg, pkcs11PinArg) != SECSuccess) {
-    return SECFailure;
-  }
-
-  // TODO: Ideally, we would do this check before we call
-  // VFY_VerifyDataWithAlgorithmID. But, VFY_VerifyDataWithAlgorithmID gives us
-  // the hash algorithm so it is more convenient to do things in this order.
-  uint32_t policy;
-  if (NSS_GetAlgorithmPolicy(hashAlg, &policy) != SECSuccess) {
-    return SECFailure;
-  }
-
-  // XXX: I'm not sure why there isn't NSS_USE_ALG_IN_SSL_SIGNATURE, but there
-  // isn't. Since we don't know the context in which we're being called, be as
-  // strict as we can be given the NSS API that is available.
-  static const uint32_t requiredPolicy = NSS_USE_ALG_IN_CERT_SIGNATURE |
-                                         NSS_USE_ALG_IN_CMS_SIGNATURE;
-  if ((policy & requiredPolicy) != requiredPolicy) {
-    PR_SetError(SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED, 0);
-    return SECFailure;
-  }
-
-  return SECSuccess;
+  // The static_cast is safe according to the check above that references
+  // bug 921585.
+  return VFY_VerifyDataDirect(sd.data.data, static_cast<int>(sd.data.len),
+                              pubKey.get(), &sd.signature, pubKeyAlg,
+                              digestAlg, nullptr, pkcs11PinArg);
 }
 
 } } // namespace mozilla::pkix

security/pkix/lib/pkixocsp.cpp

@@ -179,7 +179,7 @@ static inline der::Result ResponseBytes(der::Input&, Context&);
 static inline der::Result BasicResponse(der::Input&, Context&);
 static inline der::Result ResponseData(
                               der::Input& tbsResponseData, Context& context,
-                              const CERTSignedData& signedResponseData,
+                              const SignedDataWithSignature& signedResponseData,
                               /*const*/ SECItem* certs, size_t numCerts);
 static inline der::Result SingleResponse(der::Input& input,
                                           Context& context);
@@ -234,7 +234,7 @@ MatchResponderID(ResponderIDType responderIDType,
 
 static Result
 VerifyOCSPSignedData(TrustDomain& trustDomain,
-                     const CERTSignedData& signedResponseData,
+                     const SignedDataWithSignature& signedResponseData,
                      const SECItem& spki)
 {
   SECStatus srv = trustDomain.VerifySignedData(signedResponseData, spki);
@@ -255,7 +255,8 @@ VerifyOCSPSignedData(TrustDomain& trustDomain,
 static Result
 VerifySignature(Context& context, ResponderIDType responderIDType,
                 const SECItem& responderID, const SECItem* certs,
-                size_t numCerts, const CERTSignedData& signedResponseData)
+                size_t numCerts,
+                const SignedDataWithSignature& signedResponseData)
 {
   bool match;
   Result rv = MatchResponderID(responderIDType, responderID,
@@ -428,7 +429,7 @@ der::Result
 BasicResponse(der::Input& input, Context& context)
 {
   der::Input tbsResponseData;
-  CERTSignedData signedData;
+  SignedDataWithSignature signedData;
   if (der::SignedData(input, tbsResponseData, signedData) != der::Success) {
     if (PR_GetError() == SEC_ERROR_BAD_SIGNATURE) {
       PR_SetError(SEC_ERROR_OCSP_BAD_SIGNATURE, 0);
@@ -483,7 +484,7 @@ BasicResponse(der::Input& input, Context& context)
 //    responseExtensions  [1] EXPLICIT Extensions OPTIONAL }
 static inline der::Result
 ResponseData(der::Input& input, Context& context,
-             const CERTSignedData& signedResponseData,
+             const SignedDataWithSignature& signedResponseData,
              /*const*/ SECItem* certs, size_t numCerts)
 {
   der::Version version;
@@ -678,8 +679,13 @@ CertID(der::Input& input, const Context& context, /*out*/ bool& match)
 {
   match = false;
 
-  SECAlgorithmID hashAlgorithm;
-  if (der::AlgorithmIdentifier(input, hashAlgorithm) != der::Success) {
+  DigestAlgorithm hashAlgorithm;
+  if (der::DigestAlgorithmIdentifier(input, hashAlgorithm) != der::Success) {
+    if (PR_GetError() == SEC_ERROR_INVALID_ALGORITHM) {
+      // Skip entries that are hashed with algorithms we don't support.
+      input.SkipToEnd();
+      return der::Success;
+    }
     return der::Failure;
   }
 
@@ -710,8 +716,7 @@ CertID(der::Input& input, const Context& context, /*out*/ bool& match)
 
   // TODO: support SHA-2 hashes.
 
-  SECOidTag hashAlg = SECOID_GetAlgorithmTag(&hashAlgorithm);
-  if (hashAlg != SEC_OID_SHA1) {
+  if (hashAlgorithm != DigestAlgorithm::sha1) {
     // Again, not interested in this response. Consume input, return success.
     input.SkipToEnd();
     return der::Success;

security/pkix/lib/pkixutil.h

@@ -103,7 +103,7 @@ public:
 
   const SECItem& GetDER() const { return der; }
   const der::Version GetVersion() const { return version; }
-  const CERTSignedData& GetSignedData() const { return signedData; }
+  const SignedDataWithSignature& GetSignedData() const { return signedData; }
   const SECItem& GetIssuer() const { return issuer; }
   // XXX: "validity" is a horrible name for the structure that holds
   // notBefore & notAfter, but that is the name used in RFC 5280 and we use the
@@ -169,11 +169,8 @@ private:
       len = 0;
     }
   };
-  struct NonOwningCERTSignedData : public CERTSignedDataStr {
-    NonOwningCERTSignedData() { memset(this, 0, sizeof(*this)); }
-  };
 
-  NonOwningCERTSignedData signedData;
+  SignedDataWithSignature signedData;
   NonOwningSECItem issuer;
   // XXX: "validity" is a horrible name for the structure that holds
   // notBefore & notAfter, but that is the name used in RFC 5280 and we use the

security/pkix/test/gtest/pkixbuild_tests.cpp

@@ -150,7 +150,7 @@ private:
     return SECSuccess;
   }
 
-  SECStatus VerifySignedData(const CERTSignedData& signedData,
+  SECStatus VerifySignedData(const SignedDataWithSignature& signedData,
                              const SECItem& subjectPublicKeyInfo)
   {
     return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,

security/pkix/test/gtest/pkixcert_extension_tests.cpp

@@ -90,7 +90,7 @@ private:
     return SECFailure;
   }
 
-  SECStatus VerifySignedData(const CERTSignedData& signedData,
+  SECStatus VerifySignedData(const SignedDataWithSignature& signedData,
                              const SECItem& subjectPublicKeyInfo)
   {
     return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,

security/pkix/test/gtest/pkixder_pki_types_tests.cpp

@@ -26,7 +26,6 @@
 #include <vector>
 #include <gtest/gtest.h>
 
-#include "pkix/bind.h"
 #include "pkixder.h"
 
 using namespace mozilla::pkix::der;
@@ -42,59 +41,6 @@ protected:
   }
 };
 
-TEST_F(pkixder_pki_types_tests, AlgorithmIdentifierNoParams)
-{
-  const uint8_t DER_ALGORITHM_IDENTIFIER_NO_PARAMS[] = {
-    0x30/*SEQUENCE*/, 0x06/*LENGTH*/,
-    0x06, 0x04, 0xde, 0xad, 0xbe, 0xef   // OID
-  };
-
-  Input input;
-  ASSERT_EQ(Success, input.Init(DER_ALGORITHM_IDENTIFIER_NO_PARAMS,
-                                sizeof DER_ALGORITHM_IDENTIFIER_NO_PARAMS));
-
-  const uint8_t expectedAlgorithmID[] = {
-    0xde, 0xad, 0xbe, 0xef
-  };
-
-  SECAlgorithmID algorithmID;
-  ASSERT_EQ(Success, AlgorithmIdentifier(input, algorithmID));
-
-  ASSERT_EQ(sizeof expectedAlgorithmID, algorithmID.algorithm.len);
-  ASSERT_EQ(0, memcmp(algorithmID.algorithm.data, expectedAlgorithmID,
-                      sizeof expectedAlgorithmID));
-
-  ASSERT_EQ(0u, algorithmID.parameters.len);
-  ASSERT_FALSE(algorithmID.parameters.data);
-}
-
-TEST_F(pkixder_pki_types_tests, AlgorithmIdentifierNullParams)
-{
-  const uint8_t DER_ALGORITHM_IDENTIFIER_NULL_PARAMS[] = {
-    0x30, 0x08,                         // SEQUENCE
-    0x06, 0x04, 0xde, 0xad, 0xbe, 0xef, // OID
-    0x05, 0x00                          // NULL
-  };
-
-  Input input;
-  ASSERT_EQ(Success, input.Init(DER_ALGORITHM_IDENTIFIER_NULL_PARAMS,
-                                sizeof DER_ALGORITHM_IDENTIFIER_NULL_PARAMS));
-
-  const uint8_t expectedAlgorithmID[] = {
-    0xde, 0xad, 0xbe, 0xef
-  };
-
-  SECAlgorithmID algorithmID;
-  ASSERT_EQ(Success, AlgorithmIdentifier(input, algorithmID));
-
-  ASSERT_EQ(sizeof expectedAlgorithmID, algorithmID.algorithm.len);
-  ASSERT_TRUE(memcmp(algorithmID.algorithm.data, expectedAlgorithmID,
-                     sizeof expectedAlgorithmID) == 0);
-
-  ASSERT_EQ((size_t) 0, algorithmID.parameters.len);
-  ASSERT_EQ(NULL, algorithmID.parameters.data);
-}
-
 TEST_F(pkixder_pki_types_tests, CertificateSerialNumber)
 {
   const uint8_t DER_CERT_SERIAL[] = {
@@ -266,4 +212,5 @@ TEST_F(pkixder_pki_types_tests, OptionalVersionMissing)
   ASSERT_EQ(Success, OptionalVersion(input, version));
   ASSERT_EQ(Version::v1, version);
 }
+
 } // unnamed namespace

security/pkix/test/lib/pkixtestutil.h

@@ -28,6 +28,7 @@
 #include <stdint.h>
 #include <stdio.h>
 
+#include "cert.h"
 #include "keyhi.h"
 #include "pkix/enumclass.h"
 #include "pkix/pkixtypes.h"