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/x509: Certificate.Verify does not consistently return UnknownAuthorityError #58777

Open
rittneje opened this issue Feb 28, 2023 · 2 comments
Open

crypto/x509: Certificate.Verify does not consistently return UnknownAuthorityError #58777

rittneje opened this issue Feb 28, 2023 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@rittneje
Copy link

rittneje commented Feb 28, 2023 •
edited

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

$ go version
go version go1.19.6 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/rittneje/.cache/go-build"
GOENV="/home/rittneje/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/rittneje/go-workspace/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/rittneje/go-workspace"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/jrittner/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/rittneje/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.6"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/rittneje/go-workspace/src/certtest/go.mod"
GOWORK=""
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 -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3903595383=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I made three certificates - root, intermediate, and leaf. The intermediate is already expired. I then ran Certificate.Verify on the leaf certificate.

https://go.dev/play/p/WIWEm7L0Bel

What did you expect to see?

I expected to get back an x509.CertificateInvalidError wrapped inside an x509.UnknownAuthorityError.

What did you see instead?

I got back an unwrapped x509.CertificateInvalidError. Consequently, the resulting error message has insufficient context.

This is due to a scoping/shadowing bug in Certificate.buildChains.

go/src/crypto/x509/verify.go

Lines 926 to 933 in dd16258

err = candidate.isValid(certType, currentChain, opts)
if err != nil {
if hintErr == nil {
hintErr = err
hintCert = candidate
}
return
}

That err is the named return value from the outer scope. So at the end of the function, because it is non-nil, it is never wrapped.

go/src/crypto/x509/verify.go

Lines 955 to 957 in dd16258

if len(chains) == 0 && err == nil {
err = UnknownAuthorityError{c, hintErr, hintCert}
}

@dmitshur
Copy link
Contributor

Thanks for catching and reporting this.

CC @golang/security.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 28, 2023
@dmitshur dmitshur added this to the Backlog milestone Feb 28, 2023
@rolandshoemaker
Copy link
Member

Probably we should (a) always return a UnknownAuthorityError from Verify, wrapping errors discovered during chain building and (b) implement Unwrap on UnknownAuthorityError so that the hint can be accessed, since it is essentially a wrapped error.

There are currently cases where we expect to return a different error type though (as indicated by the fact we explicitly test for those cases). Perhaps it's reasonable to break those, especially if we implement Unwrap, but it is worth consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@dmitshur @rolandshoemaker @rittneje