Open Bug 1024781 Opened 10 years ago Updated 2 years ago

when creating a domain mismatch error string, check that the subject alt name dNSName entries are valid names

Categories

(Core :: Security: PSM, defect, P5)

x86_64
Linux
defect

Tracking

()

People

(Reporter: cviecco, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-backlog])

Attachments

(1 obsolete file)

When creating a certificate error using the certificate's CN field we just use the field and assume it is a valid name for a host. We should be checking that the value is ether something that looks like a dns entry or an ip (v4 or v6) address.
Similarly, GetSubjectAltNames doesn't check that DNSName entries are valid DNS names.
Summary: creating a certificate error using the CN field of the cert should check that is a valid name → when creating a domain mismatch error string, check that the common name and subject alt name dNSName entries are valid names
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8557381 - Flags: review?(jjones)
Comment on attachment 8557381 [details] [diff] [review]
patch

Brian, this patch adds a few name-checking-related functions to pkix/pkix.h - I'd appreciate if you had a quick look at that. Thanks!
Attachment #8557381 - Flags: review?(brian)
Comment on attachment 8557381 [details] [diff] [review]
patch

Review of attachment 8557381 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't review the tests.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "TransportSecurityInfo.h"
>  
> +#include "pkix/pkix.h"

put in alphabetical order (after all the ns* ones).

@@ +692,5 @@
> +        if (current->name.other.len == 4) {
> +          addr.inet.family = PR_AF_INET;
> +          memcpy(&addr.inet.ip, current->name.other.data,
> +                 current->name.other.len);
> +          PR_NetAddrToString(&addr, buf, sizeof(buf));

Check error code.

@@ +698,5 @@
> +        } else if (current->name.other.len == 16) {
> +          addr.ipv6.family = PR_AF_INET6;
> +          memcpy(&addr.ipv6.ip, current->name.other.data,
> +                 current->name.other.len);
> +          PR_NetAddrToString(&addr, buf, sizeof(buf));

Check error code.

@@ +750,5 @@
>  
>    nsString allNames;
>    uint32_t nameCount = 0;
> +  bool useSAN = GetSubjectAltNames(nssCert.get(), component, allNames,
> +                                   nameCount);

I am not sure that GetSubjectAltNames has the same semantics as pkixnames.cpp uses. There are some subtleties such as "empty SAN" vs "non-empty SAN without any DNS names" vs. "non-empty SAN without any DNS names or IP addresses" that I've already forgotten. It might be better to add a function to pkixnames.cpp that goes through all the names and calls a caller-supplied callback with the encoded name, so that the semantics exactly match.

@@ +755,4 @@
>  
>    if (!useSAN) {
> +    ScopedPtr<char, PORT_Free_string> certName(
> +      CERT_GetCommonName(&nssCert->subject));

Simiarly, I'm can't remember whether CERT_GetCommonName has the same semantics as pkixnames.cpp. e.g. what happens when there are multiple CN AVAs in an RDN or multiple RDNs with CN attributes, etc.

Again, this would be something where the new function I suggest above would help.

@@ +760,5 @@
> +    Result result = certNameInput.Init(
> +      reinterpret_cast<uint8_t*>(certName.get()), strlen(certName.get()));
> +    if (result == Success &&
> +        (IsValidPresentedDNSID(certNameInput) ||
> +         IsValidIPv4Address(certNameInput))) {

Here, you're duplicating the logic in pkixnames.cpp that avoids IPv6 addresses in the CN attribute. At the very least, document that. But, again, the new callback I suggest would take care of that for you.

::: security/pkix/include/pkix/pkix.h
@@ +117,5 @@
>  //   backward compatibility (see SearchNames in pkixnames.cpp)
>  // - A wildcard in a DNS-ID may only appear as the entirety of the first label.
>  Result CheckCertHostname(Input cert, Input hostname);
>  
> +// Validate the given hostname. Returns true if it is a valid presented DNSID

s/DNSID/DNS-ID/

@@ +122,5 @@
> +// and false otherwise (see RFC 6125 and pkixnames.cpp). In particular,
> +// wildcards of the form *.example.com are valid.
> +bool IsValidPresentedDNSID(Input hostname);
> +
> +// Validate the given ip address. Returns true if it is a valid string

s/ip/IP/

::: security/pkix/lib/pkixnames.cpp
@@ -191,5 @@
>  
>  } // unnamed namespace
>  
>  bool IsValidReferenceDNSID(Input hostname);
> -bool IsValidPresentedDNSID(Input hostname);

Also remove the declaration in pkixnames_test.cpp.
Attachment #8557381 - Flags: review?(brian) → feedback+
Comment on attachment 8557381 [details] [diff] [review]
patch

Review of attachment 8557381 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the changes to security/pkix/*
Attachment #8557381 - Flags: review+
Comment on attachment 8557381 [details] [diff] [review]
patch

r=jcj

I looked at everything, but Brian covered the prime code well. I went through the tests too, and other than being distressed that the argument to certutil to add SANs is "-8 <DNS-names>", all looks well there too.
Attachment #8557381 - Flags: review?(jjones) → review+
Thank you both for the reviews. I think Brian is right in that it would be best to reimplement this in terms of a mozilla::pkix function that takes a callback and processes names the same way that SearchNames does. I'm not sure checking this in as an intermediate step is worth it, so I'm not going to for now.
This will actually probably be part of a future certificate linter written in JS that the frontend can use to provide more helpful information to the user about what's wrong with their certificate. In any case, I'm not working on this at the moment.
Assignee: dkeeler → nobody
Status: ASSIGNED → NEW
Whiteboard: [psm-backlog]
As of bug 1261936 we don't include the common name any more.
Summary: when creating a domain mismatch error string, check that the common name and subject alt name dNSName entries are valid names → when creating a domain mismatch error string, check that the subject alt name dNSName entries are valid names
Priority: -- → P5
Attachment #8557381 - Attachment is obsolete: true
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: