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/tls: RequireAndVerifyClientCert not rejecting bad client cert on 1.10 that it was on 1.9 #23884

Closed
psanford opened this issue Feb 17, 2018 · 7 comments

Comments

@psanford
Copy link

@psanford psanford commented Feb 17, 2018

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

$ go version
go version go1.10 linux/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="/home/psanford/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/psanford/projects/nearbuy/storenet/go"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/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-build417289417=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran the following program on 1.9.4 and 1.10:

https://play.golang.org/p/gt67v9Ih7Te

The https server is requiring client certs with RequireAndVerifyClientCert. The client is using a weird (bad?) cert that has ExtKeyUsage: x509.ExtKeyUsageServerAuth instead of x509.ExtKeyUsageClientAuth.

What did you expect to see?

On 1.9.4 the https server rejects the weird client cert:

2018/02/17 02:19:06 http: TLS handshake error from 127.0.0.1:57370: tls: failed to verify client's certificate: x509: certificate specifies an incompatible key usage
panic: Get https://127.0.0.1:4443: remote error: tls: bad certificate

What did you see instead?

On 1.10 it accepts the client cert.

response status: 200 OK
Hello, "/"
done
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 17, 2018

@bradfitz bradfitz added this to the Go1.10.1 milestone Feb 17, 2018
@bradfitz bradfitz changed the title https: RequireAndVerifyClientCert not rejecting bad client cert on 1.10 that it was on 1.9 crypto/tls: RequireAndVerifyClientCert not rejecting bad client cert on 1.10 that it was on 1.9 Feb 17, 2018
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Feb 17, 2018

It's because we have this mess to try and work around the fact that CAs have repeatedly botched extended key usage in the past. It was made by running lots of tests on the CT logs and ServerAuth is counted as sufficient to allow ClientAuth. But I think there's a fair argument to be made that, while that might be needed when checking a chain, we don't need it when checking elements of the leaf cert. (Although we do need other exceptions then.)

The best evidence for that is probably that we evidently didn't previously allow it and that didn't break the world.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 22, 2018

Change https://golang.org/cl/96379 mentions this issue: crypto/x509: tighten EKU checking for requested EKUs.

@gopherbot gopherbot closed this in 0681c7c Feb 23, 2018
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Feb 23, 2018

Reopening to consider for backport to 1.10.1. (Is this how it works these days?)

@liggitt

This comment has been minimized.

Copy link
Contributor

@liggitt liggitt commented Mar 1, 2018

Reopening to consider for backport to 1.10.1. (Is this how it works these days?)

given that 1.10.0 validates plain serving certificates as if they were client certificates, I'd expect a pick to 1.10.x

@andybons andybons added the NeedsFix label Mar 13, 2018
@cblecker cblecker mentioned this issue Mar 16, 2018
3 of 4 tasks complete
@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Mar 27, 2018

CL 96379 OK for Go 1.10.1

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 27, 2018

Change https://golang.org/cl/102780 mentions this issue: [release-branch.go1.10] crypto/x509: tighten EKU checking for requested EKUs.

gopherbot pushed a commit that referenced this issue Mar 29, 2018
…ed EKUs.

There are, sadly, many exceptions to EKU checking to reflect mistakes
that CAs have made in practice. However, the requirements for checking
requested EKUs against the leaf should be tighter than for checking leaf
EKUs against a CA.

Fixes #23884

Change-Id: I05ea874c4ada0696d8bb18cac4377c0b398fcb5e
Reviewed-on: https://go-review.googlesource.com/96379
Reviewed-by: Jonathan Rudenberg <jonathan@titanous.com>
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Run-TryBot: Filippo Valsorda <hi@filippo.io>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/102780
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
@andybons andybons closed this Mar 29, 2018
andir added a commit to andir/nixpkgs that referenced this issue Mar 31, 2018
This updates go to the latest version of the golang 1.10 branch.
A few minor (but important) things are fixed in this version:

* CVE-2018-7187 - arbitrary code execution in `go get` (when used with
  --insecure) [1]
* Extended Key Usage verification in client certificate scenarios [3]
* a bunch of stability changes

The full list of changes can se been on GitHub [2] & [4].

[1] https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-7187
[2] https://github.com/golang/go/issues?q=milestone%3AGo1.10.1
[3] golang/go#23884
[4] golang/go#24563
@andir andir mentioned this issue Mar 31, 2018
4 of 8 tasks complete
andir added a commit to andir/nixpkgs that referenced this issue Mar 31, 2018
This updates go to the latest version of the golang 1.10 branch.
A few minor (but important) things are fixed in this version:

* CVE-2018-7187 - arbitrary code execution in `go get` (when used with
  --insecure) [1]
* Extended Key Usage verification in client certificate scenarios [3]
* a bunch of stability changes

The full list of changes can se been on GitHub [2] & [4].

[1] https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-7187
[2] https://github.com/golang/go/issues?q=milestone%3AGo1.10.1
[3] golang/go#23884
[4] golang/go#24563

(cherry picked from commit 568d30b)
Mic92 added a commit to NixOS/nixpkgs that referenced this issue Apr 4, 2018
This updates go to the latest version of the golang 1.10 branch.
A few minor (but important) things are fixed in this version:

* CVE-2018-7187 - arbitrary code execution in `go get` (when used with
  --insecure) [1]
* Extended Key Usage verification in client certificate scenarios [3]
* a bunch of stability changes

The full list of changes can se been on GitHub [2] & [4].

[1] https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-7187
[2] https://github.com/golang/go/issues?q=milestone%3AGo1.10.1
[3] golang/go#23884
[4] golang/go#24563

(cherry picked from commit 568d30b)
@golang golang locked and limited conversation to collaborators Mar 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.