-
Notifications
You must be signed in to change notification settings - Fork 18k
crypto/x509: Certificate Name Constraints starting with "." not validated properly #16347
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
Comments
/cc @agl |
This looks like an off-by-one in 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! |
@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 |
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:
However, this applies to URIs. My CA certificate (and essentially all others using Name Constraints) looks like this:
Because this is a DNS Name (and not a URI), that paragraph does not apply. Instead, the following paragraph does:
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 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
|
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;
} |
Interesting: The code I've shown above is from the current OpenSSL 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.
|
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).
Relevant code in NSS: https://github.com/racktopsystems/nss/blob/master/lib/certdb/genname.c#L1203 |
Please let me know what you think of https://go-review.googlesource.com/30155. |
CL https://golang.org/cl/30155 mentions this issue. |
30155 looks good to me. |
go version
)?go1.6.2
go env
)?darwin/amd64
A certificate with domain
test.example.com
is validated with an intermediate certificate having a name constraint of.example.com
.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).The certificate chain is not considered valid (
x509: a root or intermediate certificate is not authorized to sign in this domain
), whiletest..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 whytest..example.com
is considered valid).The text was updated successfully, but these errors were encountered: