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

x/crypto/ocsp: Verify ProducedAt, ThisUpdate, and NextUpdate against cert validity window #45244

Open
twifkak opened this issue Mar 25, 2021 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@twifkak
Copy link

twifkak commented Mar 25, 2021

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

$ go version
go version go1.15.8 linux/amd64

Does this issue reproduce with the latest release?

Almost certainly. I don't see anything in https://github.com/golang/crypto/blob/master/ocsp/ocsp.go that would change the situation.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/twifkak/.cache/go-build"
GOENV="/home/twifkak/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/twifkak/.go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/twifkak/.go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.15"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.15/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build079748278=/tmp/go-build -gno-record-gcc-switches"

What did you do?

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

ocsp.ParseResponse() does not validate that the response's ProducedAt is within the NotBefore/NotAfter range of the provided `cert.

ocsp.ParseResponseForCert(), if given an OCSP response with multiple SingleResponses, does not necessarily pick the one with the correct ProducedAt per the same logic.

What did you expect to see?

&ocsp.Response{Status:2, ...
panic: no response matching the supplied certificate

What did you see instead?

&ocsp.Response{Status:0, ...
&ocsp.Response{Status:0, ...
@gopherbot gopherbot added this to the Unreleased milestone Mar 25, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 26, 2021
@seankhliao
Copy link
Member

cc @agl

@twifkak twifkak changed the title x/crypto/ocsp: Verify ProducedAt x/crypto/ocsp: Verify ProducedAt, ThisUpdate, and NextUpdate against cert validity window Mar 29, 2021
@twifkak
Copy link
Author

twifkak commented Mar 29, 2021

It looks like the library ought to check ThisUpdate and NextUpdate against the cert's validity window, too. LMK if you want me to provide a minified test case for you like I did with ProducedAt.

@twifkak
Copy link
Author

twifkak commented Mar 30, 2021

/cc @sleevi

@sleevi
Copy link

sleevi commented Mar 30, 2021

/cc @FiloSottile @rolandshoemaker

Note: I think historically, Go has taken a conservative approach, even though specific certificate profiles (e.g. Web PKI) may make more stringent requirements. I know @FiloSottile tries to find the right balance, but I did want to explicitly acknowledge he may decide to WontFix this and leave it to implementations to enforce :)

So, enforcing thisUpdate, nextUpdate, or producedAt is <= validity.notAfter should definitely not be done, because they are valid OCSP responses providing data for historic certificates ("archival purposes" in RFC 6960). Certain certificate profiles and application classes may totally want to prohibit such a scenario, but that's not something that generalizes to OCSP.

Enforcing thisUpdate is >= validity.notBefore is also something that should not be done. It's a statement of the responder's knowledge (as covered in https://tools.ietf.org/html/rfc6960#section-2.4 ), and it's conceivable they have PKI-specific context (e.g. it may be a reflection of an underlying credential type used by the CA to issue the actual certificate).

For the other bits, I think it may make sense specific to the application? We've not yet profiled these in the Web PKI, and the closest profile I'm aware of is the (mandatory-but-not-mandatory for Web PKI) RFC 5019, Section 2.2.4

@twifkak
Copy link
Author

twifkak commented Mar 30, 2021

Good to know. For ParseResponse it seems OK to me if the solution is documentation to explain that callers may need to check other things for specific profiles. (And even better if it provides specific advice or links.)

For ParseResponseForCert, it means that callers looking to enforce a specific profile may have to reimplement that function, because the ocsp lib doesn't provide a way to iterate over the SingleResponses of a Response. Hypothetically, a Response may contain two SingleResponses for the same cert, one of which has invalid dates and one of which has valid dates. If this case is more than hypothetical, then one solution is a new function, ParseResponsesForCert that returns a []*Response of all matching SingleResponses.

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

No branches or pull requests

4 participants