Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
floridoo opened this issue Jul 13, 2016 · 10 comments
Milestone

Comments

@floridoo
Copy link

floridoo commented Jul 13, 2016

  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
Copy link
Contributor

/cc @agl

@quentinmit quentinmit added this to the Go1.8 milestone Jul 20, 2016
@JonathonReinhart
Copy link

JonathonReinhart commented Jul 25, 2016

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
Copy link
Author

@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
Copy link

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:

Other examples that don't specify:

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

@JonathonReinhart
Copy link

JonathonReinhart commented Jul 26, 2016

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

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
Copy link

JonathonReinhart commented Jul 26, 2016

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 referenced this issue in JonathonReinhart/go Jul 26, 2016
As specified in RFC5280:

  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.

The first conditional `opts.DNSName == domain` takes care of the case
where the constraint applies to a host; if the domain matches the
constraint exactly, then it is satisfied.

Now we are left with the case where the constraint is a domain (which
can match sub-domains). We can verify this with two checks:
- Is the first character of the constraint a period?
- Is the constraint a suffix of the domain in question?

The second check ensures that we are not matching part of a label in the
domain name.

This change simplifies the code, and makes it read just like the RFC.

The existing code exhibits an off-by-one error. It is effectively
checking to see if the character in the DNS name, right before the
matched suffix is a period. This is incorrect, because it doesn't
account for the period in the matched suffix, and instead looks at the
character *before* the period. (e.g. the 'b' in sub.example.com, if
domain = ".example.com"). We could simply remove the -1, but the code
would still be overly complex, considering we already verified the
period is present in opts.DNSName (by checking HasSuffix).
@JonathonReinhart
Copy link

@agl agl self-assigned this Aug 19, 2016
@agl
Copy link
Contributor

agl commented Oct 1, 2016

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

@gopherbot
Copy link

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

@JonathonReinhart
Copy link

30155 looks good to me.

@golang golang locked and limited conversation to collaborators Oct 2, 2017
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants