feat: extra checks on signatures/pubkeys + check the signature first in antehandle

crypto/keys/secp256k1/secp256k1.go

@@ -9,7 +9,7 @@ import (
 	"math/big"
 
 	"github.com/cometbft/cometbft/crypto"
-	secp256k1 "github.com/decred/dcrd/dcrec/secp256k1/v4"
+	secp256k1dcrd "github.com/decred/dcrd/dcrec/secp256k1/v4"
 	"golang.org/x/crypto/ripemd160" //nolint: staticcheck // keep around for backwards compatibility
 
 	errorsmod "cosmossdk.io/errors"
@@ -39,7 +39,8 @@ func (privKey *PrivKey) Bytes() []byte {
 // PubKey performs the point-scalar multiplication from the privKey on the
 // generator point to get the pubkey.
 func (privKey *PrivKey) PubKey() cryptotypes.PubKey {
-	pubkeyObject := secp256k1.PrivKeyFromBytes(privKey.Key).PubKey()
+	pubkeyObject := secp256k1dcrd.PrivKeyFromBytes(privKey.Key).PubKey()
+
 	pk := pubkeyObject.SerializeCompressed()
 	return &PubKey{Key: pk}
 }
@@ -100,7 +101,7 @@ func genPrivKey(rand io.Reader) []byte {
 
 		d.SetBytes(privKeyBytes[:])
 		// break if we found a valid point (i.e. > 0 and < N == curverOrder)
-		isValidFieldElement := 0 < d.Sign() && d.Cmp(secp256k1.S256().N) < 0
+		isValidFieldElement := 0 < d.Sign() && d.Cmp(secp256k1dcrd.S256().N) < 0
 		if isValidFieldElement {
 			break
 		}
@@ -128,7 +129,7 @@ func GenPrivKeyFromSecret(secret []byte) *PrivKey {
 	// https://apps.nsa.gov/iaarchive/library/ia-guidance/ia-solutions-for-classified/algorithm-guidance/suite-b-implementers-guide-to-fips-186-3-ecdsa.cfm
 	// see also https://github.com/golang/go/blob/0380c9ad38843d523d9c9804fe300cb7edd7cd3c/src/crypto/ecdsa/ecdsa.go#L89-L101
 	fe := new(big.Int).SetBytes(secHash[:])
-	n := new(big.Int).Sub(secp256k1.S256().N, one)
+	n := new(big.Int).Sub(secp256k1dcrd.S256().N, one)
 	fe.Mod(fe, n)
 	fe.Add(fe, one)
 

testutil/testdata/animal.go

@@ -1,7 +1,5 @@
 package testdata
 
-// nolint
-
 import (
 	"fmt"
 

testutil/testdata/grpc_query.go

@@ -5,12 +5,11 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/cosmos/cosmos-sdk/codec/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/gogoproto/proto"
 	grpc "google.golang.org/grpc"
 	"gotest.tools/v3/assert"
-
-	"github.com/cosmos/cosmos-sdk/codec/types"
-	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
 // iterCount defines the number of iterations to run on each query to test

testutil/testdata/tx.go

@@ -3,15 +3,15 @@ package testdata
 import (
 	"testing"
 
-	"gotest.tools/v3/assert"
-	"pgregory.net/rapid"
-
+	"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256r1"
 	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/cosmos/cosmos-sdk/types/query"
+	"gotest.tools/v3/assert"
+	"pgregory.net/rapid"
 )
 
 // AddressGenerator creates and returns a random address generator using rapid.
@@ -43,7 +43,7 @@ func KeyTestPubAddr() (cryptotypes.PrivKey, cryptotypes.PubKey, sdk.AccAddress)
 	return key, pub, addr
 }
 
-// KeyTestPubAddr generates a new secp256r1 keypair.
+// KeyTestPubAddrSecp256R1 generates a new secp256r1 keypair.
 func KeyTestPubAddrSecp256R1(t *testing.T) (cryptotypes.PrivKey, cryptotypes.PubKey, sdk.AccAddress) {
 	key, err := secp256r1.GenPrivKey()
 	assert.NilError(t, err)
@@ -52,6 +52,14 @@ func KeyTestPubAddrSecp256R1(t *testing.T) (cryptotypes.PrivKey, cryptotypes.Pub
 	return key, pub, addr
 }
 
+// KeyTestPubAddrED25519 generates a new ed25519 keypair.
+func KeyTestPubAddrED25519() (cryptotypes.PrivKey, cryptotypes.PubKey, sdk.AccAddress) {
+	key := ed25519.GenPrivKey()
+	pub := key.PubKey()
+	addr := sdk.AccAddress(pub.Address())
+	return key, pub, addr
+}
+
 // NewTestFeeAmount is a test fee amount.
 func NewTestFeeAmount() sdk.Coins {
 	return sdk.NewCoins(sdk.NewInt64Coin("atom", 150))

x/auth/ante/sigverify.go

@@ -5,7 +5,9 @@ import (
 	"encoding/base64"
 	"encoding/hex"
 	"fmt"
+	"strings"
 
+	secp256k1dcrd "github.com/decred/dcrd/dcrec/secp256k1/v4"
 	"google.golang.org/protobuf/types/known/anypb"
 
 	errorsmod "cosmossdk.io/errors"
@@ -94,6 +96,9 @@ func (spkd SetPubKeyDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b
 			return ctx, errorsmod.Wrapf(sdkerrors.ErrInvalidPubKey,
 				"pubKey does not match signer address %s with signer index: %d", signerStrs[i], i)
 		}
+		if err := verifyIsOnCurve(pk); err != nil {
+			return ctx, err
+		}
 
 		acc, err := GetSignerAcc(ctx, spkd.ak, signers[i])
 		if err != nil {
@@ -187,6 +192,12 @@ func (sgcd SigGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 		}
 
 		pubKey := signerAcc.GetPubKey()
+		if !simulate && pubKey == nil {
+			return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey on account is not set")
+		}
+		if err := verifyIsOnCurve(pubKey); err != nil {
+			return ctx, err
+		}
 
 		// In simulate mode the transaction comes with no signatures, thus if the
 		// account's pubkey is nil, both signature verification and gasKVStore.Set()
@@ -250,6 +261,46 @@ func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool {
 	}
 }
 
+func verifyIsOnCurve(pubKey cryptotypes.PubKey) (err error) {
+	switch typedPubKey := pubKey.(type) {
+	case *ed25519.PubKey:
+		return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "ED25519 public keys are unsupported")
+	case *secp256k1.PubKey:
+		pubKeyObject, err := secp256k1dcrd.ParsePubKey(typedPubKey.Bytes())
+		if err != nil {
+			if strings.Contains(err.Error(), "not on the secp256k1 curve") {
+				return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "secp256k1 key is not on curve")
+			}
+			return err
+		}
+		if !pubKeyObject.IsOnCurve() {
+			return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "secp256k1 key is not on curve")
+		}
+	case *secp256r1.PubKey:
+		pubKeyObject := typedPubKey.Key.PublicKey
+		if !pubKeyObject.IsOnCurve(pubKeyObject.X, pubKeyObject.Y) {
+			return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "secp256r1 key is not on curve")
+		}
+	case multisig.PubKey:
+		pubKeysObjects := typedPubKey.GetPubKeys()
+		ok := true
+		for _, pubKeyObject := range pubKeysObjects {
+			if err := verifyIsOnCurve(pubKeyObject); err != nil {
+				ok = false
+				break
+			}
+		}
+		if !ok {
+			return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "some keys in multisig are not on curve")
+		}
+
+	default:
+		return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "unrecognized public key type")
+	}
+
+	return nil
+}
+
 func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
 	sigTx, ok := tx.(authsigning.Tx)
 	if !ok {
@@ -285,7 +336,10 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul
 			return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey on account is not set")
 		}
 
-		// Check account sequence number.
+		if err := verifyIsOnCurve(pubKey); err != nil {
+			return ctx, err
+		}
+
 		if sig.Sequence != acc.GetSequence() {
 			return ctx, errorsmod.Wrapf(
 				sdkerrors.ErrWrongSequence,
@@ -376,6 +430,14 @@ func (isd IncrementSequenceDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, sim
 			panic(err)
 		}
 
+		pubKey := acc.GetPubKey()
+		if !simulate && pubKey == nil {
+			return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey on account is not set")
+		}
+		if err := verifyIsOnCurve(pubKey); err != nil {
+			return ctx, err
+		}
+
 		isd.ak.SetAccount(ctx, acc)
 	}
 

x/auth/ante/sigverify_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	secp256k1dcrd "github.com/decred/dcrd/dcrec/secp256k1/v4"
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
 
@@ -341,8 +342,9 @@ func TestIncrementSequenceDecorator(t *testing.T) {
 	tx, err := suite.CreateTestTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), signing.SignMode_SIGN_MODE_DIRECT)
 	require.NoError(t, err)
 
-	isd := ante.NewIncrementSequenceDecorator(suite.accountKeeper)
-	antehandler := sdk.ChainAnteDecorators(isd)
+	pubKeyDecorator := ante.NewSetPubKeyDecorator(suite.accountKeeper)
+	IncrementSequenceDecorator := ante.NewIncrementSequenceDecorator(suite.accountKeeper)
+	antehandler := sdk.ChainAnteDecorators(pubKeyDecorator, IncrementSequenceDecorator)
 
 	testCases := []struct {
 		ctx         sdk.Context
@@ -357,8 +359,113 @@ func TestIncrementSequenceDecorator(t *testing.T) {
 	}
 
 	for i, tc := range testCases {
-		_, err := antehandler(tc.ctx, tx, tc.simulate)
-		require.NoError(t, err, "unexpected error; tc #%d, %v", i, tc)
-		require.Equal(t, tc.expectedSeq, suite.accountKeeper.GetAccount(suite.ctx, addr).GetSequence())
+		t.Run(fmt.Sprintf("%d test", i), func(t *testing.T) {
+			_, err = antehandler(tc.ctx, tx, tc.simulate)
+			require.NoError(t, err, "unexpected error; tc #%d, %v", i, tc)
+			require.Equal(t, tc.expectedSeq, suite.accountKeeper.GetAccount(suite.ctx, addr).GetSequence())
+		})
+	}
+}
+
+func TestAnteHandlerChecks(t *testing.T) {
+	suite := SetupTestSuite(t, true)
+	suite.txBankKeeper.EXPECT().DenomMetadataV2(gomock.Any(), gomock.Any()).Return(&bankv1beta1.QueryDenomMetadataResponse{}, nil).AnyTimes()
+
+	feeAmount := testdata.NewTestFeeAmount()
+	gasLimit := testdata.NewTestGasLimit()
+	enabledSignModes := []signing.SignMode{signing.SignMode_SIGN_MODE_DIRECT, signing.SignMode_SIGN_MODE_TEXTUAL, signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON}
+	// Since TEXTUAL is not enabled by default, we create a custom TxConfig
+	// here which includes it.
+	txConfigOpts := authtx.ConfigOptions{
+		TextualCoinMetadataQueryFn: txmodule.NewGRPCCoinMetadataQueryFn(suite.clientCtx),
+		EnabledSignModes:           enabledSignModes,
+	}
+
+	anteTxConfig, err := authtx.NewTxConfigWithOptions(
+		codec.NewProtoCodec(suite.encCfg.InterfaceRegistry),
+		txConfigOpts,
+	)
+	require.NoError(t, err)
+
+	// make block height non-zero to ensure account numbers part of signBytes
+	suite.ctx = suite.ctx.WithBlockHeight(1)
+
+	// keys and addresses
+	priv1, _, addr1 := testdata.KeyTestPubAddr()
+
+	secp256k1NotOnCurve, _ := secp256k1dcrd.GeneratePrivateKey()
+	secp256k1NotOnCurve.Key.SetInt(0)
+	priv12 := &secp256k1.PrivKey{Key: secp256k1NotOnCurve.Serialize()}
+	addr12 := sdk.AccAddress(priv12.PubKey().Address())
+
+	priv2, _, addr2 := testdata.KeyTestPubAddrSecp256R1(t)
+	priv3, _, addr3 := testdata.KeyTestPubAddrED25519()
+
+	addrs := []sdk.AccAddress{addr1, addr12, addr2, addr3}
+
+	msgs := make([]sdk.Msg, len(addrs))
+	accs := make([]sdk.AccountI, len(addrs))
+	// set accounts and create msg for each address
+	for i, addr := range addrs {
+		acc := suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr)
+		require.NoError(t, acc.SetAccountNumber(uint64(i)+1000))
+		suite.accountKeeper.SetAccount(suite.ctx, acc)
+		msgs[i] = testdata.NewTestMsg(addr)
+		accs[i] = acc
+	}
+
+	setPubKeyDecorator := ante.NewSetPubKeyDecorator(suite.accountKeeper)
+	sigGasConsumeDecorator := ante.NewSigGasConsumeDecorator(suite.accountKeeper, ante.DefaultSigVerificationGasConsumer)
+	sigVerificationDecorator := ante.NewSigVerificationDecorator(suite.accountKeeper, anteTxConfig.SignModeHandler())
+	IncrementSequenceDecorator := ante.NewIncrementSequenceDecorator(suite.accountKeeper)
+
+	anteHandler := sdk.ChainAnteDecorators(setPubKeyDecorator, sigGasConsumeDecorator, sigVerificationDecorator, IncrementSequenceDecorator)
+
+	type testCase struct {
+		name      string
+		privs     []cryptotypes.PrivKey
+		msg       sdk.Msg
+		accNums   []uint64
+		accSeqs   []uint64
+		shouldErr bool
+		suported  bool
+	}
+
+	// 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},
+		{"secp256k1_NotOnCurve", []cryptotypes.PrivKey{priv12}, msgs[1], []uint64{accs[1].GetAccountNumber()}, []uint64{1}, true, true},
+		{"secp256r1_onCurve", []cryptotypes.PrivKey{priv2}, msgs[2], []uint64{accs[2].GetAccountNumber()}, []uint64{0}, false, true},
+		{"ed255619", []cryptotypes.PrivKey{priv3}, msgs[3], []uint64{accs[2].GetAccountNumber()}, []uint64{3}, true, false},
+	}
+
+	for i, 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
+
+			require.NoError(t, suite.txBuilder.SetMsgs(tc.msg))
+
+			suite.txBuilder.SetFeeAmount(feeAmount)
+			suite.txBuilder.SetGasLimit(gasLimit)
+
+			tx, err := suite.CreateTestTx(suite.ctx, tc.privs, tc.accNums, tc.accSeqs, suite.ctx.ChainID(), signing.SignMode_SIGN_MODE_DIRECT)
+			require.NoError(t, err)
+
+			txBytes, err := suite.clientCtx.TxConfig.TxEncoder()(tx)
+			require.NoError(t, err)
+
+			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.suported {
+					require.ErrorContains(t, err, "is not on curve")
+				} else {
+					require.ErrorContains(t, err, "public keys are unsupported")
+				}
+			} else {
+				require.Nil(t, err, "TestCase %d: %s errored unexpectedly. Err: %v", i, tc.name, err)
+			}
+		})
 	}
 }