Fix multisig LegacyAminoPubKey Amino marshaling (bp #8841)

CHANGELOG.md

@@ -38,6 +38,35 @@ 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.
+
+
+>>>>>>> d4d27e1c0... Fix multisig LegacyAminoPubKey Amino marshaling (#8841)
 
 ## [v0.42.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.42.0) - 2021-03-08
 

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
+}

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() {

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)
+}

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
 		}

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)

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)