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.

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

x/crypto/ocsp: Support multiple OCSP responses/requests #30651

Open
paultag opened this issue Mar 7, 2019 · 13 comments · May be fixed by golang/crypto#122
Open

x/crypto/ocsp: Support multiple OCSP responses/requests #30651

paultag opened this issue Mar 7, 2019 · 13 comments · May be fixed by golang/crypto#122

Comments

@paultag
Copy link

@paultag paultag commented Mar 7, 2019

What version of Go are you using (go version)?

$ go version
go version go1.11.5 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/paultag/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/paultag/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go-1.11"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.11/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build935539835=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Send an OCSP to a server that replies with multiple responses

What did you expect to see?

One or all of the responses

What did you see instead?

An error, OCSP response contains bad number of responses

Actual bug report

I'm basically reopening #17950

Some servers I've tried (such as http://ocsp.disa.mil or http://ocsp.managed.entrust.co m/OCSP/EMSSSPCAResponder) reply with a payload that crashes x/crypto/ocsp with an error OCSP response contains bad number of responses.

Having a function that returns all responses received, or something else sensible would be ideal. I don't appear to be able to work around this as a library user without doing some serious asn1 mundging and repacking the ASN1 back to the OCSP library, which seems like it defeats the purpose.

This error appears to be triggering if you click on crt.sh asking for an OCSP check on the following cert

@gopherbot gopherbot added this to the Unreleased milestone Mar 7, 2019
@paultag
Copy link
Author

@paultag paultag commented Mar 7, 2019

The last bug was closed due to a belief that there was no real life use-case. The fact this is triggering on a real life CA, duplicate-able on a production website is reason enough to me that there is a real life use-case for multiple response parsing.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Mar 8, 2019

Would a simple ParseResponses(bytes []byte, cert, issuer *x509.Certificate) ([]*Response, error) function (where cert can be nil) address the use case?

@paultag
Copy link
Author

@paultag paultag commented Mar 8, 2019

Very much so yes!

@bennapp
Copy link

@bennapp bennapp commented Mar 11, 2019

What did you do?

Send an OCSP to a server that replies with multiple responses

Hey @paultag I want help out with the fix but I am a little unclear on the steps to reproduce, could you make small code example of what you did to get the error? Thank you!

@paultag
Copy link
Author

@paultag paultag commented Mar 11, 2019

Cert, CA, for each do an openssl x509 -in file.crt -inform pem -out file.der -outform der (to convert from PEM to DER) and run the following code

package main

import (
	"bytes"
	"crypto"
	"crypto/x509"
	"encoding/pem"
	"fmt"
	"io/ioutil"
	"log"
	"net/http"
	"net/url"
	"os"

	"golang.org/x/crypto/ocsp"
)

func loadCert(path string) (*x509.Certificate, error) {
	fd, err := os.Open(path)
	if err != nil {
		return nil, err
	}
	defer fd.Close()
	bytes, err := ioutil.ReadAll(fd)
	if err != nil {
		return nil, err
	}
	block, _ := pem.Decode(bytes)
	_ = block
	// return x509.ParseCertificate(bytes)
	return x509.ParseCertificate(bytes)
}

func main() {
	cert, err := loadCert(os.Args[1])
	if err != nil {
		panic(err)
	}
	ca, err := loadCert(os.Args[2])
	if err != nil {
		panic(err)
	}

	revoked := isCertificateRevokedByOCSP(cert, ca, "http://ocsp.disa.mil")
	fmt.Printf("Revoked: %t\n", revoked)
}

func isCertificateRevokedByOCSP(clientCert, issuerCert *x509.Certificate, ocspServer string) bool {
	opts := &ocsp.RequestOptions{Hash: crypto.SHA1}
	buffer, err := ocsp.CreateRequest(clientCert, issuerCert, opts)
	if err != nil {
		fmt.Printf("err: %s\n", err)
		return false
	}
	httpRequest, err := http.NewRequest(http.MethodPost, ocspServer, bytes.NewBuffer(buffer))
	if err != nil {
		fmt.Printf("err: %s\n", err)
		return false
	}
	ocspUrl, err := url.Parse(ocspServer)
	if err != nil {
		fmt.Printf("err: %s\n", err)
		return false
	}
	httpRequest.Header.Add("Content-Type", "application/ocsp-request")
	httpRequest.Header.Add("Accept", "application/ocsp-response")
	httpRequest.Header.Add("host", ocspUrl.Host)
	httpClient := &http.Client{}
	httpResponse, err := httpClient.Do(httpRequest)
	if err != nil {
		fmt.Printf("err: %s\n", err)
		return false
	}
	defer httpResponse.Body.Close()
	output, err := ioutil.ReadAll(httpResponse.Body)
	if err != nil {
		fmt.Printf("err: %s\n", err)
		return false
	}

	ocspResponse, err := ocsp.ParseResponse(output, issuerCert)
	if err != nil {
		fmt.Printf("err: %s\n", err)
		return false
	}
	if ocspResponse.Status == ocsp.Revoked {
		log.Printf("certificate has been revoked by OCSP server %s, refusing connection", ocspServer)
		return true
	} else {
		return false
	}
}
@paultag
Copy link
Author

@paultag paultag commented Mar 11, 2019

Also maybe save the response to avoid hammering the OCSP server :)

Output:

err: OCSP response contains bad number of responses
@bennapp
Copy link

@bennapp bennapp commented Mar 16, 2019

Would it be odd to introduce ParseResponses given that ParseResponse already exists? If we did this, how would the client / user know to use ParseResponses or ParseResponse?

I gave this a shot, but I am not sure I am on the right path or have the OSCP expertise for this one :-/

func ParseResponses(bytes []byte, issuer *x509.Certificate) ([]*Response, error) {
	return ParseResponsesForCert(bytes, nil, issuer)
}

func ParseResponsesForCert(bytes []byte, cert, issuer *x509.Certificate) ([]*Response, error) {
	var resp responseASN1
	rest, err := asn1.Unmarshal(bytes, &resp)
	if err != nil {
		return nil, err
	}
	if len(rest) > 0 {
		return nil, ParseError("trailing data in OCSP response")
	}

	if status := ResponseStatus(resp.Status); status != Success {
		return nil, ResponseError{status}
	}

	if !resp.Response.ResponseType.Equal(idPKIXOCSPBasic) {
		return nil, ParseError("bad OCSP response type")
	}

	var basicResp basicResponse
	rest, err = asn1.Unmarshal(resp.Response.Response, &basicResp)
	if err != nil {
		return nil, err
	}
	if len(rest) > 0 {
		return nil, ParseError("trailing data in OCSP response")
	}

	numResponses := len(basicResp.TBSResponseData.Responses)
	var responses []*Response

	for responseIndex := 0; responseIndex < numResponses; responseIndex++ {
		var singleResp singleResponse
		if cert == nil {
			singleResp = basicResp.TBSResponseData.Responses[responseIndex]
		} else {
			match := false
			for _, resp := range basicResp.TBSResponseData.Responses {
				if cert.SerialNumber.Cmp(resp.CertID.SerialNumber) == 0 {
					singleResp = resp
					match = true
					break
				}
			}
			if !match {
				return nil, ParseError("no response matching the supplied certificate")
			}
		}

		ret := &Response{
			TBSResponseData:    basicResp.TBSResponseData.Raw,
			Signature:          basicResp.Signature.RightAlign(),
			SignatureAlgorithm: getSignatureAlgorithmFromOID(basicResp.SignatureAlgorithm.Algorithm),
			Extensions:         singleResp.SingleExtensions,
			SerialNumber:       singleResp.CertID.SerialNumber,
			ProducedAt:         basicResp.TBSResponseData.ProducedAt,
			ThisUpdate:         singleResp.ThisUpdate,
			NextUpdate:         singleResp.NextUpdate,
		}

		// Handle the ResponderID CHOICE tag. ResponderID can be flattened into
		// TBSResponseData once https://go-review.googlesource.com/34503 has been
		// released.
		rawResponderID := basicResp.TBSResponseData.RawResponderID
		switch rawResponderID.Tag {
		case 1: // Name
			var rdn pkix.RDNSequence
			if rest, err := asn1.Unmarshal(rawResponderID.Bytes, &rdn); err != nil || len(rest) != 0 {
				return nil, ParseError("invalid responder name")
			}
			ret.RawResponderName = rawResponderID.Bytes
		case 2: // KeyHash
			if rest, err := asn1.Unmarshal(rawResponderID.Bytes, &ret.ResponderKeyHash); err != nil || len(rest) != 0 {
				return nil, ParseError("invalid responder key hash")
			}
		default:
			return nil, ParseError("invalid responder id tag")
		}

		if len(basicResp.Certificates) > 0 {
			// 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.
			//
			// [1] https://github.com/golang/go/issues/21527
			ret.Certificate, err = x509.ParseCertificate(basicResp.Certificates[0].FullBytes)
			if err != nil {
				return nil, err
			}

			if err := ret.CheckSignatureFrom(ret.Certificate); err != nil {
				return nil, ParseError("bad signature on embedded certificate: " + err.Error())
			}

			if issuer != nil {
				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 {
			if ext.Critical {
				return nil, ParseError("unsupported critical extension")
			}
		}

		for h, oid := range hashOIDs {
			if singleResp.CertID.HashAlgorithm.Algorithm.Equal(oid) {
				ret.IssuerHash = h
				break
			}
		}
		if ret.IssuerHash == 0 {
			return nil, ParseError("unsupported issuer hash algorithm")
		}

		switch {
		case bool(singleResp.Good):
			ret.Status = Good
		case bool(singleResp.Unknown):
			ret.Status = Unknown
		default:
			ret.Status = Revoked
			ret.RevokedAt = singleResp.Revoked.RevocationTime
			ret.RevocationReason = int(singleResp.Revoked.Reason)
		}

		responses = append(responses, ret)
	}

	return responses, nil
}
@paultag
Copy link
Author

@paultag paultag commented Mar 31, 2019

@bennapp I'm not sure (I'm not a Go developer) but maybe they would appreciate a Pull Request and refactor of the old method to use this new one (and maintain old behavior if len > 1)

Thanks for working on this!

@rolandshoemaker
Copy link
Contributor

@rolandshoemaker rolandshoemaker commented Feb 14, 2020

To add a little context: ParseResponseForCert does, kind of, support responses containing multiple SingleResponses, but only if you provide a certificate. In that case it'll iterate through the responses and pick the one that matches the serial for the provided certificate (or if there isn't one, it fails). This should probably be better documented.

While the example behavior is quite rare (the DISA case is a real... doozy) it is still technically standards compliant (although strongly suggested against).

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Feb 19, 2020

So this is only needed to parse responses that violate a SHOULD NOT?

@rolandshoemaker
Copy link
Contributor

@rolandshoemaker rolandshoemaker commented Feb 19, 2020

Yup, I think probably the best improvement here would just be clarifying the documentation for ParseResponseForCert.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Feb 19, 2020

Great, I'd appreciate a documentation CL. I personally also find "If the response contains a certificate then the signature over the response is checked." confusing.

rolandshoemaker added a commit to rolandshoemaker/crypto that referenced this issue Feb 21, 2020
…nseForCert

This change clarifies the behaviors of ParseResponse and ParseResponseForCert,
particularly when parsing responses that contain multiple certificate statuses.

Fixes golang/go#30651

Change-Id: Ia632c8c2a69d1b0c17d71f9f9dcb59ddb0be401b
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 21, 2020

Change https://golang.org/cl/220353 mentions this issue: ocsp: Improve documentation for ParseResponse and ParseResponseForCert

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.