diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c0f1f48767e..08ed46823ffd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,9 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i * (baseapp) [#20291](https://github.com/cosmos/cosmos-sdk/pull/20291) Simulate nested messages. * (client/keys) [#21829](https://github.com/cosmos/cosmos-sdk/pull/21829) Add support for importing hex key using standard input. +* (x/auth/ante) [#23128](https://github.com/cosmos/cosmos-sdk/pull/23128) Allow custom verifyIsOnCurve when validate tx for public key like ethsecp256k1. +* (x/auth/ante) [#23283](https://github.com/cosmos/cosmos-sdk/pull/23283) Allow ed25519 transaction signatures. + ### Improvements diff --git a/crypto/keys/ed25519/ed25519.go b/crypto/keys/ed25519/ed25519.go index 792777b17de2..884d06f16782 100644 --- a/crypto/keys/ed25519/ed25519.go +++ b/crypto/keys/ed25519/ed25519.go @@ -7,6 +7,7 @@ import ( "fmt" "io" + "filippo.io/edwards25519" "github.com/cometbft/cometbft/crypto" "github.com/cometbft/cometbft/crypto/tmhash" "github.com/hdevalence/ed25519consensus" @@ -18,7 +19,7 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) -//------------------------------------- +// ------------------------------------- const ( PrivKeyName = "tendermint/PrivKeyEd25519" @@ -153,7 +154,7 @@ func GenPrivKeyFromSecret(secret []byte) *PrivKey { return &PrivKey{Key: ed25519.NewKeyFromSeed(seed)} } -//------------------------------------- +// ------------------------------------- var ( _ cryptotypes.PubKey = &PubKey{} @@ -230,3 +231,34 @@ func (pubKey PubKey) MarshalAminoJSON() ([]byte, error) { func (pubKey *PubKey) UnmarshalAminoJSON(bz []byte) error { return pubKey.UnmarshalAmino(bz) } + +// identityPoint is the “neutral element” in the ed25519 group, where +// point addition with identityPoint leaves the other point unchanged. +// It corresponds to coordinates (0, 1) in Edwards form and is not a valid public key +var identityPoint = edwards25519.NewIdentityPoint() + +// IsOnCurve checks that a 32B ed25519 public key is on the curve. +// The check fails for ed25519 identity points +func (pubKey *PubKey) IsOnCurve() bool { + // Make sure the public key is exactly 32B + if len(pubKey.Key) != ed25519.PublicKeySize { + // Invalid key size + return false + } + + // Make sure the public key bytes decodes into an ed25519 point + point, err := new(edwards25519.Point).SetBytes(pubKey.Key) + if err != nil || point == nil { + // Not a valid point on the curve + return false + } + + // Make sure the public key is not the identity point (all zeroes) + if point.Equal(identityPoint) == 1 { + // Public key is the identity point (useless) + return false + } + + // Public key is a valid point on the ed25519 curve + return true +} diff --git a/crypto/keys/ed25519/ed25519_test.go b/crypto/keys/ed25519/ed25519_test.go index 57c6f0563f3a..0708d3c32f8c 100644 --- a/crypto/keys/ed25519/ed25519_test.go +++ b/crypto/keys/ed25519/ed25519_test.go @@ -2,9 +2,11 @@ package ed25519_test import ( stded25519 "crypto/ed25519" + "crypto/rand" "encoding/base64" "testing" + "filippo.io/edwards25519" "github.com/cometbft/cometbft/crypto" tmed25519 "github.com/cometbft/cometbft/crypto/ed25519" "github.com/stretchr/testify/assert" @@ -254,3 +256,40 @@ func TestMarshalJSON(t *testing.T) { require.NoError(err) require.True(pk2.Equals(pk)) } + +func TestPubKeyOnCurve(t *testing.T) { + t.Parallel() + + t.Run("invalid public key size", func(t *testing.T) { + t.Parallel() + + key := &ed25519.PubKey{ + Key: make(stded25519.PublicKey, ed25519.PubKeySize+1), + } + + assert.False(t, key.IsOnCurve()) + }) + + t.Run("identity point", func(t *testing.T) { + t.Parallel() + + key := &ed25519.PubKey{ + Key: stded25519.PublicKey(edwards25519.NewIdentityPoint().Bytes()), + } + + assert.False(t, key.IsOnCurve()) + }) + + t.Run("valid public key", func(t *testing.T) { + t.Parallel() + + publicKey, _, err := stded25519.GenerateKey(rand.Reader) + require.NoError(t, err) + + key := &ed25519.PubKey{ + Key: publicKey, + } + + assert.True(t, key.IsOnCurve()) + }) +} diff --git a/go.mod b/go.mod index c513d1ff35a7..e772fc085892 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( cosmossdk.io/x/bank v0.0.0-00010101000000-000000000000 cosmossdk.io/x/staking v0.0.0-00010101000000-000000000000 cosmossdk.io/x/tx v1.0.1 + filippo.io/edwards25519 v1.1.0 github.com/99designs/keyring v1.2.2 github.com/bgentry/speakeasy v0.2.0 github.com/cometbft/cometbft v1.0.0 @@ -59,9 +60,8 @@ require ( ) require ( - buf.build/gen/go/cometbft/cometbft/protocolbuffers/go v1.36.3-20241120201313-68e42a58b301.1 // indirect - buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go v1.36.3-20240130113600-88ef6483f90f.1 // indirect - filippo.io/edwards25519 v1.1.0 // indirect + buf.build/gen/go/cometbft/cometbft/protocolbuffers/go v1.36.1-20241120201313-68e42a58b301.1 // indirect + buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go v1.36.1-20240130113600-88ef6483f90f.1 // indirect github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4 // indirect github.com/DataDog/datadog-go v4.8.3+incompatible // indirect github.com/DataDog/zstd v1.5.6 // indirect diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index 0e52f349a381..69c5666a9f57 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -125,6 +125,10 @@ func (svd SigVerificationDecorator) VerifyIsOnCurve(pubKey cryptotypes.PubKey) e } switch typedPubKey := pubKey.(type) { + case *ed25519.PubKey: + if !typedPubKey.IsOnCurve() { + return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "ed25519 key is not on curve") + } case *secp256k1.PubKey: pubKeyObject, err := secp256k1dcrd.ParsePubKey(typedPubKey.Bytes()) if err != nil { @@ -535,10 +539,7 @@ func DefaultSigVerificationGasConsumer(meter gas.Meter, sig signing.SignatureV2, switch pubkey := pubkey.(type) { case *ed25519.PubKey: - if err := meter.Consume(params.SigVerifyCostED25519, "ante verify: ed25519"); err != nil { - return err - } - return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "ED25519 public keys are unsupported") + return meter.Consume(params.SigVerifyCostED25519, "ante verify: ed25519") case *secp256k1.PubKey: return meter.Consume(params.SigVerifyCostSecp256k1, "ante verify: secp256k1") diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index b014ba51006d..621843676ba4 100644 --- a/x/auth/ante/sigverify_test.go +++ b/x/auth/ante/sigverify_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -74,7 +75,7 @@ func TestConsumeSignatureVerificationGas(t *testing.T) { args{nil, ed25519.GenPrivKey().PubKey(), params, func(mm *gastestutil.MockMeter) { mm.EXPECT().Consume(p.SigVerifyCostED25519, "ante verify: ed25519").Times(1) }}, - true, + false, }, { "PubKeySecp256k1", @@ -390,23 +391,21 @@ func TestAnteHandlerChecks(t *testing.T) { anteHandler := sdk.ChainAnteDecorators(sigVerificationDecorator) type testCase struct { - name string - privs []cryptotypes.PrivKey - msg sdk.Msg - accNums []uint64 - accSeqs []uint64 - shouldErr bool - supported bool + name string + privs []cryptotypes.PrivKey + msg sdk.Msg + accNums []uint64 + accSeqs []uint64 } // Secp256r1 keys that are not on curve will fail before even doing any operation i.e when trying to get the pubkey testCases := []testCase{ - {"secp256k1_onCurve", []cryptotypes.PrivKey{priv1}, msgs[0], []uint64{accs[0].GetAccountNumber()}, []uint64{0}, false, true}, - {"secp256r1_onCurve", []cryptotypes.PrivKey{priv2}, msgs[1], []uint64{accs[1].GetAccountNumber()}, []uint64{0}, false, true}, - {"ed255619", []cryptotypes.PrivKey{priv3}, msgs[2], []uint64{accs[2].GetAccountNumber()}, []uint64{2}, true, false}, + {"secp256k1_onCurve", []cryptotypes.PrivKey{priv1}, msgs[0], []uint64{accs[0].GetAccountNumber()}, []uint64{0}}, + {"secp256r1_onCurve", []cryptotypes.PrivKey{priv2}, msgs[1], []uint64{accs[1].GetAccountNumber()}, []uint64{0}}, + {"ed255619", []cryptotypes.PrivKey{priv3}, msgs[2], []uint64{accs[2].GetAccountNumber()}, []uint64{2}}, } - for i, tc := range testCases { + for _, tc := range testCases { t.Run(fmt.Sprintf("%s key", tc.name), func(t *testing.T) { suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() // Create new txBuilder for each test @@ -423,16 +422,8 @@ func TestAnteHandlerChecks(t *testing.T) { byteCtx := suite.ctx.WithTxBytes(txBytes) _, err = anteHandler(byteCtx, tx, true) - if tc.shouldErr { - require.NotNil(t, err, "TestCase %d: %s did not error as expected", i, tc.name) - if tc.supported { - require.ErrorContains(t, err, "not on curve") - } else { - require.ErrorContains(t, err, "unsupported key type") - } - } else { - require.Nil(t, err, "TestCase %d: %s errored unexpectedly. Err: %v", i, tc.name, err) - } + + assert.NoError(t, err) }) } }