Bug 1107791 Remove support for unusual wildcard names in certificates, r=keeler
authorBrian Smith <brian@briansmith.org>
Thu, 04 Dec 2014 17:12:09 -0800
changeset 219058 4530482cc605262eb598f4170ee7e35b3ae8d080
parent 219057 cfe200a463ab938dea48151f82c5534f146b96b7
child 219059 c1accea055e5e1a6e7accf5feb6c9683eee8eb2d
push id27954
push userryanvm@gmail.com
push dateWed, 10 Dec 2014 21:10:24 +0000
treeherdermozilla-central@0cf461e62ce5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1107791
milestone37.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 1107791 Remove support for unusual wildcard names in certificates, 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
@@ -1038,66 +1038,51 @@ PresentedDNSIDMatchesReferenceDNSID(
     }
 
     case ValidDNSIDMatchType::PresentedID: // fall through
     default:
       assert(false);
       return false;
   }
 
-  bool isFirstPresentedByte = true;
+  // We only allow wildcard labels that consist only of '*'.
+  if (presented.Peek('*')) {
+    Result rv = presented.Skip(1);
+    if (rv != Success) {
+      assert(false);
+      return false;
+    }
+    do {
+      uint8_t referenceByte;
+      if (reference.Read(referenceByte) != Success) {
+        return false;
+      }
+    } while (!reference.Peek('.'));
+  }
+
   for (;;) {
     uint8_t presentedByte;
     if (presented.Read(presentedByte) != Success) {
       return false;
     }
-    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;
-        if (reference.Read(referenceByte) != Success) {
-          return false;
-        }
-      } while (!reference.Peek('.'));
-
-      // 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).
-      if (!isFirstPresentedByte && StartsWithIDNALabel(referenceDNSID)) {
+    uint8_t referenceByte;
+    if (reference.Read(referenceByte) != Success) {
+      return false;
+    }
+    if (LocaleInsensitveToLower(presentedByte) !=
+        LocaleInsensitveToLower(referenceByte)) {
+      return false;
+    }
+    if (presented.AtEnd()) {
+      // Don't allow presented IDs to be absolute.
+      if (presentedByte == '.') {
         return false;
       }
-    } else {
-      uint8_t referenceByte;
-      if (reference.Read(referenceByte) != Success) {
-        return false;
-      }
-      if (LocaleInsensitveToLower(presentedByte) !=
-          LocaleInsensitveToLower(referenceByte)) {
-        return false;
-      }
-
-      if (presented.AtEnd()) {
-        // Don't allow presented IDs to be absolute.
-        if (presentedByte == '.') {
-          return false;
-        }
-        break;
-      }
+      break;
     }
-    isFirstPresentedByte = false;
   }
 
   // Allow a relative presented DNS ID to match an absolute reference DNS ID,
   // unless we're matching a name constraint.
   if (!reference.AtEnd()) {
     if (referenceDNSIDMatchType != ValidDNSIDMatchType::NameConstraint) {
       uint8_t referenceByte;
       if (reference.Read(referenceByte) != Success) {
@@ -1574,42 +1559,52 @@ IsValidDNSID(Input hostname, ValidDNSIDM
   }
 
   Reader input(hostname);
 
   if (matchType == ValidDNSIDMatchType::NameConstraint && input.AtEnd()) {
     return true;
   }
 
-  bool allowWildcard = matchType == ValidDNSIDMatchType::PresentedID;
-  bool isWildcard = false;
   size_t dotCount = 0;
-
   size_t labelLength = 0;
   bool labelIsAllNumeric = false;
-  bool labelIsWildcard = false;
   bool labelEndsWithHyphen = false;
 
-  bool isFirstByte = true;
+  // Only presented IDs are allowed to have wildcard labels. And, like
+  // Chromium, be stricter than RFC 6125 requires by insisting that a
+  // wildcard label consist only of '*'.
+  bool isWildcard = matchType == ValidDNSIDMatchType::PresentedID &&
+                    input.Peek('*');
+  bool isFirstByte = !isWildcard;
+  if (isWildcard) {
+    Result rv = input.Skip(1);
+    if (rv != Success) {
+      assert(false);
+      return false;
+    }
+
+    uint8_t b;
+    rv = input.Read(b);
+    if (rv != Success) {
+      return false;
+    }
+    if (b != '.') {
+      return false;
+    }
+    ++dotCount;
+  }
 
   do {
     static const size_t MAX_LABEL_LENGTH = 63;
 
     uint8_t b;
     if (input.Read(b) != Success) {
       return false;
     }
-    if (labelIsWildcard) {
-      // 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 (b != '.') {
-        return false;
-      }
-    }
     switch (b) {
       case '-':
         if (labelLength == 0) {
           return false; // Labels must not start with a hyphen.
         }
         labelIsAllNumeric = false;
         labelEndsWithHyphen = true;
         ++labelLength;
@@ -1654,42 +1649,26 @@ IsValidDNSID(Input hostname, ValidDNSIDM
         labelIsAllNumeric = false;
         labelEndsWithHyphen = false;
         ++labelLength;
         if (labelLength > MAX_LABEL_LENGTH) {
           return false;
         }
         break;
 
-      case '*':
-        if (!allowWildcard) {
-          return false;
-        }
-        labelIsWildcard = true;
-        isWildcard = true;
-        labelIsAllNumeric = false;
-        labelEndsWithHyphen = false;
-        ++labelLength;
-        if (labelLength > MAX_LABEL_LENGTH) {
-          return false;
-        }
-        break;
-
       case '.':
         ++dotCount;
         if (labelLength == 0 &&
             (matchType != ValidDNSIDMatchType::NameConstraint ||
              !isFirstByte)) {
           return false;
         }
         if (labelEndsWithHyphen) {
           return false; // Labels must not end with a hyphen.
         }
-        allowWildcard = false; // only allowed in the first label.
-        labelIsWildcard = false;
         labelLength = 0;
         break;
 
       default:
         return false; // Invalid character.
     }
     isFirstByte = false;
   } while (!input.AtEnd());
--- a/security/pkix/test/gtest/pkixnames_tests.cpp
+++ b/security/pkix/test/gtest/pkixnames_tests.cpp
@@ -99,19 +99,19 @@ static const PresentedMatchesReference D
   DNS_ID_MISMATCH("example.com.", "example.com."),
   DNS_ID_MATCH("example.com", "example.com."),
   DNS_ID_MISMATCH("example.com.", "example.com"),
   DNS_ID_MISMATCH("example.com..", "example.com."),
   DNS_ID_MISMATCH("example.com..", "example.com"),
   DNS_ID_MISMATCH("example.com...", "example.com."),
 
   // xn-- IDN prefix
-  DNS_ID_MATCH("x*.b.a", "xa.b.a"),
-  DNS_ID_MATCH("x*.b.a", "xna.b.a"),
-  DNS_ID_MATCH("x*.b.a", "xn-a.b.a"),
+  DNS_ID_MISMATCH("x*.b.a", "xa.b.a"),
+  DNS_ID_MISMATCH("x*.b.a", "xna.b.a"),
+  DNS_ID_MISMATCH("x*.b.a", "xn-a.b.a"),
   DNS_ID_MISMATCH("x*.b.a", "xn--a.b.a"),
   DNS_ID_MISMATCH("xn*.b.a", "xn--a.b.a"),
   DNS_ID_MISMATCH("xn-*.b.a", "xn--a.b.a"),
   DNS_ID_MISMATCH("xn--*.b.a", "xn--a.b.a"),
   DNS_ID_MISMATCH("xn*.b.a", "xn--a.b.a"),
   DNS_ID_MISMATCH("xn-*.b.a", "xn--a.b.a"),
   DNS_ID_MISMATCH("xn--*.b.a", "xn--a.b.a"),
   DNS_ID_MISMATCH("xn---*.b.a", "xn--a.b.a"),
@@ -145,17 +145,18 @@ static const PresentedMatchesReference D
   DNS_ID_MISMATCH("ww*ww.bar.foo.com", "www.bar.foo.com"),
   DNS_ID_MISMATCH("ww*ww.bar.foo.com", "wwww.bar.foo.com"),
 
   // Different than Chromium, matches NSS.
   DNS_ID_MISMATCH("w*w.bar.foo.com", "wwww.bar.foo.com"),
 
   DNS_ID_MISMATCH("w*w.bar.foo.c0m", "wwww.bar.foo.com"),
 
-  DNS_ID_MATCH("wa*.bar.foo.com", "WALLY.bar.foo.com"),
+  // '*' must be the only character in the wildcard label
+  DNS_ID_MISMATCH("wa*.bar.foo.com", "WALLY.bar.foo.com"),
 
   // We require "*" to be the last character in a wildcard label, but
   // Chromium does not.
   DNS_ID_MISMATCH("*Ly.bar.foo.com", "wally.bar.foo.com"),
 
   // Chromium does URL decoding of the reference ID, but we don't, and we also
   // require that the reference ID is valid, so we can't test these two.
   // DNS_ID_MATCH("www.foo.com", "ww%57.foo.com"),
@@ -194,18 +195,19 @@ static const PresentedMatchesReference D
   //   http://tools.ietf.org/html/rfc6125#section-6.4.3
   // (e.g., *.example.com would match foo.example.com but
   // not bar.foo.example.com or example.com).
   DNS_ID_MATCH("*.example.com", "foo.example.com"),
   DNS_ID_MISMATCH("*.example.com", "bar.foo.example.com"),
   DNS_ID_MISMATCH("*.example.com", "example.com"),
   // (e.g., baz*.example.net and *baz.example.net and b*z.example.net would
   // be taken to match baz1.example.net and foobaz.example.net and
-  // buzz.example.net, respectively
-  DNS_ID_MATCH("baz*.example.net", "baz1.example.net"),
+  // buzz.example.net, respectively. However, we don't allow any characters
+  // other than '*' in the wildcard label.
+  DNS_ID_MISMATCH("baz*.example.net", "baz1.example.net"),
 
   // Both of these are different from Chromium, but match NSS, becaues the
   // wildcard character "*" is not the last character of the label.
   DNS_ID_MISMATCH("*baz.example.net", "foobaz.example.net"),
   DNS_ID_MISMATCH("b*z.example.net", "buzz.example.net"),
 
   // Wildcards should not be valid for public registry controlled domains,
   // and unknown/unrecognized domains, at least three domain components must
@@ -430,27 +432,28 @@ static const InputValidity DNSNAMES_VALI
   I("a.a-1", true, true),
 
   // multiple consecutive hyphens allowed
   I("a--1", true, true),
   I("1---a", true, true),
   I("a-----------------b", true, true),
 
   // Wildcard specifications are not valid reference names, but are valid
-  // presented names if there are enough labels
+  // presented names if there are enough labels and if '*' is the only
+  // character in the wildcard label.
   I("*.a", false, false),
   I("a*", false, false),
   I("a*.", false, false),
   I("a*.a", false, false),
   I("a*.a.", false, false),
   I("*.a.b", false, true),
   I("*.a.b.", false, false),
-  I("a*.b.c", false, true),
+  I("a*.b.c", false, false),
   I("*.a.b.c", false, true),
-  I("a*.b.c.d", false, true),
+  I("a*.b.c.d", false, false),
 
   // Multiple wildcards are not allowed.
   I("a**.b.c", false, false),
   I("a*b*.c.d", false, false),
   I("a*.b*.c", false, false),
 
   // Wildcards are only allowed in the first label.
   I("a.*", false, false),
@@ -461,19 +464,19 @@ static const InputValidity DNSNAMES_VALI
   I(".*.a.b", false, false),
   I(".a*.b.c", false, false),
 
   // Wildcards must be at the *end* of the first label.
   I("*a.b.c", false, false),
   I("a*b.c.d", false, false),
 
   // Wildcards not allowed with IDNA prefix
-  I("x*.a.b", false, true),
-  I("xn*.a.b", false, true),
-  I("xn-*.a.b", false, true),
+  I("x*.a.b", false, false),
+  I("xn*.a.b", false, false),
+  I("xn-*.a.b", false, false),
   I("xn--*.a.b", false, false),
   I("xn--w*.a.b", false, false),
 
   // Redacted labels from RFC6962bis draft 4
   // https://tools.ietf.org/html/draft-ietf-trans-rfc6962-bis-04#section-3.2.2
   I("(PRIVATE).foo", false, false),
 
   // maximum label length is 63 characters