From 6393796b3242bfd3081720879fc295e75535e04f Mon Sep 17 00:00:00 2001 From: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com> Date: Wed, 27 Mar 2024 21:54:22 +0900 Subject: [PATCH 1/2] fix(crypto): error if incorrect ledger public key (#1316) (cherry picked from commit e62a33006bbe19f12309be75a2771c6623eea155) # Conflicts: # CHANGELOG.md # crypto/keyring/keyring.go --- CHANGELOG.md | 21 +++++++++ crypto/keyring/keyring.go | 17 +++++++- crypto/keyring/keyring_ledger_test.go | 63 +++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb197ca3e7..4110f9a694 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,11 +43,32 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements ### Bug Fixes +<<<<<<< HEAD * (x/auth) [#1281](https://github.com/Finschia/finschia-sdk/pull/1281) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking. (backport #1274) * (x/foundation) [\#1283](https://github.com/Finschia/finschia-sdk/pull/1283) add init logic of foundation module accounts to InitGenesis in order to eliminate potential panic (backport #1277) * (x/collection) [\#1282](https://github.com/Finschia/finschia-sdk/pull/1282) eliminates potential risk for Insufficient Sanity Check of tokenID in Genesis (backport #1276) * (x/collection) [\#1290](https://github.com/Finschia/finschia-sdk/pull/1290) export x/collection params into genesis (backport #1268) * (x/foundation) [\#1295](https://github.com/Finschia/finschia-sdk/pull/1295) add missing error handling for migration +======= +* chore(deps) [\#1141](https://github.com/Finschia/finschia-sdk/pull/1141) Bump github.com/cosmos/ledger-cosmos-go from 0.12.2 to 0.13.2 to fix ledger signing issue +* (x/auth, x/slashing) [\#1179](https://github.com/Finschia/finschia-sdk/pull/1179) modify missing changes of converting to tendermint +* (x/auth) [#1274](https://github.com/Finschia/finschia-sdk/pull/1274) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking. +* (x/collection) [\#1276](https://github.com/Finschia/finschia-sdk/pull/1276) eliminates potential risk for Insufficient Sanity Check of tokenID in Genesis +* (x/foundation) [\#1277](https://github.com/Finschia/finschia-sdk/pull/1277) add init logic of foundation module accounts to InitGenesis in order to eliminate potential panic +* (x/collection, x/token) [\#1288](https://github.com/Finschia/finschia-sdk/pull/1288) use accAddress to compare in validatebasic function in collection & token modules +* (x/collection) [\#1268](https://github.com/Finschia/finschia-sdk/pull/1268) export x/collection params into genesis +* (x/collection) [\#1294](https://github.com/Finschia/finschia-sdk/pull/1294) reject NFT coins on FT APIs +* (sec) [\#1302](https://github.com/Finschia/finschia-sdk/pull/1302) remove map iteration non-determinism with keys + sorting +* (client) [\#1303](https://github.com/Finschia/finschia-sdk/pull/1303) fix possible overflow in BuildUnsignedTx +* (types) [\#1299](https://github.com/Finschia/finschia-sdk/pull/1299) add missing nil checks +* (x/staking) [\#1301](https://github.com/Finschia/finschia-sdk/pull/1301) Use bytes instead of string comparison in delete validator queue (backport cosmos/cosmos-sdk#12303) +* (x/gov) [\#1304](https://github.com/Finschia/finschia-sdk/pull/1304) fetch a failed proposal tally from proposal.FinalTallyResult in the gprc query +* (x/staking) [\#1306](https://github.com/Finschia/finschia-sdk/pull/1306) add validation for potential slashing evasion during re-delegation +* (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) +>>>>>>> e62a33006 (fix(crypto): error if incorrect ledger public key (#1316)) ### Removed diff --git a/crypto/keyring/keyring.go b/crypto/keyring/keyring.go index 5171f2965c..0a6b107f01 100644 --- a/crypto/keyring/keyring.go +++ b/crypto/keyring/keyring.go @@ -11,8 +11,12 @@ import ( "strings" "github.com/99designs/keyring" +<<<<<<< HEAD occrypto "github.com/Finschia/ostracon/crypto" bip39 "github.com/cosmos/go-bip39" +======= + "github.com/cosmos/go-bip39" +>>>>>>> e62a33006 (fix(crypto): error if incorrect ledger public key (#1316)) "github.com/pkg/errors" "github.com/tendermint/crypto/bcrypt" @@ -614,18 +618,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 } diff --git a/crypto/keyring/keyring_ledger_test.go b/crypto/keyring/keyring_ledger_test.go index 27503fc8e1..fc604702e6 100644 --- a/crypto/keyring/keyring_ledger_test.go +++ b/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) + } + }) + } +} From 9a1ac634412197f34918bfaf302813ff067a1d16 Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Fri, 29 Mar 2024 02:41:57 +0000 Subject: [PATCH 2/2] Apply suggestions from code review --- CHANGELOG.md | 20 -------------------- crypto/keyring/keyring.go | 4 ---- 2 files changed, 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4110f9a694..84d70cfcac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,32 +43,12 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements ### Bug Fixes -<<<<<<< HEAD * (x/auth) [#1281](https://github.com/Finschia/finschia-sdk/pull/1281) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking. (backport #1274) * (x/foundation) [\#1283](https://github.com/Finschia/finschia-sdk/pull/1283) add init logic of foundation module accounts to InitGenesis in order to eliminate potential panic (backport #1277) * (x/collection) [\#1282](https://github.com/Finschia/finschia-sdk/pull/1282) eliminates potential risk for Insufficient Sanity Check of tokenID in Genesis (backport #1276) * (x/collection) [\#1290](https://github.com/Finschia/finschia-sdk/pull/1290) export x/collection params into genesis (backport #1268) * (x/foundation) [\#1295](https://github.com/Finschia/finschia-sdk/pull/1295) add missing error handling for migration -======= -* chore(deps) [\#1141](https://github.com/Finschia/finschia-sdk/pull/1141) Bump github.com/cosmos/ledger-cosmos-go from 0.12.2 to 0.13.2 to fix ledger signing issue -* (x/auth, x/slashing) [\#1179](https://github.com/Finschia/finschia-sdk/pull/1179) modify missing changes of converting to tendermint -* (x/auth) [#1274](https://github.com/Finschia/finschia-sdk/pull/1274) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking. -* (x/collection) [\#1276](https://github.com/Finschia/finschia-sdk/pull/1276) eliminates potential risk for Insufficient Sanity Check of tokenID in Genesis -* (x/foundation) [\#1277](https://github.com/Finschia/finschia-sdk/pull/1277) add init logic of foundation module accounts to InitGenesis in order to eliminate potential panic -* (x/collection, x/token) [\#1288](https://github.com/Finschia/finschia-sdk/pull/1288) use accAddress to compare in validatebasic function in collection & token modules -* (x/collection) [\#1268](https://github.com/Finschia/finschia-sdk/pull/1268) export x/collection params into genesis -* (x/collection) [\#1294](https://github.com/Finschia/finschia-sdk/pull/1294) reject NFT coins on FT APIs -* (sec) [\#1302](https://github.com/Finschia/finschia-sdk/pull/1302) remove map iteration non-determinism with keys + sorting -* (client) [\#1303](https://github.com/Finschia/finschia-sdk/pull/1303) fix possible overflow in BuildUnsignedTx -* (types) [\#1299](https://github.com/Finschia/finschia-sdk/pull/1299) add missing nil checks -* (x/staking) [\#1301](https://github.com/Finschia/finschia-sdk/pull/1301) Use bytes instead of string comparison in delete validator queue (backport cosmos/cosmos-sdk#12303) -* (x/gov) [\#1304](https://github.com/Finschia/finschia-sdk/pull/1304) fetch a failed proposal tally from proposal.FinalTallyResult in the gprc query -* (x/staking) [\#1306](https://github.com/Finschia/finschia-sdk/pull/1306) add validation for potential slashing evasion during re-delegation -* (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) ->>>>>>> e62a33006 (fix(crypto): error if incorrect ledger public key (#1316)) ### Removed diff --git a/crypto/keyring/keyring.go b/crypto/keyring/keyring.go index 0a6b107f01..a38f57ba8a 100644 --- a/crypto/keyring/keyring.go +++ b/crypto/keyring/keyring.go @@ -11,12 +11,8 @@ import ( "strings" "github.com/99designs/keyring" -<<<<<<< HEAD occrypto "github.com/Finschia/ostracon/crypto" - bip39 "github.com/cosmos/go-bip39" -======= "github.com/cosmos/go-bip39" ->>>>>>> e62a33006 (fix(crypto): error if incorrect ledger public key (#1316)) "github.com/pkg/errors" "github.com/tendermint/crypto/bcrypt"