From fcfec2cb34423c546d24388987045a274db706f5 Mon Sep 17 00:00:00 2001 From: lestrrat <49281+lestrrat@users.noreply.github.com> Date: Wed, 18 Oct 2023 16:31:48 +0900 Subject: [PATCH] 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 --- Changes | 22 ++++++++++++-- jws/ecdsa.go | 3 ++ jws/eddsa.go | 6 +++- jws/jws.go | 48 ++++++++++++++++++++++++++++++ jws/options.go | 12 +++++++- jws/rsa.go | 6 +++- jwt/options.go | 7 +++-- jwx_test.go | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 177 insertions(+), 7 deletions(-) diff --git a/Changes b/Changes index 8cad1bc75..a04229acb 100644 --- a/Changes +++ b/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 UNRELEASED +[Bug fixes] + * [jws] jws.Sign() now properly check for valid algorithm / key type pair. + This was caused by the fact that jws.WithKey() accepted keys that implemented + crypto.Signer, and 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 diff --git a/jws/ecdsa.go b/jws/ecdsa.go index a2d644e43..9629303cb 100644 --- a/jws/ecdsa.go +++ b/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 diff --git a/jws/eddsa.go b/jws/eddsa.go index 78c1a2d68..643698701 100644 --- a/jws/eddsa.go +++ b/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 diff --git a/jws/jws.go b/jws/jws.go index 09100ec17..7e9eecefb 100644 --- a/jws/jws.go +++ b/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 +} diff --git a/jws/options.go b/jws/options.go index 4509efa08..e374a85d0 100644 --- a/jws/options.go +++ b/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 diff --git a/jws/rsa.go b/jws/rsa.go index e239330a2..35c450184 100644 --- a/jws/rsa.go +++ b/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) diff --git a/jwt/options.go b/jwt/options.go index c75758822..0403e0df9 100644 --- a/jwt/options.go +++ b/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...)) // diff --git a/jwx_test.go b/jwx_test.go index b74243404..00f8bd25c 100644 --- a/jwx_test.go +++ b/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) + }) + } + }) + } +}