Skip to content

Commit

Permalink
ocsp: better validate OCSP response's certificates
Browse files Browse the repository at this point in the history 
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 <alex.scheel@hashicorp.com>
  • Loading branch information
@cipherboy
cipherboy committed Apr 17, 2023
1 parent 1faeef9 commit 7ee4c84
Show file tree
Hide file tree
Showing 2 changed files with 145 additions and 13 deletions.
65 changes: 52 additions & 13 deletions ocsp/ocsp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
_ "crypto/sha1"
_ "crypto/sha256"
_ "crypto/sha512"
"crypto/subtle"
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
93 changes: 93 additions & 0 deletions ocsp/ocsp_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"

0 comments on commit 7ee4c84

Please sign in to comment.