From 1f021d1dc255c1585f097316b24ac6ab0decae59 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Mon, 8 Dec 2014 16:42:54 -0800 Subject: [PATCH] Bug 1107790: Remove support for absolute hostnames in presented DNS IDs and name constraints, r=keeler --HG-- extra : rebase_source : cf402f902196e729026d713cd6d62f5c3b889a12 --- security/pkix/lib/pkixnames.cpp | 70 ++++++++++--------- security/pkix/test/gtest/pkixnames_tests.cpp | 72 +++++++++++--------- 2 files changed, 80 insertions(+), 62 deletions(-) diff --git a/security/pkix/lib/pkixnames.cpp b/security/pkix/lib/pkixnames.cpp index 3499bf8d793..0185708e6ea 100644 --- a/security/pkix/lib/pkixnames.cpp +++ b/security/pkix/lib/pkixnames.cpp @@ -757,6 +757,12 @@ CheckPresentedIDConformsToNameConstraintsSubtrees( 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: @@ -846,15 +852,15 @@ CheckPresentedIDConformsToNameConstraintsSubtrees( // follow NSS's stricter policy by accepting wildcards only of the form // *., where 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. // @@ -929,22 +935,16 @@ CheckPresentedIDConformsToNameConstraintsSubtrees( // 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 @@ -1044,7 +1044,7 @@ PresentedDNSIDMatchesReferenceDNSID( } bool isFirstPresentedByte = true; - do { + for (;;) { uint8_t presentedByte; if (presented.Read(presentedByte) != Success) { return false; @@ -1075,12 +1075,6 @@ PresentedDNSIDMatchesReferenceDNSID( 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; @@ -1089,9 +1083,17 @@ PresentedDNSIDMatchesReferenceDNSID( 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. @@ -1692,6 +1694,12 @@ IsValidDNSID(Input hostname, ValidDNSIDMatchType matchType) 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. } diff --git a/security/pkix/test/gtest/pkixnames_tests.cpp b/security/pkix/test/gtest/pkixnames_tests.cpp index a439a842634..c066245f53a 100644 --- a/security/pkix/test/gtest/pkixnames_tests.cpp +++ b/security/pkix/test/gtest/pkixnames_tests.cpp @@ -89,16 +89,16 @@ static const PresentedMatchesReference DNSID_MATCH_PARAMS[] = // 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."), @@ -245,17 +245,18 @@ static const PresentedMatchesReference DNSID_MATCH_PARAMS[] = // 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. @@ -269,8 +270,10 @@ static const PresentedMatchesReference DNSID_MATCH_PARAMS[] = // 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 @@ -309,10 +312,10 @@ static const InputValidity DNSNAMES_VALIDITY[] = 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), @@ -439,7 +442,7 @@ static const InputValidity DNSNAMES_VALIDITY[] = 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), @@ -1761,25 +1764,30 @@ static const NameConstraintParams NAME_CONSTRAINT_PARAMS[] = // 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? @@ -1787,17 +1795,19 @@ static const NameConstraintParams NAME_CONSTRAINT_PARAMS[] = 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 }, /////////////////////////////////////////////////////////////////////////////