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: "OCSP response contains bad number of certificates" error #21527

Closed
joeshaw opened this issue Aug 18, 2017 · 8 comments
Closed

x/crypto/ocsp: "OCSP response contains bad number of certificates" error #21527

joeshaw opened this issue Aug 18, 2017 · 8 comments
Milestone

Comments

@joeshaw
Copy link
Contributor

@joeshaw joeshaw commented Aug 18, 2017

Currently in the ocsp.ParseResponseForCert function is a block:

	if len(basicResp.Certificates) > 1 {
		return nil, ParseError("OCSP response contains bad number of certificates")
	}

I can't find any justification for this check (it dates back to @rsc's initial commit). RFC 6960 section 4.2.1 says,

   The value for response SHALL be the DER encoding of
   BasicOCSPResponse.

   BasicOCSPResponse       ::= SEQUENCE {
      tbsResponseData      ResponseData,
      signatureAlgorithm   AlgorithmIdentifier,
      signature            BIT STRING,
      certs            [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL }

   The value for signature SHALL be computed on the hash of the DER
   encoding of ResponseData.  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.

This leads me to believe that more than one cert is valid here, and in fact the http://sureseries-ocsp.cybertrust.ne.jp/OcspServer responder sends certs chaining up to a root.

The Go OCSP implementation doesn't verify the response certificate to a root, it just checks the signature against the issuer. (Is this sufficient? OpenSSL seems to do more.) But it seems like it shouldn't be an error to receive more than one certificate; it could simply do the check it currently does against the first one in the list.

/cc @agl

@gopherbot gopherbot added this to the Unreleased milestone Aug 18, 2017
@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented Aug 18, 2017

Sorry, misattributed the initial commit to Russ. He just moved it to a subrepo from the Go repo. Initial commit was 8286ee4 by Adam.

@kreichgauer
Copy link
Contributor

@kreichgauer kreichgauer commented Aug 18, 2017

This leads me to believe that more than one cert is valid here

That is correct, the Go ocsp package is just a bit limited and doesn't have support for more complex responses. From https://godoc.org/golang.org/x/crypto/ocsp#ParseResponse:

ParseResponse parses an OCSP response in DER form. It only supports responses for a single certificate.

/cc @agl

@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented Aug 18, 2017

From https://godoc.org/golang.org/x/crypto/ocsp#ParseResponse:

ParseResponse parses an OCSP response in DER form. It only supports responses for a single certificate.

I interpreted this to mean a single certificate sent in the request. Not a single OCSP response certificate.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 21, 2017

Change https://golang.org/cl/57510 mentions this issue: ocsp: remove error for > 1 certificate in response

@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented May 1, 2018

@agl @FiloSottile I would love to get this in for 1.11 if the approach makes sense. It causes us to get "OCSP response contains bad number of certificates" for any certificates that use cybertrust.ne.jp's various OCSP responders. For example, https://www.superchoice.bet/

@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented May 1, 2018

🤦‍♂️ duh, I forgot this was in x/crypto and doesn't follow the release process. Still, please take a look once the 1.11 freeze fury has died down!

@joeshaw
Copy link
Contributor Author

@joeshaw joeshaw commented May 2, 2018

It was mentioned in the CL, but for better visibility: At the time this was merged there were many OCSP responders that triggered this. 209 by my count: https://crt.sh/ocsp-responders?get=OCSP+response+contains+bad+number+of+certificates&sort=2&dir=v

@robstradling
Copy link

@robstradling robstradling commented May 9, 2018

Just FYI, crt.sh now has this fix, and all of the "bad number of certificates" errors have disappeared.

@golang golang locked and limited conversation to collaborators May 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.