fix(crypto): error if incorrect ledger public key (#19691)

Co-authored-by: Marko <marbar3778@yahoo.com>

Author: rootulp

CHANGELOG.md

@@ -96,6 +96,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
 * (simulation) [#18196](https://github.com/cosmos/cosmos-sdk/pull/18196) Fix the problem of `validator set is empty after InitGenesis` in simulation test.
 * (baseapp) [#18551](https://github.com/cosmos/cosmos-sdk/pull/18551) Fix SelectTxForProposal the calculation method of tx bytes size is inconsistent with CometBFT
 * (server) [#18994](https://github.com/cosmos/cosmos-sdk/pull/18994) Update server context directly rather than a reference to a sub-object
+* (crypto) [#19691](https://github.com/cosmos/cosmos-sdk/pull/19691) Fix tx sign doesn't throw an error when incorrect Ledger is used.
 
 ### API Breaking Changes
 
@@ -143,7 +144,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
 * (types) [#18607](https://github.com/cosmos/cosmos-sdk/pull/18607) Removed address verifier from global config, moved verifier function to bech32 codec.
 * (server) [#18909](https://github.com/cosmos/cosmos-sdk/pull/18909) Remove configuration endpoint on grpc reflection endpoint in favour of auth module bech32prefix endpoint already exposed.
 * (crypto) [#19541](https://github.com/cosmos/cosmos-sdk/pull/19541) The deprecated `FromTmProtoPublicKey`, `ToTmProtoPublicKey`, `FromTmPubKeyInterface` and `ToTmPubKeyInterface` functions have been removed. Use their replacements (`Cmt` instead of `Tm`) instead.
-* (types) [#19652](https://github.com/cosmos/cosmos-sdk/pull/19652) 
+* (types) [#19652](https://github.com/cosmos/cosmos-sdk/pull/19652)
     * Moved`types/module.HasRegisterInterfaces` to `cosmossdk.io/core`.
     * Moved `RegisterInterfaces` and `RegisterImplementations` from `InterfaceRegistry` to `cosmossdk.io/core/registry.LegacyRegistry` interface.
 * (types) [#19627](https://github.com/cosmos/cosmos-sdk/pull/19627) and [#19735](https://github.com/cosmos/cosmos-sdk/pull/19735) All genesis interfaces now don't take `codec.JsonCodec`.

crypto/keyring/keyring.go

@@ -634,6 +634,14 @@ func SignWithLedger(k *Record, msg []byte, signMode signing.SignMode) (sig []byt
 	if err != nil {
 		return nil, nil, err
 	}
+	ledgerPubKey := priv.PubKey()
+	pubKey, err := k.GetPubKey()
+	if err != nil {
+		return nil, nil, err
+	}
+	if !pubKey.Equals(ledgerPubKey) {
+		return nil, nil, fmt.Errorf("the public key that the user attempted to sign with does not match the public key on the ledger device. %v does not match %v", pubKey.String(), ledgerPubKey.String())
+	}
 
 	switch signMode {
 	case signing.SignMode_SIGN_MODE_TEXTUAL:

crypto/keyring/keyring_ledger_test.go

@@ -8,9 +8,13 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/cosmos/cosmos-sdk/crypto/hd"
+	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
+	"github.com/cosmos/cosmos-sdk/crypto/ledger"
+	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	"github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 )
@@ -133,3 +137,65 @@ func TestAltKeyring_SaveLedgerKey(t *testing.T) {
 	path := ledgerInfo.GetPath()
 	require.Equal(t, "m/44'/118'/3'/0/1", path.String())
 }
+
+func TestSignWithLedger(t *testing.T) {
+	// Create two distinct Ledger records: recordA and recordB.
+	// RecordA is added to the Ledger but recordB is not added.
+	pathA := hd.NewFundraiserParams(0, types.CoinType, 0)
+	privA, _, err := ledger.NewPrivKeySecp256k1(*pathA, "cosmos")
+	require.NoError(t, err)
+	recordA, err := NewLedgerRecord("ledgerA", privA.PubKey(), pathA)
+	require.NoError(t, err)
+	pubA, err := recordA.GetPubKey()
+	require.NoError(t, err)
+
+	pathB := hd.NewFundraiserParams(0, types.CoinType, 1)
+	// privB won't be added to the Ledger because it doesn't use ledger.NewPrivKeySecp256k1
+	privB := secp256k1.GenPrivKey()
+	recordB, err := NewLedgerRecord("ledgerB", privB.PubKey(), pathB)
+	require.NoError(t, err)
+	pubB, err := recordB.GetPubKey()
+	require.NoError(t, err)
+
+	require.NotEqual(t, pubA, pubB)
+	type testCase struct {
+		name            string
+		record          *Record
+		msg             []byte
+		wantSig         []byte
+		wantPub         cryptotypes.PubKey
+		wantErr         bool
+		wantErrContains string
+	}
+	testCases := []testCase{
+		{
+			name:    "ordinary ledger tx",
+			record:  recordA,
+			msg:     []byte("msg"),
+			wantSig: []byte{0xfb, 0x93, 0x1b, 0xb9, 0x75, 0x25, 0xe7, 0x99, 0x64, 0xc2, 0x78, 0xf7, 0x94, 0x9a, 0x63, 0x83, 0xe2, 0x59, 0x76, 0x48, 0x1d, 0x2, 0xbc, 0xc2, 0x83, 0x21, 0x24, 0x4b, 0x95, 0x99, 0x25, 0x8b, 0x30, 0x38, 0x6, 0x61, 0x79, 0x9a, 0x9e, 0x8, 0x98, 0xfd, 0x34, 0xc6, 0x7e, 0x47, 0x4d, 0x5f, 0xe, 0xf3, 0xc3, 0xe7, 0xdd, 0xe3, 0x89, 0x80, 0xda, 0x8b, 0x48, 0x15, 0x34, 0xce, 0xdf, 0x1c},
+			wantPub: pubA,
+			wantErr: false,
+		},
+		{
+			name:            "want error when the public key the user attempted to sign with doesn't match the public key on the ledger",
+			record:          recordB,
+			msg:             []byte("msg"),
+			wantSig:         []byte(nil),
+			wantPub:         nil,
+			wantErr:         true,
+			wantErrContains: "the public key that the user attempted to sign with does not match the public key on the ledger device",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			sig, pub, err := SignWithLedger(tc.record, tc.msg, signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
+			assert.Equal(t, tc.wantSig, sig)
+			assert.Equal(t, tc.wantPub, pub)
+			if tc.wantErr {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), tc.wantErrContains)
+			}
+		})
+	}
+}