x/crypto/ocsp: ParseResponse makes incorrect choices about response and issuer verification #59641
Comments
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 <alex.scheel@hashicorp.com>
|
cc @golang/security |
seankhliao
added
the
NeedsInvestigation
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 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).
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.
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
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).
Seems reasonable to handle this redundancy, since it does appear in the wild. |
|
@rolandshoemaker writes:
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:
and further, the extended note is of relevance:
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:
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. |
|
Yeah, I think it'd probably also be reasonable to just iterate across all of the certificates in |
|
Thanks Roland! I'll pick this up on Monday with a patch set then.
- A
|
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 <alex.scheel@hashicorp.com>
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 <alex.scheel@hashicorp.com>
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 <alex.scheel@hashicorp.com>
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
|
Change https://go.dev/cl/485055 mentions this issue: |
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 <alex.scheel@hashicorp.com>
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 <alex.scheel@hashicorp.com>
* 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 <alex.scheel@hashicorp.com> * Add test case with Vault PKI Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Add changelog entry Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> --------- Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
x/crypto/ocspmakes an incorrect assumption about the contents of theBasicOCSPResponse'scertsfield and has a broken API as a result.Per RFC 6960 Section 4.2.1 ASN.1 Specification of the OCSP Response:
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'sParseResponseandParseResponseForCertificateincorrectly 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
issuerparameter value, Go incorrectly assumes that this issuer must have either directly issued the OCSP response or issued the first certificate in theBasicOCSPResponse'scertsfield.This has no basis in the RFC.
Further, and more risky is the beavhior around
issuer=nil: here, while theBasicOCSPResponse'scertsfield 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
BasicOCSPResponseis at least historically invalid for public CAs.For private CAs:
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
openssldo the right thing), it seems like fixing Go is the correct approach.On the usage side, the story is more complicated.
issuerin the response field, meaning if they'd be affected by this issue in Go's OCSP parsing if contacting one of these private CAs.nilissuer, though it looks like the chain-related version below has correctly specified an issuer.pdfsignappears 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-
nilissuer.Most usages of
nilparameters 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:
nilissuer here.Update the API to correctly return allSee conversation below.certsfields, allowing callers to perform more advanced chain building with anilissuer parameter if they desire.issuer == certs[0](i.e., ifissuer != certs[0], do the signature check that exists today).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)?Does this issue reproduce with the latest release?
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Above, including reproducer.
What did you expect to see?
Go should not have erred on the first
ocsp.ParseResponsecall with the issuer.What did you see instead?
Go erred with
bad OCSP signature: crypto/rsa: verification errorThe text was updated successfully, but these errors were encountered: