Bug 1107790: Remove support for absolute hostnames in presented DNS IDs and name constraints, r=keeler
authorBrian Smith <brian@briansmith.org>
Mon, 08 Dec 2014 16:42:54 -0800
changeset 219057 cfe200a463ab938dea48151f82c5534f146b96b7
parent 219056 526bb391ba63f5c567f8c6f03f18c2e3c20d3d55
child 219058 4530482cc605262eb598f4170ee7e35b3ae8d080
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
bugs1107790
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 1107790: Remove support for absolute hostnames in presented DNS IDs and name constraints, 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
@@ -752,16 +752,22 @@ CheckPresentedIDConformsToNameConstraint
 
     if (presentedIDType == nameConstraintType) {
       bool matches;
 
       switch (presentedIDType) {
         case GeneralNameType::dNSName:
           matches = PresentedDNSIDMatchesReferenceDNSID(
                       presentedID, ValidDNSIDMatchType::NameConstraint, base);
+          // If matches is not false, then base must be syntactically valid
+          // because PresentedDNSIDMatchesReferenceDNSID verifies that.
+          if (!matches &&
+              !IsValidDNSID(base, ValidDNSIDMatchType::NameConstraint)) {
+            return Result::ERROR_CERT_NOT_IN_NAME_SPACE;
+          }
           break;
 
         case GeneralNameType::iPAddress:
           rv = MatchPresentedIPAddressWithConstraint(presentedID, base,
                                                      matches);
           if (rv != Success) {
             return rv;
           }
@@ -841,25 +847,25 @@ CheckPresentedIDConformsToNameConstraint
 // 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.
 //
 // 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.
 //
-// An absolute presented DNS ID matches an absolute reference ID and a relative
-// reference ID, and vice-versa. For example, all of these are matches:
+// An relative presented DNS ID matches both an absolute reference ID and a
+// relative reference ID. Absolute presented DNS IDs are not supported:
 //
-//      Presented ID   Reference ID
-//      ---------------------------
-//      example.com    example.com
-//      example.com.   example.com
-//      example.com    example.com.
-//      example.com.   exmaple.com.
+//      Presented ID   Reference ID  Result
+//      -------------------------------------
+//      example.com    example.com   Match
+//      example.com.   example.com   Mismatch
+//      example.com    example.com.  Match
+//      example.com.   example.com.  Mismatch
 //
 // There are more subtleties documented inline in the code.
 //
 // Name constraints ///////////////////////////////////////////////////////////
 //
 // This is all RFC 5280 has to say about DNSName constraints:
 //
 //     DNS name restrictions are expressed as host.example.com.  Any DNS
@@ -924,32 +930,26 @@ CheckPresentedIDConformsToNameConstraint
 //
 //        However, it can be done with a combination of permittedSubtrees and
 //        excludedSubtrees, e.g. "example.com" in permittedSubtrees and
 //        ".example.com" in excudedSubtrees.
 //
 //     Q: Are name constraints allowed to be specified as absolute names?
 //        For example, does a presented ID of "example.com" match a name
 //        constraint of "example.com." and vice versa.
-//     A: Relative DNSNames match relative DNSName constraints but not
-//        absolute DNSName constraints. Absolute DNSNames match absolute
-//        DNSName constraints but not relative DNSName constraints (except "";
-//        see below). This follows from the requirement that matching DNSNames
-//        are constructed "by simply adding zero or more labels to the
-//        left-hand side" of the constraint.
+//     A: Absolute names are not supported as presented IDs or name
+//        constraints. Only reference IDs may be absolute.
 //
-//     Q: Are "" and "." valid DNSName constraints? If so, what do they mean?
-//     A: Yes, both are valid. All relative and absolute DNSNames match
-//        a constraint of "" because any DNSName can be formed "by simply
-//        adding zero or more labels to the left-hand side" of "". In
-//        particular, an excludedSubtrees DNSName constraint of "" forbids all
-//        DNSNames. Only absolute names match a DNSName constraint of ".";
-//        relative DNSNames do not match "." because one cannot form a relative
-//        DNSName "by simply adding zero or more labels to the left-hand side"
-//        of "." (all such names would be absolute).
+//     Q: Is "" a valid DNSName constraints? If so, what does it mean?
+//     A: Yes. Any valid presented DNSName can be formed "by simply adding zero
+//        or more labels to the left-hand side" of "". In particular, an
+//        excludedSubtrees DNSName constraint of "" forbids all DNSNames.
+//
+//     Q: Is "." a valid DNSName constraints? If so, what does it mean?
+//     A: No, because absolute names are not allowed (see above).
 //
 // [0] RFC 6265 (Cookies) Domain Matching rules:
 //     http://tools.ietf.org/html/rfc6265#section-5.1.3
 // [1] NSS source code:
 //     https://mxr.mozilla.org/nss/source/lib/certdb/genname.c?rev=2a7348f013cb#1209
 // [2] Description of SChannel's behavior from Microsoft:
 //     http://www.imc.org/ietf-pkix/mail-archive/msg04668.html
 // [3] Proposal to add such support to OpenSSL:
@@ -1039,17 +1039,17 @@ PresentedDNSIDMatchesReferenceDNSID(
 
     case ValidDNSIDMatchType::PresentedID: // fall through
     default:
       assert(false);
       return false;
   }
 
   bool isFirstPresentedByte = true;
-  do {
+  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:
       //
@@ -1070,33 +1070,35 @@ PresentedDNSIDMatchesReferenceDNSID(
       // "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)) {
         return false;
       }
     } else {
-      // Allow an absolute presented DNS ID to match a relative reference DNS
-      // ID.
-      if (reference.AtEnd() && presented.AtEnd() && presentedByte == '.') {
-        return true;
-      }
-
       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;
+      }
     }
     isFirstPresentedByte = false;
-  } while (!presented.AtEnd());
+  }
 
   // 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) {
         return false;
@@ -1687,16 +1689,22 @@ IsValidDNSID(Input hostname, ValidDNSIDM
         break;
 
       default:
         return false; // Invalid character.
     }
     isFirstByte = false;
   } while (!input.AtEnd());
 
+  // Only reference IDs, not presented IDs or name constraints, may be
+  // absolute.
+  if (labelLength == 0 && matchType != ValidDNSIDMatchType::ReferenceID) {
+    return false;
+  }
+
   if (labelEndsWithHyphen) {
     return false; // Labels must not end with a hyphen.
   }
 
   if (labelIsAllNumeric) {
     return false; // Last label must not be all numeric.
   }
 
--- a/security/pkix/test/gtest/pkixnames_tests.cpp
+++ b/security/pkix/test/gtest/pkixnames_tests.cpp
@@ -84,26 +84,26 @@ static const PresentedMatchesReference D
   // case sensitivity
   DNS_ID_MATCH("abcdefghijklmnopqrstuvwxyz", "ABCDEFGHIJKLMNOPQRSTUVWXYZ"),
   DNS_ID_MATCH("ABCDEFGHIJKLMNOPQRSTUVWXYZ", "abcdefghijklmnopqrstuvwxyz"),
   DNS_ID_MATCH("aBc", "Abc"),
 
   // digits
   DNS_ID_MATCH("a1", "a1"),
 
-  // A trailing dot indicates an absolute name, and absolute names can match
-  // relative names, and vice-versa.
+  // A trailing dot indicates an absolute name. Absolute presented names are
+  // not allowed, but absolute reference names are allowed.
   DNS_ID_MATCH("example", "example"),
-  DNS_ID_MATCH("example.", "example."),
+  DNS_ID_MISMATCH("example.", "example."),
   DNS_ID_MATCH("example", "example."),
-  DNS_ID_MATCH("example.", "example"),
+  DNS_ID_MISMATCH("example.", "example"),
   DNS_ID_MATCH("example.com", "example.com"),
-  DNS_ID_MATCH("example.com.", "example.com."),
+  DNS_ID_MISMATCH("example.com.", "example.com."),
   DNS_ID_MATCH("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"),
@@ -240,42 +240,45 @@ static const PresentedMatchesReference D
   DNS_ID_MATCH("*.s3.amazonaws.com", "foo.s3.amazonaws.com"),
 
   // Multiple wildcards are not valid.
   DNS_ID_MISMATCH("*.*.com", "foo.example.com"),
   DNS_ID_MISMATCH("*.bar.*.com", "foo.bar.example.com"),
 
   // Absolute vs relative DNS name tests. Although not explicitly specified
   // in RFC 6125, absolute reference names (those ending in a .) should
-  // match either absolute or relative presented names.
+  // match either absolute or relative presented names. We don't allow
+  // absolute presented names.
   // TODO: File errata against RFC 6125 about this.
-  DNS_ID_MATCH("foo.com.", "foo.com"),
+  DNS_ID_MISMATCH("foo.com.", "foo.com"),
   DNS_ID_MATCH("foo.com", "foo.com."),
-  DNS_ID_MATCH("foo.com.", "foo.com."),
-  DNS_ID_MATCH("f.", "f"),
+  DNS_ID_MISMATCH("foo.com.", "foo.com."),
+  DNS_ID_MISMATCH("f.", "f"),
   DNS_ID_MATCH("f", "f."),
-  DNS_ID_MATCH("f.", "f."),
-  DNS_ID_MATCH("*.bar.foo.com.", "www-3.bar.foo.com"),
+  DNS_ID_MISMATCH("f.", "f."),
+  DNS_ID_MISMATCH("*.bar.foo.com.", "www-3.bar.foo.com"),
   DNS_ID_MATCH("*.bar.foo.com", "www-3.bar.foo.com."),
-  DNS_ID_MATCH("*.bar.foo.com.", "www-3.bar.foo.com."),
+  DNS_ID_MISMATCH("*.bar.foo.com.", "www-3.bar.foo.com."),
 
   // We require the reference ID to be a valid DNS name, so we cannot test this
   // case.
   // DNS_ID_MISMATCH(".", "."),
 
   DNS_ID_MISMATCH("*.com.", "example.com"),
   DNS_ID_MISMATCH("*.com", "example.com."),
   DNS_ID_MISMATCH("*.com.", "example.com."),
   DNS_ID_MISMATCH("*.", "foo."),
   DNS_ID_MISMATCH("*.", "foo"),
 
   // The result is different than Chromium because we don't know that co.uk is
   // a TLD.
-  DNS_ID_MATCH("*.co.uk.", "foo.co.uk"),
-  DNS_ID_MATCH("*.co.uk.", "foo.co.uk."),
+  DNS_ID_MATCH("*.co.uk", "foo.co.uk"),
+  DNS_ID_MATCH("*.co.uk", "foo.co.uk."),
+  DNS_ID_MISMATCH("*.co.uk.", "foo.co.uk"),
+  DNS_ID_MISMATCH("*.co.uk.", "foo.co.uk."),
 };
 
 struct InputValidity
 {
   ByteString input;
   bool isValidReferenceID;
   bool isValidPresentedID;
 };
@@ -304,20 +307,20 @@ static const InputValidity DNSNAMES_VALI
   I(".a.b", false, false), // PresentedDNSIDMatchesReferenceDNSID depends on this
   I("..a", false, false),
   I("a..b", false, false),
   I("a...b", false, false),
   I("a..b.c", false, false),
   I("a.b..c", false, false),
   I(".a.b.c.", false, false),
 
-  // absolute names
-  I("a.", true, true),
-  I("a.b.", true, true),
-  I("a.b.c.", true, true),
+  // absolute names (only allowed for reference names)
+  I("a.", true, false),
+  I("a.b.", true, false),
+  I("a.b.c.", true, false),
 
   // absolute names with empty label at end
   I("a..", false, false),
   I("a.b..", false, false),
   I("a.b.c..", false, false),
   I("a...", false, false),
 
   // Punycode
@@ -434,17 +437,17 @@ static const InputValidity DNSNAMES_VALI
   // Wildcard specifications are not valid reference names, but are valid
   // presented names if there are enough labels
   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, true),
+  I("*.a.b.", false, false),
   I("a*.b.c", false, true),
   I("*.a.b.c", false, true),
   I("a*.b.c.d", false, true),
 
   // Multiple wildcards are not allowed.
   I("a**.b.c", false, false),
   I("a*b*.c.d", false, false),
   I("a*.b*.c", false, false),
@@ -1756,53 +1759,60 @@ static const NameConstraintParams NAME_C
   // Q: Is there a way to prevent subdomain matches?
   // (This is tested in a different set of tests because it requires a
   // combination of permittedSubtrees and excludedSubtrees.)
 
   // Q: Are name constraints allowed to be specified as absolute names?
   //    For example, does a presented ID of "example.com" match a name
   //    constraint of "example.com." and vice versa.
   //
-  { ByteString(), DNSName("example.com"),
+  { // The DNSName in the constraint is not valid because constraint DNS IDs
+    // are not allowed to be absolute.
+    ByteString(), DNSName("example.com"),
     GeneralSubtree(DNSName("example.com.")),
-    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Success,
+    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Result::ERROR_CERT_NOT_IN_NAME_SPACE,
   },
-  { ByteString(), DNSName("example.com."),
+  { // The DNSName in the SAN is not valid because presented DNS IDs are not
+    // allowed to be absolute.
+    ByteString(), DNSName("example.com."),
     GeneralSubtree(DNSName("example.com")),
     Result::ERROR_CERT_NOT_IN_NAME_SPACE, Success,
   },
   { // The presented ID is the same length as the constraint, because the
     // subdomain is only one character long and because the constraint both
-    // begins and ends with ".".
+    // begins and ends with ".". But, it doesn't matter because absolute names
+    // are not allowed for DNSName constraints.
     ByteString(), DNSName("p.example.com"),
     GeneralSubtree(DNSName(".example.com.")),
-    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Success,
+    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Result::ERROR_CERT_NOT_IN_NAME_SPACE,
   },
   { // Same as previous test case, but using a wildcard presented ID.
     ByteString(), DNSName("*.example.com"),
     GeneralSubtree(DNSName(".example.com.")),
-    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Success
+    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Result::ERROR_CERT_NOT_IN_NAME_SPACE
   },
 
   // Q: Are "" and "." valid DNSName constraints? If so, what do they mean?
   { ByteString(), DNSName("example.com"),
     GeneralSubtree(DNSName("")),
     Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE
   },
-  { ByteString(), DNSName("example.com."),
+  { // The malformed (absolute) presented ID does not match.
+    ByteString(), DNSName("example.com."),
     GeneralSubtree(DNSName("")),
-    Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE
+    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Success
   },
-  { ByteString(), DNSName("example.com"),
+  { // Invalid syntax in name constraint -> ERROR_CERT_NOT_IN_NAME_SPACE.
+    ByteString(), DNSName("example.com"),
     GeneralSubtree(DNSName(".")),
-    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Success,
+    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Result::ERROR_CERT_NOT_IN_NAME_SPACE,
   },
   { ByteString(), DNSName("example.com."),
     GeneralSubtree(DNSName(".")),
-    Success, Result::ERROR_CERT_NOT_IN_NAME_SPACE
+    Result::ERROR_CERT_NOT_IN_NAME_SPACE, Result::ERROR_CERT_NOT_IN_NAME_SPACE
   },
 
   /////////////////////////////////////////////////////////////////////////////
   // Basic IP Address constraints (non-CN-ID)
 
   // The Mozilla CA Policy says this means "no IPv4 addresses allowed."
   { ByteString(), IPAddress(ipv4_addr_bytes),
     GeneralSubtree(IPAddress(ipv4_constraint_all_zeros_bytes)),