BaseApp Security Improvements

PENDING.md

@@ -92,6 +92,9 @@ CLI flag.
 * [\#3300] Update the spec-spec, spec file reorg, and TOC updates.
 * [\#3694] Push tagged docker images on docker hub when tag is created.
 * [\#3716] Update file permissions the client keys directory and contents to `0700`.
+* [\#3791] Ensure proper handling of invalid block maximum gas in the `BaseApp`.
+* [\#3790] Ensure a valid height is provided by `abci.RequestBeginBlock` in
+`BeginBlock`.
 * [\#3681](https://github.com/cosmos/cosmos-sdk/issues/3681) Migrate ledger-cosmos-go from ZondaX to Cosmos organization 
 
 ### Tendermint

baseapp/baseapp.go

@@ -288,10 +288,13 @@ func (app *BaseApp) storeConsensusParams(consensusParams *abci.ConsensusParams)
 }
 
 // getMaximumBlockGas gets the maximum gas from the consensus params.
-func (app *BaseApp) getMaximumBlockGas() (maxGas uint64) {
- if app.consensusParams == nil || app.consensusParams.BlockSize == nil {
+func (app *BaseApp) getMaximumBlockGas() uint64 {
+ if app.consensusParams == nil ||
+ app.consensusParams.BlockSize == nil ||
+ app.consensusParams.BlockSize.MaxGas <= 0 {
  return 0
  }
+
  return uint64(app.consensusParams.BlockSize.MaxGas)
 }
 
@@ -510,6 +513,19 @@ func handleQueryCustom(app *BaseApp, path []string, req abci.RequestQuery) (res
  }
 }
 
+func (app *BaseApp) validateHeight(req abci.RequestBeginBlock) error {
+ if req.Header.Height < 0 {
+ return fmt.Errorf("invalid height: %d", req.Header.Height)
+ }
+
+ prevHeight := app.LastBlockHeight()
+ if prevHeight != 0 && req.Header.Height != prevHeight+1 {
+ return fmt.Errorf("invalid height: %d; expected: %d", req.Header.Height, prevHeight+1)
+ }
+
+ return nil
+}
+
 // BeginBlock implements the ABCI application interface.
 func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeginBlock) {
  if app.cms.TracingEnabled() {
@@ -518,6 +534,10 @@ func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeg
  ))
  }
 
+ if err := app.validateHeight(req); err != nil {
+ panic(err)
+ }
+
  // Initialize the DeliverTx state. If this is the first block, it should
  // already be initialized in InitChain. Otherwise app.deliverState will be
  // nil, since it is reset on Commit.

baseapp/baseapp_test.go

@@ -296,6 +296,7 @@ func TestInitChainer(t *testing.T) {
  // stores are mounted and private members are set - sealing baseapp
  err := app.LoadLatestVersion(capKey) // needed to make stores non-nil
  require.Nil(t, err)
+ require.Equal(t, int64(0), app.LastBlockHeight())
 
  app.InitChain(abci.RequestInitChain{AppStateBytes: []byte("{}"), ChainId: "test-chain-id"}) // must have valid JSON genesis file, even if empty
 
@@ -308,6 +309,7 @@ func TestInitChainer(t *testing.T) {
 
  app.Commit()
  res = app.Query(query)
+ require.Equal(t, int64(1), app.LastBlockHeight())
  require.Equal(t, value, res.Value)
 
  // reload app
@@ -316,14 +318,17 @@ func TestInitChainer(t *testing.T) {
  app.MountStores(capKey, capKey2)
  err = app.LoadLatestVersion(capKey) // needed to make stores non-nil
  require.Nil(t, err)
+ require.Equal(t, int64(1), app.LastBlockHeight())
 
  // ensure we can still query after reloading
  res = app.Query(query)
  require.Equal(t, value, res.Value)
 
  // commit and ensure we can still query
- app.BeginBlock(abci.RequestBeginBlock{})
+ header := abci.Header{Height: app.LastBlockHeight() + 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
  app.Commit()
+
  res = app.Query(query)
  require.Equal(t, value, res.Value)
 }
@@ -514,7 +519,6 @@ func TestCheckTx(t *testing.T) {
  app := setupBaseApp(t, anteOpt, routerOpt)
 
  nTxs := int64(5)
-
  app.InitChain(abci.RequestInitChain{})
 
  // Create same codec used in txDecoder
@@ -567,16 +571,22 @@ func TestDeliverTx(t *testing.T) {
 
  nBlocks := 3
  txPerHeight := 5
+
  for blockN := 0; blockN < nBlocks; blockN++ {
- app.BeginBlock(abci.RequestBeginBlock{})
+ header := abci.Header{Height: int64(blockN) + 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  for i := 0; i < txPerHeight; i++ {
  counter := int64(blockN*txPerHeight + i)
  tx := newTxCounter(counter, counter)
+
  txBytes, err := codec.MarshalBinaryLengthPrefixed(tx)
  require.NoError(t, err)
+
  res := app.DeliverTx(txBytes)
  require.True(t, res.IsOK(), fmt.Sprintf("%v", res))
  }
+
  app.EndBlock(abci.RequestEndBlock{})
  app.Commit()
  }
@@ -692,7 +702,8 @@ func TestSimulateTx(t *testing.T) {
  nBlocks := 3
  for blockN := 0; blockN < nBlocks; blockN++ {
  count := int64(blockN + 1)
- app.BeginBlock(abci.RequestBeginBlock{})
+ header := abci.Header{Height: count}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
 
  tx := newTxCounter(count, count)
  txBytes, err := cdc.MarshalBinaryLengthPrefixed(tx)
@@ -1216,3 +1227,16 @@ func TestP2PQuery(t *testing.T) {
  res = app.Query(idQuery)
  require.Equal(t, uint32(4), res.Code)
 }
+
+func TestGetMaximumBlockGas(t *testing.T) {
+ app := setupBaseApp(t)
+
+ app.setConsensusParams(&abci.ConsensusParams{BlockSize: &abci.BlockSizeParams{MaxGas: 0}})
+ require.Equal(t, uint64(0), app.getMaximumBlockGas())
+
+ app.setConsensusParams(&abci.ConsensusParams{BlockSize: &abci.BlockSizeParams{MaxGas: 5000000}})
+ require.Equal(t, uint64(5000000), app.getMaximumBlockGas())
+
+ app.setConsensusParams(&abci.ConsensusParams{BlockSize: &abci.BlockSizeParams{MaxGas: -5000000}})
+ require.Equal(t, uint64(0), app.getMaximumBlockGas())
+}

x/bank/app_test.go

@@ -123,7 +123,8 @@ func TestSendNotEnoughBalance(t *testing.T) {
  origSeq := res1.GetSequence()
 
  sendMsg := NewMsgSend(addr1, addr2, sdk.Coins{sdk.NewInt64Coin("foocoin", 100)})
- mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, []sdk.Msg{sendMsg}, []uint64{origAccNum}, []uint64{origSeq}, false, false, priv1)
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, header, []sdk.Msg{sendMsg}, []uint64{origAccNum}, []uint64{origSeq}, false, false, priv1)
 
  mock.CheckBalance(t, mapp, addr1, sdk.Coins{sdk.NewInt64Coin("foocoin", 67)})
 
@@ -173,7 +174,8 @@ func TestMsgMultiSendWithAccounts(t *testing.T) {
  }
 
  for _, tc := range testCases {
- mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, header, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
 
  for _, eb := range tc.expectedBalances {
  mock.CheckBalance(t, mapp, eb.addr, eb.coins)
@@ -212,7 +214,8 @@ func TestMsgMultiSendMultipleOut(t *testing.T) {
  }
 
  for _, tc := range testCases {
- mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, header, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
 
  for _, eb := range tc.expectedBalances {
  mock.CheckBalance(t, mapp, eb.addr, eb.coins)
@@ -241,7 +244,7 @@ func TestMsgMultiSendMultipleInOut(t *testing.T) {
  testCases := []appTestCase{
  {
  msgs: []sdk.Msg{multiSendMsg3},
- accNums: []uint64{0, 0},
+ accNums: []uint64{0, 2},
  accSeqs: []uint64{0, 0},
  expSimPass: true,
  expPass: true,
@@ -256,7 +259,8 @@ func TestMsgMultiSendMultipleInOut(t *testing.T) {
  }
 
  for _, tc := range testCases {
- mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, header, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
 
  for _, eb := range tc.expectedBalances {
  mock.CheckBalance(t, mapp, eb.addr, eb.coins)
@@ -289,7 +293,7 @@ func TestMsgMultiSendDependent(t *testing.T) {
  },
  {
  msgs: []sdk.Msg{multiSendMsg4},
- accNums: []uint64{0},
+ accNums: []uint64{1},
  accSeqs: []uint64{0},
  expSimPass: true,
  expPass: true,
@@ -301,7 +305,8 @@ func TestMsgMultiSendDependent(t *testing.T) {
  }
 
  for _, tc := range testCases {
- mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mock.SignCheckDeliver(t, mapp.Cdc, mapp.BaseApp, header, tc.msgs, tc.accNums, tc.accSeqs, tc.expSimPass, tc.expPass, tc.privKeys...)
 
  for _, eb := range tc.expectedBalances {
  mock.CheckBalance(t, mapp, eb.addr, eb.coins)

x/gov/endblocker_test.go

@@ -13,7 +13,10 @@ import (
 
 func TestTickExpiredDepositPeriod(t *testing.T) {
  mapp, keeper, _, addrs, _, _ := getMockApp(t, 10, GenesisState{}, nil)
- mapp.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx := mapp.BaseApp.NewContext(false, abci.Header{})
  keeper.ck.SetSendEnabled(ctx, true)
  govHandler := NewHandler(keeper)
@@ -56,7 +59,10 @@ func TestTickExpiredDepositPeriod(t *testing.T) {
 
 func TestTickMultipleExpiredDepositPeriod(t *testing.T) {
  mapp, keeper, _, addrs, _, _ := getMockApp(t, 10, GenesisState{}, nil)
- mapp.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx := mapp.BaseApp.NewContext(false, abci.Header{})
  keeper.ck.SetSendEnabled(ctx, true)
  govHandler := NewHandler(keeper)
@@ -113,7 +119,10 @@ func TestTickMultipleExpiredDepositPeriod(t *testing.T) {
 
 func TestTickPassedDepositPeriod(t *testing.T) {
  mapp, keeper, _, addrs, _, _ := getMockApp(t, 10, GenesisState{}, nil)
- mapp.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx := mapp.BaseApp.NewContext(false, abci.Header{})
  keeper.ck.SetSendEnabled(ctx, true)
  govHandler := NewHandler(keeper)
@@ -156,7 +165,10 @@ func TestTickPassedDepositPeriod(t *testing.T) {
 func TestTickPassedVotingPeriod(t *testing.T) {
  mapp, keeper, _, addrs, _, _ := getMockApp(t, 10, GenesisState{}, nil)
  SortAddresses(addrs)
- mapp.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx := mapp.BaseApp.NewContext(false, abci.Header{})
  keeper.ck.SetSendEnabled(ctx, true)
  govHandler := NewHandler(keeper)

x/gov/genesis_test.go

@@ -27,7 +27,10 @@ func TestEqualProposals(t *testing.T) {
  // Generate mock app and keepers
  mapp, keeper, _, addrs, _, _ := getMockApp(t, 2, GenesisState{}, nil)
  SortAddresses(addrs)
- mapp.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx := mapp.BaseApp.NewContext(false, abci.Header{})
 
  // Create two proposals
@@ -56,11 +59,13 @@ func TestEqualProposals(t *testing.T) {
 }
 
 func TestImportExportQueues(t *testing.T) {
-
  // Generate mock app and keepers
  mapp, keeper, _, addrs, _, _ := getMockApp(t, 2, GenesisState{}, nil)
  SortAddresses(addrs)
- mapp.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx := mapp.BaseApp.NewContext(false, abci.Header{})
 
  // Create two proposals, put the second into the voting period
@@ -82,7 +87,9 @@ func TestImportExportQueues(t *testing.T) {
  genState := ExportGenesis(ctx, keeper)
  mapp2, keeper2, _, _, _, _ := getMockApp(t, 2, genState, genAccs)
 
- mapp2.BeginBlock(abci.RequestBeginBlock{})
+ header = abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp2.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx2 := mapp2.BaseApp.NewContext(false, abci.Header{})
 
  // Jump the time forward past the DepositPeriod and VotingPeriod

x/gov/keeper_test.go

@@ -13,7 +13,10 @@ import (
 
 func TestGetSetProposal(t *testing.T) {
  mapp, keeper, _, _, _, _ := getMockApp(t, 0, GenesisState{}, nil)
- mapp.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx := mapp.BaseApp.NewContext(false, abci.Header{})
 
  proposal := keeper.NewTextProposal(ctx, "Test", "description", ProposalTypeText)
@@ -26,7 +29,10 @@ func TestGetSetProposal(t *testing.T) {
 
 func TestIncrementProposalNumber(t *testing.T) {
  mapp, keeper, _, _, _, _ := getMockApp(t, 0, GenesisState{}, nil)
- mapp.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx := mapp.BaseApp.NewContext(false, abci.Header{})
 
  keeper.NewTextProposal(ctx, "Test", "description", ProposalTypeText)
@@ -41,9 +47,11 @@ func TestIncrementProposalNumber(t *testing.T) {
 
 func TestActivateVotingPeriod(t *testing.T) {
  mapp, keeper, _, _, _, _ := getMockApp(t, 0, GenesisState{}, nil)
- mapp.BeginBlock(abci.RequestBeginBlock{})
- ctx := mapp.BaseApp.NewContext(false, abci.Header{})
 
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
+ ctx := mapp.BaseApp.NewContext(false, abci.Header{})
  proposal := keeper.NewTextProposal(ctx, "Test", "description", ProposalTypeText)
 
  require.True(t, proposal.GetVotingStartTime().Equal(time.Time{}))
@@ -63,9 +71,11 @@ func TestActivateVotingPeriod(t *testing.T) {
 func TestDeposits(t *testing.T) {
  mapp, keeper, _, addrs, _, _ := getMockApp(t, 2, GenesisState{}, nil)
  SortAddresses(addrs)
- mapp.BeginBlock(abci.RequestBeginBlock{})
- ctx := mapp.BaseApp.NewContext(false, abci.Header{})
 
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
+ ctx := mapp.BaseApp.NewContext(false, abci.Header{})
  proposal := keeper.NewTextProposal(ctx, "Test", "description", ProposalTypeText)
  proposalID := proposal.GetProposalID()
 
@@ -149,9 +159,11 @@ func TestDeposits(t *testing.T) {
 func TestVotes(t *testing.T) {
  mapp, keeper, _, addrs, _, _ := getMockApp(t, 2, GenesisState{}, nil)
  SortAddresses(addrs)
- mapp.BeginBlock(abci.RequestBeginBlock{})
- ctx := mapp.BaseApp.NewContext(false, abci.Header{})
 
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
+ ctx := mapp.BaseApp.NewContext(false, abci.Header{})
  proposal := keeper.NewTextProposal(ctx, "Test", "description", ProposalTypeText)
  proposalID := proposal.GetProposalID()
 
@@ -204,7 +216,10 @@ func TestVotes(t *testing.T) {
 
 func TestProposalQueues(t *testing.T) {
  mapp, keeper, _, _, _, _ := getMockApp(t, 0, GenesisState{}, nil)
- mapp.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: mapp.LastBlockHeight() + 1}
+ mapp.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  ctx := mapp.BaseApp.NewContext(false, abci.Header{})
  mapp.InitChainer(ctx, abci.RequestInitChain{})