Repository: go
Issue:  x/crypto/ocsp: ParseResponse makes incorrect choices about response and issuer verification 
Labels: NeedsInvestigation

`x/crypto/ocsp` makes an incorrect assumption about the contents of the `BasicOCSPResponse`'s `certs` field and has a broken API as a result.

---

Per [RFC 6960 Section 4.2.1 ASN.1 Specification of the OCSP Response](https://datatracker.ietf.org/doc/html/rfc6960#section-4.2.1): 

>  The responder MAY include certificates in
>  the `certs` field of `BasicOCSPResponse` that help the OCSP client verify
>  the responder's signature.  If no certificates are included, then
>  `certs` SHOULD be absent.

Notably, the RFC makes no assertions about the contents of these certificates, whether they are strictly delegated OCSP issuers (as discussed in [another Go issue](https://github.com/golang/go/issues/40017) or whether they can include or not include intermediate CAs necessary to tie the OCSP responder's cert back to a concrete root.

However, `x/crypto/ocsp`'s `ParseResponse` and `ParseResponseForCertificate` incorrectly assume that any certificates included in the response are a delegated OCSP signer and its chain, assuming the delegated OCSP signer is the first certificate. This is not guaranteed by the RFC and is not assumed by other OCSP libraries. Go is unique and non-conformant in this regard.

In particular, _if_ the caller passes an `issuer` parameter value, Go [incorrectly assumes](https://github.com/golang/crypto/blob/1faeef9713563c936e077b84e4c3a0f3cac0fbe4/ocsp/ocsp.go#L569-L579) that this issuer _must_ have either directly issued the OCSP response or issued the first certificate in the `BasicOCSPResponse`'s `certs` field. 

This has no basis in the RFC.

Further, and more risky is the beavhior around `issuer=nil`: here, while the `BasicOCSPResponse`'s `certs` field is used to verify the signature, no grounding to a trusted certificate. This is _only_ safe over a secure transport for OCSP requests/responses and the docs make no mention of this. (cc @golang/security and @rolandshoemaker).

---

As discussed at [a previous issue](https://github.com/golang/go/issues/21527), this assumption about the structure of `BasicOCSPResponse` is at least historically invalid for public CAs.

For private CAs:

 * Vault's PKI OCSP responser provisions the [Certificates](https://github.com/hashicorp/vault/blob/19f7eb6eeb0d7c698a9d748db2b58889ca8d0720/builtin/logical/pki/path_ocsp.go#L508) field pointing to the leaf's issuing CA (which also issued the OCSP response).
 * Dogtag PKI's OCSP [responder does similarly](https://github.com/dogtagpki/pki/blob/0270f3aa981970014574429d4abc50907110830d/base/ocsp/src/main/java/com/netscape/ocsp/OCSPAuthority.java#L558-L574), but supports external (delegated) OCSP certificates in addition to provisioning the full chain of the leaf.
 * Without running, it appears the Packetfence OCSP responder [follows the same approach](https://github.com/inverse-inc/packetfence/blob/9b792facf54420720c3b317900ee4d16c31a0abb/go/caddy/pfpki/ocspresponder/ocspresponder.go#L174-L212) wherein the CA certificate is again provisioned here in the Certificate field.

I've not checked other CA libraries, but given we've now found three independent private CAs with this behavior that aren't forbidden by spec, and given Go seems alone in this behavior (and other tools like `openssl` do the right thing), it seems like fixing Go is the correct approach.

On the _usage_ side, the story is more complicated.

 * Square's [certigo](https://github.com/square/certigo/blob/b78a846ab962134ea5c48b746e0e99199d55da28/lib/ocsp.go#L98-L101) library uses the `issuer` in the response field, meaning if they'd be affected by this issue in Go's OCSP parsing if contacting one of these private CAs.
 * Snowflake's [ocsp.go](https://github.com/snowflakedb/gosnowflake/blob/22e2112c59681537b0dc9e443f41d9dc2f3721a4/ocsp.go#L440-L446) library has the same issue.
 * Caddy's [certmagic](https://github.com/caddyserver/certmagic/blob/462f5e2334a762adc04d2e9ca6200239ffb49a5c/ocsp.go#L192-L214) has the same issue.
 * Google's [webpacker](https://github.com/google/webpackager/blob/53a1486f4205374a111a683a64aa5eedbacbbc7c/certchain/ocsp_response.go#L50-L64), might _appear_ to use this in a security-critical context with a `nil` issuer, though it looks like the chain-related version below has correctly specified an issuer.
 * Digitorus's `pdfsign` appears to read [OCSP info](https://github.com/digitorus/pdfsign/blob/4a3aef1578dab1b4d09385a201862d9f1aed01e5/verify/verify.go#L223-L237) out of the PDF context and attempt to verify the info there, which might be considered a security problem.

This shows that consumers of Go's OCSP library are likely unaware of the implications of the API design of passing a non-`nil` issuer.

Most usages of `nil` parameters _seem_ safe on first glance, but their authors should probably investigate further. Since I didn't see any _really_ obvious incorrect usages, and the other behavior is a fail-closed behavior, I did not think this warranted a security issue.

---

I'd propose the following fixes:

 1. Update the docs to more adequately warn about specifying a `nil` issuer here.
 2. ~Update the API to correctly return all `certs` fields, allowing callers to perform more advanced chain building with a `nil` issuer parameter if they desire.~ See conversation below.
 3. Fix the library to not err if `issuer == certs[0]` (i.e., if `issuer != certs[0]`, do the signature check that exists today).
 4. Iterate over passed certificates to find the correct one that signed this request.

This appears to be the minimally invasive sets of changes that allow more complex libraries tao get more correct behavior while affecting the library the least.

As I get time, and if people here are happy with this, I'll open a CL with these changes.

---

See also: https://github.com/golang/go/issues/21527

See also: https://github.com/golang/go/issues/40017

Reproducer: https://go.dev/play/p/Nr-VKOD_fxH

---

### What version of Go are you using (`go version`)?

<pre>
$ go version
go version go1.20.3 linux/amd64
</pre>

### Does this issue reproduce with the latest release?



### What operating system and processor architecture are you using (`go env`)?

<details><summary><code>go env</code> Output</summary><br><pre>
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cipherboy/.cache/go-build"
GOENV="/home/cipherboy/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/cipherboy/go/pkg/mod"
GONOPROXY="github.com/hashicorp"
GONOSUMDB="github.com/hashicorp"
GOOS="linux"
GOPATH="/home/cipherboy/go"
GOPRIVATE="github.com/hashicorp"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/cipherboy/GitHub/cipherboy/vault/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build652088920=/tmp/go-build -gno-record-gcc-switches"
</pre></details>

### What did you do?

Above, including reproducer.

### What did you expect to see?

Go should not have erred on the first `ocsp.ParseResponse` call with the issuer.

### What did you see instead?

Go erred with `bad OCSP signature: crypto/rsa: verification error`