crypto/x509: verification fails with "cannot parse dnsName" in intermediate

freman commented 6 years ago

What version of Go are you using (`go version`)?

go version go1.10 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (`go env`)?

GOARCH="amd64" GOBIN="" GOCACHE="/Users/shannon/Library/Caches/go-build" GOEXE="" GOHOSTARCH="amd64" GOHOSTOS="darwin" GOOS="darwin" GOPATH="/Users/shannon/go" GORACE="" GOROOT="/usr/local/go" GOTMPDIR="" GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64" GCCGO="gccgo" CC="clang" CXX="clang++" 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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/08/9qf64r3d209cvmgznzj49zg40000gn/T/go-build638418624=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

So it seems at some point in our distant past, we ended up with an intermediate CA that had an altname incorrectly configured. Probably something we munged while configuring Hashicorps Vault as our CA so many months ago.

The end result being that anything written in go and compiled with go 1.10 now fails all our tls connections (where curl, and openssl succeed)

Example with a dodgy intermediate: https://play.golang.org/p/SeS_uNyqnci
Same code with a better intermediate: https://play.golang.org/p/-JaU_x318rO

nb, these examples don't actually run on play due to the time being pinned in the past :)

In the real world these certs are part of a bundle offered up by nginx, caddy, vault, consul, and individual services so its a tls error like this we see

tls: failed to parse certificate from server: x509: cannot parse dnsName "SnakeOil Intermediate CA 2"

One might argue that the intermediate certificates are invalid, but another equally fair argument is that maybe we don't need to verify altnames on certificates that are marked for CA's

            X509v3 Basic Constraints: critical
                CA:TRUE

The only evidence I have to support that is that curl, openssl, and chrome don't seem to care.

What did you expect to see?

yay

What did you see instead?

panic: x509: cannot parse dnsName "SnakeOil Intermediate CA 2"

goroutine 1 [running]:
main.main()
    /Users/shannon/tlsissue/ca/test/main.go:164 +0x59f
exit status 2

FiloSottile commented 6 years ago

This is #23711, except here the invalid dnsName is in an intermediate instead of in a leaf. As @agl said this was part of the work to introduce name constraints #15196.

We might decide to ignore these errors in the chain in some cases, but I suspect that would introduce a lot of complexity. To @agl as I'm still getting familiar with crypto/x509.

1.10.1 because technically a regression.

freman commented 6 years ago

I'm pretty sure that it's (in this case) on the intermediate and the root for us.

I figured it would be a case of checking the if it's a CA cert before checking if it has alt names but I confess I didn't look that deep in the code.

agl commented 6 years ago

This is horrifying. None the less, it could be called a regression since invalid SANs in CA certificates are not a security problem that I can see.

I've prepared a change to address in case Go folks want to land it, although my recommendation would be a weak no.

gopherbot commented 6 years ago

Change https://golang.org/cl/96378 mentions this issue: crypto/x509: allow invalid SANs in CA certificates.

FiloSottile commented 6 years ago

I made the title more discoverable, let's see if this affects a lot of people. (If this affects you, please comment or use the GitHub reaction feature.) I would like Go to be a force for cleanup in the X.509 ecosystem too, but a widespread regression would not be acceptable.

We should check that CreateCertificate will refuse to make invalid dnsNames too before closing.

l33t0 commented 6 years ago

Ah the joys, I've been bitten by this as well.

freman commented 6 years ago

I would definitely be in favor of making CreateCertificate incapable of generating bad certs, but breaking certs that are already out there is causing inconvenience. In fact I'm pro breaking those certs too, but it'd be nice if that was some how optional because people have to work.

jamie-digital commented 6 years ago

Our service identifies issues in customers' websites. One part of this is to identify cert chains involving blacklisted certs, such as DigiNotar's. The update to go1.10 has broken this, as it now fails to parse the invalid but blacklisted cert /C=US/O=Global Trustee/OU=Global Trustee/L=Tampa/ST=Florida/SA=Sea Village 10/PC=38477/CN=global trustee:

x509: cannot parse dnsName "global trustee"

FiloSottile commented 6 years ago

@jamie-digital For my own curiosity, where does that certificate show up? Because I see it has CA:FALSE.

jamie-digital commented 6 years ago

@FiloSottile I found it in a list of blacklisted CA certs, so presumably someone has/had it in their trust pool. Earlier versions of X.509 didn't have the isCA field, so implementations often don't require the flag for root certs.

FiloSottile commented 6 years ago

@jamie-digital I found the source of that certificate, and it's a weird artefact of a breach. The other certificates were for real sites. https://bugzilla.mozilla.org/show_bug.cgi?id=642395

This means it was never trusted as a root, and with CA:FALSE it's harmless, so in this case you can safely drop checking for it.

pschultz commented 6 years ago

This will affect all CA certs that have been generated by HashiCorp Vault 0.9.3 or earlier. Those versions always put the CN also into the DNS alt names, without checking if it actually is a DNS name. hashicorp/vault#3953 fixed that in anticipation of the change in Go.

Relaxing the verification for CA certs would greatly reduce pain for Vault operators because they wouldn't have to re-issue all of their cert chains.

msvechla commented 6 years ago

This currently affects our entire vault setup as-well, due to reasons already mentioned by @pschultz. This forces us to re-issue all our CAs, including our root-ca. Making this verification optional would really help us out a lot.

bradfitz commented 6 years ago

I'm fine including @agl's fix for this in 1.10.1. @ianlancetaylor?

agl commented 6 years ago

Given the unexpected spread of this mistake, I'm now at "weak yes" for including this change.

titanous commented 6 years ago

In my opinion this isn't a regression in Go. It is an error to put something that isn't a DNS hostname in a dNSName SAN, and failing to parse/verify such certificates is a reasonable and good thing.

It's unfortunate that some software was generating invalid certificates, but I think they should be replaced instead of letting Go's (already intentionally strict) parser accept them.

FiloSottile commented 6 years ago

Yeah, alas, I agree we need to fix (well, break) this for 1.10.1. It's a fairly widespread break in functionality and a semantic change (parsing/verifying unneeded fields) not just an increase in verification strictness.

ianlancetaylor commented 6 years ago

Are we talking about putting https://golang.org/cl/96378 into 1.10.1? I'm fine with that if everyone else is although it would be nice to commit it to tip first.

agl commented 6 years ago

Reopening because of potential 1.10.1 inclusion.

pzduniak commented 6 years ago

I actually used an EasyRSA CA setup that generated such invalid certs. Nothing complained about it before.

jefferai commented 6 years ago

Just to clarify re: https://github.com/golang/go/issues/23995#issuecomment-368784560 this didn't break any cert issued where the CN was a valid dns name, and adding the CN to SANs is optional but on by default. Basically, it's in the same boat as a lot of other CAs that didn't validate DNS compatibility, but saying it broke all certs issued by Vault is overkill and incorrect.

jefferai commented 6 years ago

@titanous

In my opinion this isn't a regression in Go. It is an error to put something that isn't a DNS hostname in a dNSName SAN, and failing to parse/verify such certificates is a reasonable and good thing.

It's unfortunate that some software was generating invalid certificates, but I think they should be replaced instead of letting Go's (already intentionally strict) parser accept them.

The problem is that such software relying on Go for their crypto stack were allowed, by Go, to generate such invalid certificates, and then disallowed, by Go, to actually read them back once Go 1.10 came out. If Go had never allowed these certificates to be generated in the first place due to strict parsing and validity requirements, I'd be more agreeable to your statement, but I strongly suggest an "allow old/broken certs, disallow generating new broken certs" approach based on where we are right now.

We can all agree it would be nice if such broken certs didn't exist in the first place, but there are plenty of things in the RFC that various libraries only adhere to to some extent or another. In fact, the x509 library explicitly breaks with the spec with verify behavior (https://golang.org/pkg/crypto/x509/#VerifyOptions). Allowing non-DNS-compatible names in DNSNames was likely an unintentional break, unlike the verification logic, but if every crypto stack out there has been successfully accepting such certs that Go created, and Go has been successfully accepting such certs that other CAs created, it's reasonable for users to assume that this was simply a widely-allowed relaxation of the spec (especially given the complexity of dealing with otherNames with Go's asn1 library, which pretty much ends up necessitating using the out-of-stdlib cryptobytes lib).

trunet commented 6 years ago

will this be included on 1.10.1, or we'll have to wait til 1.11?

andybons commented 6 years ago

CL 96378 OK for Go 1.10.1

gopherbot commented 6 years ago

Change https://golang.org/cl/102783 mentions this issue: [release-branch.go1.10] crypto/x509: parse invalid DNS names and email addresses.

golang / go