Bug 1058812 - mozilla::pkix: Add SignatureAlgorithm::unsupported_algorithm to better handle e.g. roots signed with RSA/MD5. r=briansmith, a=sledru
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 07 Oct 2014 09:35:42 -0700
changeset 258703 4c62d5e8d5fc
parent 258702 96bcea5ee703
child 258704 fe4f4c9342b1
push id4700
push userryanvm@gmail.com
push date2015-04-21 23:53 +0000
treeherdermozilla-beta@d27c9211ebb3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbriansmith, sledru
bugs1058812
milestone33.0
Bug 1058812 - mozilla::pkix: Add SignatureAlgorithm::unsupported_algorithm to better handle e.g. roots signed with RSA/MD5. r=briansmith, a=sledru
security/pkix/include/pkix/pkixtypes.h
security/pkix/lib/pkixbuild.cpp
security/pkix/lib/pkixder.cpp
security/pkix/lib/pkixnss.cpp
security/pkix/lib/pkixocsp.cpp
security/pkix/lib/pkixutil.h
--- a/security/pkix/include/pkix/pkixtypes.h
+++ b/security/pkix/include/pkix/pkixtypes.h
@@ -71,16 +71,19 @@ MOZILLA_PKIX_ENUM_CLASS SignatureAlgorit
   // 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,
+
+  // Used to indicate any unsupported algorithm.
+  unsupported_algorithm = 19,
 };
 
 struct SignedDataWithSignature
 {
 public:
   SignedDataWithSignature()
   {
     data.data = nullptr;
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -176,18 +176,18 @@ PathBuildingStep::Check(const SECItem& p
   // subject public key MUST NOT be used to verify signatures on certificates
   // or CRLs unless the corresponding keyCertSign or cRLSign bit is set."
   rv = BuildForward(trustDomain, potentialIssuer, time, KeyUsage::keyCertSign,
                     requiredEKUIfPresent, requiredPolicy, nullptr, subCACount);
   if (rv != Success) {
     return RecordResult(rv, keepGoing);
   }
 
-  rv = trustDomain.VerifySignedData(subject.GetSignedData(),
-                                    potentialIssuer.GetSubjectPublicKeyInfo());
+  rv = WrappedVerifySignedData(trustDomain, subject.GetSignedData(),
+                               potentialIssuer.GetSubjectPublicKeyInfo());
   if (rv != Success) {
     return RecordResult(rv, keepGoing);
   }
 
   // We avoid doing revocation checking for expired certificates because OCSP
   // responders are allowed to forget about expired certificates, and many OCSP
   // responders return an error when asked for the status of an expired
   // certificate.
--- a/security/pkix/lib/pkixder.cpp
+++ b/security/pkix/lib/pkixder.cpp
@@ -233,18 +233,17 @@ SignatureAlgorithmOIDValue(Input& algori
   } 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 if (algorithmID.MatchRest(sha1WithRSASignature)) {
     // XXX(bug 1042479): recognize this old OID for compatibility.
     algorithm = SignatureAlgorithm::rsa_pkcs1_with_sha1;
   } else {
-    // Any MD5-based signature algorithm, or any unknown signature algorithm.
-    return Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED;
+    algorithm = SignatureAlgorithm::unsupported_algorithm;
   }
 
   return Success;
 }
 
 template <typename OidValueParser, typename Algorithm>
 Result
 AlgorithmIdentifier(OidValueParser oidValueParser, Input& input,
--- a/security/pkix/lib/pkixnss.cpp
+++ b/security/pkix/lib/pkixnss.cpp
@@ -139,16 +139,17 @@ VerifySignedData(const SignedDataWithSig
     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;
+    case SignatureAlgorithm::unsupported_algorithm:
     default:
       PR_NOT_REACHED("unknown signature algorithm");
       return Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED;
   }
 
   Result rv;
   ScopedSECKeyPublicKey pubKey;
   rv = CheckPublicKeySize(subjectPublicKeyInfo, pubKey);
--- a/security/pkix/lib/pkixocsp.cpp
+++ b/security/pkix/lib/pkixocsp.cpp
@@ -126,18 +126,18 @@ CheckOCSPResponseSignerCert(TrustDomain&
   // XXX(bug 926270) XXX(bug 1008133) XXX(bug 980163): Improve name
   // comparison.
   // TODO: needs test
   if (!SECITEM_ItemsAreEqual(&potentialSigner.GetIssuer(), &issuerSubject)) {
     return Result::ERROR_OCSP_RESPONDER_CERT_INVALID;
   }
 
   // TODO(bug 926260): check name constraints
-  rv = trustDomain.VerifySignedData(potentialSigner.GetSignedData(),
-                                    issuerSubjectPublicKeyInfo);
+  rv = WrappedVerifySignedData(trustDomain, potentialSigner.GetSignedData(),
+                               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 rv;
 }
 
@@ -208,17 +208,17 @@ MatchResponderID(TrustDomain& trustDomai
   }
 }
 
 static Result
 VerifyOCSPSignedData(TrustDomain& trustDomain,
                      const SignedDataWithSignature& signedResponseData,
                      const SECItem& spki)
 {
-  Result rv = trustDomain.VerifySignedData(signedResponseData, spki);
+  Result rv = WrappedVerifySignedData(trustDomain, signedResponseData, spki);
   if (rv == Result::ERROR_BAD_SIGNATURE) {
     rv = Result::ERROR_OCSP_BAD_SIGNATURE;
   }
   return rv;
 }
 
 // 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
--- a/security/pkix/lib/pkixutil.h
+++ b/security/pkix/lib/pkixutil.h
@@ -177,11 +177,25 @@ public:
 
   // Public so we can static_assert on this. Keep in sync with MAX_SUBCA_COUNT.
   static const size_t MAX_LENGTH = 8;
 private:
   const SECItem* items[MAX_LENGTH]; // avoids any heap allocations
   size_t numItems;
 };
 
+// Ensures that we do not call the TrustDomain's VerifySignedData function if
+// the algorithm is unsupported.
+inline Result
+WrappedVerifySignedData(TrustDomain& trustDomain,
+                        const SignedDataWithSignature& signedData,
+                        const SECItem& subjectPublicKeyInfo)
+{
+  if (signedData.algorithm == SignatureAlgorithm::unsupported_algorithm) {
+    return Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED;
+  }
+
+  return trustDomain.VerifySignedData(signedData, subjectPublicKeyInfo);
+}
+
 } } // namespace mozilla::pkix
 
 #endif // mozilla_pkix__pkixutil_h