Bug 1330968 - Make OneCRL name comparisons encoding agnostic (alternative approach) r?keeler draft
authorMark Goodwin <mgoodwin@mozilla.com>
Mon, 16 Oct 2017 13:29:20 +0100
changeset 680818 a12e32930e9c8ab9eb8168c19680fe6ab01f7760
parent 676640 d54bb49a08e9d1bdab303bc901062092159927c7
child 735980 96908aaea900fff9dac3df066532bd1deefef33f
push id84641
push usermgoodwin@mozilla.com
push dateMon, 16 Oct 2017 12:31:01 +0000
reviewerskeeler
bugs1330968
milestone58.0a1
Bug 1330968 - Make OneCRL name comparisons encoding agnostic (alternative approach) r?keeler MozReview-Commit-ID: UcAB4AEwFM
security/manager/ssl/CertBlocklist.cpp
security/manager/ssl/tests/unit/test_cert_blocklist.js
security/pkix/include/pkix/pkix.h
security/pkix/lib/pkixnames.cpp
--- a/security/manager/ssl/CertBlocklist.cpp
+++ b/security/manager/ssl/CertBlocklist.cpp
@@ -22,16 +22,17 @@
 #include "nsISafeOutputStream.h"
 #include "nsIX509Cert.h"
 #include "nsNetCID.h"
 #include "nsNetUtil.h"
 #include "nsPromiseFlatString.h"
 #include "nsTHashtable.h"
 #include "nsThreadUtils.h"
 #include "pkix/Input.h"
+#include "pkix/pkix.h"
 #include "prtime.h"
 
 NS_IMPL_ISUPPORTS(CertBlocklist, nsICertBlocklist)
 
 using namespace mozilla;
 using namespace mozilla::pkix;
 
 #define PREF_BACKGROUND_UPDATE_TIMER "app.update.lastUpdateTime.blocklist-background-update-timer"
@@ -94,25 +95,62 @@ CertBlocklistItem::ToBase64(nsACString& 
   }
   rv = Base64Encode(otherString, b64OtherOut);
   return rv;
 }
 
 bool
 CertBlocklistItem::operator==(const CertBlocklistItem& aItem) const
 {
+  // if the mechanisms do not match, the items are not equal
   if (aItem.mItemMechanism != mItemMechanism) {
     return false;
   }
-  if (aItem.mDNLength != mDNLength ||
-      aItem.mOtherLength != mOtherLength) {
+
+  // if the non-name data do not match, the items are not equal
+  // TODO: We should probably do a more sophisticated check for serial numbers
+  // to address bug 1300150
+  if (aItem.mOtherLength != mOtherLength ||
+      memcmp(aItem.mOtherData, mOtherData, mOtherLength) != 0) {
+    return false;
+  }
+
+  // if the name data are identical, the items match so we don't need to perform
+  // further checks
+  if (aItem.mDNLength == mDNLength &&
+      memcmp(aItem.mDNData, mDNData, mDNLength) == 0) {
+    return true;
+  }
+
+  // Check that the names are equal
+  Input issuerThis;
+  if (issuerThis.Init(mDNData, mDNLength) != Success) {
     return false;
   }
-  return memcmp(aItem.mDNData, mDNData, mDNLength) == 0 &&
-         memcmp(aItem.mOtherData, mOtherData, mOtherLength) == 0;
+
+  Input issuerThat;
+  if (issuerThat.Init(aItem.mDNData, aItem.mDNLength) != Success) {
+    return false;
+  }
+
+  bool match;
+  size_t issuerThisDNCount;
+  size_t issuerThatDNCount;
+  pkix::Result rv = MatchDirectoryNamePrefix(issuerThis, issuerThat,
+                                             issuerThisDNCount,
+                                             issuerThatDNCount, match);
+  if (rv != Success) {
+    // it's possible that the call to NamesMatch could fail when parsing the DER
+    // data. Given that one of the RDNs is mozilla-curated (from OneCRL) data
+    // it's reasonable to assume the this record parses correctly and therefore
+    // a failure is a mismatch.
+    return false;
+  }
+
+  return match && issuerThisDNCount == issuerThatDNCount;
 }
 
 uint32_t
 CertBlocklistItem::Hash() const
 {
   uint32_t hash;
   // there's no requirement for a serial to be as large as the size of the hash
   // key; if it's smaller, fall back to the first octet (otherwise, the last
--- a/security/manager/ssl/tests/unit/test_cert_blocklist.js
+++ b/security/manager/ssl/tests/unit/test_cert_blocklist.js
@@ -417,10 +417,48 @@ function run_test() {
   });
 
   add_test(function() {
     // Check the blocklist entry has not changed
     check_revocations_txt_contents(expected);
     run_next_test();
   });
 
+  add_test(function() {
+    // check that RDNs with different encodings are treated as equal by the
+    // CertBlocklist - first revoke PrintableString, check UTF8String
+    let issuer1PrintableString = "MEAxCzAJBgNVBAYTAkZSMRIwEAYDVQQKEwlPcGVuVHJ1c3QxHTAbBgNVBAMTFE9wZW5UcnVzdCBSb290IENBIEcx";
+    let issuer1UTF8String = "MEAxCzAJBgNVBAYTAkZSMRIwEAYDVQQKDAlPcGVuVHJ1c3QxHTAbBgNVBAMMFE9wZW5UcnVzdCBSb290IENBIEcx";
+    let serial = "c2VyaWFsMi4=";
+    certList.revokeCertByIssuerAndSerial(issuer1PrintableString, serial);
+    ok(test_is_revoked(certList, atob(issuer1UTF8String), atob(serial)),
+       "issuer / serial should be blocked despite different encoding");
+
+    // next revoke UTF8String, check PrintableString
+    let issuer2UTF8String = "MEAxCzAJBgNVBAYTAkZSMRIwEAYDVQQKDAlhYWFhYWFhYWExHTAbBgNVBAMMFGFhYWFhYWFhYWFhYWFhYWFhYWFh";
+    let issuer2PrintableString = "MEAxCzAJBgNVBAYTAkZSMRIwEAYDVQQKEwlhYWFhYWFhYWExHTAbBgNVBAMTFGFhYWFhYWFhYWFhYWFhYWFhYWFh";
+    certList.revokeCertByIssuerAndSerial(issuer2UTF8String, serial);
+    ok(test_is_revoked(certList, atob(issuer2PrintableString), atob(serial)),
+       "issuer / serial should be blocked despite different encoding");
+
+    // next revoke a UTF8String, try a teletext string (this should fail)
+    let issuer3TeletextString = "MEAxCzAJBgNVBAYTAkZSMRIwEAYDVQQKFAlhYWFhYWFhYWExHTAbBgNVBAMUFGFhYWFhYWFhYWFhYWFhYWFhYWFh";
+    ok(!test_is_revoked(certList, atob(issuer3TeletextString), atob(serial)),
+       "issuer / serial should not be blocked");
+
+    // Revoke and check DNs which are sub / supersets of the RDNs in the revoked DNs
+    let issuer4SupersetString = "MDExCzAJBgNVBAYTAlhYMRMwEQYDVQQIEwpTb21lLVN0YXRlMQ0wCwYDVQQHEwRDaXR5";
+    let issuer5SubsetString = "MCIxCzAJBgNVBAYTAlhYMRMwEQYDVQQIEwpTb21lLVN0YXRl";
+    certList.revokeCertByIssuerAndSerial(issuer4SupersetString, serial);
+    ok(!test_is_revoked(certList, atob(issuer5SubsetString), atob(serial)),
+       "issuer / serial should not be blocked");
+
+    let issuer6SubsetString = "MCgxCzAJBgNVBAYTAllZMRkwFwYDVQQIExBTb21lLU90aGVyLVN0YXRl";
+    let issuer7SupersetString = "MD0xCzAJBgNVBAYTAllZMRkwFwYDVQQIExBTb21lLU90aGVyLVN0YXRlMRMwEQYDVQQHEwpPdGhlci1DaXR5";
+    certList.revokeCertByIssuerAndSerial(issuer6SubsetString, serial);
+    ok(!test_is_revoked(certList, atob(issuer7SupersetString), atob(serial)),
+       "issuer / serial should not be blocked");
+
+    run_next_test();
+  });
+
   run_next_test();
 }
--- a/security/pkix/include/pkix/pkix.h
+++ b/security/pkix/include/pkix/pkix.h
@@ -114,16 +114,23 @@ Result BuildCertChain(TrustDomain& trust
 // validated with BuildCertChain, is valid for the given hostname. The matching
 // function attempts to implement RFC 6125 with a couple of differences:
 // - IP addresses are out of scope of RFC 6125, but this method accepts them for
 //   backward compatibility (see SearchNames in pkixnames.cpp)
 // - A wildcard in a DNS-ID may only appear as the entirety of the first label.
 Result CheckCertHostname(Input cert, Input hostname,
                          NameMatchingPolicy& nameMatchingPolicy);
 
+Result
+MatchDirectoryNamePrefix(Input directoryName,
+                         Input directoryNamePrefix,
+                         /*out*/ size_t& directoryNameCount,
+                         /*out*/ size_t& directoryNamePrefixCount,
+                         /*out*/ bool& matches);
+
 // Construct an RFC-6960-encoded OCSP request, ready for submission to a
 // responder, for the provided CertID. The request has no extensions.
 static const size_t OCSP_REQUEST_MAX_LENGTH = 127;
 Result CreateEncodedOCSPRequest(TrustDomain& trustDomain,
                                 const CertID& certID,
                                 /*out*/ uint8_t (&out)[OCSP_REQUEST_MAX_LENGTH],
                                 /*out*/ size_t& outLen);
 
@@ -150,12 +157,11 @@ Result VerifyEncodedOCSPResponse(TrustDo
 
 // Check that the TLSFeature extensions in a given end-entity cert (which is
 // assumed to have been already validated with BuildCertChain) are satisfied.
 // The only feature which we cancurrently process a requirement for is
 // status_request (OCSP stapling) so we reject any extension that specifies a
 // requirement for another value. Empty extensions are also rejected.
 Result CheckTLSFeaturesAreSatisfied(Input& cert,
                                     const Input* stapledOCSPResponse);
-
 } } // namespace mozilla::pkix
 
 #endif // mozilla_pkix_pkix_h
--- a/security/pkix/lib/pkixnames.cpp
+++ b/security/pkix/lib/pkixnames.cpp
@@ -31,16 +31,17 @@
 // one in the subjectAltName of the certificate, or sometimes within a CN of
 // the certificate's subject. The "reference identifier" is the one we are
 // being asked to match the certificate against. When checking name
 // constraints, the reference identifier is the entire encoded name constraint
 // extension value.
 
 #include "pkixcheck.h"
 #include "pkixutil.h"
+#include "pkix/pkix.h"
 
 namespace mozilla { namespace pkix {
 
 namespace {
 
 // GeneralName ::= CHOICE {
 //      otherName                       [0]     OtherName,
 //      rfc822Name                      [1]     IA5String,
@@ -1345,98 +1346,31 @@ ReadAVA(Reader& rdn,
 // DirectoryName constraints in excludedSubtrees so that a CA can say "Do not
 // allow any DirectoryNames in issued certificates."
 Result
 MatchPresentedDirectoryNameWithConstraint(NameConstraintsSubtrees subtreesType,
                                           Input presentedID,
                                           Input directoryNameConstraint,
                                           /*out*/ bool& matches)
 {
-  Reader constraintRDNs;
-  Result rv = der::ExpectTagAndGetValueAtEnd(directoryNameConstraint,
-                                             der::SEQUENCE, constraintRDNs);
-  if (rv != Success) {
-    return rv;
-  }
-  Reader presentedRDNs;
-  rv = der::ExpectTagAndGetValueAtEnd(presentedID, der::SEQUENCE,
-                                      presentedRDNs);
-  if (rv != Success) {
-    return rv;
-  }
-
-  switch (subtreesType) {
-    case NameConstraintsSubtrees::permittedSubtrees:
-      break; // dealt with below
-    case NameConstraintsSubtrees::excludedSubtrees:
-      if (!constraintRDNs.AtEnd() || !presentedRDNs.AtEnd()) {
-        return Result::ERROR_CERT_NOT_IN_NAME_SPACE;
-      }
-      matches = true;
-      return Success;
-  }
+   size_t presentedIDCount;
+   size_t directoryNameConstraintCount;
 
-  for (;;) {
-    // The AVAs have to be fully equal, but the constraint RDNs just need to be
-    // a prefix of the presented RDNs.
-    if (constraintRDNs.AtEnd()) {
-      matches = true;
-      return Success;
-    }
-    if (presentedRDNs.AtEnd()) {
-      matches = false;
-      return Success;
-    }
-    Reader constraintRDN;
-    rv = der::ExpectTagAndGetValue(constraintRDNs, der::SET, constraintRDN);
-    if (rv != Success) {
-      return rv;
-    }
-    Reader presentedRDN;
-    rv = der::ExpectTagAndGetValue(presentedRDNs, der::SET, presentedRDN);
-    if (rv != Success) {
-      return rv;
-    }
-    while (!constraintRDN.AtEnd() && !presentedRDN.AtEnd()) {
-      Input constraintType;
-      uint8_t constraintValueTag;
-      Input constraintValue;
-      rv = ReadAVA(constraintRDN, constraintType, constraintValueTag,
-                   constraintValue);
-      if (rv != Success) {
-        return rv;
-      }
-      Input presentedType;
-      uint8_t presentedValueTag;
-      Input presentedValue;
-      rv = ReadAVA(presentedRDN, presentedType, presentedValueTag,
-                   presentedValue);
-      if (rv != Success) {
-        return rv;
-      }
-      // TODO (bug 1155767): verify that if an AVA is a PrintableString it
-      // consists only of characters valid for PrintableStrings.
-      bool avasMatch =
-        InputsAreEqual(constraintType, presentedType) &&
-        InputsAreEqual(constraintValue, presentedValue) &&
-        (constraintValueTag == presentedValueTag ||
-         (constraintValueTag == der::Tag::UTF8String &&
-          presentedValueTag == der::Tag::PrintableString) ||
-         (constraintValueTag == der::Tag::PrintableString &&
-          presentedValueTag == der::Tag::UTF8String));
-      if (!avasMatch) {
-        matches = false;
-        return Success;
-      }
-    }
-    if (!constraintRDN.AtEnd() || !presentedRDN.AtEnd()) {
-      matches = false;
-      return Success;
-    }
+   Result rv = MatchDirectoryNamePrefix(presentedID, directoryNameConstraint,
+                                        presentedIDCount,
+                                        directoryNameConstraintCount, matches);
+   if (rv != Success) {
+     return rv;
+   }
+
+   if (subtreesType == NameConstraintsSubtrees::excludedSubtrees &&
+      (directoryNameConstraintCount != 0 || presentedIDCount != 0)) {
+      return Result::ERROR_CERT_NOT_IN_NAME_SPACE;
   }
+  return Success;
 }
 
 // RFC 5280 says:
 //
 //     The format of an rfc822Name is a "Mailbox" as defined in Section 4.1.2
 //     of [RFC2821]. A Mailbox has the form "Local-part@Domain".  Note that a
 //     Mailbox has no phrase (such as a common name) before it, has no comment
 //     (text surrounded in parentheses) after it, and is not surrounded by "<"
@@ -1454,16 +1388,131 @@ MatchPresentedDirectoryNameWithConstrain
 //     particular host, the constraint is specified as the host name.  For
 //     example, the constraint "example.com" is satisfied by any mail
 //     address at the host "example.com".  To specify any address within a
 //     domain, the constraint is specified with a leading period (as with
 //     URIs).  For example, ".example.com" indicates all the Internet mail
 //     addresses in the domain "example.com", but not Internet mail
 //     addresses on the host "example.com".
 
+} // namespace
+
+Result
+MatchDirectoryNamePrefix(Input directoryName,
+                         Input directoryNamePrefix,
+                         /*out*/ size_t& directoryNameCount,
+                         /*out*/ size_t& directoryNamePrefixCount,
+                         /*out*/ bool& matches)
+{
+  directoryNameCount = 0;
+  directoryNamePrefixCount = 0;
+
+  Reader prefixRDNs;
+  Result rv = der::ExpectTagAndGetValueAtEnd(directoryNamePrefix,
+                                             der::SEQUENCE, prefixRDNs);
+  if (rv != Success) {
+    return rv;
+  }
+  Reader directoryNameRDNs;
+  rv = der::ExpectTagAndGetValueAtEnd(directoryName, der::SEQUENCE,
+                                      directoryNameRDNs);
+  if (rv != Success) {
+    return rv;
+  }
+
+  for (;;) {
+    // The AVAs have to be fully equal, but the prefix RDNs just need to be
+    // a prefix of the presented RDNs.
+    Reader directoryNameRDN;
+    if (prefixRDNs.AtEnd()) {
+      while (!directoryNameRDNs.AtEnd()) {
+        rv = der::ExpectTagAndGetValue(directoryNameRDNs, der::SET, directoryNameRDN);
+        directoryNameCount++;
+        if (rv != Success) {
+          return rv;
+        }
+      }
+      matches = true;
+      return Success;
+    }
+
+    Reader prefixRDN;
+    if (directoryNameRDNs.AtEnd()) {
+      while (!prefixRDNs.AtEnd()) {
+        rv = der::ExpectTagAndGetValue(prefixRDNs, der::SET, prefixRDN);
+        directoryNamePrefixCount++;
+        if (rv != Success) {
+          return rv;
+        }
+      }
+      matches = false;
+      return Success;
+    }
+
+    rv = der::ExpectTagAndGetValue(prefixRDNs, der::SET, prefixRDN);
+    directoryNamePrefixCount++;
+    if (rv != Success) {
+      return rv;
+    }
+    rv = der::ExpectTagAndGetValue(directoryNameRDNs, der::SET, directoryNameRDN);
+    directoryNameCount++;
+    if (rv != Success) {
+      return rv;
+    }
+
+    while (!prefixRDN.AtEnd() && !directoryNameRDN.AtEnd()) {
+      Input prefixType;
+      uint8_t prefixValueTag;
+      Input prefixValue;
+      rv = ReadAVA(prefixRDN, prefixType, prefixValueTag,
+                   prefixValue);
+      if (rv != Success) {
+        return rv;
+      }
+      Input directoryNameType;
+      uint8_t directoryNameValueTag;
+      Input directoryNameValue;
+      rv = ReadAVA(directoryNameRDN, directoryNameType, directoryNameValueTag,
+                   directoryNameValue);
+      if (rv != Success) {
+        return rv;
+      }
+
+      // In RFCs 2459, 3280 and 5280, attribute values encoded in different
+      // types may be assumed to represent different strings, however,
+      // implementations are permitted to use X.500 comparison rules (e.g. see
+      // RFC 2459 4.1.2.4). Some implementations have done this and, as a
+      // result, there exist CRLs for which the CRL issuer and the cert issuer
+      // differ only in encodings. Because of this, we must map between
+      //
+      // TODO (bug 1155767): verify that if an AVA is a PrintableString it
+      // consists only of characters valid for PrintableStrings.
+      bool avasMatch =
+        InputsAreEqual(prefixType, directoryNameType) &&
+        InputsAreEqual(prefixValue, directoryNameValue) &&
+        (prefixValueTag == directoryNameValueTag ||
+         (prefixValueTag == der::Tag::UTF8String &&
+          directoryNameValueTag == der::Tag::PrintableString) ||
+         (prefixValueTag == der::Tag::PrintableString &&
+          directoryNameValueTag == der::Tag::UTF8String));
+      if (!avasMatch) {
+        matches = false;
+        return Success;
+      }
+    }
+
+    if (!prefixRDN.AtEnd() || !directoryNameRDN.AtEnd()) {
+      matches = false;
+      return Success;
+    }
+  }
+}
+
+namespace {
+
 bool
 IsValidRFC822Name(Input input)
 {
   Reader reader(input);
 
   // Local-part@.
   bool startOfAtom = true;
   for (;;) {