ocsp: update ParseRequest docs & error conditions ParseRequest was documented as erroring for inputs that contained an optional request signature, and as only supporting requests for a single certificate. However, prior to this commit DER inputs that contained a signature succeeded without verifying the signature, and inputs that contained multiple certificate identifiers succeeded but only the first certificate identifier was unmarshalled. This commit updates the function behaviour to return an error for the signature case as described in the docs, and to clarify the documentation for inputs with multiple certificate identifiers. Fixes golang/go#80300 Change-Id: I7446422d6c98bd406b8c43e55fea9a16ace9d4a6 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/798220 Reviewed-by: Roland Shoemaker <roland@golang.org> Auto-Submit: Daniel McCarney <daniel@binaryparadox.net> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Nicholas Husin <nsh@golang.org> Reviewed-by: Nicholas Husin <husin@google.com>
diff --git a/ocsp/ocsp.go b/ocsp/ocsp.go index e6c645e..c6bc424 100644 --- a/ocsp/ocsp.go +++ b/ocsp/ocsp.go
@@ -85,7 +85,8 @@ // https://tools.ietf.org/html/rfc2560#section-4.1.1 type ocspRequest struct { - TBSRequest tbsRequest + TBSRequest tbsRequest + OptionalSignature asn1.RawValue `asn1:"explicit,tag:0,optional"` } type tbsRequest struct { @@ -324,7 +325,7 @@ return nil, errors.New("Unknown hash algorithm") } return asn1.Marshal(ocspRequest{ - tbsRequest{ + TBSRequest: tbsRequest{ Version: 0, RequestList: []request{ { @@ -418,8 +419,10 @@ } // ParseRequest parses an OCSP request in DER form. It only supports -// requests for a single certificate. Signed requests are not supported. -// If a request includes a signature, it will result in a ParseError. +// requests for a single certificate identifier. If a request includes +// multiple certificate identifiers, only the first will be included in +// the parsed Request. Signed requests are not supported. If a request +// includes a signature, it will result in a ParseError. func ParseRequest(bytes []byte) (*Request, error) { var req ocspRequest rest, err := asn1.Unmarshal(bytes, &req) @@ -430,6 +433,10 @@ return nil, ParseError("trailing data in OCSP request") } + if len(req.OptionalSignature.FullBytes) > 0 { + return nil, ParseError("signed OCSP requests are not supported") + } + if len(req.TBSRequest.RequestList) == 0 { return nil, ParseError("OCSP request contains no request body") }
diff --git a/ocsp/ocsp_test.go b/ocsp/ocsp_test.go index 6dc273e..abdea02 100644 --- a/ocsp/ocsp_test.go +++ b/ocsp/ocsp_test.go
@@ -15,10 +15,15 @@ "encoding/asn1" "encoding/hex" "encoding/pem" + "errors" "math/big" "reflect" + "strings" "testing" "time" + + "golang.org/x/crypto/cryptobyte" + cbasn1 "golang.org/x/crypto/cryptobyte/asn1" ) func TestOCSPDecode(t *testing.T) { @@ -199,6 +204,127 @@ } } +func TestOCSPRequestErrors(t *testing.T) { + tests := []struct { + name string + input []byte + wantErr string + }{ + { + name: "signed request", + input: testOCSPRequest(1, true), + wantErr: "signed", + }, + { + name: "trailing data", + input: append(testOCSPRequest(1, false), 0x00), + wantErr: "trailing", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := ParseRequest(tc.input) + if err == nil { + t.Fatalf( + "ParseRequest succeeded, want ParseError containing %q", + tc.wantErr, + ) + } + var parseError ParseError + if !errors.As(err, &parseError) { + t.Fatalf("ParseRequest returned %T, want ParseError", err) + } + if !strings.Contains(parseError.Error(), tc.wantErr) { + t.Fatalf( + "ParseRequest error = %q, want it to contain %q", + parseError.Error(), tc.wantErr, + ) + } + }) + } +} + +func TestOCSPMultipleCertRequest(t *testing.T) { + req, err := ParseRequest(testOCSPRequest(2, false)) + if err != nil { + t.Fatalf("ParseRequest errored for multiple request input: %q", err) + } + + // testOCSPRequest uses the request cert counter as the issuerNameHash, + // issuerKeyHash and serial. We expect the first request cert to be used + // (counter=1), not the second (counter=2). + wantHash := []byte{0x01} + if actual := req.IssuerNameHash; !bytes.Equal(actual, wantHash) { + t.Errorf( + "ParseRequest.IssuerNameHash returned = %q, wanted %q", + actual, wantHash, + ) + } + if actual := req.IssuerKeyHash; !bytes.Equal(actual, wantHash) { + t.Errorf("ParseRequest.IssuerKeyHash returned = %q, wanted %q", + actual, wantHash) + } + wantSerial := big.NewInt(0x01) + if actual := req.SerialNumber; actual.Cmp(wantSerial) != 0 { + t.Errorf("ParseRequest.SerialNumber returned = %q, wanted %q", + actual, wantHash) + } +} + +// testOCSPRequest constructs an unsupported OCSP request in DER form for testing. +// +// We build these by hand using cryptobyte to specifically target shapes that are +// not supported by ParseRequest, and can't be created with CreateRequest. +// +// The resulting requestList will contain numCerts RequestList entries. If +// includeSignature is true, the request will carry a signature field (with an +// arbitrary signatureAlgorithm and signature). +// +// See RFC 6960, §4.1.1. +func testOCSPRequest(numCerts int, includeSignature bool) []byte { + sha1AlgID, err := asn1.Marshal(pkix.AlgorithmIdentifier{ + Algorithm: asn1.ObjectIdentifier{1, 3, 14, 3, 2, 26}, + Parameters: asn1.RawValue{Tag: 5 /* ASN.1 NULL */}, + }) + if err != nil { + panic(err) + } + + var b cryptobyte.Builder + b.AddASN1(cbasn1.SEQUENCE, func(b *cryptobyte.Builder) { // OCSPRequest + b.AddASN1(cbasn1.SEQUENCE, func(b *cryptobyte.Builder) { // tbsRequest + b.AddASN1(cbasn1.SEQUENCE, func(b *cryptobyte.Builder) { // requestList + for i := 0; i < numCerts; i++ { + b.AddASN1(cbasn1.SEQUENCE, func(b *cryptobyte.Builder) { // Request + b.AddASN1(cbasn1.SEQUENCE, func(b *cryptobyte.Builder) { // CertID + b.AddBytes(sha1AlgID) // hashAlgorithm + b.AddASN1OctetString([]byte{byte(i + 1)}) // issuerNameHash + b.AddASN1OctetString([]byte{byte(i + 1)}) // issuerKeyHash + b.AddASN1Int64(int64(i + 1)) // serialNumber + }) + }) + } + }) + }) + if includeSignature { + // optionalSignature [0] EXPLICIT Signature + b.AddASN1(cbasn1.Tag(0).ContextSpecific().Constructed(), func(b *cryptobyte.Builder) { + b.AddASN1(cbasn1.SEQUENCE, func(b *cryptobyte.Builder) { // Signature + b.AddBytes(sha1AlgID) // signatureAlgorithm + b.AddASN1BitString([]byte{0x09, 0x0A, 0x0B, 0x0C}) // signature + }) + }) + } + }) + + der, err := b.Bytes() + if err != nil { + panic(err) + } + + return der +} + func TestOCSPResponse(t *testing.T) { leafCert, _ := hex.DecodeString(leafCertHex) leaf, err := x509.ParseCertificate(leafCert)