From e7c4368fe9ddd156b5f1463283cb51c5b400c373 Mon Sep 17 00:00:00 2001
From: edef <edef@edef.eu>
Date: Mon, 25 Nov 2019 19:39:25 +0000
Subject: [PATCH] acme: expect standard ASN.1 signatures from ECDSA Client.Key

Previously, an ECDSA crypto.Signer would have been expected to return a
signature in RFC7518 format, which violates crypto.Signer's interface
contract.

Fixes golang/go#35829

Change-Id: Id0cc2d9296cfb9f89925ab9ac02e12d68eec734b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/209537
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
---
 acme/jws.go      | 27 +++++++++++++++------------
 acme/jws_test.go | 28 +++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/acme/jws.go b/acme/jws.go
index cac8b678..76e3fdac 100644
--- a/acme/jws.go
+++ b/acme/jws.go
@@ -11,6 +11,7 @@ import (
 	"crypto/rsa"
 	"crypto/sha256"
 	_ "crypto/sha512" // need for EC keys
+	"encoding/asn1"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
@@ -126,21 +127,23 @@ func jwkEncode(pub crypto.PublicKey) (string, error) {
 
 // jwsSign signs the digest using the given key.
 // The hash is unused for ECDSA keys.
-//
-// Note: non-stdlib crypto.Signer implementations are expected to return
-// the signature in the format as specified in RFC7518.
-// See https://tools.ietf.org/html/rfc7518 for more details.
 func jwsSign(key crypto.Signer, hash crypto.Hash, digest []byte) ([]byte, error) {
-	if key, ok := key.(*ecdsa.PrivateKey); ok {
-		// The key.Sign method of ecdsa returns ASN1-encoded signature.
-		// So, we use the package Sign function instead
-		// to get R and S values directly and format the result accordingly.
-		r, s, err := ecdsa.Sign(rand.Reader, key, digest)
+	switch pub := key.Public().(type) {
+	case *rsa.PublicKey:
+		return key.Sign(rand.Reader, digest, hash)
+	case *ecdsa.PublicKey:
+		sigASN1, err := key.Sign(rand.Reader, digest, hash)
 		if err != nil {
 			return nil, err
 		}
-		rb, sb := r.Bytes(), s.Bytes()
-		size := key.Params().BitSize / 8
+
+		var rs struct{ R, S *big.Int }
+		if _, err := asn1.Unmarshal(sigASN1, &rs); err != nil {
+			return nil, err
+		}
+
+		rb, sb := rs.R.Bytes(), rs.S.Bytes()
+		size := pub.Params().BitSize / 8
 		if size%8 > 0 {
 			size++
 		}
@@ -149,7 +152,7 @@ func jwsSign(key crypto.Signer, hash crypto.Hash, digest []byte) ([]byte, error)
 		copy(sig[size*2-len(sb):], sb)
 		return sig, nil
 	}
-	return key.Sign(rand.Reader, digest, hash)
+	return nil, ErrUnsupportedKey
 }
 
 // jwsHasher indicates suitable JWS algorithm name and a hash function
diff --git a/acme/jws_test.go b/acme/jws_test.go
index 560ab96a..3507fe9d 100644
--- a/acme/jws_test.go
+++ b/acme/jws_test.go
@@ -323,6 +323,14 @@ func TestJWSEncodeJSONCustom(t *testing.T) {
 		// printf 'testsig' | b64raw
 		testsig = "dGVzdHNpZw"
 
+		// the example P256 curve point from https://tools.ietf.org/html/rfc7515#appendix-A.3.1
+		// encoded as ASN.1…
+		es256stdsig = "MEUCIA7RIVN5Y2xIPC9/FVgH1AKjsigDOvl8fheBmsMWnqZlAiEA" +
+			"xQoH04w8cOXY8S2vCEpUgKZlkMXyk1Cajz9/ioOjVNU"
+		// …and RFC7518 (https://tools.ietf.org/html/rfc7518#section-3.4)
+		es256jwsig = "DtEhU3ljbEg8L38VWAfUAqOyKAM6-Xx-F4GawxaepmXFCgfTjDxw" +
+			"5djxLa8ISlSApmWQxfKTUJqPP3-Kg6NU1Q"
+
 		// printf '{"alg":"ES256","jwk":{"crv":"P-256","kty":"EC","x":<testKeyECPubY>,"y":<testKeyECPubY>},"nonce":"nonce","url":"url"}' | b64raw
 		es256phead = "eyJhbGciOiJFUzI1NiIsImp3ayI6eyJjcnYiOiJQLTI1NiIsImt0" +
 			"eSI6IkVDIiwieCI6IjVsaEV1ZzV4SzR4QkRaMm5BYmF4THRhTGl2" +
@@ -345,19 +353,25 @@ func TestJWSEncodeJSONCustom(t *testing.T) {
 	)
 
 	tt := []struct {
-		alg, phead string
-		pub        crypto.PublicKey
+		alg, phead    string
+		pub           crypto.PublicKey
+		stdsig, jwsig string
 	}{
-		{"ES256", es256phead, testKeyEC.Public()},
-		{"RS256", rs256phead, testKey.Public()},
+		{"ES256", es256phead, testKeyEC.Public(), es256stdsig, es256jwsig},
+		{"RS256", rs256phead, testKey.Public(), testsig, testsig},
 	}
 	for _, tc := range tt {
 		tc := tc
 		t.Run(tc.alg, func(t *testing.T) {
+			stdsig, err := base64.RawStdEncoding.DecodeString(tc.stdsig)
+			if err != nil {
+				t.Errorf("couldn't decode test vector: %v", err)
+			}
 			signer := &customTestSigner{
-				sig: []byte("testsig"),
+				sig: stdsig,
 				pub: tc.pub,
 			}
+
 			b, err := jwsEncodeJSON(claims, signer, noKeyID, "nonce", "url")
 			if err != nil {
 				t.Fatal(err)
@@ -372,8 +386,8 @@ func TestJWSEncodeJSONCustom(t *testing.T) {
 			if j.Payload != payload {
 				t.Errorf("j.Payload = %q\nwant %q", j.Payload, payload)
 			}
-			if j.Signature != testsig {
-				t.Errorf("j.Signature = %q\nwant %q", j.Signature, testsig)
+			if j.Signature != tc.jwsig {
+				t.Errorf("j.Signature = %q\nwant %q", j.Signature, tc.jwsig)
 			}
 		})
 	}