diff --git a/ocsp/ocsp.go b/ocsp/ocsp.go index 4269ed113b..4ed71560cf 100644 --- a/ocsp/ocsp.go +++ b/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 { diff --git a/ocsp/ocsp_test.go b/ocsp/ocsp_test.go index 0bc194b2f4..267c95bbe2 100644 --- a/ocsp/ocsp_test.go +++ b/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"