merge for v2.0.15 (#998)

* jws: Check key types for those that implement crypto.Signer (#997)

* jws: Check key types for those that implement crypto.Signer

* guide users to jws/jwe documentation

* Update Changes

* tweak message

* Update Changes

Changes

@@ -4,12 +4,30 @@ Changes
 v2 has many incompatibilities with v1. To see the full list of differences between
 v1 and v2, please read the Changes-v2.md file (https://github.com/lestrrat-go/jwx/blob/develop/v2/Changes-v2.md)
 
+v2.0.15 19 20 Oct 2023
+[Bug fixes]
+  * [jws] jws.Sign() now properly check for valid algorithm / key type pair when
+    the key implements crypto.Signer. This was caused by the fact that when 
+    jws.WithKey() accepted keys that implemented crypto.Signer, there really
+    is no way to robustly check what algorithm the crypto.Signer implements.
+
+    The code has now been modified to check for KNOWN key types, i.e. those
+    that are defined in Go standard library, and those that are defined in
+    this library. For example, now calling jws.Sign() with jws.WithKey(jwa.RS256, ecdsaKey)
+    where ecdsaKey is either an instance of *ecdsa.PrivateKey or jwk.ECDSAPrivateKey
+    will produce an error.
+
+    However, if you use a separate library that wraps some KMS library which implements
+    crypto.Signer, this same check will not be performed due to the fact that
+    it is an unknown library to us. And there's no way to query a crypto.Signer
+    for its algorithm family.
+
 v2.0.14 17 Oct 2023
-  [New Features]
+[New Features]
   * [jwk] jwk.IsPrivateKey(), as well as jwk.AsymmetricKey has been added.
     The function can be used to tell if a jwk.Key is a private key of an
     asymmetric key pair.
-  [Security]
+[Security]
   * golang.org/x/crypto has been updated to 0.14.0. The update contains a fix for HTTP/2
     rapid reset DoS vulnerability, which some security scanning softwares may flag.
     However, do note that this library is NOT affected by the issue, as it does not have

jws/ecdsa.go

@@ -64,6 +64,9 @@ func (es *ecdsaSigner) Sign(payload []byte, key interface{}) ([]byte, error) {
 
 	signer, ok := key.(crypto.Signer)
 	if ok {
+		if !isValidECDSAKey(key) {
+			return nil, fmt.Errorf(`cannot use key of type %T to generate ECDSA based signatures`, key)
+		}
 		switch key.(type) {
 		case ecdsa.PrivateKey, *ecdsa.PrivateKey:
 			// if it's a ecdsa.PrivateKey, it's more efficient to

jws/eddsa.go

@@ -28,7 +28,11 @@ func (s eddsaSigner) Sign(payload []byte, key interface{}) ([]byte, error) {
 	// The ed25519.PrivateKey object implements crypto.Signer, so we should
 	// simply accept a crypto.Signer here.
 	signer, ok := key.(crypto.Signer)
-	if !ok {
+	if ok {
+		if !isValidEDDSAKey(key) {
+			return nil, fmt.Errorf(`cannot use key of type %T to generate EdDSA based signatures`, key)
+		}
+	} else {
 		// This fallback exists for cases when jwk.Key was passed, or
 		// users gave us a pointer instead of non-pointer, etc.
 		var privkey ed25519.PrivateKey

jws/jws.go

@@ -747,3 +747,51 @@ func AlgorithmsForKey(key interface{}) ([]jwa.SignatureAlgorithm, error) {
 	}
 	return algs, nil
 }
+
+// Because the keys defined in github.com/lestrrat-go/jwx/jwk may also implement
+// crypto.Signer, it would be possible for to mix up key types when signing/verifying
+// for example, when we specify jws.WithKey(jwa.RSA256, cryptoSigner), the cryptoSigner
+// can be for RSA, or any other type that implements crypto.Signer... even if it's for the
+// wrong algorithm.
+//
+// These functions are there to differentiate between the valid KNOWN key types.
+// For any other key type that is outside of the Go std library and our own code,
+// we must rely on the user to be vigilant.
+//
+// Notes: symmetric keys are obviously not part of this. for v2 OKP keys,
+// x25519 does not implement Sign()
+func isValidRSAKey(key interface{}) bool {
+	switch key.(type) {
+	case
+		ecdsa.PrivateKey, *ecdsa.PrivateKey,
+		ed25519.PrivateKey,
+		jwk.ECDSAPrivateKey, jwk.OKPPrivateKey:
+		// these are NOT ok
+		return false
+	}
+	return true
+}
+
+func isValidECDSAKey(key interface{}) bool {
+	switch key.(type) {
+	case
+		ed25519.PrivateKey,
+		rsa.PrivateKey, *rsa.PrivateKey,
+		jwk.RSAPrivateKey, jwk.OKPPrivateKey:
+		// these are NOT ok
+		return false
+	}
+	return true
+}
+
+func isValidEDDSAKey(key interface{}) bool {
+	switch key.(type) {
+	case
+		ecdsa.PrivateKey, *ecdsa.PrivateKey,
+		rsa.PrivateKey, *rsa.PrivateKey,
+		jwk.RSAPrivateKey, jwk.ECDSAPrivateKey:
+		// these are NOT ok
+		return false
+	}
+	return true
+}

jws/options.go

@@ -72,6 +72,16 @@ func (w *withKey) Protected(v Headers) Headers {
 // * A crypto.Signer
 // * A jwk.Key
 //
+// Note that due to technical reasons, this library is NOT able to differentiate
+// between a valid/invalid key for given algorithm if the key implements crypto.Signer
+// and the key is from an external library. For example, while we can tell that it is
+// invalid to use `jwk.WithKey(jwa.RSA256, ecdsaPrivateKey)` because the key is
+// presumably from `crypto/ecdsa` or this library, if you use a KMS wrapper
+// that implements crypto.Signer that is outside of the go standard library or this
+// library, we will not be able to properly catch the misuse of such keys --
+// the output will happily generate an ECDSA signature even in the presence of
+// `jwa.RSA256`
+//
 // A `crypto.Signer` is used when the private part of a key is
 // kept in an inaccessible location, such as hardware.
 // `crypto.Signer` is currently supported for RSA, ECDSA, and EdDSA
@@ -89,7 +99,7 @@ func (w *withKey) Protected(v Headers) Headers {
 // is respected when serializing. That is, if you specify a header with
 // `{"b64": false}`, then the payload is not base64 encoded.
 //
-// These suboptions are ignored whe the `jws.WithKey()` option is used with `jws.Verify()`.
+// These suboptions are ignored when the `jws.WithKey()` option is used with `jws.Verify()`.
 func WithKey(alg jwa.KeyAlgorithm, key interface{}, options ...WithKeySuboption) SignVerifyOption {
 	// Implementation note: this option is shared between Sign() and
 	// Verify(). As such we don't create a KeyProvider here because

jws/rsa.go

@@ -77,7 +77,11 @@ func (rs *rsaSigner) Sign(payload []byte, key interface{}) ([]byte, error) {
 	}
 
 	signer, ok := key.(crypto.Signer)
-	if !ok {
+	if ok {
+		if !isValidRSAKey(key) {
+			return nil, fmt.Errorf(`cannot use key of type %T to generate RSA based signatures`, key)
+		}
+	} else {
 		var privkey rsa.PrivateKey
 		if err := keyconv.RSAPrivateKey(&privkey, key); err != nil {
 			return nil, fmt.Errorf(`failed to retrieve rsa.PrivateKey out of %T: %w`, key, err)

jwt/options.go

@@ -119,10 +119,13 @@ type withKey struct {
 }
 
 // WithKey is a multi-purpose option. It can be used for either jwt.Sign, jwt.Parse (and
-// its siblings), and jwt.Serializer methods.
+// its siblings), and jwt.Serializer methods. For signatures, please see the documentation
+// for `jws.WithKey` for more details. For encryption, please see the documentation
+// for `jwe.WithKey`.
 //
 // It is the caller's responsibility to match the suboptions to the operation that they
-// are performing. For example, you are not allowed to do this:
+// are performing. For example, you are not allowed to do this, because the operation
+// is to generate a signature, and yet you are passing options for jwe:
 //
 //	jwt.Sign(token, jwt.WithKey(alg, key, jweOptions...))
 //

jwx_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/lestrrat-go/jwx/v2/jwa"
 	"github.com/lestrrat-go/jwx/v2/jwe"
 	"github.com/lestrrat-go/jwx/v2/jwk"
+	"github.com/lestrrat-go/jwx/v2/jws"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -547,3 +548,82 @@ func TestFormat(t *testing.T) {
 		})
 	}
 }
+
+func TestGH996(t *testing.T) {
+	ecdsaKey, err := jwxtest.GenerateEcdsaKey(jwa.P256)
+	require.NoError(t, err, `jwxtest.GenerateEcdsaKey should succeed`)
+
+	rsaKey, err := jwxtest.GenerateRsaKey()
+	require.NoError(t, err, `jwxtest.GenerateRsaKey should succeed`)
+
+	okpKey, err := jwxtest.GenerateEd25519Key()
+	require.NoError(t, err, `jwxtest.GenerateEd25519Key should succeed`)
+
+	symmetricKey := []byte(`abracadabra`)
+
+	testcases := []struct {
+		Name      string
+		Algorithm jwa.SignatureAlgorithm
+		Valid     []interface{}
+		Invalid   []interface{}
+	}{
+		{
+			Name:      `ECDSA`,
+			Algorithm: jwa.ES256,
+			Valid:     []interface{}{ecdsaKey},
+			Invalid:   []interface{}{rsaKey, okpKey, symmetricKey},
+		},
+		{
+			Name:      `RSA`,
+			Algorithm: jwa.RS256,
+			Valid:     []interface{}{rsaKey},
+			Invalid:   []interface{}{ecdsaKey, okpKey, symmetricKey},
+		},
+		{
+			Name:      `OKP`,
+			Algorithm: jwa.EdDSA,
+			Valid:     []interface{}{okpKey},
+			Invalid:   []interface{}{ecdsaKey, rsaKey, symmetricKey},
+		},
+	}
+
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.Name, func(t *testing.T) {
+			for _, valid := range tc.Valid {
+				valid := valid
+				t.Run(fmt.Sprintf("Sign Valid(%T)", valid), func(t *testing.T) {
+					_, err := jws.Sign([]byte("Lorem Ipsum"), jws.WithKey(tc.Algorithm, valid))
+					require.NoError(t, err, `signing with %T should succeed`, valid)
+				})
+			}
+
+			for _, invalid := range tc.Invalid {
+				invalid := invalid
+				t.Run(fmt.Sprintf("Sign Invalid(%T)", invalid), func(t *testing.T) {
+					_, err := jws.Sign([]byte("Lorem Ipsum"), jws.WithKey(tc.Algorithm, invalid))
+					require.Error(t, err, `signing with %T should fail`, invalid)
+				})
+			}
+
+			signed, err := jws.Sign([]byte("Lorem Ipsum"), jws.WithKey(tc.Algorithm, tc.Valid[0]))
+			require.NoError(t, err, `jws.Sign with valid key should succeed`)
+
+			for _, valid := range tc.Valid {
+				valid := valid
+				t.Run(fmt.Sprintf("Verify Valid(%T)", valid), func(t *testing.T) {
+					_, err := jws.Verify(signed, jws.WithKey(tc.Algorithm, valid))
+					require.NoError(t, err, `verifying with %T should succeed`, valid)
+				})
+			}
+
+			for _, invalid := range tc.Invalid {
+				invalid := invalid
+				t.Run(fmt.Sprintf("Verify Invalid(%T)", invalid), func(t *testing.T) {
+					_, err := jws.Verify(signed, jws.WithKey(tc.Algorithm, invalid))
+					require.Error(t, err, `verifying with %T should fail`, invalid)
+				})
+			}
+		})
+	}
+}