From 346599ec9ccf3e0daf6b0aebb50e94179c6e7f40 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Thu, 4 Dec 2014 17:12:09 -0800 Subject: [PATCH] Bug 1107791 Remove support for unusual wildcard names in certificates, r=keeler --HG-- extra : rebase_source : bd142d2e85059a0d0fd36325242553e94a7d4377 --- security/pkix/lib/pkixnames.cpp | 125 ++++++++----------- security/pkix/test/gtest/pkixnames_tests.cpp | 27 ++-- 2 files changed, 67 insertions(+), 85 deletions(-) diff --git a/security/pkix/lib/pkixnames.cpp b/security/pkix/lib/pkixnames.cpp index 0185708e6eab..9f9d8e5a7f4a 100644 --- a/security/pkix/lib/pkixnames.cpp +++ b/security/pkix/lib/pkixnames.cpp @@ -1043,56 +1043,41 @@ PresentedDNSIDMatchesReferenceDNSID( 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)) { - 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; - } + 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; } // Allow a relative presented DNS ID to match an absolute reference DNS ID, @@ -1579,16 +1564,34 @@ IsValidDNSID(Input hostname, ValidDNSIDMatchType matchType) 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; @@ -1597,14 +1600,6 @@ IsValidDNSID(Input hostname, ValidDNSIDMatchType matchType) 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) { @@ -1659,20 +1654,6 @@ IsValidDNSID(Input hostname, ValidDNSIDMatchType matchType) } 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 && @@ -1683,8 +1664,6 @@ IsValidDNSID(Input hostname, ValidDNSIDMatchType matchType) if (labelEndsWithHyphen) { return false; // Labels must not end with a hyphen. } - allowWildcard = false; // only allowed in the first label. - labelIsWildcard = false; labelLength = 0; break; diff --git a/security/pkix/test/gtest/pkixnames_tests.cpp b/security/pkix/test/gtest/pkixnames_tests.cpp index c066245f53ad..061f3942cf02 100644 --- a/security/pkix/test/gtest/pkixnames_tests.cpp +++ b/security/pkix/test/gtest/pkixnames_tests.cpp @@ -104,9 +104,9 @@ static const PresentedMatchesReference DNSID_MATCH_PARAMS[] = 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"), @@ -150,7 +150,8 @@ static const PresentedMatchesReference DNSID_MATCH_PARAMS[] = 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. @@ -199,8 +200,9 @@ static const PresentedMatchesReference DNSID_MATCH_PARAMS[] = 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. @@ -435,7 +437,8 @@ static const InputValidity DNSNAMES_VALIDITY[] = 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), @@ -443,9 +446,9 @@ static const InputValidity DNSNAMES_VALIDITY[] = 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), @@ -466,9 +469,9 @@ static const InputValidity DNSNAMES_VALIDITY[] = 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),