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: NameConstraintsWithoutSANs when checking signing certificate #24151

Open
thsnr opened this Issue Feb 27, 2018 · 41 comments

Comments

Projects
None yet
10 participants
@thsnr

thsnr commented Feb 27, 2018

What did you do?

A CA which issues personal signing certificates has specified X.509 Name Constraints to exclude any DNS names and IP addresses:

X509v3 Name Constraints:
    Excluded:
      DNS:""
      IP:0.0.0.0/0.0.0.0
      IP:0:0:0:0:0:0:0:0/0:0:0:0:0:0:0:0

This is good practice to protect against misissued certificates.

Attempt to verify a test certificate issued by that CA: https://play.golang.org/p/y4l1JJqDQPs

What did you expect to see?

I expected the verification to succeed as it did in go 1.9 and earlier.

What did you see instead?

Starting from go 1.10, verification fails with NameConstraintsWithoutSANs:

x509: issuer has name constraints but leaf doesn't have a SAN extension

It is true that the signing certificates do not contain SAN extensions, because they have no need for one. This error did not trigger before, because when verifying a signing certificate, no DNS name is specified. But as stated in the change log for go 1.10:

Certificate.Verify now enforces the name constraints for all names contained in the certificate, not just the one name that a client has asked about.

I believe this is a bug, because RFC 5280 Section 4.2.1.10 regarding Name Constraints states:

Restrictions apply only when the specified name form is present. If no name of the type is in the certificate, the certificate is acceptable.

I understand this behavior is there for cases where we encounter a legacy TLS server certificate which relies on the Common Name as the hostname, but other certificates are now also hit by this. Maybe Certificate.Verify should distinguish between TLS server certificates and other X.509 certificates and have NameConstraintsWithoutSANs only trigger for the first ones?

Does this issue reproduce with the latest release (go1.10)?

Yes, go 1.10 is where it was introduced.

System details

go version go1.10rc2 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/tiit/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/tiit/go"
GORACE=""
GOROOT="/usr/lib/go-1.10"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.10/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build045278460=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.10rc2 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.10rc2
uname -sr: Linux 4.14.0-3-amd64
Distributor ID:	Debian
Description:	Debian GNU/Linux testing (buster)
Release:	testing
Codename:	buster
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Debian GLIBC 2.26-6) stable release version 2.26, by Roland McGrath et al.
gdb --version: GNU gdb (Debian 7.12-6+b1) 7.12.0.20161007-git
@agl

This comment has been minimized.

Contributor

agl commented Feb 27, 2018

If there are no SANs in the certificate, then how are you assigning names to the leafs? crypto/x509 is intended to implement the WebPKI and, there, using common names as hostnames has been deprecated for years and support is being dropped in major clients.

@thsnr

This comment has been minimized.

thsnr commented Feb 27, 2018

The name is specified in the Subject. For the example test certificate it is:

C = EE, O = ESTEID, OU = digital signature, CN = "\C5\BDAIKOVSKI,IGOR,37101010021", SN = \C5\BDAIKOVSKI, GN = IGOR, serialNumber = 37101010021

As mentioned, these are personal signing certificates (nonRepudiation/contentCommitment; specifically, signing certificates of the Estonian national ID-card) and not server certificates, so the Common Name is not used as a hostname, but just to identify the signing person.

I understand that the primary use case is TLS and WebPKI, but before this change, the package could also be successfully used for other RFC-compliant X.509 certificates.

@agl

This comment has been minimized.

Contributor

agl commented Feb 27, 2018

Do the in-use Estonian ID cards also have this property, or is this just a test CA?

@thsnr

This comment has been minimized.

thsnr commented Feb 27, 2018

Both the in-use and test chains have this property. Example OpenSSL output from actual chain:
ca.txt
cert.txt

@agl

This comment has been minimized.

Contributor

agl commented Feb 27, 2018

This is fairly dodgy by the Estonian ID system: they're setting DNS constraints but expecting those constraints not to apply to the CN (where DNS names historically went) and yet are also not including the SAN extension to signal that the certificate is new enough to know not to wedge DNS names into the CN.

A workaround for this would look like ignoring name constraints when no SANs exist if no |DNSName| is requested in the VerifyOptions. It's a hack, but it is, admittedly only a small one. I'll code it up and see how it looks.

@thsnr

This comment has been minimized.

thsnr commented Feb 27, 2018

Would this break the explicit goal set by Go 1.10?

As a result, after a certificate has been validated, now it can be trusted in its entirety. It is no longer necessary to revalidate the certificate for each additional name or key usage.

As I understand, the idea is that now I can do Certificate.Verify with an empty DNSName which checks all the names on the certificate and later just call Certificate.VerifyHostname with any hostname without having to reverify. With the proposed hack, if presented with a legacy TLS certificate that has no SAN and a Common Name which does NOT satisfy the Name Constraints of the issuer, then both Certificate.Verify and Certificate.VerifyHostname will succeed.

@agl

This comment has been minimized.

Contributor

agl commented Feb 27, 2018

Yes, that's a good point—it would break that.

We could add a flag on the Certificate to indicate that the CN shouldn't be used by a later VerifyHostname, but that seems fragile and surprising. We could drop support for stuffing DNS names in the CN, but that's likely to cause many more problems.

Unless someone has a clever suggestion, it's not clear to me that we should change anything here.

@martinpaljak

This comment has been minimized.

martinpaljak commented Feb 28, 2018

@agl Nowhere in https://golang.org/pkg/crypto/x509/ does it read that this package is (solely) about WebPKI. Might want to clarify that in the docs?

@thsnr

This comment has been minimized.

thsnr commented Feb 28, 2018

@agl What about moving the NameConstraintsWithoutSANs check into Certificate.VerifyHostname?

We do not do any Name Constraints validation in Certificate.isValid if no SAN is present, but return NameConstraintsWithoutSANs from Certificate.VerifyHostname if we end up matching against the Common Name and the new Certificate.PermittedDNSDomains or Certificate.ExcludedDNSDomains fields are not empty.

This would also work if only Certificate.Verify is called with a DNSName, because all it does is call Certificate.VerifyHostname after Certificate.isValid.

EDIT: After reading the RFC in more detail, the following requirement should be kept in mind if considering my proposed solution:

Legacy implementations exist where an electronic mail address is embedded in the subject distinguished name in an attribute of type emailAddress (Section 4.1.2.6). When constraints are imposed on the rfc822Name name form, but the certificate does not include a subject alternative name, the rfc822Name constraint MUST be applied to the attribute of type emailAddress in the subject distinguished name.

The Estonian ID system does not set any constraints on the email, but this could affect some other CAs.

@agl

This comment has been minimized.

Contributor

agl commented Feb 28, 2018

@agl What about moving the NameConstraintsWithoutSANs check into Certificate.VerifyHostname?

VerifyHostname takes only a leaf certificate, but the property of whether or not constraints apply is a property of the validation chain. (I.e. the root certificate can trigger it.)

My best idea for this is to allow constraints without SANs if the CN doesn't parse as a valid DNS name, and have VerifyHostname ignore the CN if it doesn't parse.

@thsnr

This comment has been minimized.

thsnr commented Feb 28, 2018

Ah, of course. For some reason I thought that the new *DNSDomains fields would include data from the entire chain, but it makes much more sense that they only reflect the extensions on the certificate itself. Maybe set some (unexported?) flag on the leaf Certificate showing if there were any Name Constraints in the chain?

Otherwise checking the CN would seem to work. Could consulting the Extended Key Usage for Server Authentication help in some way?

@thsnr

This comment has been minimized.

thsnr commented Mar 22, 2018

@agl Any updates on this?

@agl

This comment has been minimized.

Contributor

agl commented Mar 22, 2018

I believe the fix for this is scheduled for 1.10.1.

@rsc rsc added the Proposal-Crypto label Mar 26, 2018

@andybons

This comment has been minimized.

Member

andybons commented Mar 26, 2018

@agl @FiloSottile can you dupe this into the 1.10.1 fix?

@andybons andybons added this to the Go1.10.1 milestone Mar 26, 2018

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Mar 26, 2018

AFAICT this one doesn't have a fix yet, and it still doesn't work on tip.

@agl

This comment has been minimized.

Contributor

agl commented Mar 26, 2018

Sorry, when I said above that "I believe the fix for this is scheduled for 1.10.1", I was confusing this with #23995 and didn't notice until Filippo just said that we didn't have a fix. I don't think we've actually decided whether we want to do something here.

@andybons

This comment has been minimized.

Member

andybons commented Mar 27, 2018

OK. Then this is outside the 1.10.1 window. Moving to 1.11 for now.

@andybons andybons modified the milestones: Go1.10.1, Go1.11 Mar 27, 2018

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Mar 27, 2018

Should we consider dropping support for hostnames in CN altogether? The browsers managed to.

@FiloSottile FiloSottile modified the milestones: Go1.11, Go1.10.2 Mar 27, 2018

@svenheiberg

This comment has been minimized.

svenheiberg commented Mar 28, 2018

So far we have seen three potential solutions to this.

a) Drop support for hostnames in CN altogether.

b) Allow for NameConstraints without SANs in Certificate.isValid. Instead set a flag in the leaf Certificate struct noting that there were NameConstraints in the chain. Later, when calling Certificate.VerifyHostName (either directly or via Certificate.Verify with a DNSName) on the leaf, return NameConstraintsWithoutSANs if the flag is set and there are no SANs. This works because Certificate.VerifyHostname is never called for signing certificates.

c) Allow for NameConstraints without SANs in Certificate.isValid ONLY IF the CN of the leaf Certificate does not parse as a valid DNS name. Otherwise return NameConstraintsWithoutSANs. Later, when calling Certificate.VerifyHostname, ignore the CN if it does not parse. This works because signing certificates usually do not have names that parse as DNS names.

From the perspective of current issue all these solutions are equally good. Is there a reason not to implement one of those for go 1.11?

@user8547

This comment has been minimized.

user8547 commented Mar 28, 2018

This is fairly dodgy by the Estonian ID system: they're setting DNS constraints but expecting those constraints not to apply to the CN (where DNS names historically went) and yet are also not including the SAN extension to signal that the certificate is new enough to know not to wedge DNS names into the CN.

@agl, can you assume that if CA uses DNS constraints then the certificates issued by that CA are new enough to know not to wedge DNS names into the CN?

Would such assumption lead to much simpler fix?

@gopherbot

This comment has been minimized.

gopherbot commented Apr 1, 2018

Change https://golang.org/cl/103868 mentions this issue: crypto/x509: allow non-DNS name constraints without SANs

@agl

This comment has been minimized.

Contributor

agl commented Apr 1, 2018

Should we consider dropping support for hostnames in CN altogether? The browsers managed to.

That would be nice. However, 1.10 has been overly "exciting" w.r.t. certificate validation so I'm dialing towards being more conservative at the moment.

I think of all the options enumerated by @svenheiberg, I like (c) the most. However, the example certificate in given above has a CN of PIKMA,TIIT,<REDACTED>. It's not clear that's an invalid DNS name. We are quite accepting of DNS names and only ban absolute names, empty labels, and bytes 0–33, 126–255. There are a bunch of more restrictive rules in RFCs about the format of hostnames, but they have rotted over time and aren't actually true in practice any longer.

So, if the REDACTED part always contains a space then it would happen to work. I guess we could also ban commas, but it feels like we're really crafting a special case for these Estonian certificates in that case.

Thus I'm wondering about an option (d): require SANs for DNS name constraints only. (I think @user8547 might have been suggesting this just above.) The point of the error is to ensure that we don't have a gap between VerifyHostname and Verify, and we already ignore CN for IP addresses.

https://go-review.googlesource.com/c/go/+/103868 to do that.

@rsc

This comment has been minimized.

Contributor

rsc commented Apr 20, 2018

It sounds like more discussion is needed about what exactly we should do in Go 1.10. The current proposal is a bit sharp to change in a point release. It might be better in Go 1.11, but that leaves open what to do for Go 1.10, if anything.

@gopherbot

This comment has been minimized.

gopherbot commented Apr 23, 2018

Backport issue(s) opened: #25016 (1.10).

Remember to create the cherry-pick CL(s) as soon as the patch is ready, according to https://golang.org/wiki/MinorReleases.

@FiloSottile FiloSottile modified the milestones: Go1.10.2, Go1.11 Apr 23, 2018

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented May 4, 2018

Based on discussion with @agl, @bradfitz and @ianlancetaylor, we will do step 1 (provided it does not cause a lot of verification errors against CT logs, or widespread test failures) and step 2 of #24151 (comment) in 1.11. Step 3 will come in 1.12, once we'll have asked people to test with the GODEBUG flag. Step 4 is TBD.

1.10 has already been too exciting without breaking another class of certificates to fix this one, so no backport.

@rsc

This comment has been minimized.

Contributor

rsc commented Jun 25, 2018

Looks like the previous comment is the decision.

@rsc rsc added NeedsFix and removed NeedsDecision labels Jun 25, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Jul 11, 2018

Change https://golang.org/cl/123355 mentions this issue: crypto/x509: ignore Common Name when it does not parse as a hostname

@gopherbot

This comment has been minimized.

gopherbot commented Jul 12, 2018

Change https://golang.org/cl/123695 mentions this issue: crypto/x509: add GODEBUG option x509ignoreCN=1

gopherbot pushed a commit that referenced this issue Jul 16, 2018

crypto/x509: ignore Common Name when it does not parse as a hostname
The Common Name is used as a hostname when there are no Subject
Alternative Names, but it is not restricted by name constraints. To
protect against a name constraints bypass, we used to require SANs for
constrained chains. See the NameConstraintsWithoutSANs error.

This change ignores the CN when it does not look like a hostname, so we
can avoid returning NameConstraintsWithoutSANs.

This makes it possible to validate certificates with non-hostname CN
against chains that use name constraints to disallow all names, like the
Estonian IDs.

Updates #24151

Change-Id: I798d797990720a01ad9b5a13336756cc472ebf44
Reviewed-on: https://go-review.googlesource.com/123355
Reviewed-by: Adam Langley <agl@golang.org>

gopherbot pushed a commit that referenced this issue Jul 16, 2018

crypto/x509: add GODEBUG option x509ignoreCN=1
When x509ignoreCN=1 is present in GODEBUG, ignore the deprecated Common
Name field. This will let people test a behavior we might make the
default in the future, and lets a final class of certificates avoid the
NameConstraintsWithoutSANs error.

Updates #24151

Change-Id: I1c397aa1fa23777b9251c311d02558f9a5bdefc0
Reviewed-on: https://go-review.googlesource.com/123695
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Aug 2, 2018

For people Googling x509: Common Name is not a valid hostname: treating Common Name as a hostname is deprecated. In 1.11, Go will not support it if the CN is not a valid hostname, future versions might not support it at all. (This is not a gratuitous change, it's required to make name constraints work, see above.) You can either change the CN to a valid hostname, and/or (preferred) switch to Subject Alternative Names. (Or, you could write your own CN verification instead of using VerifyHostname if you know you don't care about name constraints, but please don't do this, and if you have to, let us know here.)

tl;dr: you can't put an invalid hostname in CN and use it to verify the cert, use SANs instead.

See also https://tip.golang.org/doc/go1.11#crypto/x509

@rsc

This comment has been minimized.

Contributor

rsc commented Aug 17, 2018

Not sure what's left but it's for Go 1.12.

@rsc rsc modified the milestones: Go1.11, Go1.12 Aug 17, 2018

@egtann

This comment has been minimized.

egtann commented Aug 25, 2018

@FiloSottile For those of us connecting to Google Cloud SQL (which uses invalid common names), what's the best way forward? At this point we're stuck using go1.10, since anything compiled as-is with go1.11 is broken.

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Aug 27, 2018

@egtann If you are using the cloudsql-proxy package, they already fixed it, you can just update. Otherwise, you will have to make a change similar to GoogleCloudPlatform/cloudsql-proxy#196 to support the CloudSQL certificates until they add SANs (which AFAIK they plan to but I have no ETA).

@FiloSottile FiloSottile assigned FiloSottile and unassigned agl Aug 27, 2018

@egtann

This comment has been minimized.

egtann commented Aug 27, 2018

Thanks @FiloSottile for your help.

We won't be adding a cloud-specific proxy to our deployments just to connect to a SQL server over TLS, so we're left with applying this patch ourselves.

As somebody who isn't well versed on the ins-and-outs of the TLS protocol, is there anything we should be concerned about when InsecureSkipVerify is set to true? If we provide a VerifyPeerCertificate function that checks the common name against the instance name as in the linked patch, is that wholly sufficient to prevent MITM attacks?

As an aside, it seems much better from a security perspective for Googlers with the appropriate experience to implement a temporary fix or provide some package we can easily import. As it is, the go1.11 patch broke builds in a way that's only discoverable at runtime, and to fix those once-working builds you're asking programmers without necessarily any security experience whatsoever to enable an Insecure option in production and implement their own individual workarounds.

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Aug 28, 2018

@egtann I realize this is not optimal, I'm sorry about that. You can lift the VerifyPeerCertificate function from that PR as is. You want to be doing both cert.Verify (although you don't need the custom Roots) and the cert.Subject.CommonName check.

I think I will just add : to the whitelist after all for now, but the final goal is to deprecate the CN field.

@egtann

This comment has been minimized.

egtann commented Aug 28, 2018

The fix worked without a hitch. Thanks again, @FiloSottile!

@gopherbot

This comment has been minimized.

gopherbot commented Sep 7, 2018

Change https://golang.org/cl/134076 mentions this issue: crypto/x509: allow ":" in Common Name hostnames

gopherbot pushed a commit that referenced this issue Sep 7, 2018

crypto/x509: allow ":" in Common Name hostnames
At least one popular service puts a hostname which contains a ":"
in the Common Name field. On the other hand, I don't know of any name
constrained certificates that only work if we ignore such CNs.

Updates #24151

Change-Id: I2d813e3e522ebd65ab5ea5cd83390467a869eea3
Reviewed-on: https://go-review.googlesource.com/134076
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Sep 7, 2018

Change https://golang.org/cl/134078 mentions this issue: [release-branch.go1.11] crypto/x509: allow ":" in Common Name hostnames

gopherbot pushed a commit that referenced this issue Sep 7, 2018

[release-branch.go1.11] crypto/x509: allow ":" in Common Name hostnames
At least one popular service puts a hostname which contains a ":"
in the Common Name field. On the other hand, I don't know of any name
constrained certificates that only work if we ignore such CNs.

Updates #24151

Change-Id: I2d813e3e522ebd65ab5ea5cd83390467a869eea3
Reviewed-on: https://go-review.googlesource.com/134076
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 03c703697f321f66d28d6223457622c5879ba37f)
Reviewed-on: https://go-review.googlesource.com/134078
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment