x/crypto/ocsp: OCSP responses signed by invalid OCSP responder certificate should return signature verification error

[pboguslawski]

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]

/cc @FiloSottile

[pboguslawski]

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]

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]

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]

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.