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 responses signed by invalid OCSP responder certificate should return signature verification error #43522

Open
pboguslawski opened this issue Jan 5, 2021 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@pboguslawski
Copy link

pboguslawski commented Jan 5, 2021

In case of OCSP reponse signed with embedded OCSP responder cert (not by CA cert directly) ParseResponse from ocsp package does not check if embedded OCSP responder certificate is expired.

It seems...

https://pkg.go.dev/golang.org/x/crypto/ocsp#ParseResponse
https://github.com/golang/crypto/blob/master/ocsp/ocsp.go#L550

...only signatures are checked. This allows one to use old, expired OCSP responder certificate and its key to sign OCSP response and go application using ocsp.ParseResponse package will accept this response but should not (checked in go1.15.2 linux/amd64 but master sources seems to contain the same problem).

OpenSSL in such scenario throws Response Verify Failure error:

OCSP routines:OCSP_basic_verify:certificate verify error:../crypto/ocsp/ocsp_vfy.c:93:Verify error:certificate has expired

Please see OpenSSL's OCSP response verification algo described on...

https://www.openssl.org/docs/man1.1.1/man1/ocsp.html#OCSP-Response-verification

...and do full responder cert verification, not just signatures.

Regards,
Paweł

@toothrot toothrot changed the title OCSP responses signed by expired OCSP responder certificate should return signature verification error x/crypto/ocsp: OCSP responses signed by expired OCSP responder certificate should return signature verification error Jan 5, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jan 5, 2021
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 5, 2021
@toothrot
Copy link
Contributor

toothrot commented Jan 5, 2021

/cc @FiloSottile

@pboguslawski pboguslawski changed the title x/crypto/ocsp: OCSP responses signed by expired OCSP responder certificate should return signature verification error x/crypto/ocsp: OCSP responses signed by invalid OCSP responder certificate should return signature verification error Jan 6, 2021
@pboguslawski
Copy link
Author

Another serious bug in OCSP response verification algo: ParseResponse from ocsp package does not check if embedded OCSP responder certificate is valid for signing OCSP responses; according to

https://tools.ietf.org/html/rfc6960#section-4.2.2.2

such cert must include id-kp-OCSPSigning in an extended key usage (not required only if response is signed by CA cert directly). Just checked that OCSP package in go1.15.2 linux/amd64 accepts OCSP response signed using simple personal certificate (no OCSPSigning EKU in this cert) signed by same CA.

OpenSSL in such scenario returns error like

OCSP routines:ocsp_check_delegated:missing ocspsigning usage:../crypto/ocsp/ocsp_vfy.c:329

Please do full embedded responder cert verification (i.e. expiration like above, EKU), not just signatures.

@pboguslawski
Copy link
Author

pboguslawski commented Jan 6, 2021

The following dirty workaround outside of ParseResponse works for us:

ocspResponse, err := ocsp.ParseResponse(httpResponseBody, issuer)
if err != nil {
	# throw OCSP verification error
}

// Verify OCSP responder certificate if embedded in OCSP response
// (https://github.com/golang/go/issues/43522 workaround).
if ocspResponse.Certificate != nil {

	// Verify chain up to issuer.
	oscpResponderCACert := x509.NewCertPool()
	oscpResponderCACert.AddCert(issuer)
	opts := x509.VerifyOptions{
		Roots:     oscpResponderCACert,
		KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageOCSPSigning},
	}
	if chains, err := ocspResponse.Certificate.Verify(opts); err == nil {
		// 1 chain with 2 certs (leaf and issuer) should be returned
		// on verification success; treat other results as an error.
		if len(chains) != 1 {
			# throw OCSP verification error
    	    	}
		if len(chains[0]) != 2 {
			# throw OCSP verification error
		}
	} else {
		# throw OCSP verification error
	}
}

Please verify and consider fixing inside ParseResponse routine.

@FiloSottile
Copy link
Contributor

As far as I can tell, this is a duplicate of #40017. ocsp.ParseResponse simply parses an OCSP response, like x509.ParseCertificate parses a certificate. We don't have an OCSP equivalent to x509.Verify yet, which is where all chain and attribute verification would happen.

It is unfortunate the ocsp.ParseResponse function does some signature verification, because indeed that makes it a pretty misleading API. I guess we could do the checks we can inside ocsp.ParseResponse, but there is a lot involved in verifying an OCSP response, and I am not sure it will all fit in the ocsp.ParseResponse API.

@pboguslawski
Copy link
Author

pboguslawski commented Jan 7, 2021

It is unfortunate the ocsp.ParseResponse function does some signature verification, because indeed that makes it a pretty misleading API.

Until OCSP Verify is implemented consider commenting ParseResponse to warn people that it is not doing full OCSP response verification just some elements of it.
If you decide to implement separate Verify, consider calling it inside ParseResponse also to avoid loosing verification in existing apps that will use ParseResponse. If people see now that ParseResponse throws an error on sig verification, should not surprise them if other checks will be done as well in this method.

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

No branches or pull requests

4 participants