fix(crypto): error if incorrect ledger public key (backport #1316) (#…

…1322)

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

(cherry picked from commit e62a330)

# Conflicts:
#	CHANGELOG.md
#	crypto/keyring/keyring.go

* Apply suggestions from code review

---------

Co-authored-by: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com>
Co-authored-by: Youngtaek Yoon <noreply@yoon.mailer.me>

CHANGELOG.md

@@ -56,6 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (client/keys) [#1312](https://github.com/Finschia/finschia-sdk/pull/1312) ignore error when key not found in `keys delete`
 * (store) [\#1310](https://github.com/Finschia/finschia-sdk/pull/1310) fix app-hash mismatch if upgrade migration commit is interrupted(backport cosmos/cosmos-sdk#13530)
 * (types) [\#1313](https://github.com/Finschia/finschia-sdk/pull/1313) fix correctly coalesce coins even with repeated denominations(backport cosmos/cosmos-sdk#13265)
+* (x/crypto) [\#1316](https://github.com/Finschia/finschia-sdk/pull/1316) error if incorrect ledger public key (backport cosmos/cosmos-sdk#14460, cosmos/cosmos-sdk#19691) 
 
 ### Removed
 

crypto/keyring/keyring.go

@@ -12,7 +12,7 @@ import (
 
 	"github.com/99designs/keyring"
 	occrypto "github.com/Finschia/ostracon/crypto"
-	bip39 "github.com/cosmos/go-bip39"
+	"github.com/cosmos/go-bip39"
 	"github.com/pkg/errors"
 	"github.com/tendermint/crypto/bcrypt"
 
@@ -614,18 +614,27 @@ func SignWithLedger(info Info, msg []byte) (sig []byte, pub types.PubKey, err er
 
 	path, err := info.GetPath()
 	if err != nil {
-		return
+		return nil, nil, err
 	}
 
 	priv, err := ledger.NewPrivKeySecp256k1Unsafe(*path)
 	if err != nil {
-		return
+		return nil, nil, err
 	}
 
 	sig, err = priv.Sign(msg)
 	if err != nil {
 		return nil, nil, err
 	}
+	ledgerPubKey := priv.PubKey()
+	pubKey := info.GetPubKey()
+	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())
+	}
+
+	if !priv.PubKey().VerifySignature(msg, sig) {
+		return nil, nil, errors.New("Ledger generated an invalid signature. Perhaps you have multiple ledgers and need to try another one")
+	}
 
 	return sig, priv.PubKey(), nil
 }

crypto/keyring/keyring_ledger_test.go

@@ -7,9 +7,13 @@ import (
 	"bytes"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/Finschia/finschia-sdk/crypto/hd"
+	"github.com/Finschia/finschia-sdk/crypto/keys/secp256k1"
+	"github.com/Finschia/finschia-sdk/crypto/ledger"
+	cryptotypes "github.com/Finschia/finschia-sdk/crypto/types"
 	sdk "github.com/Finschia/finschia-sdk/types"
 )
 
@@ -120,3 +124,62 @@ func TestAltKeyring_SaveLedgerKey(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, "m/44'/438'/3'/0/1", path.String())
 }
+
+func TestSignWithLedger(t *testing.T) {
+	// Create two distinct Ledger records: infoA and infoB.
+	// InfoA is added to the Ledger but infoB is not added.
+	pathA := hd.NewFundraiserParams(0, sdk.CoinType, 0)
+	privA, _, err := ledger.NewPrivKeySecp256k1(*pathA, "cosmos")
+	require.NoError(t, err)
+	infoA := newLedgerInfo("ledgerA", privA.PubKey(), *pathA, hd.Secp256k1Type)
+	pubA := infoA.GetPubKey()
+
+	pathB := hd.NewFundraiserParams(0, sdk.CoinType, 1)
+	// privB won't be added to the Ledger because it doesn't use ledger.NewPrivKeySecp256k1
+	privB := secp256k1.GenPrivKey()
+	infoB := newLedgerInfo("ledgerB", privB.PubKey(), *pathB, hd.Secp256k1Type)
+	require.NoError(t, err)
+	pubB := infoB.GetPubKey()
+
+	require.NotEqual(t, pubA, pubB)
+	type testCase struct {
+		name            string
+		info            Info
+		msg             []byte
+		wantSig         []byte
+		wantPub         cryptotypes.PubKey
+		wantErr         bool
+		wantErrContains string
+	}
+	testCases := []testCase{
+		{
+			name:    "ordinary ledger tx",
+			info:    infoA,
+			msg:     []byte("msg"),
+			wantSig: []byte{0xf6, 0xa4, 0x8d, 0x2b, 0x57, 0xd, 0x24, 0x8e, 0x37, 0x66, 0xd9, 0x1b, 0xe9, 0x2b, 0x5, 0x12, 0x48, 0x62, 0x72, 0x8f, 0x87, 0x26, 0x16, 0x36, 0x46, 0x15, 0xff, 0xa4, 0xa7, 0x7, 0xdf, 0x74, 0x20, 0xcd, 0x92, 0x9b, 0x5e, 0xab, 0x7b, 0x63, 0xfb, 0x13, 0x75, 0xa6, 0x74, 0xb4, 0x3d, 0x61, 0x34, 0x33, 0x6, 0x2a, 0x7, 0x59, 0x9b, 0x45, 0x62, 0xc6, 0xb6, 0x59, 0xcc, 0x38, 0xad, 0xbd},
+			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",
+			info:            infoB,
+			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.info, tc.msg)
+			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)
+			}
+		})
+	}
+}