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.

Sign up for GitHub

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

Jump to bottom

ocsp: better validate OCSP response's certificates #256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ocsp: better validate OCSP response's certificates #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 52 additions & 13 deletions ocsp/ocsp.go
View file
Open in desktop
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
Open in desktop
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"