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

Jump to bottom

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
Closed

crypto/tls: RequireAndVerifyClientCert not rejecting bad client cert on 1.10 that it was on 1.9 #23884

psanford opened this issue Feb 17, 2018 · 7 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.10.1

Comments

@psanford
Copy link

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

/cc @agl @FiloSottile

@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label 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
Copy link
Contributor

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

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

@FiloSottile
Copy link
Contributor

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

@FiloSottile FiloSottile reopened this Feb 23, 2018
@FiloSottile FiloSottile removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 23, 2018
@FiloSottile FiloSottile mentioned this issue Feb 28, 2018
Closed
@liggitt liggitt mentioned this issue Mar 1, 2018
Closed
@liggitt
Copy link
Contributor

liggitt commented Mar 1, 2018 •
edited

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 The path to resolution is known, but the work has not been done. label Mar 13, 2018
@cblecker cblecker mentioned this issue Mar 16, 2018
Closed
4 tasks
@andybons andybons mentioned this issue Mar 27, 2018
Closed
@andybons
Copy link
Member

CL 96379 OK for Go 1.10.1

@andybons andybons added CherryPickApproved Used during the release process for point releases and removed NeedsFix The path to resolution is known, but the work has not been done. labels Mar 27, 2018
@gopherbot
Copy link

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
@agl @andybons
[release-branch.go1.10] crypto/x509: tighten EKU checking for request…
fe0d248 
…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>
andir added a commit to andir/nixpkgs that referenced this issue Mar 31, 2018
@andir
go_1_10: 1.10 -> 1.10.1
568d30b 
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
Merged
8 tasks
andir added a commit to andir/nixpkgs that referenced this issue Mar 31, 2018
@andir
go_1_10: 1.10 -> 1.10.1
19944b1 
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 pushed a commit to NixOS/nixpkgs that referenced this issue Apr 4, 2018
@andir @Mic92
go_1_10: 1.10 -> 1.10.1
ba49d72 
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.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.10.1
Development

No branches or pull requests
7 participants
@bradfitz @agl @psanford @andybons @liggitt @FiloSottile @gopherbot