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

Closed
paultag opened this issue Mar 7, 2019 · 13 comments
Closed

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

paultag opened this issue Mar 7, 2019 · 13 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@paultag
Copy link

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

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 commented Mar 8, 2019

Very much so yes!

@FiloSottile FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Mar 8, 2019
@bennapp
Copy link

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

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
Contributor

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

@rolandshoemaker
Copy link
Member

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

@FiloSottile
Copy link
Contributor

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

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

rolandshoemaker added a commit to rolandshoemaker/crypto that referenced this issue Oct 1, 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: Ifa3c51583fa94a3e6e0c3f874213b8a702b7f82a
@golang golang locked and limited conversation to collaborators Oct 1, 2021
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
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
GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e
GitHub-Pull-Request: golang/crypto#122
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
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
GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e
GitHub-Pull-Request: golang/crypto#122
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
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
GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e
GitHub-Pull-Request: golang/crypto#122
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
jasonwvh pushed a commit to jasonwvh/ocsp that referenced this issue Jul 13, 2022
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
GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e
GitHub-Pull-Request: golang/crypto#122
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
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
GitHub-Last-Rev: 481f613438c91521fd90078cc0915cf01cf44b4e
GitHub-Pull-Request: golang/crypto#122
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
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
GitHub-Last-Rev: 481f613
GitHub-Pull-Request: golang#122
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/220353
Reviewed-by: Katie Hockman <katie@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
5 participants