Bug 1041344: Refactor mozilla::pkix::CheckCertificatePolicies, r=cviecco
authorBrian Smith <brian@briansmith.org>
Sat, 19 Jul 2014 18:51:10 -0700
changeset 197617 039dd305c56744b09adb910e07cc406df72712ac
parent 197616 ba49cca9883ef1e9fa855c20685334743656a58c
child 197618 64719bb171797b81c6d155251da939904777fa31
push id27249
push userryanvm@gmail.com
push dateMon, 04 Aug 2014 20:14:35 +0000
treeherdermozilla-central@7f81be7db528 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscviecco
bugs1041344
milestone34.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 1041344: Refactor mozilla::pkix::CheckCertificatePolicies, r=cviecco
security/pkix/include/pkix/Input.h
security/pkix/include/pkix/bind.h
security/pkix/lib/pkixcheck.cpp
--- a/security/pkix/include/pkix/Input.h
+++ b/security/pkix/include/pkix/Input.h
@@ -190,42 +190,33 @@ public:
     // here we want to be sure that there is nothing following the matched
     // bytes
     if (static_cast<size_t>(end - input) != N) {
       return false;
     }
     if (memcmp(input, toMatch, N)) {
       return false;
     }
-    input += N;
+    input = end;
     return true;
   }
 
-  template <uint16_t N>
-  bool MatchTLV(uint8_t tag, uint16_t len, const uint8_t (&value)[N])
+  bool MatchRest(Input toMatch)
   {
-    static_assert(N <= 127, "buffer larger than largest length supported");
-    if (len > N) {
-      PR_NOT_REACHED("overflow prevented dynamically instead of statically");
+    // Normally we use EnsureLength which compares (input + len < end), but
+    // here we want to be sure that there is nothing following the matched
+    // bytes
+    size_t remaining = static_cast<size_t>(end - input);
+    if (toMatch.GetLength() != remaining) {
       return false;
     }
-    uint16_t totalLen = 2u + len;
-    if (EnsureLength(totalLen) != Success) {
-      return false;
-    }
-    if (*input != tag) {
+    if (std::memcmp(input, toMatch.UnsafeGetData(), remaining)) {
       return false;
     }
-    if (*(input + 1) != len) {
-      return false;
-    }
-    if (memcmp(input + 2, value, len)) {
-      return false;
-    }
-    input += totalLen;
+    input = end;
     return true;
   }
 
   Result Skip(uint16_t len)
   {
     Result rv = EnsureLength(len);
     if (rv != Success) {
       return rv;
--- a/security/pkix/include/pkix/bind.h
+++ b/security/pkix/include/pkix/bind.h
@@ -90,31 +90,16 @@ public:
   R operator()(P1& p1) const { return f(p1, b1, b2); }
 private:
   const F f;
   B1& b1;
   B2& b2;
   void operator=(const Bind2&) /*= delete*/;
 };
 
-template <typename R, typename P1, typename B1, typename B2, typename B3>
-class Bind3
-{
-public:
-  typedef R (*F)(P1&, B1, B2&, B3&);
-  Bind3(F f, B1& b1, B2& b2, B3& b3) : f(f), b1(b1), b2(b2), b3(b3) { }
-  R operator()(P1& p1) const { return f(p1, b1, b2, b3); }
-private:
-  const F f;
-  B1& b1;
-  B2& b2;
-  B3& b3;
-  void operator=(const Bind3&) /*= delete*/;
-};
-
 template <typename R, typename P1, typename B1, typename B2, typename B3,
           typename B4>
 class Bind4
 {
 public:
   typedef R (*F)(P1&, B1, B2, B3&, B4&);
   Bind4(F f, B1& b1, B2& b2, B3& b3, B4& b4)
     : f(f), b1(b1), b2(b2), b3(b3), b4(b4) { }
@@ -152,23 +137,16 @@ bind(R (*f)(P1&, B1&), Placeholder1&, B1
 
 template <typename R, typename P1, typename B1, typename B2>
 inline internal::Bind2<R, P1, B1, B2>
 bind(R (*f)(P1&, B1&, B2&), Placeholder1&, B1& b1, B2& b2)
 {
   return internal::Bind2<R, P1, B1, B2>(f, b1, b2);
 }
 
-template <typename R, typename P1, typename B1, typename B2, typename B3>
-inline internal::Bind3<R, P1, const B1, const B2, B3>
-bind(R (*f)(P1&, B1, const B2&, B3&), Placeholder1&, B1& b1, const B2& b2, B3& b3)
-{
-  return internal::Bind3<R, P1, const B1, const B2, B3>(f, b1, b2, b3);
-}
-
 template <typename R, typename P1, typename B1, typename B2, typename B3,
           typename B4>
 inline internal::Bind4<R, P1, const B1, const B2, B3, B4>
 bind(R (*f)(P1&, B1, B2, B3&, B4&), Placeholder1&, const B1& b1, const B2& b2,
      B3& b3, B4& b4)
 {
   return internal::Bind4<R, P1, const B1, const B2, B3, B4>(f, b1, b2, b3, b4);
 }
--- a/security/pkix/lib/pkixcheck.cpp
+++ b/security/pkix/lib/pkixcheck.cpp
@@ -174,114 +174,123 @@ CheckKeyUsage(EndEntityOrCA endEntityOrC
   return Success;
 }
 
 // RFC5820 4.2.1.4. Certificate Policies
 
 // "The user-initial-policy-set contains the special value any-policy if the
 // user is not concerned about certificate policy."
 //
-// id-ce OBJECT IDENTIFIER  ::=  {joint-iso-ccitt(2) ds(5) 29}
-// id-ce-certificatePolicies OBJECT IDENTIFIER ::=  { id-ce 32 }
-// anyPolicy OBJECT IDENTIFIER ::= { id-ce-certificatePolicies 0 }
+// python DottedOIDToCode.py anyPolicy 2.5.29.32.0
 
-/*static*/ const CertPolicyId CertPolicyId::anyPolicy = {
-  4, { (40*2)+5, 29, 32, 0 }
+static const uint8_t anyPolicy[] = {
+  0x55, 0x1d, 0x20, 0x00
 };
 
-bool CertPolicyId::IsAnyPolicy() const
-{
-  return this == &anyPolicy ||
-         (numBytes == anyPolicy.numBytes &&
-          !memcmp(bytes, anyPolicy.bytes, anyPolicy.numBytes));
-}
+/*static*/ const CertPolicyId CertPolicyId::anyPolicy = {
+  4, { 0x55, 0x1d, 0x20, 0x00 }
+};
 
-// PolicyInformation ::= SEQUENCE {
-//         policyIdentifier   CertPolicyId,
-//         policyQualifiers   SEQUENCE SIZE (1..MAX) OF
-//                                 PolicyQualifierInfo OPTIONAL }
-inline Result
-CheckPolicyInformation(Reader& input, EndEntityOrCA endEntityOrCA,
-                       const CertPolicyId& requiredPolicy,
-                       /*in/out*/ bool& found)
-{
-  if (input.MatchTLV(der::OIDTag, requiredPolicy.numBytes,
-                     requiredPolicy.bytes)) {
-    found = true;
-  } else if (endEntityOrCA == EndEntityOrCA::MustBeCA &&
-             input.MatchTLV(der::OIDTag, CertPolicyId::anyPolicy.numBytes,
-                            CertPolicyId::anyPolicy.bytes)) {
-    found = true;
+bool
+CertPolicyId::IsAnyPolicy() const {
+  if (this == &CertPolicyId::anyPolicy) {
+    return true;
   }
-
-  // RFC 5280 Section 4.2.1.4 says "Optional qualifiers, which MAY be present,
-  // are not expected to change the definition of the policy." Also, it seems
-  // that Section 6, which defines validation, does not require any matching of
-  // qualifiers. Thus, doing anything with the policy qualifiers would be a
-  // waste of time and a source of potential incompatibilities, so we just
-  // ignore them.
-
-  // Skip unmatched OID and/or policyQualifiers
-  input.SkipToEnd();
-
-  return Success;
+  return numBytes == PR_ARRAY_SIZE(::mozilla::pkix::anyPolicy) &&
+         !memcmp(bytes, ::mozilla::pkix::anyPolicy,
+                 PR_ARRAY_SIZE(::mozilla::pkix::anyPolicy));
 }
 
 // certificatePolicies ::= SEQUENCE SIZE (1..MAX) OF PolicyInformation
 Result
 CheckCertificatePolicies(EndEntityOrCA endEntityOrCA,
                          const Input* encodedCertificatePolicies,
                          const Input* encodedInhibitAnyPolicy,
                          TrustLevel trustLevel,
                          const CertPolicyId& requiredPolicy)
 {
   if (requiredPolicy.numBytes == 0 ||
       requiredPolicy.numBytes > sizeof requiredPolicy.bytes) {
     return Result::FATAL_ERROR_INVALID_ARGS;
   }
 
-  // Ignore all policy information if the caller indicates any policy is
-  // acceptable. See TrustDomain::GetCertTrust and the policy part of
-  // BuildCertChain's documentation.
-  if (requiredPolicy.IsAnyPolicy()) {
+  bool requiredPolicyFound = requiredPolicy.IsAnyPolicy();
+  if (requiredPolicyFound) {
     return Success;
   }
 
   // Bug 989051. Until we handle inhibitAnyPolicy we will fail close when
-  // inhibitAnyPolicy extension is present and we need to evaluate certificate
-  // policies.
-  if (encodedInhibitAnyPolicy) {
+  // inhibitAnyPolicy extension is present and we are validating for a policy.
+  if (!requiredPolicyFound && encodedInhibitAnyPolicy) {
     return Result::ERROR_POLICY_VALIDATION_FAILED;
   }
 
   // The root CA certificate may omit the policies that it has been
   // trusted for, so we cannot require the policies to be present in those
   // certificates. Instead, the determination of which roots are trusted for
   // which policies is made by the TrustDomain's GetCertTrust method.
   if (trustLevel == TrustLevel::TrustAnchor &&
       endEntityOrCA == EndEntityOrCA::MustBeCA) {
-    return Success;
+    requiredPolicyFound = true;
   }
 
-  if (!encodedCertificatePolicies) {
-    return Result::ERROR_POLICY_VALIDATION_FAILED;
+  Input requiredPolicyDER;
+  if (requiredPolicyDER.Init(requiredPolicy.bytes, requiredPolicy.numBytes)
+        != Success) {
+    return Result::FATAL_ERROR_INVALID_ARGS;
   }
 
-  bool found = false;
+  if (encodedCertificatePolicies) {
+    Reader extension(*encodedCertificatePolicies);
+    Reader certificatePolicies;
+    Result rv = der::ExpectTagAndGetValue(extension, der::SEQUENCE,
+                                          certificatePolicies);
+    if (rv != Success) {
+      return Result::ERROR_POLICY_VALIDATION_FAILED;
+    }
+    if (!extension.AtEnd()) {
+      return Result::ERROR_POLICY_VALIDATION_FAILED;
+    }
+
+    do {
+      // PolicyInformation ::= SEQUENCE {
+      //         policyIdentifier   CertPolicyId,
+      //         policyQualifiers   SEQUENCE SIZE (1..MAX) OF
+      //                                 PolicyQualifierInfo OPTIONAL }
+      Reader policyInformation;
+      rv = der::ExpectTagAndGetValue(certificatePolicies, der::SEQUENCE,
+                                     policyInformation);
+      if (rv != Success) {
+        return Result::ERROR_POLICY_VALIDATION_FAILED;
+      }
 
-  Reader input(*encodedCertificatePolicies);
-  if (der::NestedOf(input, der::SEQUENCE, der::SEQUENCE, der::EmptyAllowed::No,
-                    bind(CheckPolicyInformation, _1, endEntityOrCA,
-                         requiredPolicy, ref(found))) != Success) {
-    return Result::ERROR_POLICY_VALIDATION_FAILED;
+      Reader policyIdentifier;
+      rv = der::ExpectTagAndGetValue(policyInformation, der::OIDTag,
+                                     policyIdentifier);
+      if (rv != Success) {
+        return rv;
+      }
+
+      if (policyIdentifier.MatchRest(requiredPolicyDER)) {
+        requiredPolicyFound = true;
+      } else if (endEntityOrCA == EndEntityOrCA::MustBeCA &&
+                 policyIdentifier.MatchRest(anyPolicy)) {
+        requiredPolicyFound = true;
+      }
+
+      // RFC 5280 Section 4.2.1.4 says "Optional qualifiers, which MAY be
+      // present, are not expected to change the definition of the policy." Also,
+      // it seems that Section 6, which defines validation, does not require any
+      // matching of qualifiers. Thus, doing anything with the policy qualifiers
+      // would be a waste of time and a source of potential incompatibilities, so
+      // we just ignore them.
+    } while (!requiredPolicyFound && !certificatePolicies.AtEnd());
   }
-  if (der::End(input) != Success) {
-    return Result::ERROR_POLICY_VALIDATION_FAILED;
-  }
-  if (!found) {
+
+  if (!requiredPolicyFound) {
     return Result::ERROR_POLICY_VALIDATION_FAILED;
   }
 
   return Success;
 }
 
 static const long UNLIMITED_PATH_LEN = -1; // must be less than zero