crypto/x509: Certificate Name Constraints starting with "." not validated properly

[floridoo]

1. What version of Go are you using (go version)?
   go1.6.2
2. What operating system and processor architecture are you using (go env)?
   darwin/amd64
3. What did you do?
   A certificate with domain test.example.com is validated with an intermediate certificate having a name constraint of .example.com.
4. What did you expect to see?
   The certificate chain should be valid. A name constraint .example.com should allow all subdomains of example.com, but not the domain itself (according to the RFC).
5. What did you see instead?
   The certificate chain is not considered valid (x509: a root or intermediate certificate is not authorized to sign in this domain), while test..example.com works.
   The problem is the check at https://github.com/golang/go/blob/master/src/crypto/x509/verify.go#L172, that enforces a . as the last character of the domain prefix (that's why test..example.com is considered valid).

[quentinmit]

/cc @agl

[JonathonReinhart]

This looks like an off-by-one in crypto/x509/verify.go:

            if opts.DNSName == domain ||
                (strings.HasSuffix(opts.DNSName, domain) &&
                    len(opts.DNSName) >= 1+len(domain) &&
                    opts.DNSName[len(opts.DNSName)-len(domain)-1] == '.') {
                ok = true

I've prepared a fix: JonathonReinhart@ca0494a

            if opts.DNSName == domain ||
                (domain[0] == '.' && strings.HasSuffix(opts.DNSName, domain)) {
                ok = true

Edit: Sorry @floridoo I totally missed your point 5!

[floridoo]

@JonathonReinhart: Your fix would disallow subdomains for rules without a leading dot. The proper check should be the existing + the leading dot case:

            if opts.DNSName == domain ||
                (strings.HasSuffix(opts.DNSName, domain) &&
                    len(opts.DNSName) >= 1+len(domain) &&
                    opts.DNSName[len(opts.DNSName)-len(domain)-1] == '.') ||
                (domain[0] == '.' && strings.HasSuffix(opts.DNSName, domain) {
                ok = true

[JonathonReinhart]

Interesting - there appears to be a disconnect between the RFC and many examples of x509 Name Constraints (for DNS names).

I was mistakenly looking at (and quoting) the following paragraph from section 4.2.1.10 of RFC 5280 which says:

For URIs, the constraint applies to the host part of the name. The
constraint MUST be specified as a fully qualified domain name and MAY
specify a host or a domain. Examples would be "host.example.com" and
".example.com". When the constraint begins with a period, it MAY be
expanded with one or more labels. That is, the constraint
".example.com" is satisfied by both host.example.com and
my.host.example.com. However, the constraint ".example.com" is not
satisfied by "example.com". When the constraint does not begin with
a period, it specifies a host.

However, this applies to URIs. My CA certificate (and essentially all others using Name Constraints) looks like this:

            X509v3 Name Constraints: critical
                Permitted:
                  DNS:..example.com

Because this is a DNS Name (and not a URI), that paragraph does not apply. Instead, the following paragraph does:

DNS name restrictions are expressed as host.example.com. Any DNS
name that can be constructed by simply adding zero or more labels to
the left-hand side of the name satisfies the name constraint. For
example, www.host.example.com would satisfy the constraint but
host1.example.com would not.

Interestingly, this never says anything about a leading period. I interpreted a leading period to mean valid certificates MUST have at least one additional leading label. (e.g. the name example.com does not satisfy the constraint .example.com). The RFC appears to leave the behavior associated with a leading period on a DNS name constraint undefined.

Here are examples that include a leading period:

• Constraints: what they are and how they’re used (Windows PKI Blog)
• Using Name Constraints (Technet)
• CAs, Name Constraints, and a Business Opportunity (karl.kornel.us blog)
• Hellenic Academic and Research Institutions Cert. Authority 2011 (An actual CA cert, trusted by Mozilla, Microsoft, and Apple)

Other examples that don't specify:

• Name Constraints in x509 Certificates
• x509v3_config (OpenSSL docs)

Finally (after hours of digging), an example that never uses a dot, and explicitly implies that DNS:bad.com means *.bad.com:

• test/certs/setup.sh (OpenSSL test suite)

[JonathonReinhart]

Relevant OpenSSL source (which I'm still digging through):

• check_name_constraints()
• crypto/x509v3/v3_ncons.c
  • static int nc_match(GENERAL_NAME *gen, NAME_CONSTRAINTS *nc)
  • static int nc_match_single(GENERAL_NAME *gen, GENERAL_NAME *base)
    • This seems to be where the types of name constraints are differentiated.
  • static int nc_dns(ASN1_IA5STRING *dns, ASN1_IA5STRING *base)
    • DNS constraints are checked

static int nc_dns(ASN1_IA5STRING *dns, ASN1_IA5STRING *base)
{
    char *baseptr = (char *)base->data;
    char *dnsptr = (char *)dns->data;
    /* Empty matches everything */
    if (!*baseptr)
        return X509_V_OK;
    /*  
     * Otherwise can add zero or more components on the left so compare RHS
     * and if dns is longer and expect '.' as preceding character.
     */
    if (dns->length > base->length) {
        dnsptr += dns->length - base->length;
        if (*baseptr != '.' && dnsptr[-1] != '.')
            return X509_V_ERR_PERMITTED_VIOLATION;
    }   

    if (strcasecmp(baseptr, dnsptr))
        return X509_V_ERR_PERMITTED_VIOLATION;

    return X509_V_OK;

}

[JonathonReinhart]

Interesting: The code I've shown above is from the current OpenSSL master branch. nc_dns() was changed via openssl/openssl@77ff1f3, after bug report RT3562 (the 3662 cited in the commit is a typo). That change allowed leading dots in DNS name constraints, even though the RFC doesn't specifically allow it.

So @floridoo I'm essentially in agreement with you now. My previous understanding, that the dot MUST be present for a subdomain match, was incorrect.

crypto/x509 should do the same thing OpenSSL did, and allow the dots to be present, but not required. I think #16347 (comment) is quite messy, and this conditional is too complex to be one expression. The way OpenSSL does it looks much cleaner, IMO.

[JonathonReinhart]

Relevant code in NSS: https://github.com/racktopsystems/nss/blob/master/lib/certdb/genname.c#L1203

[agl]

Please let me know what you think of https://go-review.googlesource.com/30155.

[gopherbot]

CL https://golang.org/cl/30155 mentions this issue.

[JonathonReinhart]

30155 looks good to me.