-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
Comments
cc @golang/security |
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 <[email protected]>
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]>
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]>
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 <[email protected]>
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 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]>
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 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.) |
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:
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. |
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. |
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.
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:
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 |
x/crypto/ocsp
makes an incorrect assumption about the contents of theBasicOCSPResponse
'scerts
field 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
'sParseResponse
andParseResponseForCertificate
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 theBasicOCSPResponse
'scerts
field.This has no basis in the RFC.
Further, and more risky is the beavhior around
issuer=nil
: here, while theBasicOCSPResponse
'scerts
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:
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.
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.nil
issuer, though it looks like the chain-related version below has correctly specified an issuer.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:
nil
issuer here.Update the API to correctly return allSee conversation below.certs
fields, allowing callers to perform more advanced chain building with anil
issuer 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 env
OutputWhat 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
The text was updated successfully, but these errors were encountered: