diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ec779236..47809fa65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,11 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (grpc) [\#10985](https://github.com/cosmos/cosmos-sdk/pull/10992) The `/cosmos/tx/v1beta1/txs/{hash}` endpoint returns a 404 when a tx does not exist. +### Improvements + +* [\#10407](https://github.com/cosmos/cosmos-sdk/pull/10407) Add validation to `x/upgrade` module's `BeginBlock` to check accidental binary downgrades + + ## [v0.45.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.45.0) - 2022-01-18 ### State Machine Breaking diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index c2521e09e..a9f6c0826 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 18f175273..731de6af8 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -411,3 +411,70 @@ func TestDumpUpgradeInfoToFile(t *testing.T) { err = os.Remove(upgradeInfoFilePath) require.Nil(t, 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(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 7c13ce6f1..7dee3efd5 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -3,6 +3,7 @@ package keeper import ( "encoding/binary" "encoding/json" + "fmt" "io/ioutil" "os" "path" @@ -32,6 +33,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,26 @@ 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 { + if len(key) < 2 { + panic(fmt.Sprintf("expected key of length at least %d, got %d", 2, len(key))) + } + + 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}) @@ -410,3 +432,13 @@ type upgradeInfo struct { // Height has types.Plan.Height value Info string `json:"info,omitempty"` } + +// 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 724848271..1cb2f072a 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)) }