Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BaseApp Security Improvements #3801

Merged
merged 12 commits into from
Mar 8, 2019
Merged

BaseApp Security Improvements #3801

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
063e739
Ensure getMaximumBlockGas panics on invalid max block gas
alexanderbez Mar 4, 2019
e9c8fb5
add pending log
alexanderbez Mar 4, 2019
993960d
Update getMaximumBlockGas to return zero instead of panic
alexanderbez Mar 4, 2019
40bccfe
Implement and assert validateHeight
alexanderbez Mar 4, 2019
2bd687c
Add a pending log entry
alexanderbez Mar 4, 2019
3ad3efd
Update mock usage to use provided block header
alexanderbez Mar 5, 2019
bd6634a
Cleanup getMaximumBlockGas
alexanderbez Mar 5, 2019
6478283
Merge branch 'develop' into bez/baseapp-safety-improvements
alexanderbez Mar 6, 2019
2790d35
Remove prevHeight check
alexanderbez Mar 6, 2019
28039d7
Update getMaximumBlockGas
alexanderbez Mar 6, 2019
ab8aa15
Update validateHeight to return an error when height < 1
alexanderbez Mar 6, 2019
f920bf5
Merge branch 'develop' into bez/baseapp-safety-improvements
jackzampolin Mar 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions PENDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
36 changes: 33 additions & 3 deletions baseapp/baseapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
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)
}
}

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -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 < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Tendermint starts at height 1 now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but can the same be said for other consensus engines?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, but I think we should panic otherwise, because elsewhere in the SDK we assume that the first block height is 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point -- updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a comment be made somewhere or we amend ABCI to stipulate that the first block is at height 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValarDragon yes, I'll follow up on this.

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() {
Expand All @@ -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.
Expand Down
127 changes: 82 additions & 45 deletions baseapp/baseapp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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)
}
Expand Down Expand Up @@ -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
Expand All @@ -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()

Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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++ {
Expand Down Expand Up @@ -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
//
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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() })
}
19 changes: 12 additions & 7 deletions x/bank/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)})

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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...)
rigelrozanski marked this conversation as resolved.
Show resolved Hide resolved

for _, eb := range tc.expectedBalances {
mock.CheckBalance(t, mapp, eb.addr, eb.coins)
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down
Loading