ocsp: better validate OCSP response's certificates

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]>

Author: cipherboy

ocsp/ocsp.go

@@ -16,6 +16,7 @@ import (
 	_ "crypto/sha1"
 	_ "crypto/sha256"
 	_ "crypto/sha512"
+	"crypto/subtle"
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/asn1"
@@ -458,7 +459,9 @@ func ParseRequest(bytes []byte) (*Request, error) {
 // the signature on the embedded certificate.
 //
 // If the response does not contain an embedded certificate and issuer is not
-// nil, then issuer will be used to verify the response signature.
+// nil, then issuer will be used to verify the response signature. This is unsafe
+// unless the OCSP request and response was transmitted over a secure channel,
+// such as HTTPS signed by an external CA.
 //
 // Invalid responses and parse failures will result in a ParseError.
 // Error responses will result in a ResponseError.
@@ -551,31 +554,67 @@ func ParseResponseForCert(bytes []byte, cert, issuer *x509.Certificate) (*Respon
 	}
 
 	if len(basicResp.Certificates) > 0 {
-		// Responders should only send a single certificate (if they
+		// Responders SHOULD only send a single certificate (if they
 		// send any) that connects the responder's certificate to the
 		// original issuer. We accept responses with multiple
-		// certificates due to a number responders sending them[1], but
-		// ignore all but the first.
+		// certificates due to a number responders sending them [1, 2].
+		// However, we only retain the one that signed this request:
+		// notably, RFC 6960 does not guarantee an order, but states
+		// that any delegated OCSP responder certificate MUST be
+		// directly issued by the CA that issued this certificate [2],
+		// so any remaining certificates presumably overlap with the
+		// existing certs known by the caller.
 		//
 		// [1] https://github.com/golang/go/issues/21527
-		ret.Certificate, err = x509.ParseCertificate(basicResp.Certificates[0].FullBytes)
-		if err != nil {
-			return nil, err
+		// [2] https://github.com/golang/go/issues/59641
+		var lastErr error
+		for _, candidateBytes := range basicResp.Certificates {
+			candidate, err := x509.ParseCertificate(candidateBytes.FullBytes)
+			if err != nil {
+				return nil, ParseError("bad embedded certificate: " + err.Error())
+			}
+
+			if err := ret.CheckSignatureFrom(candidate); err != nil {
+				lastErr = err
+				continue
+			}
+
+			// We found a certificate which directly issued this OCSP
+			// request; set it as our certificate object and clear any
+			// errors.
+			lastErr = nil
+			ret.Certificate = candidate
+			break
 		}
 
-		if err := ret.CheckSignatureFrom(ret.Certificate); err != nil {
+		if lastErr != nil {
+			// n.b.: this error message string is checked by callers but is
+			// incorrectly worded: the signature on the _response_ is bad,
+			// from this selected certificate, not the signature on this
+			// embedded certificate from some parent issuer.
 			return nil, ParseError("bad signature on embedded certificate: " + err.Error())
 		}
+	}
 
-		if issuer != nil {
+	// When given a trusted issuer CA, use it to validate that this
+	// response is valid.
+	if issuer != nil {
+		if ret.Certificate == nil {
+			// We have no other information about this request, so require
+			// it to be signed by this trusted issuer.
+			if err := ret.CheckSignatureFrom(issuer); err != nil {
+				return nil, ParseError("bad OCSP signature: " + err.Error())
+			}
+		} else if subtle.ConstantTimeCompare(ret.Certificate.Raw, issuer.Raw) == 0 {
+			// Since we had a certificate, we had two scenarios: (1) either the
+			// responder unnecessarily sent us the issuer again (guarded by
+			// the above if), or (2), we have a different certificate here in
+			// this branch and thus we need to check the signature from
+			// issuer->ret.Certificate.
 			if err := issuer.CheckSignature(ret.Certificate.SignatureAlgorithm, ret.Certificate.RawTBSCertificate, ret.Certificate.Signature); err != nil {
 				return nil, ParseError("bad OCSP signature: " + err.Error())
 			}
 		}
-	} else if issuer != nil {
-		if err := ret.CheckSignatureFrom(issuer); err != nil {
-			return nil, ParseError("bad OCSP signature: " + err.Error())
-		}
 	}
 
 	for _, ext := range singleResp.SingleExtensions {

ocsp/ocsp_test.go

@@ -439,6 +439,42 @@ func TestOCSPDecodeMultiResponseWithoutMatchingCert(t *testing.T) {
 	}
 }
 
+func TestOCSPResponseWithIntermediate(t *testing.T) {
+	intBlock, _ := pem.Decode([]byte(ocspResponseWithIssuerIssuerPem))
+	intCert, err := x509.ParseCertificate(intBlock.Bytes)
+	if err != nil {
+		t.Errorf("failed to parse issuer certificate: %v", err)
+	}
+
+	ocspBlock, _ := pem.Decode([]byte(ocspResponseWithIssuerPem))
+	resp, err := ParseResponse(ocspBlock.Bytes, intCert)
+	if err != nil {
+		t.Errorf("expected nil error when parsing OCSP response with embedded issuer equal to passed issuer; got: %v", err)
+	}
+	if resp.Certificate == nil || !bytes.Equal(resp.Certificate.Raw, intCert.Raw) {
+		t.Errorf("expected response to contain embedded certificate pointing to issuer")
+	}
+
+	resp, err = ParseResponse(ocspBlock.Bytes, nil)
+	if err != nil {
+		t.Errorf("expected nil error when parsing OCSP response with embedded issuer with nil passed issuer; got: %v", err)
+	}
+	if resp.Certificate == nil || !bytes.Equal(resp.Certificate.Raw, intCert.Raw) {
+		t.Errorf("expected response to contain embedded certificate pointing to issuer")
+	}
+
+	rootBlock, _ := pem.Decode([]byte(GTSRoot))
+	rootCert, err := x509.ParseCertificate(rootBlock.Bytes)
+	if err != nil {
+		t.Errorf("failed to parse root certificate: %v", err)
+	}
+
+	_, err = ParseResponse(ocspBlock.Bytes, rootCert)
+	if err == nil {
+		t.Errorf("expected error when parsing OCSP request with embedded issuer and unrelated issuer passed; got nil")
+	}
+}
+
 // This OCSP response was taken from GTS's public OCSP responder.
 // To recreate:
 //   $ openssl s_client -tls1 -showcerts -servername golang.org -connect golang.org:443
@@ -743,4 +779,61 @@ const responderCertHex = "308202e2308201caa003020102020101300d06092a864886f70d01
 	"66705de17afa19d6e8ae91ddf33179d16ebb6ac2c69cae8373d408ebf8c55308be6c04d9" +
 	"3a25439a94299a65a709756c7a3e568be049d5c38839"
 
+// The below OCSP response and issuer certificate are from
+// https://go.dev/play/p/Nr-VKOD_fxH; the OCSP response contains
+// an embedded copy of the below issuer certificate.
+const ocspResponseWithIssuerPem = "-----BEGIN OCSP RESPONSE-----\n" +
+	"MIIF2AoBAKCCBdEwggXNBgkrBgEFBQcwAQEEggW+MIIFujCBv6EvMC0xKzApBgNVBAMTImV4YW1w\n" +
+	"bGUuY29tIEludGVybWVkaWF0ZSBBdXRob3JpdHkYDzIwMjMwNDE0MTgzNjAwWjB7MHkwTTAJBgUr\n" +
+	"DgMCGgUABBQUuUZ7MA/tkAtRYo9Qxgrp+5AlEwQUOBTBNthss1lVUQGZW7AJQddI0NkCFHyAKcL4\n" +
+	"qKzgh1qwqIl2jZ7b1Np7gAAYDzIwMjMwNDE0MTgzNjQ4WqARGA8yMDIzMDQxNTA2MzY0OFqhAjAA\n" +
+	"MA0GCSqGSIb3DQEBCwUAA4IBAQAwTMgZ/ebI/hXQ0lZfPAD9i4lAIM1cvPUH+m/j4SG9s3EnFOwg\n" +
+	"/LdJ2WWnsafd9Eu6Lrf2mWwZtOuLp03WC4AjY7ghcQU89h1/R/6xv0cjvPIFmh1QP4/ipuGMNOfn\n" +
+	"Utw/0o6vFgtViryLz+mWJ+zVntcVF8eMgi/68pCJteyyi2eQZXlHWpmHzbUeVDSaCNHsB6e1gI1w\n" +
+	"cn0aVRG0HVfO6fHUhQlFfGPR0Zd73iU/oyx1XPxO5okBIlwoepWASflDp+11dS9ehQIJVcs9/5LB\n" +
+	"8RnkMAven/1WNaWWwcKBRtGN7TFdAde7sWS9RDdLfosAWsWQdF6as2JvMdYRsc8zoIID4DCCA9ww\n" +
+	"ggPYMIICwKADAgECAhR/tZoxrlMDTnlt++SzzH62XFNczjANBgkqhkiG9w0BAQsFADAWMRQwEgYD\n" +
+	"VQQDEwtleGFtcGxlLmNvbTAeFw0yMzA0MTQxODMxMzRaFw0yMzA1MTYxODMyMDRaMC0xKzApBgNV\n" +
+	"BAMTImV4YW1wbGUuY29tIEludGVybWVkaWF0ZSBBdXRob3JpdHkwggEiMA0GCSqGSIb3DQEBAQUA\n" +
+	"A4IBDwAwggEKAoIBAQCdWc6C4prDkjxrBiguqvXPNFsKY2jWpyR22ex7hbkAy+XDwjAQu2d3jnUZ\n" +
+	"VKSHlbvAbJlwOhZ9jiSAwotsDspoUzNnOhR7XFBR/HTcWAPXkeX16fJHmR67G7bJ35kb2r5P6vFx\n" +
+	"5R7mqkQqEfk9hSrcj3KVCL/wotyY++tLlIAD7XFr+Sbjqbtkq9wE1wPwVSc5nQexbjlD27T82CBi\n" +
+	"44D5BeQD+5N/YoWhz+MyPgKcx5UUk3Efkp2l4tEnYh9tJVcJr1o2wqwJRCnlPpY8PUuh8gZmCKIv\n" +
+	"w+riEEFGvopD8n29dROxjuDHs2kqyw8pp4UkkYwxjajwxyiyNI93p9X5AgMBAAGjggEFMIIBATAO\n" +
+	"BgNVHQ8BAf8EBAMCAQYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQUOBTBNthss1lVUQGZW7AJ\n" +
+	"QddI0NkwHwYDVR0jBBgwFoAUyMh8vow7VI0BZZg/7cJldteWVpEwagYIKwYBBQUHAQEEXjBcMC0G\n" +
+	"CCsGAQUFBzABhiFodHRwOi8vbG9jYWxob3N0OjgzMDAvdjEvcGtpL29jc3AwKwYIKwYBBQUHMAKG\n" +
+	"H2h0dHA6Ly9sb2NhbGhvc3Q6ODMwMC92MS9wa2kvY2EwMgYDVR0fBCswKTAnoCWgI4YhaHR0cDov\n" +
+	"L2xvY2FsaG9zdDo4MzAwL3YxL3BraS9jcmxzMA0GCSqGSIb3DQEBCwUAA4IBAQBsF1JxQvASHFl0\n" +
+	"zPJMRscAHKRXjgOrkm1N9J4DMphC3lWZ6RRHQvLZB7rfUAx9/eVF+UCKcvx4eldZJzxp5fe5K5OR\n" +
+	"1Vuh65unnlq1rkNy6WqGhL8aMuUZ5NGFAKRTONEX5tKGixAy+JM4xX0iIU3OZhlfGx4J+xMyLWy/\n" +
+	"xRiHALqaCJsSGM8DafNawN6e73ZCQfTng7bCeiwyCV4YgvSFBXQ/zs0nZ9PFn5orDHqZOPS1s9vT\n" +
+	"hVGqWYJUgARvWfj3jXhAGAry9IujUmPZWeDttX8j7u4h4O2/UJ6fYCIDCGbQsA4Bwm+tCqgLZ8JQ\n" +
+	"vBfNItA0VDnFokkM4cWpWcfc\n" +
+	"-----END OCSP RESPONSE-----"
+
+const ocspResponseWithIssuerIssuerPem = "-----BEGIN CERTIFICATE-----\n" +
+	"MIID2DCCAsCgAwIBAgIUf7WaMa5TA055bfvks8x+tlxTXM4wDQYJKoZIhvcNAQEL\n" +
+	"BQAwFjEUMBIGA1UEAxMLZXhhbXBsZS5jb20wHhcNMjMwNDE0MTgzMTM0WhcNMjMw\n" +
+	"NTE2MTgzMjA0WjAtMSswKQYDVQQDEyJleGFtcGxlLmNvbSBJbnRlcm1lZGlhdGUg\n" +
+	"QXV0aG9yaXR5MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnVnOguKa\n" +
+	"w5I8awYoLqr1zzRbCmNo1qckdtnse4W5AMvlw8IwELtnd451GVSkh5W7wGyZcDoW\n" +
+	"fY4kgMKLbA7KaFMzZzoUe1xQUfx03FgD15Hl9enyR5keuxu2yd+ZG9q+T+rxceUe\n" +
+	"5qpEKhH5PYUq3I9ylQi/8KLcmPvrS5SAA+1xa/km46m7ZKvcBNcD8FUnOZ0HsW45\n" +
+	"Q9u0/NggYuOA+QXkA/uTf2KFoc/jMj4CnMeVFJNxH5KdpeLRJ2IfbSVXCa9aNsKs\n" +
+	"CUQp5T6WPD1LofIGZgiiL8Pq4hBBRr6KQ/J9vXUTsY7gx7NpKssPKaeFJJGMMY2o\n" +
+	"8McosjSPd6fV+QIDAQABo4IBBTCCAQEwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB\n" +
+	"/wQFMAMBAf8wHQYDVR0OBBYEFDgUwTbYbLNZVVEBmVuwCUHXSNDZMB8GA1UdIwQY\n" +
+	"MBaAFMjIfL6MO1SNAWWYP+3CZXbXllaRMGoGCCsGAQUFBwEBBF4wXDAtBggrBgEF\n" +
+	"BQcwAYYhaHR0cDovL2xvY2FsaG9zdDo4MzAwL3YxL3BraS9vY3NwMCsGCCsGAQUF\n" +
+	"BzAChh9odHRwOi8vbG9jYWxob3N0OjgzMDAvdjEvcGtpL2NhMDIGA1UdHwQrMCkw\n" +
+	"J6AloCOGIWh0dHA6Ly9sb2NhbGhvc3Q6ODMwMC92MS9wa2kvY3JsczANBgkqhkiG\n" +
+	"9w0BAQsFAAOCAQEAbBdScULwEhxZdMzyTEbHABykV44Dq5JtTfSeAzKYQt5VmekU\n" +
+	"R0Ly2Qe631AMff3lRflAinL8eHpXWSc8aeX3uSuTkdVboeubp55ata5DculqhoS/\n" +
+	"GjLlGeTRhQCkUzjRF+bShosQMviTOMV9IiFNzmYZXxseCfsTMi1sv8UYhwC6mgib\n" +
+	"EhjPA2nzWsDenu92QkH054O2wnosMgleGIL0hQV0P87NJ2fTxZ+aKwx6mTj0tbPb\n" +
+	"04VRqlmCVIAEb1n49414QBgK8vSLo1Jj2Vng7bV/I+7uIeDtv1Cen2AiAwhm0LAO\n" +
+	"AcJvrQqoC2fCULwXzSLQNFQ5xaJJDOHFqVnH3A==\n" +
+	"-----END CERTIFICATE-----"
+
 const errorResponseHex = "30030a0101"