From 56221158654f338b2b3490c07896eb1502d307bc Mon Sep 17 00:00:00 2001 From: MD Aleem <72057206+aleem1314@users.noreply.github.com> Date: Tue, 25 Jan 2022 21:53:38 +0530 Subject: [PATCH 1/2] feat!: add protection against accidental downgrades (#10407) ## Description Closes: #10318 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- CHANGELOG.md | 1 + x/upgrade/abci.go | 17 +++++++++ x/upgrade/abci_test.go | 67 +++++++++++++++++++++++++++++++++ x/upgrade/keeper/keeper.go | 29 ++++++++++++++ x/upgrade/keeper/keeper_test.go | 39 +++++++++++++++++++ 5 files changed, 153 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e3fc88c0c9a..5d63200ae976 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#10311](https://github.com/cosmos/cosmos-sdk/pull/10311) Adds cli to use tips transactions. It adds an `--aux` flag to all CLI tx commands to generate the aux signer data (with optional tip), and a new `tx aux-to-fee` subcommand to let the fee payer gather aux signer data and broadcast the tx * [\#10430](https://github.com/cosmos/cosmos-sdk/pull/10430) ADR-040: Add store/v2 `MultiStore` implementation * [\#10947](https://github.com/cosmos/cosmos-sdk/pull/10947) Add `AllowancesByGranter` query to the feegrant module +* [\#10407](https://github.com/cosmos/cosmos-sdk/pull/10407) Add validation to `x/upgrade` module's `BeginBlock` to check accidental binary downgrades ### API Breaking Changes diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index e543bcd41c66..f390a873d134 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -22,7 +22,24 @@ import ( // skipUpgradeHeightArray is a set of block heights for which the upgrade must be skipped func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) + plan, found := k.GetUpgradePlan(ctx) + + if !k.DowngradeVerified() { + k.SetDowngradeVerified(true) + lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx) + // This check will make sure that we are using a valid binary. + // It'll panic in these cases if there is no upgrade handler registered for the last applied upgrade. + // 1. If there is no scheduled upgrade. + // 2. If the plan is not ready. + // 3. If the plan is ready and skip upgrade height is set for current height. + if !found || !plan.ShouldExecute(ctx) || (plan.ShouldExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) { + if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { + panic(fmt.Sprintf("Wrong app version %d, upgrade handler is missing for %s upgrade plan", ctx.ConsensusParams().Version.AppVersion, lastAppliedPlan)) + } + } + } + if !found { return } diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index 915f5c36cafe..c3f1ea6bd1a8 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -410,3 +410,70 @@ func TestDumpUpgradeInfoToFile(t *testing.T) { err = os.Remove(upgradeInfoFilePath) require.Nil(err) } + +// TODO: add testcase to for `no upgrade handler is present for last applied upgrade`. +func TestBinaryVersion(t *testing.T) { + var skipHeight int64 = 15 + s := setupTest(t, 10, map[int64]bool{skipHeight: true}) + + testCases := []struct { + name string + preRun func() (sdk.Context, abci.RequestBeginBlock) + expectPanic bool + }{ + { + "test not panic: no scheduled upgrade or applied upgrade is present", + func() (sdk.Context, abci.RequestBeginBlock) { + req := abci.RequestBeginBlock{Header: s.ctx.BlockHeader()} + return s.ctx, req + }, + false, + }, + { + "test not panic: upgrade handler is present for last applied upgrade", + func() (sdk.Context, abci.RequestBeginBlock) { + s.keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { + return vm, nil + }) + + err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test0", Height: s.ctx.BlockHeight() + 2}}) + require.Nil(t, err) + + newCtx := s.ctx.WithBlockHeight(12) + s.keeper.ApplyUpgrade(newCtx, types.Plan{ + Name: "test0", + Height: 12, + }) + + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + false, + }, + { + "test panic: upgrade needed", + func() (sdk.Context, abci.RequestBeginBlock) { + err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test2", Height: 13}}) + require.Nil(t, err) + + newCtx := s.ctx.WithBlockHeight(13) + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + true, + }, + } + + for _, tc := range testCases { + ctx, req := tc.preRun() + if tc.expectPanic { + require.Panics(t, func() { + s.module.BeginBlock(ctx, req) + }) + } else { + require.NotPanics(t, func() { + s.module.BeginBlock(ctx, req) + }) + } + } +} diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index d342ad9a45df..a209777c66ab 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -17,6 +17,7 @@ import ( "github.com/cosmos/cosmos-sdk/store/prefix" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/types/kv" "github.com/cosmos/cosmos-sdk/types/module" xp "github.com/cosmos/cosmos-sdk/x/upgrade/exported" "github.com/cosmos/cosmos-sdk/x/upgrade/types" @@ -33,6 +34,7 @@ type Keeper struct { cdc codec.BinaryCodec // App-wide binary codec upgradeHandlers map[string]types.UpgradeHandler // map of plan name to upgrade handler versionSetter xp.ProtocolVersionSetter // implements setting the protocol version field on BaseApp + downgradeVerified bool // tells if we've already sanity checked that this binary version isn't being used against an old state. } // NewKeeper constructs an upgrade Keeper which requires the following arguments: @@ -228,6 +230,23 @@ func (k Keeper) GetUpgradedConsensusState(ctx sdk.Context, lastHeight int64) ([] return bz, true } +// GetLastCompletedUpgrade returns the last applied upgrade name and height. +func (k Keeper) GetLastCompletedUpgrade(ctx sdk.Context) (string, int64) { + iter := sdk.KVStoreReversePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) + defer iter.Close() + if iter.Valid() { + return parseDoneKey(iter.Key()), int64(binary.BigEndian.Uint64(iter.Value())) + } + + return "", 0 +} + +// parseDoneKey - split upgrade name from the done key +func parseDoneKey(key []byte) string { + kv.AssertKeyAtLeastLength(key, 2) + return string(key[1:]) +} + // GetDoneHeight returns the height at which the given upgrade was executed func (k Keeper) GetDoneHeight(ctx sdk.Context, name string) int64 { store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) @@ -389,3 +408,13 @@ func (k Keeper) ReadUpgradeInfoFromDisk() (types.Plan, error) { return upgradeInfo, nil } + +// SetDowngradeVerified updates downgradeVerified. +func (k *Keeper) SetDowngradeVerified(v bool) { + k.downgradeVerified = v +} + +// DowngradeVerified returns downgradeVerified. +func (k Keeper) DowngradeVerified() bool { + return k.downgradeVerified +} diff --git a/x/upgrade/keeper/keeper_test.go b/x/upgrade/keeper/keeper_test.go index c8df562f5de4..b572a89ac1b4 100644 --- a/x/upgrade/keeper/keeper_test.go +++ b/x/upgrade/keeper/keeper_test.go @@ -232,6 +232,45 @@ func (s *KeeperTestSuite) TestMigrations() { s.Require().Equal(vmBefore["bank"]+1, vm["bank"]) } +func (s *KeeperTestSuite) TestLastCompletedUpgrade() { + keeper := s.app.UpgradeKeeper + require := s.Require() + + s.T().Log("verify empty name if applied upgrades are empty") + name, height := keeper.GetLastCompletedUpgrade(s.ctx) + require.Equal("", name) + require.Equal(int64(0), height) + + keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { + return vm, nil + }) + + keeper.ApplyUpgrade(s.ctx, types.Plan{ + Name: "test0", + Height: 10, + }) + + s.T().Log("verify valid upgrade name and height") + name, height = keeper.GetLastCompletedUpgrade(s.ctx) + require.Equal("test0", name) + require.Equal(int64(10), height) + + keeper.SetUpgradeHandler("test1", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { + return vm, nil + }) + + newCtx := s.ctx.WithBlockHeight(15) + keeper.ApplyUpgrade(newCtx, types.Plan{ + Name: "test1", + Height: 15, + }) + + s.T().Log("verify valid upgrade name and height with multiple upgrades") + name, height = keeper.GetLastCompletedUpgrade(newCtx) + require.Equal("test1", name) + require.Equal(int64(15), height) +} + func TestKeeperTestSuite(t *testing.T) { suite.Run(t, new(KeeperTestSuite)) } From d9033e01c69b0be4b5a391c48f3142569996b621 Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Tue, 25 Jan 2022 15:06:21 -0800 Subject: [PATCH 2/2] chore: crypto/xsalsa20symmetric: remove dependency on tendermint core (#11027) The xsalsa20symmetric package is otherwise unused in Tendermint Core. Move the dependency to a subpackage of crypto and update the imports. No functional changes. --- CHANGELOG.md | 3 +- crypto/armor.go | 2 +- crypto/xsalsa20symmetric/symmetric.go | 63 ++++++++++++++++++++++ crypto/xsalsa20symmetric/symmetric_test.go | 45 ++++++++++++++++ 4 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 crypto/xsalsa20symmetric/symmetric.go create mode 100644 crypto/xsalsa20symmetric/symmetric_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d63200ae976..1f91faa1788d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -176,6 +176,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#10842](https://github.com/cosmos/cosmos-sdk/pull/10842) Fix error when `--generate-only`, `--max-msgs` fags set while executing `WithdrawAllRewards` command. * [\#10897](https://github.com/cosmos/cosmos-sdk/pull/10897) Fix: set a non-zero value on gas overflow. * [#9790](https://github.com/cosmos/cosmos-sdk/pull/10687) Fix behavior of `DecCoins.MulDecTruncate`. +* (crypto) [#11027] Remove dependency on Tendermint core for xsalsa20symmetric. ### State Machine Breaking @@ -191,7 +192,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [#10770](https://github.com/cosmos/cosmos-sdk/pull/10770) revert tx when block gas limit exceeded * [\#10868](https://github.com/cosmos/cosmos-sdk/pull/10868) Bump gov to v1beta2. Both v1beta1 and v1beta2 queries and Msgs are accepted. - ### Deprecated +### Deprecated * (x/upgrade) [\#9906](https://github.com/cosmos/cosmos-sdk/pull/9906) Deprecate `UpgradeConsensusState` gRPC query since this functionality is only used for IBC, which now has its own [IBC replacement](https://github.com/cosmos/ibc-go/blob/2c880a22e9f9cc75f62b527ca94aa75ce1106001/proto/ibc/core/client/v1/query.proto#L54) diff --git a/crypto/armor.go b/crypto/armor.go index 6e1c739a40c0..2a45e67ce586 100644 --- a/crypto/armor.go +++ b/crypto/armor.go @@ -8,11 +8,11 @@ import ( "github.com/tendermint/crypto/bcrypt" "github.com/tendermint/tendermint/crypto" - "github.com/tendermint/tendermint/crypto/xsalsa20symmetric" "golang.org/x/crypto/openpgp/armor" // nolint: staticcheck "github.com/cosmos/cosmos-sdk/codec/legacy" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + "github.com/cosmos/cosmos-sdk/crypto/xsalsa20symmetric" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) diff --git a/crypto/xsalsa20symmetric/symmetric.go b/crypto/xsalsa20symmetric/symmetric.go new file mode 100644 index 000000000000..cf24daea35ce --- /dev/null +++ b/crypto/xsalsa20symmetric/symmetric.go @@ -0,0 +1,63 @@ +package xsalsa20symmetric + +import ( + "crypto/rand" + "errors" + "fmt" + + "golang.org/x/crypto/nacl/secretbox" +) + +// TODO, make this into a struct that implements crypto.Symmetric. + +const nonceLen = 24 +const secretLen = 32 + +// secret must be 32 bytes long. Use something like Sha256(Bcrypt(passphrase)) +// The ciphertext is (secretbox.Overhead + 24) bytes longer than the plaintext. +func EncryptSymmetric(plaintext []byte, secret []byte) (ciphertext []byte) { + if len(secret) != secretLen { + panic(fmt.Sprintf("Secret must be 32 bytes long, got len %v", len(secret))) + } + nonce := randBytes(nonceLen) + nonceArr := [nonceLen]byte{} + copy(nonceArr[:], nonce) + secretArr := [secretLen]byte{} + copy(secretArr[:], secret) + ciphertext = make([]byte, nonceLen+secretbox.Overhead+len(plaintext)) + copy(ciphertext, nonce) + secretbox.Seal(ciphertext[nonceLen:nonceLen], plaintext, &nonceArr, &secretArr) + return ciphertext +} + +// secret must be 32 bytes long. Use something like Sha256(Bcrypt(passphrase)) +// The ciphertext is (secretbox.Overhead + 24) bytes longer than the plaintext. +func DecryptSymmetric(ciphertext []byte, secret []byte) (plaintext []byte, err error) { + if len(secret) != secretLen { + panic(fmt.Sprintf("Secret must be 32 bytes long, got len %v", len(secret))) + } + if len(ciphertext) <= secretbox.Overhead+nonceLen { + return nil, errors.New("ciphertext is too short") + } + nonce := ciphertext[:nonceLen] + nonceArr := [nonceLen]byte{} + copy(nonceArr[:], nonce) + secretArr := [secretLen]byte{} + copy(secretArr[:], secret) + plaintext = make([]byte, len(ciphertext)-nonceLen-secretbox.Overhead) + _, ok := secretbox.Open(plaintext[:0], ciphertext[nonceLen:], &nonceArr, &secretArr) + if !ok { + return nil, errors.New("ciphertext decryption failed") + } + return plaintext, nil +} + +// This only uses the OS's randomness +func randBytes(numBytes int) []byte { + b := make([]byte, numBytes) + _, err := rand.Read(b) + if err != nil { + panic(err) + } + return b +} diff --git a/crypto/xsalsa20symmetric/symmetric_test.go b/crypto/xsalsa20symmetric/symmetric_test.go new file mode 100644 index 000000000000..b35633b96cb3 --- /dev/null +++ b/crypto/xsalsa20symmetric/symmetric_test.go @@ -0,0 +1,45 @@ +package xsalsa20symmetric + +import ( + "crypto/sha256" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "golang.org/x/crypto/bcrypt" +) + +func TestSimple(t *testing.T) { + + plaintext := []byte("sometext") + secret := []byte("somesecretoflengththirtytwo===32") + ciphertext := EncryptSymmetric(plaintext, secret) + plaintext2, err := DecryptSymmetric(ciphertext, secret) + + require.NoError(t, err, "%+v", err) + assert.Equal(t, plaintext, plaintext2) +} + +func TestSimpleWithKDF(t *testing.T) { + + plaintext := []byte("sometext") + secretPass := []byte("somesecret") + secret, err := bcrypt.GenerateFromPassword(secretPass, 12) + if err != nil { + t.Error(err) + } + secret = sha256Sum(secret) + + ciphertext := EncryptSymmetric(plaintext, secret) + plaintext2, err := DecryptSymmetric(ciphertext, secret) + + require.NoError(t, err, "%+v", err) + assert.Equal(t, plaintext, plaintext2) +} + +func sha256Sum(bytes []byte) []byte { + hasher := sha256.New() + hasher.Write(bytes) + return hasher.Sum(nil) +}