From 5424b55c5706c2acfd98898b9877c3b846583af5 Mon Sep 17 00:00:00 2001 From: Rootul P Date: Wed, 13 Mar 2024 15:07:34 -0400 Subject: [PATCH] fix(crypto): error if incorrect ledger public key (#19691) Co-authored-by: Marko --- CHANGELOG.md | 3 +- crypto/keyring/keyring.go | 8 ++++ crypto/keyring/keyring_ledger_test.go | 66 +++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4886f460ef5..32447138ffd 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/crypto/keyring/keyring.go b/crypto/keyring/keyring.go index e46db74693b..d5392821404 100644 --- a/crypto/keyring/keyring.go +++ b/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: diff --git a/crypto/keyring/keyring_ledger_test.go b/crypto/keyring/keyring_ledger_test.go index 89f1013b642..244e877f174 100644 --- a/crypto/keyring/keyring_ledger_test.go +++ b/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) + } + }) + } +}