Skip to content

Commit

Permalink
Fix multisig LegacyAminoPubKey Amino marshaling (#8841)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>
(cherry picked from commit d4d27e1)

# Conflicts:
#	CHANGELOG.md
#	x/auth/legacy/v040/migrate.go
  • Loading branch information
amaury1093 authored and mergify-bot committed Mar 15, 2021
1 parent e23c0ce commit 0253544
Show file tree
Hide file tree
Showing 7 changed files with 226 additions and 3 deletions.
52 changes: 52 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
88 changes: 88 additions & 0 deletions crypto/keys/multisig/amino.go
Original file line number Diff line number Diff line change
@@ -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
}
3 changes: 3 additions & 0 deletions crypto/keys/multisig/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
78 changes: 76 additions & 2 deletions crypto/keys/multisig/multisig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
2 changes: 1 addition & 1 deletion x/auth/legacy/legacytx/stdtx.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
4 changes: 4 additions & 0 deletions x/auth/legacy/v040/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions x/genutil/legacy/v038/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand Down

0 comments on commit 0253544

Please sign in to comment.