From 4c2f56626ac3161d871bd4b44ec80ea0ff3b1284 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Sat, 10 Mar 2018 21:15:55 -0800 Subject: [PATCH 1/4] lite/proxy: Validation* tests and hardening for nil dereferences Updates https://github.com/tendermint/tendermint/issues/1017 Ensure that the Validate* functions in proxy are tests and cover the case of sneakish bugs that have been encountered a few times from nil dereferences. The lite package should theoretically never panic with a nil dereference. It is meant to contain the certifiers hence it should never panic with such. Requires the following bugs to be fixed first; * https://github.com/tendermint/tendermint/issues/1298 * https://github.com/tendermint/tendermint/issues/1299 --- lite/proxy/block.go | 9 ++ lite/proxy/validate_test.go | 256 ++++++++++++++++++++++++++++++++++++ 2 files changed, 265 insertions(+) create mode 100644 lite/proxy/validate_test.go diff --git a/lite/proxy/block.go b/lite/proxy/block.go index 60cd00f0d..4cff9ee68 100644 --- a/lite/proxy/block.go +++ b/lite/proxy/block.go @@ -11,11 +11,17 @@ import ( ) func ValidateBlockMeta(meta *types.BlockMeta, check lite.Commit) error { + if meta == nil { + return errors.New("expecting a non-nil BlockMeta") + } // TODO: check the BlockID?? return ValidateHeader(meta.Header, check) } func ValidateBlock(meta *types.Block, check lite.Commit) error { + if meta == nil { + return errors.New("expecting a non-nil Block") + } err := ValidateHeader(meta.Header, check) if err != nil { return err @@ -27,6 +33,9 @@ func ValidateBlock(meta *types.Block, check lite.Commit) error { } func ValidateHeader(head *types.Header, check lite.Commit) error { + if head == nil { + return errors.New("expecting a non-nil Header") + } // make sure they are for the same height (obvious fail) if head.Height != check.Height() { return certerr.ErrHeightMismatch(head.Height, check.Height()) diff --git a/lite/proxy/validate_test.go b/lite/proxy/validate_test.go new file mode 100644 index 000000000..971a5e5b3 --- /dev/null +++ b/lite/proxy/validate_test.go @@ -0,0 +1,256 @@ +package proxy_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "github.com/tendermint/tendermint/lite" + "github.com/tendermint/tendermint/lite/proxy" + "github.com/tendermint/tendermint/types" +) + +var ( + deabBeefTxs = types.Txs{[]byte("DE"), []byte("AD"), []byte("BE"), []byte("EF")} + deadBeefRipEmd160Hash = deabBeefTxs.Hash() +) + +func TestValidateBlock(t *testing.T) { + tests := []struct { + block *types.Block + commit lite.Commit + wantErr string + }{ + { + block: nil, wantErr: "non-nil Block", + }, + { + block: &types.Block{}, wantErr: "nil Header", + }, + { + block: &types.Block{Header: new(types.Header)}, + }, + + // Start Header.Height mismatch test + { + block: &types.Block{Header: &types.Header{Height: 10}}, + commit: lite.Commit{Header: &types.Header{Height: 11}}, + wantErr: "don't match - 10 vs 11", + }, + + { + block: &types.Block{Header: &types.Header{Height: 11}}, + commit: lite.Commit{Header: &types.Header{Height: 11}}, + }, + // End Header.Height mismatch test + + // Start Header.Hash mismatch test + { + block: &types.Block{ + Header: &types.Header{ + Height: 11, + Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + ValidatorsHash: []byte("Tendermint"), + }, + }, + commit: lite.Commit{Header: &types.Header{Height: 11}}, + wantErr: "Headers don't match", + }, + + { + block: &types.Block{ + Header: &types.Header{ + Height: 11, + Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + ValidatorsHash: []byte("Tendermint"), + }, + }, + commit: lite.Commit{ + Header: &types.Header{ + Height: 11, + Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + ValidatorsHash: []byte("Tendermint"), + }, + }, + }, + // End Header.Hash mismatch test + + // Start Header.Data hash mismatch test + { + block: &types.Block{ + Header: &types.Header{Height: 11}, + Data: &types.Data{Txs: []types.Tx{[]byte("0xDE"), []byte("AD")}}, + }, + commit: lite.Commit{ + Header: &types.Header{Height: 11}, + Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("0xDEADBEEF")}}, + }, + wantErr: "Data hash doesn't match header", + }, + { + block: &types.Block{ + Header: &types.Header{Height: 11, DataHash: deadBeefRipEmd160Hash}, + Data: &types.Data{Txs: deabBeefTxs}, + }, + commit: lite.Commit{ + Header: &types.Header{Height: 11}, + Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("DEADBEEF")}}, + }, + }, + // End Header.Data hash mismatch test + } + + assert := assert.New(t) + + for i, tt := range tests { + err := proxy.ValidateBlock(tt.block, tt.commit) + if tt.wantErr != "" { + if err == nil { + assert.FailNowf("Unexpectedly passed", "#%d", i) + } else { + assert.Contains(err.Error(), tt.wantErr, "#%d should contain the substring\n\n", i) + } + continue + } + + assert.Nil(err, "#%d: expecting a nil error", i) + } +} + +func TestValidateBlockMeta(t *testing.T) { + tests := []struct { + meta *types.BlockMeta + commit lite.Commit + wantErr string + }{ + { + meta: nil, wantErr: "non-nil BlockMeta", + }, + { + meta: &types.BlockMeta{}, wantErr: "non-nil Header", + }, + { + meta: &types.BlockMeta{Header: new(types.Header)}, + }, + + // Start Header.Height mismatch test + { + meta: &types.BlockMeta{Header: &types.Header{Height: 10}}, + commit: lite.Commit{Header: &types.Header{Height: 11}}, + wantErr: "don't match - 10 vs 11", + }, + + { + meta: &types.BlockMeta{Header: &types.Header{Height: 11}}, + commit: lite.Commit{Header: &types.Header{Height: 11}}, + }, + // End Header.Height mismatch test + + // Start Headers don't match test + { + meta: &types.BlockMeta{ + Header: &types.Header{ + Height: 11, + Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + ValidatorsHash: []byte("Tendermint"), + }, + }, + commit: lite.Commit{Header: &types.Header{Height: 11}}, + wantErr: "Headers don't match", + }, + + { + meta: &types.BlockMeta{ + Header: &types.Header{ + Height: 11, + Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + ValidatorsHash: []byte("Tendermint"), + }, + }, + commit: lite.Commit{ + Header: &types.Header{ + Height: 11, + Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + ValidatorsHash: []byte("Tendermint"), + }, + }, + }, + + { + meta: &types.BlockMeta{ + Header: &types.Header{ + Height: 11, + // TODO: (@odeke-em) inquire why ValidatorsHash has to be non-blank + // for the Header to be hashed. Perhaps this is a security hole because + // an aggressor could perhaps pass in headers that don't have + // ValidatorsHash set and we won't be able to validate blocks. + ValidatorsHash: []byte("lite-test"), + // TODO: (@odeke-em) file an issue with Tendermint to get them to update + // to the latest go-wire, then no more need for this value fill to avoid + // the time zero value of less than 1970. + Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + }, + }, + commit: lite.Commit{ + Header: &types.Header{Height: 11, DataHash: deadBeefRipEmd160Hash}, + }, + wantErr: "Headers don't match", + }, + + { + meta: &types.BlockMeta{ + Header: &types.Header{ + Height: 11, DataHash: deadBeefRipEmd160Hash, + ValidatorsHash: []byte("Tendermint"), + Time: time.Date(2017, 1, 2, 1, 1, 1, 1, time.UTC), + }, + }, + commit: lite.Commit{ + Header: &types.Header{ + Height: 11, DataHash: deadBeefRipEmd160Hash, + ValidatorsHash: []byte("Tendermint"), + Time: time.Date(2017, 1, 2, 2, 1, 1, 1, time.UTC), + }, + Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("DEADBEEF")}}, + }, + wantErr: "Headers don't match", + }, + + { + meta: &types.BlockMeta{ + Header: &types.Header{ + Height: 11, DataHash: deadBeefRipEmd160Hash, + ValidatorsHash: []byte("Tendermint"), + Time: time.Date(2017, 1, 2, 1, 1, 1, 1, time.UTC), + }, + }, + commit: lite.Commit{ + Header: &types.Header{ + Height: 11, DataHash: deadBeefRipEmd160Hash, + ValidatorsHash: []byte("Tendermint-x"), + Time: time.Date(2017, 1, 2, 1, 1, 1, 1, time.UTC), + }, + Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("DEADBEEF")}}, + }, + wantErr: "Headers don't match", + }, + // End Headers don't match test + } + + assert := assert.New(t) + + for i, tt := range tests { + err := proxy.ValidateBlockMeta(tt.meta, tt.commit) + if tt.wantErr != "" { + if err == nil { + assert.FailNowf("Unexpectedly passed", "#%d: wanted error %q", i, tt.wantErr) + } else { + assert.Contains(err.Error(), tt.wantErr, "#%d should contain the substring\n\n", i) + } + continue + } + + assert.Nil(err, "#%d: expecting a nil error", i) + } +} From 58f36bb3215c54287d6693119d0071b97052b673 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Sat, 10 Mar 2018 21:50:15 -0800 Subject: [PATCH 2/4] Review feedback from @melekes * Fix typo on naming s/deabBeef/deadBeef/g * Use `assert.*(t,` instead of `assert.New(t);...;assert.*(` --- lite/proxy/validate_test.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/lite/proxy/validate_test.go b/lite/proxy/validate_test.go index 971a5e5b3..653bfb753 100644 --- a/lite/proxy/validate_test.go +++ b/lite/proxy/validate_test.go @@ -12,8 +12,8 @@ import ( ) var ( - deabBeefTxs = types.Txs{[]byte("DE"), []byte("AD"), []byte("BE"), []byte("EF")} - deadBeefRipEmd160Hash = deabBeefTxs.Hash() + deadBeefTxs = types.Txs{[]byte("DE"), []byte("AD"), []byte("BE"), []byte("EF")} + deadBeefRipEmd160Hash = deadBeefTxs.Hash() ) func TestValidateBlock(t *testing.T) { @@ -91,7 +91,7 @@ func TestValidateBlock(t *testing.T) { { block: &types.Block{ Header: &types.Header{Height: 11, DataHash: deadBeefRipEmd160Hash}, - Data: &types.Data{Txs: deabBeefTxs}, + Data: &types.Data{Txs: deadBeefTxs}, }, commit: lite.Commit{ Header: &types.Header{Height: 11}, @@ -101,20 +101,18 @@ func TestValidateBlock(t *testing.T) { // End Header.Data hash mismatch test } - assert := assert.New(t) - for i, tt := range tests { err := proxy.ValidateBlock(tt.block, tt.commit) if tt.wantErr != "" { if err == nil { - assert.FailNowf("Unexpectedly passed", "#%d", i) + assert.FailNowf(t, "Unexpectedly passed", "#%d", i) } else { - assert.Contains(err.Error(), tt.wantErr, "#%d should contain the substring\n\n", i) + assert.Contains(t, err.Error(), tt.wantErr, "#%d should contain the substring\n\n", i) } continue } - assert.Nil(err, "#%d: expecting a nil error", i) + assert.Nil(t, err, "#%d: expecting a nil error", i) } } @@ -238,19 +236,17 @@ func TestValidateBlockMeta(t *testing.T) { // End Headers don't match test } - assert := assert.New(t) - for i, tt := range tests { err := proxy.ValidateBlockMeta(tt.meta, tt.commit) if tt.wantErr != "" { if err == nil { - assert.FailNowf("Unexpectedly passed", "#%d: wanted error %q", i, tt.wantErr) + assert.FailNowf(t, "Unexpectedly passed", "#%d: wanted error %q", i, tt.wantErr) } else { - assert.Contains(err.Error(), tt.wantErr, "#%d should contain the substring\n\n", i) + assert.Contains(t, err.Error(), tt.wantErr, "#%d should contain the substring\n\n", i) } continue } - assert.Nil(err, "#%d: expecting a nil error", i) + assert.Nil(t, err, "#%d: expecting a nil error", i) } } From 8813684040212782a76e4ccf9627d7be8901f771 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Sun, 25 Mar 2018 00:27:38 -0600 Subject: [PATCH 3/4] lite/proxy: consolidate some common test headers into a variable Addressing some feedback from @ebuchman in regards to consolidating some common test headers into a variable. I've added that for simple cases, trying to meet in the middle instead of creating helpers that obscure readibility and easy comparison of test cases. --- lite/proxy/validate_test.go | 59 ++++++++++--------------------------- 1 file changed, 15 insertions(+), 44 deletions(-) diff --git a/lite/proxy/validate_test.go b/lite/proxy/validate_test.go index 653bfb753..bd57994d7 100644 --- a/lite/proxy/validate_test.go +++ b/lite/proxy/validate_test.go @@ -12,10 +12,17 @@ import ( ) var ( - deadBeefTxs = types.Txs{[]byte("DE"), []byte("AD"), []byte("BE"), []byte("EF")} + deadBeefTxs = types.Txs{[]byte("DE"), []byte("AD"), []byte("BE"), []byte("EF")} + deadBeefRipEmd160Hash = deadBeefTxs.Hash() ) +var hdrHeight11Tendermint = &types.Header{ + Height: 11, + Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + ValidatorsHash: []byte("Tendermint"), +} + func TestValidateBlock(t *testing.T) { tests := []struct { block *types.Block @@ -47,32 +54,14 @@ func TestValidateBlock(t *testing.T) { // Start Header.Hash mismatch test { - block: &types.Block{ - Header: &types.Header{ - Height: 11, - Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), - ValidatorsHash: []byte("Tendermint"), - }, - }, + block: &types.Block{Header: hdrHeight11Tendermint}, commit: lite.Commit{Header: &types.Header{Height: 11}}, wantErr: "Headers don't match", }, { - block: &types.Block{ - Header: &types.Header{ - Height: 11, - Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), - ValidatorsHash: []byte("Tendermint"), - }, - }, - commit: lite.Commit{ - Header: &types.Header{ - Height: 11, - Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), - ValidatorsHash: []byte("Tendermint"), - }, - }, + block: &types.Block{Header: hdrHeight11Tendermint}, + commit: lite.Commit{Header: hdrHeight11Tendermint}, }, // End Header.Hash mismatch test @@ -147,32 +136,14 @@ func TestValidateBlockMeta(t *testing.T) { // Start Headers don't match test { - meta: &types.BlockMeta{ - Header: &types.Header{ - Height: 11, - Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), - ValidatorsHash: []byte("Tendermint"), - }, - }, + meta: &types.BlockMeta{Header: hdrHeight11Tendermint}, commit: lite.Commit{Header: &types.Header{Height: 11}}, wantErr: "Headers don't match", }, { - meta: &types.BlockMeta{ - Header: &types.Header{ - Height: 11, - Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), - ValidatorsHash: []byte("Tendermint"), - }, - }, - commit: lite.Commit{ - Header: &types.Header{ - Height: 11, - Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), - ValidatorsHash: []byte("Tendermint"), - }, - }, + meta: &types.BlockMeta{Header: hdrHeight11Tendermint}, + commit: lite.Commit{Header: hdrHeight11Tendermint}, }, { @@ -208,7 +179,7 @@ func TestValidateBlockMeta(t *testing.T) { Header: &types.Header{ Height: 11, DataHash: deadBeefRipEmd160Hash, ValidatorsHash: []byte("Tendermint"), - Time: time.Date(2017, 1, 2, 2, 1, 1, 1, time.UTC), + Time: time.Date(2018, 1, 2, 1, 1, 1, 1, time.UTC), }, Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("DEADBEEF")}}, }, From 39ff4d22e93495e11df6f9f75b0e1335590ef744 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Tue, 3 Apr 2018 22:34:18 +0300 Subject: [PATCH 4/4] minor cleanup --- lite/proxy/validate_test.go | 55 +++++++++++++++++-------------------- types/block.go | 4 ++- 2 files changed, 28 insertions(+), 31 deletions(-) diff --git a/lite/proxy/validate_test.go b/lite/proxy/validate_test.go index bd57994d7..782a6aabb 100644 --- a/lite/proxy/validate_test.go +++ b/lite/proxy/validate_test.go @@ -12,14 +12,15 @@ import ( ) var ( - deadBeefTxs = types.Txs{[]byte("DE"), []byte("AD"), []byte("BE"), []byte("EF")} - - deadBeefRipEmd160Hash = deadBeefTxs.Hash() + deadBeefTxs = types.Txs{[]byte("DE"), []byte("AD"), []byte("BE"), []byte("EF")} + deadBeefHash = deadBeefTxs.Hash() + testTime1 = time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC) + testTime2 = time.Date(2017, 1, 2, 1, 1, 1, 1, time.UTC) ) -var hdrHeight11Tendermint = &types.Header{ +var hdrHeight11 = &types.Header{ Height: 11, - Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + Time: testTime1, ValidatorsHash: []byte("Tendermint"), } @@ -54,14 +55,14 @@ func TestValidateBlock(t *testing.T) { // Start Header.Hash mismatch test { - block: &types.Block{Header: hdrHeight11Tendermint}, + block: &types.Block{Header: hdrHeight11}, commit: lite.Commit{Header: &types.Header{Height: 11}}, wantErr: "Headers don't match", }, { - block: &types.Block{Header: hdrHeight11Tendermint}, - commit: lite.Commit{Header: hdrHeight11Tendermint}, + block: &types.Block{Header: hdrHeight11}, + commit: lite.Commit{Header: hdrHeight11}, }, // End Header.Hash mismatch test @@ -79,7 +80,7 @@ func TestValidateBlock(t *testing.T) { }, { block: &types.Block{ - Header: &types.Header{Height: 11, DataHash: deadBeefRipEmd160Hash}, + Header: &types.Header{Height: 11, DataHash: deadBeefHash}, Data: &types.Data{Txs: deadBeefTxs}, }, commit: lite.Commit{ @@ -136,33 +137,27 @@ func TestValidateBlockMeta(t *testing.T) { // Start Headers don't match test { - meta: &types.BlockMeta{Header: hdrHeight11Tendermint}, + meta: &types.BlockMeta{Header: hdrHeight11}, commit: lite.Commit{Header: &types.Header{Height: 11}}, wantErr: "Headers don't match", }, { - meta: &types.BlockMeta{Header: hdrHeight11Tendermint}, - commit: lite.Commit{Header: hdrHeight11Tendermint}, + meta: &types.BlockMeta{Header: hdrHeight11}, + commit: lite.Commit{Header: hdrHeight11}, }, { meta: &types.BlockMeta{ Header: &types.Header{ - Height: 11, - // TODO: (@odeke-em) inquire why ValidatorsHash has to be non-blank - // for the Header to be hashed. Perhaps this is a security hole because - // an aggressor could perhaps pass in headers that don't have - // ValidatorsHash set and we won't be able to validate blocks. + Height: 11, ValidatorsHash: []byte("lite-test"), - // TODO: (@odeke-em) file an issue with Tendermint to get them to update - // to the latest go-wire, then no more need for this value fill to avoid - // the time zero value of less than 1970. - Time: time.Date(2018, 1, 1, 1, 1, 1, 1, time.UTC), + // TODO: should be able to use empty time after Amino upgrade + Time: testTime1, }, }, commit: lite.Commit{ - Header: &types.Header{Height: 11, DataHash: deadBeefRipEmd160Hash}, + Header: &types.Header{Height: 11, DataHash: deadBeefHash}, }, wantErr: "Headers don't match", }, @@ -170,16 +165,16 @@ func TestValidateBlockMeta(t *testing.T) { { meta: &types.BlockMeta{ Header: &types.Header{ - Height: 11, DataHash: deadBeefRipEmd160Hash, + Height: 11, DataHash: deadBeefHash, ValidatorsHash: []byte("Tendermint"), - Time: time.Date(2017, 1, 2, 1, 1, 1, 1, time.UTC), + Time: testTime1, }, }, commit: lite.Commit{ Header: &types.Header{ - Height: 11, DataHash: deadBeefRipEmd160Hash, + Height: 11, DataHash: deadBeefHash, ValidatorsHash: []byte("Tendermint"), - Time: time.Date(2018, 1, 2, 1, 1, 1, 1, time.UTC), + Time: testTime2, }, Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("DEADBEEF")}}, }, @@ -189,16 +184,16 @@ func TestValidateBlockMeta(t *testing.T) { { meta: &types.BlockMeta{ Header: &types.Header{ - Height: 11, DataHash: deadBeefRipEmd160Hash, + Height: 11, DataHash: deadBeefHash, ValidatorsHash: []byte("Tendermint"), - Time: time.Date(2017, 1, 2, 1, 1, 1, 1, time.UTC), + Time: testTime2, }, }, commit: lite.Commit{ Header: &types.Header{ - Height: 11, DataHash: deadBeefRipEmd160Hash, + Height: 11, DataHash: deadBeefHash, ValidatorsHash: []byte("Tendermint-x"), - Time: time.Date(2017, 1, 2, 1, 1, 1, 1, time.UTC), + Time: testTime2, }, Commit: &types.Commit{BlockID: types.BlockID{Hash: []byte("DEADBEEF")}}, }, diff --git a/types/block.go b/types/block.go index 970ca36f4..774c69f32 100644 --- a/types/block.go +++ b/types/block.go @@ -177,7 +177,9 @@ type Header struct { } // Hash returns the hash of the header. -// Returns nil if ValidatorHash is missing. +// Returns nil if ValidatorHash is missing, +// since a Header is not valid unless there is +// a ValidaotrsHash (corresponding to the validator set). func (h *Header) Hash() cmn.HexBytes { if h == nil || len(h.ValidatorsHash) == 0 { return nil