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: ParseResponse makes incorrect choices about response and issuer verification #59641

Open
cipherboy opened this issue Apr 14, 2023 · 10 comments · May be fixed by golang/crypto#256
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@cipherboy
Copy link
Contributor

cipherboy commented Apr 14, 2023

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:

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 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 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, 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 field pointing to the leaf's issuing CA (which also issued the OCSP response).
  • Dogtag PKI's OCSP responder does similarly, 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 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 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 library has the same issue.
  • Caddy's certmagic has the same issue.
  • Google's webpacker, 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 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: #21527

See also: #40017

Reproducer: https://go.dev/play/p/Nr-VKOD_fxH


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

$ go version
go version go1.20.3 linux/amd64

Does this issue reproduce with the latest release?

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

go env Output
$ 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"

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

@seankhliao
Copy link
Member

cc @golang/security

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 14, 2023
@rolandshoemaker
Copy link
Member

Notably, the RFC makes no assertions about the contents of these certificates, whether they are strictly delegated OCSP issuers (as discussed in #40017 or whether they can include or not include intermediate CAs necessary to tie the OCSP responder's cert back to a concrete root.

I think the RFC is a little vague and confusing (in an extremely PKIX-y way) here. There is no restriction put on what can be in certs, just saying that they help the OCSP client verify the responder's signature. That said RFC 6960 Section 4.2.2.2. requires that if a delegated signing certificate is used it should be issued by the certificate that signed the certificate being checked. In the delegation case certs then only really needs to contain a single certificate, but the RFC leaves the door open to put whatever else you want in there.

In theory you can put the rest of the chain (e.g. delegated signer -> intermediate issuer -> root) in the response, but we have no interest in verifying the rest of that chain (or any other chain in the response) since we only care that the delegated signer links back to the issuer of the certificate being checked (https://go.dev/cl/57510 contains some previous discussion of this behavior, and why we do what we do).

I think one of the reasons the API is designed in the way it is, and assumes that the caller has the issuer on hand, is that if you are verifying the validity of a certificate, you are, 99% of the time, going to have its issuer on hand. In theory there could be value in verifying OCSP responses that contain the fully chain from OCSP signer to certificate issuer for cases where you do not have the issuer already, but I'm not sure there are any real world cases where this is the case (at least ones I'd want to explicitly support).

For private CAs

These are interesting examples, and given there are implementations in the wild we probably need to consider supporting them, but I'm not entirely clear what value is being added here.

From the Vault perspective, what is the rationale for including the issuing certificate (which isn't delegated) to the response? Is it expected that the relying party won't already have it? It seems like this unnecessarily inflates the size of the response.

  1. Update the docs to more adequately warn about specifying a nil issuer here.

Seems reasonable to say this should only be done if the response was provided over an otherwise trusted channel. Even with that said though we probably also should be explicit that if issuer == nil you need to check that the response was signed by a certificate issued by the certificate that signed the response (I wish we'd never included signature verification in parsing, this API is cursed).

  1. 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.

Not sure I'm convinced this is something we really want to support. Just exposing all of the certificates sent isn't particularly complicated, but it encourages users to use OCSP in a way that isn't really relevant to web PKI (which is the subset of PKIX that we target, in an attempt to prune away the overwhelming complexity that is PKIX), and that I don't think we have any particular interest in providing broader support for elsewhere (i.e. in the standard library).

  1. Fix the library to not err if issuer == certs[0] (i.e., if issuer != certs[0], do the signature check that exists today).

Seems reasonable to handle this redundancy, since it does appear in the wild.

@cipherboy
Copy link
Contributor Author

@rolandshoemaker writes:

That said RFC 6960 Section 4.2.2.2. requires that if a delegated signing certificate is used it should be issued by the certificate that signed the certificate being checked. In the delegation case certs then only really needs to contain a single certificate, but the RFC leaves the door open to put whatever else you want in there.

I don't think this is quite a valid read, though I agree in practice.

Notably, that paragraph in RFC 6960 goes on to write:

The CA SHOULD use the same issuing key to issue a delegation certificate as that used to sign the certificate being checked for revocation.

and further, the extended note is of relevance:

Note: For backwards compatibility with RFC 2560, it is not
prohibited to issue a certificate for an Authorized
using a different issuing key than the key used to issue the
certificate being checked for revocation. However, such a
practice is strongly discouraged, since clients are not
required to recognize a responder with such a certificate as an
Authorized Responder.

Which is to say, this is likely another case where the CA the Entity (with a capital E) is different than "the concrete certificate with the IsCA basis constraint assert) and something legacy did happen, I agree.

But, more to the point, nothing guarantees the first item is also the responder.


Finishing the investigation, I think EJBCA has the same issue:

Following that BouncyCastle lead:

  • Maritime Connectivity PKI seems to have the same behavior as Vault, assuming it is configured to directly use the ICA and not a delegated signer (unverified whether this is possible or not).
  • If my read of otoroshi is correct this directly uses the root CA and again places it in the certs field.

So yeah, we can fix this on the Vault side, but this is a lot of additional private CAs--including EJBCA--that have this same behavior.

Go is broken validating all of them.

@rolandshoemaker
Copy link
Member

Yeah, I think it'd probably also be reasonable to just iterate across all of the certificates in certs to find one that validates the signature, rather than always picking the first one 🤷.

@cipherboy
Copy link
Contributor Author

cipherboy commented Apr 14, 2023 via email

cipherboy added a commit to cipherboy/crypto that referenced this issue Apr 17, 2023
We make three changes here:

 1. Allow iterating over all given certificates to find the one that
    signed this OCSP response, as RFC 6960 does not guarantee an order
    and some CAs send multiple certificates, and
 2. Allow the passed issuer to match the certificate that directly
    signed this response, and
 3. Lastly, we document the unsafe behavior of calling these functions
    with issuer=nil, indicating that it performs no trust verification.

Previously, when a CA returned the intermediate CA that signed a leaf
cert as an additional cert in the response field (without using a
delegated OCSP certificate), Go would err with a bad signature, as it
expected the intermediate CA to have signed the wire copy.

Also includes a code comment around the "bad signature on embedded
certificate" error message, indicating that this isn't strictly
the correct preposition choice.

See also: https://github.com/crtsh/test_websites_monitor/blob/1bd8226b5f963e91d7889ea432a36e3173be8eec/test_websites_monitor.go#L267
See also: golang/go#59641

Signed-off-by: Alexander Scheel <[email protected]>
cipherboy added a commit to cipherboy/crypto that referenced this issue Apr 17, 2023
We make three changes here:

 1. Allow iterating over all given certificates to find the one that
    signed this OCSP response, as RFC 6960 does not guarantee an order
    and some CAs send multiple certificates, and
 2. Allow the passed issuer to match the certificate that directly
    signed this response, and
 3. Lastly, we document the unsafe behavior of calling these functions
    with issuer=nil, indicating that it performs no trust verification.

Previously, when a CA returned the intermediate CA that signed a leaf
cert as an additional cert in the response field (without using a
delegated OCSP certificate), Go would err with a bad signature, as it
expected the intermediate CA to have signed the wire copy.

Also includes a code comment around the "bad signature on embedded
certificate" error message, indicating that this isn't strictly
the correct preposition choice.

See also: https://github.com/crtsh/test_websites_monitor/blob/1bd8226b5f963e91d7889ea432a36e3173be8eec/test_websites_monitor.go#L267
See also: golang/go#59641

Signed-off-by: Alexander Scheel <[email protected]>
cipherboy added a commit to cipherboy/crypto that referenced this issue Apr 17, 2023
We make three changes here:

 1. Allow iterating over all given certificates to find the one that
    signed this OCSP response, as RFC 6960 does not guarantee an order
    and some CAs send multiple certificates, and
 2. Allow the passed issuer to match the certificate that directly
    signed this response, and
 3. Lastly, we document the unsafe behavior of calling these functions
    with issuer=nil, indicating that it performs no trust verification.

Previously, when a CA returned the intermediate CA that signed a leaf
cert as an additional cert in the response field (without using a
delegated OCSP certificate), Go would err with a bad signature, as it
expected the intermediate CA to have signed the wire copy (even though
it was the exact same certificate).

Also includes a code comment around the "bad signature on embedded
certificate" error message, indicating that this isn't strictly
the correct preposition choice.

See also: https://github.com/crtsh/test_websites_monitor/blob/1bd8226b5f963e91d7889ea432a36e3173be8eec/test_websites_monitor.go#L267
See also: golang/go#59641

Signed-off-by: Alexander Scheel <[email protected]>
cipherboy added a commit to cipherboy/crypto that referenced this issue Apr 17, 2023
We make three changes here:

 1. Allow iterating over all given certificates to find the one that
    signed this OCSP response, as RFC 6960 does not guarantee an order
    and some CAs send multiple certificates, and
 2. Allow the passed issuer to match the certificate that directly
    signed this response, and
 3. Lastly, we document the unsafe behavior of calling these functions
    with issuer=nil, indicating that it performs no trust verification.

Previously, when a CA returned the intermediate CA that signed a leaf
cert as an additional cert in the response field (without using a
delegated OCSP certificate), Go would err with a bad signature, as it
expected the intermediate CA to have signed the wire copy (even though
it was the exact same certificate).

Also includes a code comment around the "bad signature on embedded
certificate" error message, indicating that this isn't strictly
the correct preposition choice.

See also: https://github.com/crtsh/test_websites_monitor/blob/1bd8226b5f963e91d7889ea432a36e3173be8eec/test_websites_monitor.go#L267
See also: golang/go#59641

Fixes golang/go#59641
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/485055 mentions this issue: ocsp: better validate OCSP response's certificates

cipherboy added a commit to cipherboy/crypto that referenced this issue Apr 17, 2023
We make three changes here:

 1. Allow iterating over all given certificates to find the one that
    signed this OCSP response, as RFC 6960 does not guarantee an order
    and some CAs send multiple certificates, and
 2. Allow the passed issuer to match the certificate that directly
    signed this response, and
 3. Lastly, we document the unsafe behavior of calling these functions
    with issuer=nil, indicating that it performs no trust verification.

Previously, when a CA returned the intermediate CA that signed a leaf
cert as an additional cert in the response field (without using a
delegated OCSP certificate), Go would err with a bad signature, as it
expected the intermediate CA to have signed the wire copy (even though
it was the exact same certificate).

Also includes a code comment around the "bad signature on embedded
certificate" error message, indicating that this isn't strictly
the correct preposition choice.

See also: https://github.com/crtsh/test_websites_monitor/blob/1bd8226b5f963e91d7889ea432a36e3173be8eec/test_websites_monitor.go#L267
See also: golang/go#59641

Fixes golang/go#59641

Signed-off-by: Alexander Scheel <[email protected]>
cipherboy added a commit to hashicorp/vault that referenced this issue Apr 17, 2023
When calling ocsp.ParseRequest(req, issue) with a non-nil issuer on a
ocsp request which _unknowingly_ contains an entry in the
BasicOCSPResponse's certs field, Go incorrectly assumes that the issuer
is a direct parent of the _first_ certificate in the certs field,
discarding the rest.

As documented in the Go issue, this is not a valid assumption and thus
causes OCSP verification to fail in Vault with an error like:

> bad OCSP signature: crypto/rsa: verification error

which ultimately leads to a cert auth login error of:

> no chain matching all constraints could be found for this login certificate

We address this by using the unsafe issuer=nil argument, taking on the
task of validating the OCSP response's signature as best we can in the
absence of full chain information on either side (both the trusted
certificate whose OCSP response we're verifying and the lack of any
additional certs the OCSP responder may have sent).

See also: golang/go#59641

Signed-off-by: Alexander Scheel <[email protected]>
cipherboy added a commit to hashicorp/vault that referenced this issue Apr 17, 2023
* Add fix for Go x/crypto/ocsp failure case

When calling ocsp.ParseRequest(req, issue) with a non-nil issuer on a
ocsp request which _unknowingly_ contains an entry in the
BasicOCSPResponse's certs field, Go incorrectly assumes that the issuer
is a direct parent of the _first_ certificate in the certs field,
discarding the rest.

As documented in the Go issue, this is not a valid assumption and thus
causes OCSP verification to fail in Vault with an error like:

> bad OCSP signature: crypto/rsa: verification error

which ultimately leads to a cert auth login error of:

> no chain matching all constraints could be found for this login certificate

We address this by using the unsafe issuer=nil argument, taking on the
task of validating the OCSP response's signature as best we can in the
absence of full chain information on either side (both the trusted
certificate whose OCSP response we're verifying and the lack of any
additional certs the OCSP responder may have sent).

See also: golang/go#59641

Signed-off-by: Alexander Scheel <[email protected]>

* Add test case with Vault PKI

Signed-off-by: Alexander Scheel <[email protected]>

* Add changelog entry

Signed-off-by: Alexander Scheel <[email protected]>

---------

Signed-off-by: Alexander Scheel <[email protected]>
@aarongable
Copy link
Contributor

aarongable commented Jul 8, 2024

It looks like the linked Gerrit CL was filed over a year ago, but it was never reviewed and no further progress was made.

This has become relevant again in this (invalid) bugzilla CA incident report. The incident report was filed because an external tool (SSLMate's OCSP Watch) was reporting that the CA's OCSP responses are malformed because it can't verify their signature. However what is actually happening is that the CA has embedded the issuing certificate (which is not a delegated responder) in the OCSP response's certs field. Therefore x/crypto/ocsp's requirement that certs[0] be signed by issuer fails, because those are in fact the same CA.

What is the status of the attached CL? Does the Go crypto team intend to review it and move towards merging it? Has there been a decision to not move forward with a slightly-more-permissive ocsp.ParseResponseForCert API?

I'd be happy to take over moving this forward, if Alex Scheel no longer wants to drive it.

(note for the purpose of keeping this cross-referenced: the fact that ocsp.ParseResponse does signature verification at all is somewhat surprising and bad. It would likely be better if there were separate dedicated parsing and verification APIs. This would also allow bugs like not verifying the delegated responder's validity period to be fixed. Such a separation is proposed in this bug, but that appears to be blocked on moving OCSP out of //x/crypto and into //crypto itself.)

@AGWA
Copy link

AGWA commented Jul 9, 2024

While the CA in question might not be explicitly violating RFC 6960, their responder is nevertheless misconfigured, and they have agreed to fix their responder to omit the superfluous certificate, which would make their responses work with x/crypto/ocsp. In light of that, I'm not sure that addressing this issue is necessary or desirable.

In particular, I'd like to echo this comment by @agl:

So the PKIX RFCs err strongly on the side of complexity, but that results in an over-complex mess and must be subsetted. The trick is pick the right battles and, over time, try to bring the complexity down.

In that case, a considerable number of WebPKI CAs had misconfigured OCSP responders, so it was pragmatic for Go to become more permissive. In this case, there is only one (brand new) WebPKI CA affected, and they're fixing their responder, which makes this a good place for Go to hold the line.

Regardless of the outcome of this CL, I will ensure that OCSP Watch continues to reject these broken respones, in order to prevent a regression towards more complexity and brokenness in the WebPKI.

@rolandshoemaker
Copy link
Member

This slipping through the cracks is my fault. Given this is the first time we've heard about it cropping up in the wild again though makes me agree with @AGWA that perhaps this does not seem so widespread of a misconfiguration to warrant additional complexity.

Our behavior here is strange though, in that the documentation is vague in what we enforce, and we should at least be documenting the constraints that ParseResponse has (and perhaps why), since they are clearly not obvious (especially to anyone who hasn't been staring at PKIX RFCs for multiple hours a day).

I think really what we need, as @aarongable mentions, is a new, somewhat less cursed OCSP API, but that requires investing the time to design one.

@aarongable
Copy link
Contributor

aarongable commented Jul 9, 2024

...their responder is nevertheless misconfigured...
...reject these broken respones...

I strongly disagree with both of these characterizations. Their OCSP responses are fully compliant with both the letter and the spirit of RFC 6960, and with other major OCSP-validation implementations. Their responder is not misconfigured, and their responses are not broken. They are just incompatible with Go's implementation decisions.

In this case, there is only one (brand new) WebPKI CA affected, and they're fixing their responder, which makes this a good place for Go to hold the line.

But I think this is a totally reasonable stance to have. I think it's fine for Go to take a strongly opinionated stance here, with two caveats:
a) as Roland says, that stance needs to be well-documented (I'm happy to file a PR to improve the documentation here, and then this issue and other related CLs should be closed); and
b) ideally the WebPKI community should have some way to know that violating Go's constraints here is a meaningful finding that should be fixed, but not a CA incident.

I think really what we need is a new, somewhat less cursed OCSP API

My back-of-the-napkin proposal would be to mirror x509.RevocationList, i.e. only provide two methods:

func ParseOCSPResponse(der []byte) (*OCSPResponse, error)
func (or *OCSPResponse) CheckSignatureFrom(parent *Certificate) (error)

The CheckSignatureFrom method would require that the signature "chain up to" to the provided issuer certificate, but would be willing to use one of the certificates from the response's certs field as a delegated responder between the response itself and the provided issuer. It would enforce that the delegated responder has the id-kp-OCSPSigning EKU, but nothing else (e.g. it wouldn't check that the delegated responder is not expired or not revoked).

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

Successfully merging a pull request may close this issue.

6 participants