From 025354465d62e8f7f9efb5840f9f798ee821307d Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Mon, 15 Mar 2021 13:42:10 +0100 Subject: [PATCH] Fix multisig LegacyAminoPubKey Amino marshaling (#8841) * Use v034auth RegisterCrypto * Add custom amino for LegacyAminoPubKey * Fix registercrypto * Revert old PR * revert some genutil stuff * Add comment * Add changelog * Remove binary marshalling * Fix lint * Fix lint again * Fix lint, 3rd time's a charm * ignore wrong linter warning * Fix UnmarshalAmioJSON Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Alessio Treglia Co-authored-by: Anil Kumar Kammari (cherry picked from commit d4d27e1c0c138d76ab3082ba8e380d4d26db050a) # Conflicts: # CHANGELOG.md # x/auth/legacy/v040/migrate.go --- CHANGELOG.md | 52 ++++++++++++++++ crypto/keys/multisig/amino.go | 88 +++++++++++++++++++++++++++ crypto/keys/multisig/codec.go | 3 + crypto/keys/multisig/multisig_test.go | 78 +++++++++++++++++++++++- x/auth/legacy/legacytx/stdtx.go | 2 +- x/auth/legacy/v040/migrate.go | 4 ++ x/genutil/legacy/v038/migrate.go | 2 + 7 files changed, 226 insertions(+), 3 deletions(-) create mode 100644 crypto/keys/multisig/amino.go diff --git a/CHANGELOG.md b/CHANGELOG.md index fc8d26655af4..2b4e792a6a8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,58 @@ Ref: https://keepachangelog.com/en/1.0.0/ This release fixes security vulnerability identified in the simapp. +<<<<<<< HEAD +======= +* [\#8559](https://github.com/cosmos/cosmos-sdk/pull/8559) Added Protobuf compatible secp256r1 ECDSA signatures. +* [\#8786](https://github.com/cosmos/cosmos-sdk/pull/8786) Enabled secp256r1 in x/auth. +* (rosetta) [\#8729](https://github.com/cosmos/cosmos-sdk/pull/8729) Data API fully supports balance tracking. Construction API can now construct any message supported by the application. + +### Client Breaking Changes + +* [\#8363](https://github.com/cosmos/cosmos-sdk/pull/8363) Addresses no longer have a fixed 20-byte length. From the SDK modules' point of view, any 1-255 bytes-long byte array is a valid address. +* [\#8346](https://github.com/cosmos/cosmos-sdk/pull/8346) All CLI `tx` commands generate ServiceMsgs by default. Graceful Amino support has been added to ServiceMsgs to support signing legacy Msgs. +* (crypto/ed25519) [\#8690] Adopt zip1215 ed2559 verification rules. +* [\#8849](https://github.com/cosmos/cosmos-sdk/pull/8849) Upgrade module no longer supports time based upgrades. + + +### API Breaking Changes + +* (keyring) [#\8662](https://github.com/cosmos/cosmos-sdk/pull/8662) `NewMnemonic` now receives an additional `passphrase` argument to secure the key generated by the bip39 mnemonic. +* (x/bank) [\#8473](https://github.com/cosmos/cosmos-sdk/pull/8473) Bank keeper does not expose unsafe balance changing methods such as `SetBalance`, `SetSupply` etc. +* (x/staking) [\#8473](https://github.com/cosmos/cosmos-sdk/pull/8473) On genesis init, if non bonded pool and bonded pool balance, coming from the bank module, does not match what is saved in the staking state, the initialization will panic. +* (x/gov) [\#8473](https://github.com/cosmos/cosmos-sdk/pull/8473) On genesis init, if the gov module account balance, coming from bank module state, does not match the one in gov module state, the initialization will panic. +* (x/distribution) [\#8473](https://github.com/cosmos/cosmos-sdk/pull/8473) On genesis init, if the distribution module account balance, coming from bank module state, does not match the one in distribution module state, the initialization will panic. +* (client/keys) [\#8500](https://github.com/cosmos/cosmos-sdk/pull/8500) `InfoImporter` interface is removed from legacy keybase. +* [\#8629](https://github.com/cosmos/cosmos-sdk/pull/8629) Deprecated `SetFullFundraiserPath` from `Config` in favor of `SetPurpose` and `SetCoinType`. +* (x/upgrade) [\#8673](https://github.com/cosmos/cosmos-sdk/pull/8673) Remove IBC logic from x/upgrade. Deprecates IBC fields in an Upgrade Plan. IBC upgrade logic moved to 02-client and an IBC UpgradeProposal is added. +* (x/bank) [\#8517](https://github.com/cosmos/cosmos-sdk/pull/8517) `SupplyI` interface and `Supply` are removed and uses `sdk.Coins` for supply tracking +* (x/auth) [\#8129](https://github.com/cosmos/cosmos-sdk/pull/8828) Updated `SigVerifiableTx.GetPubKeys` method signature to return error. + + +### State Machine Breaking + +* (x/{bank,distrib,gov,slashing,staking}) [\#8363](https://github.com/cosmos/cosmos-sdk/issues/8363) Store keys have been modified to allow for variable-length addresses. +* (x/evidence) [\#8502](https://github.com/cosmos/cosmos-sdk/pull/8502) `HandleEquivocationEvidence` persists the evidence to state. +* (x/gov) [\#7733](https://github.com/cosmos/cosmos-sdk/pull/7733) ADR 037 Implementation: Governance Split Votes +* (x/bank) [\#8656](https://github.com/cosmos/cosmos-sdk/pull/8656) balance and supply are now correctly tracked via `coin_spent`, `coin_received`, `coinbase` and `burn` events. +* (x/bank) [\#8517](https://github.com/cosmos/cosmos-sdk/pull/8517) Supply is now stored and tracked as `sdk.Coins` +* (store) [\#8790](https://github.com/cosmos/cosmos-sdk/pull/8790) Reduce gas costs by 10x for transient store operations. + +### Improvements + +* (x/bank) [\#8614](https://github.com/cosmos/cosmos-sdk/issues/8614) Add `Name` and `Symbol` fields to denom metadata +* (x/auth) [\#8522](https://github.com/cosmos/cosmos-sdk/pull/8522) Allow to query all stored accounts +* (crypto/types) [\#8600](https://github.com/cosmos/cosmos-sdk/pull/8600) `CompactBitArray`: optimize the `NumTrueBitsBefore` method and add an `Equal` method. +* (grpc) [\#8815](https://github.com/cosmos/cosmos-sdk/pull/8815) Add orderBy parameter to `TxsByEvents` endpoint. + +### Bug Fixes + +* (keyring) [#\8635](https://github.com/cosmos/cosmos-sdk/issues/8635) Remove hardcoded default passphrase value on `NewMnemonic` +* (x/bank) [\#8434](https://github.com/cosmos/cosmos-sdk/pull/8434) Fix legacy REST API `GET /bank/total` and `GET /bank/total/{denom}` in swagger +* (x/slashing) [\#8427](https://github.com/cosmos/cosmos-sdk/pull/8427) Fix query signing infos command +* (server) [\#8399](https://github.com/cosmos/cosmos-sdk/pull/8399) fix gRPC-web flag default value +* (crypto) [\#8841](https://github.com/cosmos/cosmos-sdk/pull/8841) Fix legacy multisig amino marshaling, allowing migrations to work between v0.39 and v0.40+. +>>>>>>> d4d27e1c0... Fix multisig LegacyAminoPubKey Amino marshaling (#8841) ## [v0.42.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.42.0) - 2021-03-08 diff --git a/crypto/keys/multisig/amino.go b/crypto/keys/multisig/amino.go new file mode 100644 index 000000000000..4849a23173d2 --- /dev/null +++ b/crypto/keys/multisig/amino.go @@ -0,0 +1,88 @@ +package multisig + +import ( + types "github.com/cosmos/cosmos-sdk/codec/types" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// tmMultisig implements a K of N threshold multisig. It is used for +// Amino JSON marshaling of LegacyAminoPubKey (see below for details). +// +// This struct is copy-pasted from: +// https://github.com/tendermint/tendermint/blob/v0.33.9/crypto/multisig/threshold_pubkey.go +// +// This struct was used in the SDK <=0.39. In 0.40 and the switch to protobuf, +// it has been converted to LegacyAminoPubKey. However, there's one difference: +// the threshold field was an `uint` before, and an `uint32` after. This caused +// amino marshaling to be breaking: amino marshals `uint32` as a JSON number, +// and `uint` as a JSON string. +// +// In this file, we're overriding LegacyAminoPubKey's default JSON Amino +// marshaling by using this struct. Please note that we are NOT overriding the +// Amino binary marshaling, as that _might_ introduce breaking changes in the +// keyring, where multisigs are amino-binary-encoded. +// +// ref: https://github.com/cosmos/cosmos-sdk/issues/8776 +type tmMultisig struct { + K uint `json:"threshold"` + PubKeys []cryptotypes.PubKey `json:"pubkeys"` +} + +// protoToTm converts a LegacyAminoPubKey into a tmMultisig. +func protoToTm(protoPk *LegacyAminoPubKey) (tmMultisig, error) { + var ok bool + pks := make([]cryptotypes.PubKey, len(protoPk.PubKeys)) + for i, pk := range protoPk.PubKeys { + pks[i], ok = pk.GetCachedValue().(cryptotypes.PubKey) + if !ok { + return tmMultisig{}, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", (cryptotypes.PubKey)(nil), pk.GetCachedValue()) + } + } + + return tmMultisig{ + K: uint(protoPk.Threshold), + PubKeys: pks, + }, nil +} + +// tmToProto converts a tmMultisig into a LegacyAminoPubKey. +func tmToProto(tmPk tmMultisig) (*LegacyAminoPubKey, error) { + var err error + pks := make([]*types.Any, len(tmPk.PubKeys)) + for i, pk := range tmPk.PubKeys { + pks[i], err = types.NewAnyWithValue(pk) + if err != nil { + return nil, err + } + } + + return &LegacyAminoPubKey{ + Threshold: uint32(tmPk.K), + PubKeys: pks, + }, nil +} + +// MarshalAminoJSON overrides amino JSON unmarshaling. +func (m LegacyAminoPubKey) MarshalAminoJSON() (tmMultisig, error) { //nolint:golint + return protoToTm(&m) +} + +// UnmarshalAminoJSON overrides amino JSON unmarshaling. +func (m *LegacyAminoPubKey) UnmarshalAminoJSON(tmPk tmMultisig) error { + protoPk, err := tmToProto(tmPk) + if err != nil { + return err + } + + // Instead of just doing `*m = *protoPk`, we prefer to modify in-place the + // existing Anys inside `m` (instead of allocating new Anys), as so not to + // break the `.compat` fields in the existing Anys. + for i := range m.PubKeys { + m.PubKeys[i].TypeUrl = protoPk.PubKeys[i].TypeUrl + m.PubKeys[i].Value = protoPk.PubKeys[i].Value + } + m.Threshold = protoPk.Threshold + + return nil +} diff --git a/crypto/keys/multisig/codec.go b/crypto/keys/multisig/codec.go index b92576e4f3f5..d501e1b427cf 100644 --- a/crypto/keys/multisig/codec.go +++ b/crypto/keys/multisig/codec.go @@ -15,6 +15,9 @@ const ( PubKeyAminoRoute = "tendermint/PubKeyMultisigThreshold" ) +//nolint +// Deprecated: Amino is being deprecated in the SDK. But even if you need to +// use Amino for some reason, please use `codec/legacy.Cdc` instead. var AminoCdc = codec.NewLegacyAmino() func init() { diff --git a/crypto/keys/multisig/multisig_test.go b/crypto/keys/multisig/multisig_test.go index 2b91e74eb6c5..1284c6c88476 100644 --- a/crypto/keys/multisig/multisig_test.go +++ b/crypto/keys/multisig/multisig_test.go @@ -6,7 +6,9 @@ import ( "github.com/stretchr/testify/require" "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/codec/legacy" "github.com/cosmos/cosmos-sdk/codec/types" + cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" @@ -250,12 +252,12 @@ func TestPubKeyMultisigThresholdAminoToIface(t *testing.T) { pubkeys, _ := generatePubKeysAndSignatures(5, msg) multisigKey := kmultisig.NewLegacyAminoPubKey(2, pubkeys) - ab, err := kmultisig.AminoCdc.MarshalBinaryLengthPrefixed(multisigKey) + ab, err := legacy.Cdc.MarshalBinaryLengthPrefixed(multisigKey) require.NoError(t, err) // like other cryptotypes.Pubkey implementations (e.g. ed25519.PubKey), // LegacyAminoPubKey should be deserializable into a cryptotypes.LegacyAminoPubKey: var pubKey kmultisig.LegacyAminoPubKey - err = kmultisig.AminoCdc.UnmarshalBinaryLengthPrefixed(ab, &pubKey) + err = legacy.Cdc.UnmarshalBinaryLengthPrefixed(ab, &pubKey) require.NoError(t, err) require.Equal(t, multisigKey.Equals(&pubKey), true) @@ -307,3 +309,75 @@ func reorderPubKey(pk *kmultisig.LegacyAminoPubKey) (other *kmultisig.LegacyAmin other = &kmultisig.LegacyAminoPubKey{Threshold: 2, PubKeys: pubkeysCpy} return } + +func TestAminoBinary(t *testing.T) { + pubKey1 := secp256k1.GenPrivKey().PubKey() + pubKey2 := secp256k1.GenPrivKey().PubKey() + multisigKey := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pubKey1, pubKey2}) + + // Do a round-trip key->bytes->key. + bz, err := legacy.Cdc.MarshalBinaryBare(multisigKey) + require.NoError(t, err) + var newMultisigKey cryptotypes.PubKey + err = legacy.Cdc.UnmarshalBinaryBare(bz, &newMultisigKey) + require.NoError(t, err) + require.Equal(t, multisigKey.Threshold, newMultisigKey.(*kmultisig.LegacyAminoPubKey).Threshold) +} + +func TestAminoMarshalJSON(t *testing.T) { + pubKey1 := secp256k1.GenPrivKey().PubKey() + pubKey2 := secp256k1.GenPrivKey().PubKey() + multisigKey := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pubKey1, pubKey2}) + + bz, err := legacy.Cdc.MarshalJSON(multisigKey) + require.NoError(t, err) + + // Note the quotes around `"2"`. They are present because we are overriding + // the Amino JSON marshaling of LegacyAminoPubKey (using tmMultisig). + // Without the override, there would not be any quotes. + require.Contains(t, string(bz), "\"threshold\":\"2\"") +} + +func TestAminoUnmarshalJSON(t *testing.T) { + // This is a real multisig from the Akash chain. It has been exported from + // v0.39, hence the `threshold` field as a string. + // We are testing that when unmarshaling this JSON into a LegacyAminoPubKey + // with amino, there's no error. + // ref: https://github.com/cosmos/cosmos-sdk/issues/8776 + pkJSON := `{ + "type": "tendermint/PubKeyMultisigThreshold", + "value": { + "pubkeys": [ + { + "type": "tendermint/PubKeySecp256k1", + "value": "AzYxq2VNeD10TyABwOgV36OVWDIMn8AtI4OFA0uQX2MK" + }, + { + "type": "tendermint/PubKeySecp256k1", + "value": "A39cdsrm00bTeQ3RVZVqjkH8MvIViO9o99c8iLiNO35h" + }, + { + "type": "tendermint/PubKeySecp256k1", + "value": "A/uLLCZph8MkFg2tCxqSMGwFfPHdt1kkObmmrqy9aiYD" + }, + { + "type": "tendermint/PubKeySecp256k1", + "value": "A4mOMhM5gPDtBAkAophjRs6uDGZm4tD4Dbok3ai4qJi8" + }, + { + "type": "tendermint/PubKeySecp256k1", + "value": "A90icFucrjNNz2SAdJWMApfSQcARIqt+M2x++t6w5fFs" + } + ], + "threshold": "3" + } +}` + + cdc := codec.NewLegacyAmino() + cryptocodec.RegisterCrypto(cdc) + + var pk cryptotypes.PubKey + err := cdc.UnmarshalJSON([]byte(pkJSON), &pk) + require.NoError(t, err) + require.Equal(t, uint32(3), pk.(*kmultisig.LegacyAminoPubKey).Threshold) +} diff --git a/x/auth/legacy/legacytx/stdtx.go b/x/auth/legacy/legacytx/stdtx.go index 2b8a6c0e0547..0aa06da06d26 100644 --- a/x/auth/legacy/legacytx/stdtx.go +++ b/x/auth/legacy/legacytx/stdtx.go @@ -287,7 +287,7 @@ func (tx StdTx) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error { // Signatures contain PubKeys, which need to be unpacked. for _, s := range tx.Signatures { - err := codectypes.UnpackInterfaces(s, unpacker) + err := s.UnpackInterfaces(unpacker) if err != nil { return err } diff --git a/x/auth/legacy/v040/migrate.go b/x/auth/legacy/v040/migrate.go index 363ec7ba82df..18a8755ff829 100644 --- a/x/auth/legacy/v040/migrate.go +++ b/x/auth/legacy/v040/migrate.go @@ -11,8 +11,12 @@ import ( // convertBaseAccount converts a 0.39 BaseAccount to a 0.40 BaseAccount. func convertBaseAccount(old *v039auth.BaseAccount) *v040auth.BaseAccount { var any *codectypes.Any +<<<<<<< HEAD // If the old genesis had a pubkey, we pack it inside an Any. Or else, we // just leave it nil. +======= + +>>>>>>> d4d27e1c0... Fix multisig LegacyAminoPubKey Amino marshaling (#8841) if old.PubKey != nil { var err error any, err = codectypes.NewAnyWithValue(old.PubKey) diff --git a/x/genutil/legacy/v038/migrate.go b/x/genutil/legacy/v038/migrate.go index d0ca4c4186bc..017d0e68fa53 100644 --- a/x/genutil/legacy/v038/migrate.go +++ b/x/genutil/legacy/v038/migrate.go @@ -3,6 +3,7 @@ package v038 import ( "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" + cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" v036auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v036" v038auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v038" v036distr "github.com/cosmos/cosmos-sdk/x/distribution/legacy/v036" @@ -19,6 +20,7 @@ import ( // Migrate migrates exported state from v0.36/v0.37 to a v0.38 genesis state. func Migrate(appState types.AppMap, _ client.Context) types.AppMap { v036Codec := codec.NewLegacyAmino() + cryptocodec.RegisterCrypto(v036Codec) v036gov.RegisterLegacyAminoCodec(v036Codec) v036distr.RegisterLegacyAminoCodec(v036Codec) v036params.RegisterLegacyAminoCodec(v036Codec)