Closed Bug 1063281 Opened 10 years ago Closed 10 years ago

Implement RFC6125-compliant name matching in mozilla::pkix

Categories

(Core :: Security: PSM, defect)

Product:
Core
Component:
Security: PSM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: briansmith, Assigned: briansmith)

References

(Blocks 2 open bugs)

Details

(Keywords: site-compat)

Attachments

(12 files, 8 obsolete files)

Part 1: Make the subjectAltName accessible to the verification logic
854 bytes, patch
keeler
: review+
Details | Diff | Splinter Review
Part 2: Implement IsValidDNSName [v2]
13.86 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Part 3: Implement ParseIPv4Address [v3]
7.41 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Part 4: Implement ParseIPv6Address [v2]
16.88 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Part 5: Implement RFC6125 DNS ID matching
22.64 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Part 8: Switch Gecko to use CheckCertHostname
5.61 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Part 6(a): Implement CheckCertHostname, with initial tests
41.49 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
Part 6(b): Allow empty subject and document syntax using RFC text
5.28 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Part 6(c): Fix CheckCertHostname bugs and expand tests
27.42 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Part 6(d): refactor CheckCertHostname tests a little
24.38 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Rewrite PresentedDNSIDMatchesReferenceDNSID, Part 1: Add IsValidPresentedDNSID
25.17 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Rewrite PresentedDNSIDMatchesReferenceDNSID, Part 2: Actually rewrite it
13.03 KB, patch
keeler
: review+
Details | Diff | Splinter Review
There are some subtle aspects of the implementation of name constraints, where it would be useful to know exactly what the name verification logic is doing. For example, is it necessary to enforce ipv6 ipAddress name constraints against the subject CN? It shouldn't be necessary, but ultimately it depends on whether the name matching code matches IPv6 addresses in the subject CN. Also, the name verification logic and name constraint logic can share a significant amount of code, and generally certificate verification isn't useful without name verification.
Comment on attachment 8484686 [details] [diff] [review] Part 1: Make the subjectAltName accessible to the verification logic Review of attachment 8484686 [details] [diff] [review]: ----------------------------------------------------------------- Looks good so far.
Attachment #8484686 - Flags: review?(dkeeler) → review+
Attached patch Part 2: Implement IsValidDNSName (obsolete) — — Splinter Review
When given the hostname to validate the certificate for, we need to distinguish between DNS names (the common case), IPv4 addresses, IPv6 addresses, and invalid garbage. This patch implements a function that checks if a given string is a syntactically-correct DNS Name.
Attachment #8487540 - Flags: review?(dkeeler)
The caller will give us a string, which may be the stringified version of an IP address (IPv4 or IPv6). IP addresses are stored in binary form in subjectAltName and in name constraints. This function attempts to parse its string input into a binary IPv4 address, returning false if the parsing failed.
Attachment #8487543 - Flags: review?(dkeeler)
I made some minor changes to this patch to better accomodate the upcoming IPv6 tests.
Attachment #8487543 - Attachment is obsolete: true
Attachment #8487543 - Flags: review?(dkeeler)
Attachment #8488177 - Flags: review?(dkeeler)
Just like IPv4 addresses, we need to parse IPv6 addresses into their binary form in order to search subjectAltName for them.
Attachment #8488181 - Flags: review?(dkeeler)
Comment on attachment 8487540 [details] [diff] [review] Part 2: Implement IsValidDNSName Review of attachment 8487540 [details] [diff] [review]: ----------------------------------------------------------------- There seem to be two related but separate parts of this patch: implementing and testing IsValidDNSName and then actually using it (which this patch doesn't appear to, yet, but lays some groundwork in pkixcheck.cpp and bind.h). I don't know if that was intentional or not. I'm assuming you might fold all of these patches together anyway, so it's not a big deal. This looks good, so r=me with some additional test cases I suggest. ::: security/pkix/lib/pkixcheck.cpp @@ +403,5 @@ > // NSS. > Result > +CheckNameConstraints(Input encodedNameConstraints, > + const BackCert& firstChild, > + KeyPurposeId requiredEKUIfPresent) These changes seem unrelated to the rest of the patch. ::: security/pkix/lib/pkixnames.cpp @@ +96,5 @@ > + labelIsAllNumeric = false; > + endsWithHyphen = false; > + ++labelLength; > + if (labelLength > 63) { > + return false; Hmmm - does it make sense to factor out the common labelLength increment/length check? ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +20,5 @@ > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > +#include <cstring> nit: I think it's nice to have a blank line after the license boilerplate before #includes. @@ +56,5 @@ > + } > + > +static const InputValidity DNSNAMES_VALIDITY[] = > +{ > + I("", false), We should also test things like "a..", ".a", and "a.a.", right? (Do we treat a FQDN as valid?) @@ +96,5 @@ > + I("a^", false), > + I("a&", false), > + I("a*", false), > + I("a(", false), > + I("a)", false), Seems like it would be good to have some examples where the invalid character is at the beginning or in the middle of a label, to ensure that we're not just checking that the last character was valid. @@ +118,5 @@ > + // labels cannot start with a hyphen > + I("-", false), > + I("-1", false), > + > + // labels cannot end with a hyphen (XXX!) Do the "XXX!" annotations mean we don't do these correctly in NSS and/or necko? @@ +156,5 @@ > + I("1234567890" "1234567890" "1234567890" "1234567890" "1234567890" "." > + "1234567890" "1234567890" "1234567890" "1234567890" "1234567890" "." > + "1234567890" "1234567890" "1234567890" "1234567890" "1234567890" "." > + "1234567890" "1234567890" "1234567890" "1234567890" "1234567890" "." > + "1234567890" "1234567890" "1234567890" "1234567890" "12345678" "a", This might be easier to interpret as: "1234567890" "1234567890" "1234567890" "1234567890" "123456789" "." (so a total of 50 per line, instead of 51) ... "abc" (with the invalid version ending with "abcd")
Attachment #8487540 - Flags: review?(dkeeler) → review+
Comment on attachment 8488177 [details] [diff] [review] Part 3: Implement ParseIPv4Address [v2] Review of attachment 8488177 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: security/pkix/lib/pkixnames.cpp @@ +36,5 @@ > namespace mozilla { namespace pkix { > > +namespace { > + > +// XXX: this code writes to C-style arrays. Change it to use std::string or This comment seems out-of-date (at least, it's unclear to me as is). ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +206,5 @@ > +#define IPV4_INVALID(str) \ > + { \ > + ByteString(reinterpret_cast<const uint8_t*>(str), sizeof(str) - 1), \ > + false, \ > + { 73, 73, 73, 73 } \ Why this value? I guess it doesn't matter since it's not getting read anyway, but 0 seems like a more natural choice. @@ +246,5 @@ > + > + // Leading zeros not allowed > + IPV4_INVALID("01.2.3.4"), > + IPV4_INVALID("001.2.3.4"), > + IPV4_INVALID("010.2.3.4"), How about "1.02.3.4" or "1.2.3.04"?
Attachment #8488177 - Flags: review?(dkeeler) → review+
Comment on attachment 8488181 [details] [diff] [review] Part 4: Implement ParseIPv6Address Review of attachment 8488181 [details] [diff] [review]: ----------------------------------------------------------------- The direct-memory accessing bits make me nervous, but I did some analysis and it appears to be at least safe, if not correct (I'm not extremely familiar with what is and isn't allowed for ipv6 addresses, but the testcases appear to be comprehensive (minus a suggestion for an additional test)). Sorry this took so long to review. ::: security/pkix/lib/pkixnames.cpp @@ +103,5 @@ > + assert(numComponents >= 0); > + assert(numComponents <= 8); > + assert(contractionIndex >= -1); > + assert(contractionIndex <= 8); > + assert(contractionIndex <= numComponents); Is assert enabled in non-debug builds? I think we should enforce these in release versions as well, just to be safe. @@ +174,5 @@ > + value = static_cast<uint8_t>(b - static_cast<uint8_t>('0')); > + break; > + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f': > + value = static_cast<uint8_t>(b - static_cast<uint8_t>('a') + > + UINT8_C(10)); nit: I would move this line one space to the left @@ +178,5 @@ > + UINT8_C(10)); > + break; > + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F': > + value = static_cast<uint8_t>(b - static_cast<uint8_t>('A') + > + UINT8_C(10)); nit: same @@ +183,5 @@ > + break; > + case '.': > + { > + // A dot indicates we hit a IPv4-syntax component. Backtrack, parsing > + // parsing the input from startOfComponent to the end of the input nit: 'parsing' appears twice in a row in this comment ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +306,5 @@ > + IPV6_INVALID("1234:5678:9abc:def0"), > + IPV6_INVALID("1234:5678:9abc:def0:1234:"), > + IPV6_INVALID("1234:5678:9abc:def0:1234:5678:"), > + IPV6_INVALID("1234:5678:9abc:def0:1234:5678:9abc:"), > + IPV6_VALID("1234:5678:9abc:def0:1234:5678:9abc:def0", What about leading 0s? Should it be "0abc::" or "abc::" or are both valid?
Attachment #8488181 - Flags: review?(dkeeler) → review+
Comment on attachment 8487540 [details] [diff] [review] Part 2: Implement IsValidDNSName Review of attachment 8487540 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixcheck.cpp @@ +403,5 @@ > // NSS. > Result > +CheckNameConstraints(Input encodedNameConstraints, > + const BackCert& firstChild, > + KeyPurposeId requiredEKUIfPresent) Sorry, I folded together two patches that shouldn't have been folded together. I will post them separately for re-review. ::: security/pkix/lib/pkixnames.cpp @@ +96,5 @@ > + labelIsAllNumeric = false; > + endsWithHyphen = false; > + ++labelLength; > + if (labelLength > 63) { > + return false; I don't like the repetitive code either, but I couldn't figure out a way to factor it out that was clearer than the current code. I did, however, give the constant 63 a name. ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +20,5 @@ > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > +#include <cstring> Added. @@ +56,5 @@ > + } > + > +static const InputValidity DNSNAMES_VALIDITY[] = > +{ > + I("", false), Added. @@ +96,5 @@ > + I("a^", false), > + I("a&", false), > + I("a*", false), > + I("a(", false), > + I("a)", false), Added. @@ +118,5 @@ > + // labels cannot start with a hyphen > + I("-", false), > + I("-1", false), > + > + // labels cannot end with a hyphen (XXX!) I dropped the "XXX!". I don't remember exactly why I added it. @@ +156,5 @@ > + I("1234567890" "1234567890" "1234567890" "1234567890" "1234567890" "." > + "1234567890" "1234567890" "1234567890" "1234567890" "1234567890" "." > + "1234567890" "1234567890" "1234567890" "1234567890" "1234567890" "." > + "1234567890" "1234567890" "1234567890" "1234567890" "1234567890" "." > + "1234567890" "1234567890" "1234567890" "1234567890" "12345678" "a", I disagree, so I didn't change it.
Comment on attachment 8488177 [details] [diff] [review] Part 3: Implement ParseIPv4Address [v2] Review of attachment 8488177 [details] [diff] [review]: ----------------------------------------------------------------- In the updated patch, I also added quite a few new test cases. ::: security/pkix/lib/pkixnames.cpp @@ +36,5 @@ > namespace mozilla { namespace pkix { > > +namespace { > + > +// XXX: this code writes to C-style arrays. Change it to use std::string or removed. ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +246,5 @@ > + > + // Leading zeros not allowed > + IPV4_INVALID("01.2.3.4"), > + IPV4_INVALID("001.2.3.4"), > + IPV4_INVALID("010.2.3.4"), Added.
Comment on attachment 8488181 [details] [diff] [review] Part 4: Implement ParseIPv6Address Review of attachment 8488181 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixnames.cpp @@ +103,5 @@ > + assert(numComponents >= 0); > + assert(numComponents <= 8); > + assert(contractionIndex >= -1); > + assert(contractionIndex <= 8); > + assert(contractionIndex <= numComponents); OK, I added it. @@ +174,5 @@ > + value = static_cast<uint8_t>(b - static_cast<uint8_t>('0')); > + break; > + case 'a': case 'b': case 'c': case 'd': case 'e': case 'f': > + value = static_cast<uint8_t>(b - static_cast<uint8_t>('a') + > + UINT8_C(10)); done. @@ +178,5 @@ > + UINT8_C(10)); > + break; > + case 'A': case 'B': case 'C': case 'D': case 'E': case 'F': > + value = static_cast<uint8_t>(b - static_cast<uint8_t>('A') + > + UINT8_C(10)); done. @@ +183,5 @@ > + break; > + case '.': > + { > + // A dot indicates we hit a IPv4-syntax component. Backtrack, parsing > + // parsing the input from startOfComponent to the end of the input nixed one. ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +306,5 @@ > + IPV6_INVALID("1234:5678:9abc:def0"), > + IPV6_INVALID("1234:5678:9abc:def0:1234:"), > + IPV6_INVALID("1234:5678:9abc:def0:1234:5678:"), > + IPV6_INVALID("1234:5678:9abc:def0:1234:5678:9abc:"), > + IPV6_VALID("1234:5678:9abc:def0:1234:5678:9abc:def0", I added some cases to explicitly test how those are dealt with
Addresses review comments. Please review the interdiff.
Attachment #8487540 - Attachment is obsolete: true
Attachment #8500185 - Flags: review?(dkeeler)
Addresses review comments. Please review the interdiff.
Attachment #8488177 - Attachment is obsolete: true
Attachment #8500186 - Flags: review?(dkeeler)
Addresses review comments. Please review the interdiff.
Attachment #8488181 - Attachment is obsolete: true
Attachment #8500187 - Flags: review?(dkeeler)
Note that this matches "foo." and "foo", unlike the current NSS code.
Attachment #8500189 - Flags: review?(dkeeler)
This patch implements the replacement for CERT_VerifyCertName. The tests will follow in separate patches.
Attachment #8500190 - Flags: review?(dkeeler)
I refactored this patch to better accommodate the upcoming implementation of name constraints, which reuses the SearchForName code.
Attachment #8500190 - Attachment is obsolete: true
Attachment #8500190 - Flags: review?(dkeeler)
Attachment #8500871 - Flags: review?(dkeeler)
Attachment #8500191 - Attachment is obsolete: true
Attachment #8500191 - Flags: review?(dkeeler)
Attachment #8500872 - Flags: review?(dkeeler)
No longer depends on: 1078108
Comment on attachment 8500185 [details] [diff] [review] Part 2: Implement IsValidDNSName [v2] Review of attachment 8500185 [details] [diff] [review]: ----------------------------------------------------------------- Interdiff looks good with comments addressed. ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ When I look at this in my editor, it appears that a non-printable character has been inserted near the beginning of this line that shouldn't be there. @@ +99,5 @@ > + I("xn--\0xc4\0x95.com", false), // UTF-8 ĕ, malformed punycode + UTF-8 mashup > + > + // Surprising punycode > + I("xn--google.com", true), // 䕮䕵䕶䕱.com > + I("xn--citibank.com", true), // 岍岊岊岅岉岎.com nit: trailing whitespace @@ +183,5 @@ > + I("a.a-.a", false), > + > + // labels can contain a hyphen in the middle > + I("a-b", true), > + I("1-2", true), // XXX! What's the issue here?
Attachment #8500185 - Flags: review?(dkeeler) → review+
Comment on attachment 8500186 [details] [diff] [review] Part 3: Implement ParseIPv4Address [v3] Review of attachment 8500186 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8500186 - Flags: review?(dkeeler) → review+
Comment on attachment 8500187 [details] [diff] [review] Part 4: Implement ParseIPv6Address [v2] Review of attachment 8500187 [details] [diff] [review]: ----------------------------------------------------------------- Interdiff looks good with comment addressed. ::: security/pkix/lib/pkixnames.cpp @@ +101,5 @@ > + assert(numComponents <= 8); > + assert(contractionIndex >= -1); > + assert(contractionIndex <= 8); > + assert(contractionIndex <= numComponents); > + if (numComponents <= 0 || nit: numComponents < 0, to be equivalent to !(numComponents >= 0)
Attachment #8500187 - Flags: review?(dkeeler) → review+
Comment on attachment 8500189 [details] [diff] [review] Part 5: Implement RFC6125 DNS ID matching Review of attachment 8500189 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Sorry this took a while to get to. ::: security/pkix/lib/pkixnames.cpp @@ +170,5 @@ > + > + // The reference ID may have a terminating dot '.' to mark it as absolute, > + // and a presented ID without that terminating dot still matches it. > + if (!reference.AtEnd()) { > + uint8_t b; To be consistent, shouldn't this be 'uint8_t referenceByte;'? @@ +220,5 @@ > +StartsWithIDNALabel(Input id) > +{ > + static const uint8_t IDN_ALABEL_PREFIX[4] = { 'x', 'n', '-', '-' }; > + Reader input(id); > + for (size_t i = 0; i < sizeof(IDN_ALABEL_PREFIX); ++i) { Hmmm - don't we have some sort of MatchBytes for this kind of situation? ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +217,5 @@ > + > + // The result is different than Chromium, because Chromium takes into account > + // the additional knowledge it has that "co.uk" is a TLD. mozilla::pkix does > + // not know that. > + DNS_ID_MATCH("*.co.uk", "example.co.uk"), Does NSS also currently match this behavior? @@ +269,5 @@ > + > + // 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."), There's a case very similar to this, above - maybe put them all in the same general area?
Attachment #8500189 - Flags: review?(dkeeler) → review+
Comment on attachment 8500871 [details] [diff] [review] Part 6: Implement CheckCertHostname [v2] Review of attachment 8500871 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. ::: security/pkix/lib/pkixnames.cpp @@ +95,5 @@ > + > +// 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. Does "normalized" in this case mean with trailing '.' (i.e. a FQDN) or not? @@ +108,5 @@ > + > + const Input* subjectAltName(cert.GetSubjectAltName()); > + Input subject(cert.GetSubject()); > + > + // TODO: RFC citations Are you intending to address this in this patch or a future patch? @@ +132,5 @@ > + } else if (ParseIPv4Address(hostname, ipv4)) { > + rv = SearchForName(subjectAltName, subject, GeneralNameType::iPAddress, > + Input(ipv4), FallBackToCommonName::Yes, found); > + } else { > + return Result::ERROR_BAD_CERT_DOMAIN; If hostname is not a DNS name or IP address, haven't we given this function bad input? I would expect the error to be INVALD_ARGS or something here. @@ +227,5 @@ > + // RDNSequence ::= SEQUENCE OF RelativeDistinguishedName > + // > + // RelativeDistinguishedName ::= > + // SET SIZE (1..MAX) OF AttributeTypeAndValue > + // TODO: is rdnSequence allowed to be empty? Well, going by the definition (i.e. (SIZE (1..MAX))), I would say no. I bet there are certificates in use with empty rdnSequences, though. @@ +372,5 @@ > + > + switch (nameType) { > + case GeneralNameType::dNSName: > + foundMatch = PresentedDNSIDMatchesReferenceDNSID(presentedID, > + referenceID, true); My one concern here is that we're not enforcing that we actually did make sure the referenceID is valid in this case and the one above. I think it would be too much effort to thread through the result of checking it, though, so it's probably fine. (What I mean is, currently we do check, but if the code paths change, this isn't enforced.)
Attachment #8500871 - Flags: review?(dkeeler) → review+
Comment on attachment 8500871 [details] [diff] [review] Part 6: Implement CheckCertHostname [v2] Review of attachment 8500871 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixnames.cpp @@ +95,5 @@ > + > +// 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. It could be either a relative (no dot) or absolute (dot) name. "Normalized" means any decoding of URL encoding or other such things has already been done. (Contrast this with Chrome's similar function, which decodes URL encoding, IIRC.) @@ +108,5 @@ > + > + const Input* subjectAltName(cert.GetSubjectAltName()); > + Input subject(cert.GetSubject()); > + > + // TODO: RFC citations It seems I uploaded the wrong version of this patch. I'd already deleted this comment in the newest version. I've already tried to cite the RFCs where I thought it was important to do so. @@ +132,5 @@ > + } else if (ParseIPv4Address(hostname, ipv4)) { > + rv = SearchForName(subjectAltName, subject, GeneralNameType::iPAddress, > + Input(ipv4), FallBackToCommonName::Yes, found); > + } else { > + return Result::ERROR_BAD_CERT_DOMAIN; My thinking is that if a user reports a cert error page of SEC_ERROR_INVALID_ARGS we're going to have no clue where to look, whereas if the user reports a cert error page of SEC_ERROR_BAD_CERT_DOMAIN we'll know to look somewhere in this file or in the calling code. So, I could go either way but I think that SEC_ERROR_BAD_CERT_DOMAIN is OK. @@ +227,5 @@ > + // RDNSequence ::= SEQUENCE OF RelativeDistinguishedName > + // > + // RelativeDistinguishedName ::= > + // SET SIZE (1..MAX) OF AttributeTypeAndValue > + // TODO: is rdnSequence allowed to be empty? The SET SIZE (1..MAX) refers to RelativeDistingishedName, not RDNSequence. The answer is actually "yes", as RFC 5280 says: If subject naming information is present only in the subjectAltName extension (e.g., a key bound only to an email address or URI), then the subject name MUST be an empty sequence and the subjectAltName extension MUST be critical. I will update the patch. @@ +372,5 @@ > + > + switch (nameType) { > + case GeneralNameType::dNSName: > + foundMatch = PresentedDNSIDMatchesReferenceDNSID(presentedID, > + referenceID, true); I agree that this is not as good as I would like. Also, I found out when modifying the code to enforce name constraints that the precondition that the referenceID is valid not helpful for the name constraint case. So, I'll update the code so that this is not an issue.
Comment on attachment 8500872 [details] [diff] [review] Part 7: Initial tests for CheckCertHostname [v2] Review of attachment 8500872 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +941,5 @@ > pkixnames_ParseIPv6Address, > testing::ValuesIn(IPV6_ADDRESSES)); > + > +static const ByteString > + NO_SAN(reinterpret_cast<const uint8_t*>("I'm a bad, bad, certificate")); Well, it's not BR-compliant, but it's not malicious or necessarily untrustworthy. @@ +1004,5 @@ > +{ > + // Control #1 > + WITH_SAN("a", Name(RDN(CN("a"))), DNSName("a"), Success), > + // Control #2 > + WITH_SAN("a", Name(RDN(CN("b"))), DNSName("a"), Success), What about: WITH_SAN("a", Name(RDN(CN("a"))), DNSName("b"), Success) (or should that not be a success?) @@ +1037,5 @@ > + WITH_SAN("example.com", Name(RDN(CN("foo"))), > + DNSName("!") + DNSName("example.com"), Success), > + > + // We match valid IPv4 addresses in the CN when there is no SAN, but not > + // when there is not a SAN. Should this be: "... but not when there is a SAN." @@ +1095,5 @@ > + return CreateEncodedCertificate( > + v3, sha256WithRSAEncryption, serialNumber, issuerDER, > + oneDayBeforeNow, oneDayAfterNow, subject, *keyPair, > + extensions, *keyPair, > + SignatureAlgorithm::rsa_pkcs1_with_sha256); Looks like this will need a quick rebase - this should be sha256WithRSAEncryption, I think. ::: security/pkix/test/lib/pkixtestutil.h @@ +84,5 @@ > + > +ByteString CN(const ByteString&); > + > +inline ByteString > +CN(const char *value) nit: 'const char* value' @@ +117,5 @@ > + > +inline ByteString > +DNSName(const ByteString& name) > +{ > + return TLV((2 << 6) | 2, name); Documentation here for these values would be appreciated. @@ +132,5 @@ > +template <size_t L> > +inline ByteString > +IPAddress(const uint8_t (&bytes)[L]) > +{ > + return TLV((2 << 6) | 7, ByteString(bytes, L)); Same here.
Attachment #8500872 - Flags: review?(dkeeler) → review+
Comment on attachment 8500192 [details] [diff] [review] Part 8: Switch Gecko to use CheckCertHostname Review of attachment 8500192 [details] [diff] [review]: ----------------------------------------------------------------- Great
Attachment #8500192 - Flags: review?(dkeeler) → review+
Comment on attachment 8500185 [details] [diff] [review] Part 2: Implement IsValidDNSName [v2] Review of attachment 8500185 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ That is the UTF-8 byte order marker, which helps peoples' editors (in theory) render 岍岊岊岅岉岎.com and the other non-ASCII characters correctly. Do you think I should remove it? @@ +99,5 @@ > + I("xn--\0xc4\0x95.com", false), // UTF-8 ĕ, malformed punycode + UTF-8 mashup > + > + // Surprising punycode > + I("xn--google.com", true), // 䕮䕵䕶䕱.com > + I("xn--citibank.com", true), // 岍岊岊岅岉岎.com nixed. @@ +183,5 @@ > + I("a.a-.a", false), > + > + // labels can contain a hyphen in the middle > + I("a-b", true), > + I("1-2", true), // XXX! I removed the comment. I think it is strange and possibly error-prone that a DNS name can consist of numbers and hyphens but no letters.
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #30) > ::: security/pkix/test/gtest/pkixnames_tests.cpp > @@ +1,1 @@ > > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > > That is the UTF-8 byte order marker, which helps peoples' editors (in > theory) render 岍岊岊岅岉岎.com and the other non-ASCII characters correctly. Do > you think I should remove it? Oh, I see. I guess we should leave it in, then.
Comment on attachment 8500187 [details] [diff] [review] Part 4: Implement ParseIPv6Address [v2] Review of attachment 8500187 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixnames.cpp @@ +101,5 @@ > + assert(numComponents <= 8); > + assert(contractionIndex >= -1); > + assert(contractionIndex <= 8); > + assert(contractionIndex <= numComponents); > + if (numComponents <= 0 || I changed it to this: if (!(numComponents >= 0 && numComponents <= 8 && contractionIndex >= -1 && contractionIndex <= 8 contractionIndex <= numComponents)) { return false; } Which will hopefully be less error-prone.
Comment on attachment 8500189 [details] [diff] [review] Part 5: Implement RFC6125 DNS ID matching Review of attachment 8500189 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixnames.cpp @@ +170,5 @@ > + > + // The reference ID may have a terminating dot '.' to mark it as absolute, > + // and a presented ID without that terminating dot still matches it. > + if (!reference.AtEnd()) { > + uint8_t b; Taking one of your suggestions in another part of the review of this patch series, I replaced this code with: // 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; } @@ +220,5 @@ > +StartsWithIDNALabel(Input id) > +{ > + static const uint8_t IDN_ALABEL_PREFIX[4] = { 'x', 'n', '-', '-' }; > + Reader input(id); > + for (size_t i = 0; i < sizeof(IDN_ALABEL_PREFIX); ++i) { No, we don't have that. I think at one point we had something similar to that (MatchTLV), but that stuff got removed as we evolved the Input/Reader API. I think that if we find another use of this kind of matching, we should definitely refactor this into such a reusable function, though. But, I don't think it needs to be done now. ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +217,5 @@ > + > + // The result is different than Chromium, because Chromium takes into account > + // the additional knowledge it has that "co.uk" is a TLD. mozilla::pkix does > + // not know that. > + DNS_ID_MATCH("*.co.uk", "example.co.uk"), Yes, according to NSS's comments: /* For a cn pattern to be considered valid, the wildcard character... * - may occur only in a DNS name with at least 3 components, and * - may occur only as last character in the first component, and * - may be preceded by additional characters, and * - must not be preceded by an IDNA ACE prefix (xn--) */ @@ +269,5 @@ > + > + // 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."), I guess I'd rather leave it as it is, if you don't mind too much, because it makes it easier to compare and keep in sync with Chrome's tests. (This test case came from Chrome's tests.)
Comment on attachment 8500872 [details] [diff] [review] Part 7: Initial tests for CheckCertHostname [v2] Review of attachment 8500872 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +941,5 @@ > pkixnames_ParseIPv6Address, > testing::ValuesIn(IPV6_ADDRESSES)); > + > +static const ByteString > + NO_SAN(reinterpret_cast<const uint8_t*>("I'm a bad, bad, certificate")); I added this comment: // This is an arbitrary string that is used to indicate that no SAN extension // should be put into the generated certificate. It needs to be different from // "" or any other subjectAltName value that we actually want to test, but its // actual value does not matter. Note that this isn't a correctly-encoded SAN // extension value! @@ +1004,5 @@ > +{ > + // Control #1 > + WITH_SAN("a", Name(RDN(CN("a"))), DNSName("a"), Success), > + // Control #2 > + WITH_SAN("a", Name(RDN(CN("b"))), DNSName("a"), Success), Good catch. That is what Control #3 is supposed to be. I changed Control #3: // Control #3 WITH_SAN("a", Name(RDN(CN("a"))), DNSName("b"), Result::ERROR_BAD_CERT_DOMAIN), It is a failure because the presence of the SubjectAltName with a DNSName entry (i.e. the presence of a DNS-ID) means that we are not supposed to match any CNs (i.e. CN-IDs). @@ +1037,5 @@ > + WITH_SAN("example.com", Name(RDN(CN("foo"))), > + DNSName("!") + DNSName("example.com"), Success), > + > + // We match valid IPv4 addresses in the CN when there is no SAN, but not > + // when there is not a SAN. fixed. @@ +1095,5 @@ > + return CreateEncodedCertificate( > + v3, sha256WithRSAEncryption, serialNumber, issuerDER, > + oneDayBeforeNow, oneDayAfterNow, subject, *keyPair, > + extensions, *keyPair, > + SignatureAlgorithm::rsa_pkcs1_with_sha256); fixed. ::: security/pkix/test/lib/pkixtestutil.h @@ +84,5 @@ > + > +ByteString CN(const ByteString&); > + > +inline ByteString > +CN(const char *value) Fixed. @@ +117,5 @@ > + > +inline ByteString > +DNSName(const ByteString& name) > +{ > + return TLV((2 << 6) | 2, name); Added: // GeneralName ::= CHOICE { // otherName [0] OtherName, // rfc822Name [1] IA5String, // dNSName [2] IA5String, // x400Address [3] ORAddress, // directoryName [4] Name, // ediPartyName [5] EDIPartyName, // uniformResourceIdentifier [6] IA5String, // iPAddress [7] OCTET STRING, // registeredID [8] OBJECT IDENTIFIER } inline ByteString DNSName(const ByteString& name) { // (2 << 6) means "context-specific", 2 is the GeneralName tag. return TLV((2 << 6) | 2, name); } @@ +132,5 @@ > +template <size_t L> > +inline ByteString > +IPAddress(const uint8_t (&bytes)[L]) > +{ > + return TLV((2 << 6) | 7, ByteString(bytes, L)); template <size_t L> inline ByteString IPAddress(const uint8_t (&bytes)[L]) { // (2 << 6) means "context-specific", 7 is the GeneralName tag. return TLV((2 << 6) | 7, ByteString(bytes, L)); }
David, regarding NSS's behavior for *.co.uk, in addition to the comment I quoted, see bug 806281, which shows NSS is doing the same thing. I hope that we improve that, in another bug.
This is the old part 6 merged with the old part 7, rebased, with review comments addressed.
Attachment #8500871 - Attachment is obsolete: true
Attachment #8500872 - Attachment is obsolete: true
Attachment #8503526 - Flags: review+
1. I reverted the behavior of checking a certificate with multiple CNs to match what NSS currently does, and what I think Chrome intends to do. 2. I removed some dead code. We don't allow fallback to CN-ID matching for IPv6 addresses, but in the original part 6, i had included code for it. I removed that code. 3. I expanded and improved the tests. I intend to merge Part 6, Part 6(b), and Part 6(c) together before merging.
Attachment #8503721 - Flags: review?(dkeeler)
Comment on attachment 8503529 [details] [diff] [review] Part 6(b): Allow empty subject and document syntax using RFC text Review of attachment 8503529 [details] [diff] [review]: ----------------------------------------------------------------- Ok - cool
Attachment #8503529 - Flags: review?(dkeeler) → review+
Comment on attachment 8503721 [details] [diff] [review] Part 6(c): Fix CheckCertHostname bugs and expand tests Review of attachment 8503721 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8503721 - Flags: review?(dkeeler) → review+
This patch changes CreateCert in the CheckCertHostname tests so that it automatically calls Name() and CreateEncodedSubjectAltName() for its arguments, so that the caller doesn't have to. It was weird that in the test cases we had to wrap the subject with Name() but we didn't have to do the same kind of wrapping for the SAN. Also, this makes the name constraint tests easier to write and read.
Attachment #8505843 - Flags: review?(dkeeler)
Sorry, my original comment got cut: These latest two patches in the bug address your review comment regarding the referenceDNSIDWasVerifiedAsValid argument value not being threaded through all the functions. Also, this rewrite makes it much easier to implement name constraint matching.
Attachment #8505843 - Flags: review?(dkeeler) → review+
Comment on attachment 8506491 [details] [diff] [review] Rewrite PresentedDNSIDMatchesReferenceDNSID, Part 1: Add IsValidPresentedDNSID Review of attachment 8506491 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with comments addressed. ::: security/pkix/lib/pkixnames.cpp @@ +908,5 @@ > + if (b != '.') { > + return false; > + } > + // There must be at most one '*' in a label. > + if (b == '*') { As the comment above mentions, the true branch of this statement will never actually be taken. Is it just here if/when the other check is removed? Maybe it should just be converted to a comment to the effect of that? Not a big deal, just thought it might be a bit confusing in the future. Also, coverity might complain mildly. ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +445,5 @@ > + // Multiple wildcards are not allowed. > + I("a**.b.c", false, false), > + I("a*b*.c.d", false, false), > + I("a*.b*.c", false, false), > + nit: whitespace @@ +454,5 @@ > + I("a.b*.c", false, false), > + I("*.b*.c", false, false), > + I(".*.a.b", false, false), > + I(".a*.b.c", false, false), > + nit: whitespace
Attachment #8506491 - Flags: review?(dkeeler) → review+
Comment on attachment 8506492 [details] [diff] [review] Rewrite PresentedDNSIDMatchesReferenceDNSID, Part 2: Actually rewrite it Review of attachment 8506492 [details] [diff] [review]: ----------------------------------------------------------------- I just have a comment about a comment. ::: security/pkix/lib/pkixnames.cpp @@ +524,3 @@ > } while (!presented.AtEnd()); > > + // Allow an absolute presented DNS ID to match a relative reference DNS ID. Isn't this backwards? The reference ID would be absolute (i.e. FQDN) and the presented ID would be relative here.
Attachment #8506492 - Flags: review?(dkeeler) → review+
Comment on attachment 8506492 [details] [diff] [review] Rewrite PresentedDNSIDMatchesReferenceDNSID, Part 2: Actually rewrite it Review of attachment 8506492 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixnames.cpp @@ +524,3 @@ > } while (!presented.AtEnd()); > > + // Allow an absolute presented DNS ID to match a relative reference DNS ID. Thanks for noticing. Fixed!
Comment on attachment 8506491 [details] [diff] [review] Rewrite PresentedDNSIDMatchesReferenceDNSID, Part 1: Add IsValidPresentedDNSID Review of attachment 8506491 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixnames.cpp @@ +908,5 @@ > + if (b != '.') { > + return false; > + } > + // There must be at most one '*' in a label. > + if (b == '*') { Thanks. I removed the (b == '*') test since it is redundant. ::: security/pkix/test/gtest/pkixnames_tests.cpp @@ +445,5 @@ > + // Multiple wildcards are not allowed. > + I("a**.b.c", false, false), > + I("a*b*.c.d", false, false), > + I("a*.b*.c", false, false), > + nixed. @@ +454,5 @@ > + I("a.b*.c", false, false), > + I("*.b*.c", false, false), > + I(".*.a.b", false, false), > + I(".a*.b.c", false, false), > + nixed.
Depends on: 1088998
See bug 1088998 comment 1, regarding the compatibility impact of this.
Keywords: site-compat
Depends on: 1089148
Depends on: 1089104
No longer depends on: 1089148
It seems clear from bug 1088998 / bug 1089104 / bug 1089527 that this isn't ready to ride the trains, and even being on Nightly is causing a lot of dogfooding issues. Shall we back this out?
Flags: needinfo?(brian)
No longer blocks: 1090151
Depends on: 1090151
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #53) > It seems clear from bug 1088998 / bug 1089104 / bug 1089527 that this isn't > ready to ride the trains, and even being on Nightly is causing a lot of > dogfooding issues. Shall we back this out? Not all the patches need to be backed out, just Part 8. I will do so now.
Flags: needinfo?(brian)
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #54) > (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #53) > > It seems clear from bug 1088998 / bug 1089104 / bug 1089527 that this isn't > > ready to ride the trains, and even being on Nightly is causing a lot of > > dogfooding issues. Shall we back this out? > > Not all the patches need to be backed out, just Part 8. I will do so now. Please respin nightlies once you're done.
This backout switching Gecko from mozilla::pkix's new function back to NSS's function, without removing the new mozilla::pkix code or tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed2cdcdb5240 (In reply to Reed Loden [:reed] from comment #55) > Please respin nightlies once you're done. I don't know how to do that, so I didn't. I will investigate the compatibility bug reports that led to me backing out this changeset, and propose a solution that addresses them. I believe that the only problem is that many websites are using TeletexString-encoded Common Name fields that contain the domain name for the certificate without the required-by-the-spec subjectAltName extension, but I will also check to see if there were other problems.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
:bsmith: I commented in another related bug about the corresponding chromium issue[1]. They've added a histogram there "for measuring the number of times we fall back to common name matching, when a certificate lacks a subjectAltName". Maybe that data would be beneficial for your investigation. Probably just ask in their issue tracker or you may have other contacts ;) [1] https://code.google.com/p/chromium/issues/detail?id=308330
https://hg.mozilla.org/mozilla-central/rev/ed2cdcdb5240
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
We're close enough to tonight's normally scheduled Nightly that I think we can skip triggering a new one.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The fix for bug 1089104 is the commit immediately ahead of this one. The bug can be RESOLVED FIXED again after this hits mozilla-central: https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c48ab6ee5e
No longer depends on: 1088998
https://hg.mozilla.org/mozilla-central/rev/c7c48ab6ee5e
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: