Merge PR #3801: BaseApp Security Improvements

PENDING.md

@@ -44,6 +44,7 @@
 * #3808 `gaiad` and `gaiacli` integration tests use ./build/ binaries.
 
 ### SDK
+* #3801 `baseapp` saftey improvements
 
 ### Tendermint
 

baseapp/baseapp.go

@@ -287,12 +287,25 @@ func (app *BaseApp) storeConsensusParams(consensusParams *abci.ConsensusParams)
  mainStore.Set(mainConsensusParamsKey, consensusParamsBz)
 }
 
-// getMaximumBlockGas gets the maximum gas from the consensus params.
-func (app *BaseApp) getMaximumBlockGas() (maxGas uint64) {
+// getMaximumBlockGas gets the maximum gas from the consensus params. It panics
+// if maximum block gas is less than negative one and returns zero if negative
+// one.
+func (app *BaseApp) getMaximumBlockGas() uint64 {
  if app.consensusParams == nil || app.consensusParams.BlockSize == nil {
  return 0
  }
- return uint64(app.consensusParams.BlockSize.MaxGas)
+
+ maxGas := app.consensusParams.BlockSize.MaxGas
+ switch {
+ case maxGas < -1:
+ panic(fmt.Sprintf("invalid maximum block gas: %d", maxGas))
+
+ case maxGas == -1:
+ return 0
+
+ default:
+ return uint64(maxGas)
+ }
 }
 
 // ----------------------------------------------------------------------------
@@ -510,6 +523,19 @@ func handleQueryCustom(app *BaseApp, path []string, req abci.RequestQuery) (res
  }
 }
 
+func (app *BaseApp) validateHeight(req abci.RequestBeginBlock) error {
+ if req.Header.Height < 1 {
+ return fmt.Errorf("invalid height: %d", req.Header.Height)
+ }
+
+ prevHeight := app.LastBlockHeight()
+ if 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 +544,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
@@ -536,7 +540,8 @@ func TestCheckTx(t *testing.T) {
  require.Equal(t, nTxs, storedCounter)
 
  // If a block is committed, CheckTx state should be reset.
- app.BeginBlock(abci.RequestBeginBlock{})
+ header := abci.Header{Height: 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
  app.EndBlock(abci.RequestEndBlock{})
  app.Commit()
 
@@ -567,16 +572,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()
  }
@@ -610,48 +621,47 @@ func TestMultiMsgDeliverTx(t *testing.T) {
 
  // run a multi-msg tx
  // with all msgs the same route
- {
- app.BeginBlock(abci.RequestBeginBlock{})
- tx := newTxCounter(0, 0, 1, 2)
- txBytes, err := codec.MarshalBinaryLengthPrefixed(tx)
- require.NoError(t, err)
- res := app.DeliverTx(txBytes)
- require.True(t, res.IsOK(), fmt.Sprintf("%v", res))
 
- store := app.deliverState.ctx.KVStore(capKey1)
+ header := abci.Header{Height: 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
+ tx := newTxCounter(0, 0, 1, 2)
+ txBytes, err := codec.MarshalBinaryLengthPrefixed(tx)
+ require.NoError(t, err)
+ res := app.DeliverTx(txBytes)
+ require.True(t, res.IsOK(), fmt.Sprintf("%v", res))
+
+ store := app.deliverState.ctx.KVStore(capKey1)
 
-  // tx counter only incremented once
-  txCounter := getIntFromStore(store, anteKey)
-  require.Equal(t, int64(1), txCounter)
+ // tx counter only incremented once
+ txCounter := getIntFromStore(store, anteKey)
+ require.Equal(t, int64(1), txCounter)
 
- // msg counter incremented three times
- msgCounter := getIntFromStore(store, deliverKey)
- require.Equal(t, int64(3), msgCounter)
- }
+ // msg counter incremented three times
+ msgCounter := getIntFromStore(store, deliverKey)
+ require.Equal(t, int64(3), msgCounter)
 
  // replace the second message with a msgCounter2
- {
- tx := newTxCounter(1, 3)
- tx.Msgs = append(tx.Msgs, msgCounter2{0})
- tx.Msgs = append(tx.Msgs, msgCounter2{1})
- txBytes, err := codec.MarshalBinaryLengthPrefixed(tx)
- require.NoError(t, err)
- res := app.DeliverTx(txBytes)
- require.True(t, res.IsOK(), fmt.Sprintf("%v", res))
 
- store := app.deliverState.ctx.KVStore(capKey1)
+ tx = newTxCounter(1, 3)
+ tx.Msgs = append(tx.Msgs, msgCounter2{0})
+ tx.Msgs = append(tx.Msgs, msgCounter2{1})
+ txBytes, err = codec.MarshalBinaryLengthPrefixed(tx)
+ require.NoError(t, err)
+ res = app.DeliverTx(txBytes)
+ require.True(t, res.IsOK(), fmt.Sprintf("%v", res))
+
+ store = app.deliverState.ctx.KVStore(capKey1)
 
-  // tx counter only incremented once
-  txCounter := getIntFromStore(store, anteKey)
-  require.Equal(t, int64(2), txCounter)
+ // tx counter only incremented once
+ txCounter = getIntFromStore(store, anteKey)
+ require.Equal(t, int64(2), txCounter)
 
- // original counter increments by one
- // new counter increments by two
- msgCounter := getIntFromStore(store, deliverKey)
- require.Equal(t, int64(4), msgCounter)
- msgCounter2 := getIntFromStore(store, deliverKey2)
- require.Equal(t, int64(2), msgCounter2)
- }
+ // original counter increments by one
+ // new counter increments by two
+ msgCounter = getIntFromStore(store, deliverKey)
+ require.Equal(t, int64(4), msgCounter)
+ msgCounter2 := getIntFromStore(store, deliverKey2)
+ require.Equal(t, int64(2), msgCounter2)
 }
 
 // Interleave calls to Check and Deliver and ensure
@@ -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)
@@ -737,7 +748,9 @@ func TestRunInvalidTransaction(t *testing.T) {
  }
 
  app := setupBaseApp(t, anteOpt, routerOpt)
- app.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
 
  // Transaction with no messages
  {
@@ -847,7 +860,8 @@ func TestTxGasLimits(t *testing.T) {
 
  app := setupBaseApp(t, anteOpt, routerOpt)
 
- app.BeginBlock(abci.RequestBeginBlock{})
+ header := abci.Header{Height: 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
 
  testCases := []struct {
  tx *txTest
@@ -962,7 +976,8 @@ func TestMaxBlockGasLimits(t *testing.T) {
  tx := tc.tx
 
  // reset the block gas
- app.BeginBlock(abci.RequestBeginBlock{})
+ header := abci.Header{Height: app.LastBlockHeight() + 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
 
  // execute the transaction multiple times
  for j := 0; j < tc.numDelivers; j++ {
@@ -1005,7 +1020,9 @@ func TestBaseAppAnteHandler(t *testing.T) {
 
  app.InitChain(abci.RequestInitChain{})
  registerTestCodec(cdc)
- app.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: app.LastBlockHeight() + 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
 
  // execute a tx that will fail ante handler execution
  //
@@ -1112,7 +1129,9 @@ func TestGasConsumptionBadTx(t *testing.T) {
  })
 
  app.InitChain(abci.RequestInitChain{})
- app.BeginBlock(abci.RequestBeginBlock{})
+
+ header := abci.Header{Height: app.LastBlockHeight() + 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
 
  tx := newTxCounter(5, 0)
  tx.setFailOnAnte(true)
@@ -1174,7 +1193,9 @@ func TestQuery(t *testing.T) {
  require.Equal(t, 0, len(res.Value))
 
  // query is still empty after a DeliverTx before we commit
- app.BeginBlock(abci.RequestBeginBlock{})
+ header := abci.Header{Height: app.LastBlockHeight() + 1}
+ app.BeginBlock(abci.RequestBeginBlock{Header: header})
+
  resTx = app.Deliver(tx)
  require.True(t, resTx.IsOK(), fmt.Sprintf("%v", resTx))
  res = app.Query(query)
@@ -1216,3 +1237,19 @@ 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: -1}})
+ 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.Panics(t, func() { 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)