Bug 1063281, Part 8: Rewrite PresentedDNSIDMatchesReferenceDNSID, r=keeler
authorBrian Smith <brian@briansmith.org>
Wed, 15 Oct 2014 19:21:35 -0700
changeset 212047 fcd878ebe03e8cb4103f7ea5eb3c6cfb4ba5eb7a
parent 212046 15f27345d25982d0c5559b53263acdeb36180b8b
child 212048 9b72d139e81766bdcf363c7b9ed0bf3f248c32d2
push id27697
push usercbook@mozilla.com
push dateFri, 24 Oct 2014 13:48:53 +0000
treeherdermozilla-central@de805196bbc4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1063281
milestone36.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 1063281, Part 8: Rewrite PresentedDNSIDMatchesReferenceDNSID, r=keeler
security/pkix/lib/pkixnames.cpp
security/pkix/test/gtest/pkixnames_tests.cpp
--- a/security/pkix/lib/pkixnames.cpp
+++ b/security/pkix/lib/pkixnames.cpp
@@ -92,18 +92,17 @@ bool IsValidDNSID(Input hostname, ValidD
 
 } // unnamed namespace
 
 bool IsValidReferenceDNSID(Input hostname);
 bool IsValidPresentedDNSID(Input hostname);
 bool ParseIPv4Address(Input hostname, /*out*/ uint8_t (&out)[4]);
 bool ParseIPv6Address(Input hostname, /*out*/ uint8_t (&out)[16]);
 bool PresentedDNSIDMatchesReferenceDNSID(Input presentedDNSID,
-                                         Input referenceDNSID,
-                                         bool referenceDNSIDWasVerifiedAsValid);
+                                         Input referenceDNSID);
 
 // Verify that the given end-entity cert, which is assumed to have been already
 // validated with BuildCertChain, is valid for the given hostname. hostname is
 // assumed to be a string representation of an IPv4 address, an IPv6 addresss,
 // or a normalized ASCII (possibly punycode) DNS name.
 Result
 CheckCertHostname(Input endEntityCertDER, Input hostname)
 {
@@ -396,17 +395,17 @@ SearchWithinAVA(Reader& rdn,
       valueEncodingTag != der::UTF8String) {
     return Success;
   }
 
   switch (referenceIDType)
   {
     case GeneralNameType::dNSName:
       foundMatch = PresentedDNSIDMatchesReferenceDNSID(presentedID,
-                                                       referenceID, true);
+                                                       referenceID);
       break;
     case GeneralNameType::iPAddress:
     {
       // We don't fall back to matching CN-IDs for IPv6 addresses, so we'll
       // never get here for an IPv6 address.
       assert(referenceID.GetLength() == 4);
       uint8_t ipv4[4];
       foundMatch = ParseIPv4Address(presentedID, ipv4) &&
@@ -427,17 +426,17 @@ MatchPresentedIDWithReferenceID(GeneralN
                                 Input referenceID,
                                 /*out*/ bool& foundMatch)
 {
   foundMatch = false;
 
   switch (nameType) {
     case GeneralNameType::dNSName:
       foundMatch = PresentedDNSIDMatchesReferenceDNSID(presentedID,
-                                                       referenceID, true);
+                                                       referenceID);
       break;
     case GeneralNameType::iPAddress:
       foundMatch = InputsAreEqual(presentedID, referenceID);
       break;
     default:
       return NotReached("Invalid nameType for SearchType::CheckName",
                         Result::FATAL_ERROR_INVALID_ARGS);
   }
@@ -450,148 +449,98 @@ MatchPresentedIDWithReferenceID(GeneralN
 // that is syntactically valid but does not match referenceDNSID; in both
 // cases, the result is false.
 //
 // We assume that both presentedDNSID and referenceDNSID are encoded in such a
 // way that US-ASCII (7-bit) characters are encoded in one byte and no encoding
 // of a non-US-ASCII character contains a code point in the range 0-127. For
 // example, UTF-8 is OK but UTF-16 is not.
 //
-// The referenceDNSIDWasVerifiedAsValid parameter must be the result of calling
-// IsValidReferenceDNSID.
-//
 // RFC6125 says that a wildcard label may be of the form <x>*<y>.<DNSID>, where
 // <x> and/or <y> may be empty. However, NSS requires <y> to be empty, and we
 // follow NSS's stricter policy by accepting wildcards only of the form
 // <x>*.<DNSID>, where <x> may be empty.
 bool
-PresentedDNSIDMatchesReferenceDNSID(Input presentedDNSID,
-                                    Input referenceDNSID,
-                                    bool referenceDNSIDWasVerifiedAsValid)
+PresentedDNSIDMatchesReferenceDNSID(Input presentedDNSID, Input referenceDNSID)
 {
-  assert(referenceDNSIDWasVerifiedAsValid);
-  if (!referenceDNSIDWasVerifiedAsValid) {
+  if (!IsValidPresentedDNSID(presentedDNSID)) {
+    return false;
+  }
+  if (!IsValidReferenceDNSID(referenceDNSID)) {
     return false;
   }
 
   Reader presented(presentedDNSID);
   Reader reference(referenceDNSID);
-
-  size_t currentLabel = 0;
-  bool hasWildcardLabel = false;
-  bool lastPresentedByteWasDot = false;
-  bool firstPresentedByteIsWildcard = presented.Peek('*');
-
+  bool isFirstPresentedByte = true;
   do {
     uint8_t presentedByte;
-    if (presented.Read(presentedByte) != Success) {
-      return false; // Reject completely empty input.
+    Result rv = presented.Read(presentedByte);
+    if (rv != Success) {
+      return false;
     }
-    if (presentedByte == '*' && currentLabel == 0) {
-      hasWildcardLabel = true;
+    if (presentedByte == '*') {
+      // RFC 6125 is unclear about whether "www*.example.org" matches
+      // "www.example.org". The Chromium test suite has this test:
+      //
+      //    { false, "w.bar.foo.com", "w*.bar.foo.com" },
+      //
+      // We agree with Chromium by forbidding "*" from expanding to the empty
+      // string.
+      do {
+        uint8_t referenceByte;
+        rv = reference.Read(referenceByte);
+        if (rv != Success) {
+          return false;
+        }
+      } while (!reference.Peek('.'));
 
-      // Like NSS, be stricter than RFC6125 requires by insisting that the
-      // "*" must be the last character in the label. This also prevents
-      // multiple "*" in the label.
-      if (!presented.Peek('.')) {
-        return false;
-      }
-
-      // RFC6125 says that we shouldn't accept wildcards within an IDN A-Label.
-      //
       // We also don't allow a non-IDN presented ID label to match an IDN
       // reference ID label, except when the entire presented ID label is "*".
       // This avoids confusion when matching a presented ID like
       // "xn-*.example.org" against "xn--www.example.org" (which attempts to
       // abuse the punycode syntax) or "www-*.example.org" against
       // "xn--www--ep4c4a2kpf" (which makes sense to match, semantically, but
       // no implementations actually do).
-      //
-      // XXX: The consequence of this is that we effectively discriminate
-      // against users of languages that cannot be encoded with ASCII.
-      if (!firstPresentedByteIsWildcard) {
-        if (StartsWithIDNALabel(presentedDNSID)) {
-          return false;
-        }
-        if (StartsWithIDNALabel(referenceDNSID)) {
-          return false;
-        }
-      }
-
-      // RFC 6125 is unclear about whether "www*.example.org" matches
-      // "www.example.org". The Chromium test suite has this test:
-      //
-      //    { false, "w.bar.foo.com", "w*.bar.foo.com" },
-      //
-      // We agree with Chromium by forbidding "*" from expanding to the empty
-      // string.
-      uint8_t referenceByte;
-      if (reference.Read(referenceByte) != Success) {
+      if (!isFirstPresentedByte && StartsWithIDNALabel(referenceDNSID)) {
         return false;
       }
-      if (referenceByte == '.') {
-        return false;
-      }
-      while (!reference.Peek('.')) {
-        if (reference.Read(referenceByte) != Success) {
-          return false;
-        }
-      }
     } else {
-      if (presentedByte == '.') {
-        // This check is needed to prevent ".." at the end of the presented ID
-        // from being accepted.
-        if (lastPresentedByteWasDot) {
-          return false;
-        }
-        lastPresentedByteWasDot = true;
-
-        if (!presented.AtEnd()) {
-          ++currentLabel;
-        }
-      } else {
-        lastPresentedByteWasDot = false;
+      // Allow an absolute presented DNS ID to match a relative reference DNS
+      // ID.
+      if (reference.AtEnd() && presented.AtEnd() && presentedByte == '.') {
+        return true;
       }
 
-      // The presented ID may have a terminating dot '.' to mark it as
-      // absolute, and it still matches a reference ID without that
-      // terminating dot.
-      if (presentedByte != '.' || !presented.AtEnd() || !reference.AtEnd()) {
-        uint8_t referenceByte;
-        if (reference.Read(referenceByte) != Success) {
-          return false;
-        }
-        if (LocaleInsensitveToLower(presentedByte) !=
-            LocaleInsensitveToLower(referenceByte)) {
-          return false;
-        }
+      uint8_t referenceByte;
+      rv = reference.Read(referenceByte);
+      if (rv != Success) {
+        return false;
+      }
+      if (LocaleInsensitveToLower(presentedByte) !=
+          LocaleInsensitveToLower(referenceByte)) {
+        return false;
       }
     }
+    isFirstPresentedByte = false;
   } while (!presented.AtEnd());
 
-  // The reference ID may have a terminating dot '.' to mark it as absolute,
-  // and a presented ID without that terminating dot still matches it.
-  static const uint8_t DOT[1] = { '.' };
-  if (!reference.AtEnd() && !reference.MatchRest(DOT)) {
-    return false;
-  }
-
-  if (hasWildcardLabel) {
-    // Like NSS, we require at least two labels after the wildcard.
-    if (currentLabel < 2) {
+  // Allow a relative presented DNS ID to match an absolute reference DNS ID.
+  if (!reference.AtEnd()) {
+    uint8_t referenceByte;
+    Result rv = reference.Read(referenceByte);
+    if (rv != Success) {
       return false;
     }
-
-    // TODO(bug XXXXXXX): Allow the TrustDomain to control this on a
-    // per-eTLD+1 basis, similar to Chromium. Even then, it might be better to
-    // still enforce that there are at least two labels after the wildcard.
-
-    // TODO(bug XXXXXXX): Wildcards are not allowed for EV certificates.
-    // Provide an option to indicate whether wildcards should be matched, for
-    // the purpose of helping the application enforce this.
+    if (referenceByte != '.') {
+      return false;
+    }
+    if (!reference.AtEnd()) {
+      return false;
+    }
   }
 
   return true;
 }
 
 namespace {
 
 // We avoid isdigit because it is locale-sensitive. See
--- a/security/pkix/test/gtest/pkixnames_tests.cpp
+++ b/security/pkix/test/gtest/pkixnames_tests.cpp
@@ -23,18 +23,17 @@
  */
 #include "pkix/pkix.h"
 #include "pkixgtest.h"
 #include "pkixtestutil.h"
 
 namespace mozilla { namespace pkix {
 
 bool PresentedDNSIDMatchesReferenceDNSID(Input presentedDNSID,
-                                         Input referenceDNSID,
-                                         bool referenceDNSIDWasVerifiedAsValid);
+                                         Input referenceDNSID);
 
 bool IsValidReferenceDNSID(Input hostname);
 bool IsValidPresentedDNSID(Input hostname);
 bool ParseIPv4Address(Input hostname, /*out*/ uint8_t (&out)[4]);
 bool ParseIPv6Address(Input hostname, /*out*/ uint8_t (&out)[16]);
 
 } } // namespace mozilla::pkix
 
@@ -859,21 +858,22 @@ TEST_P(pkixnames_PresentedDNSIDMatchesRe
   SCOPED_TRACE(param.presentedDNSID.c_str());
   SCOPED_TRACE(param.referenceDNSID.c_str());
   Input presented;
   ASSERT_EQ(Success, presented.Init(param.presentedDNSID.data(),
                                     param.presentedDNSID.length()));
   Input reference;
   ASSERT_EQ(Success, reference.Init(param.referenceDNSID.data(),
                                     param.referenceDNSID.length()));
-  bool referenceIsValidReferenceDNSID = IsValidReferenceDNSID(reference);
-  ASSERT_TRUE(referenceIsValidReferenceDNSID); // sanity check that test makes sense
+
+  // sanity check that test makes sense
+  ASSERT_TRUE(IsValidReferenceDNSID(reference));
+
   ASSERT_EQ(param.matches,
-            PresentedDNSIDMatchesReferenceDNSID(presented, reference,
-                                                referenceIsValidReferenceDNSID));
+            PresentedDNSIDMatchesReferenceDNSID(presented, reference));
 }
 
 INSTANTIATE_TEST_CASE_P(pkixnames_PresentedDNSIDMatchesReferenceDNSID,
                         pkixnames_PresentedDNSIDMatchesReferenceDNSID,
                         testing::ValuesIn(DNSID_MATCH_PARAMS));
 
 class pkixnames_Turkish_I_Comparison
   : public ::testing::Test
@@ -888,20 +888,18 @@ TEST_P(pkixnames_Turkish_I_Comparison, P
 
   const InputValidity& inputValidity(GetParam());
   SCOPED_TRACE(inputValidity.input.c_str());
   Input input;
   ASSERT_EQ(Success, input.Init(inputValidity.input.data(),
                                 inputValidity.input.length()));
   bool isASCII = InputsAreEqual(LOWERCASE_I, input) ||
                  InputsAreEqual(UPPERCASE_I, input);
-  ASSERT_EQ(isASCII, PresentedDNSIDMatchesReferenceDNSID(input, LOWERCASE_I,
-                                                         true));
-  ASSERT_EQ(isASCII, PresentedDNSIDMatchesReferenceDNSID(input, UPPERCASE_I,
-                                                         true));
+  ASSERT_EQ(isASCII, PresentedDNSIDMatchesReferenceDNSID(input, LOWERCASE_I));
+  ASSERT_EQ(isASCII, PresentedDNSIDMatchesReferenceDNSID(input, UPPERCASE_I));
 }
 
 INSTANTIATE_TEST_CASE_P(pkixnames_Turkish_I_Comparison,
                         pkixnames_Turkish_I_Comparison,
                         testing::ValuesIn(DNSNAMES_VALIDITY_TURKISH_I));
 
 class pkixnames_IsValidReferenceDNSID
   : public ::testing::Test