Skip to content

Commit d766d20

Browse files
greg-szabosergio-mena
greg-szabo
and
sergio-mena
authored
Merge pull request from GHSA-qr8r-m495-7hc4
* Added comments, tests and more validation * Added unclog message * Update types/params.go Co-authored-by: Sergio Mena <sergio@informal.systems> * Update types/params.go Co-authored-by: Sergio Mena <sergio@informal.systems> * Update types/params.go Co-authored-by: Greg Szabo <16846635+greg-szabo@users.noreply.github.com> * Fixed unclog message --------- Co-authored-by: Sergio Mena <sergio@informal.systems>
1 parent dab72ad commit d766d20

File tree

3 files changed

+115
-67
lines changed

3 files changed

+115
-67
lines changed

‎.changelog/unreleased/bug-fixes/0000-asa-2024-001-fix-validate.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
`[consensus]` Fix for "Validation of `VoteExtensionsEnableHeight` can cause chain halt"
2+
([ASA-2024-001](https://github.com/cometbft/cometbft/security/advisories/GHSA-qr8r-m495-7hc4))

‎types/params.go‎

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -205,27 +205,58 @@ func (params ConsensusParams) ValidateBasic() error {
205205
return nil
206206
}
207207

208+
// ValidateUpdate validates the updated VoteExtensionsEnableHeight.
209+
// | r | params...EnableHeight | updated...EnableHeight | result (nil == pass)
210+
// | 1 | * | (nil) | nil
211+
// | 2 | * | < 0 | VoteExtensionsEnableHeight must be positive
212+
// | 3 | <=0 | 0 | nil
213+
// | 4 | > 0; <=height | 0 | vote extensions cannot be disabled once enabled
214+
// | 5 | > 0; > height | 0 | nil (disable a previous proposal)
215+
// | 6 | * | <=height | vote extensions cannot be updated to a past height
216+
// | 7 | <=0 | > height (*) | nil
217+
// | 8 | (> 0) <=height | > height (*) | vote extensions cannot be modified once enabled
218+
// | 9 | (> 0) > height | > height (*) | nil
208219
func (params ConsensusParams) ValidateUpdate(updated *cmtproto.ConsensusParams, h int64) error {
209-
if updated.Abci == nil {
220+
// 1
221+
if updated == nil || updated.Abci == nil {
210222
return nil
211223
}
212-
if params.ABCI.VoteExtensionsEnableHeight == updated.Abci.VoteExtensionsEnableHeight {
224+
// 2
225+
if updated.Abci.VoteExtensionsEnableHeight < 0 {
226+
return errors.New("VoteExtensionsEnableHeight must be positive")
227+
}
228+
// 3
229+
if params.ABCI.VoteExtensionsEnableHeight <= 0 && updated.Abci.VoteExtensionsEnableHeight == 0 {
213230
return nil
214231
}
215-
if params.ABCI.VoteExtensionsEnableHeight != 0 && updated.Abci.VoteExtensionsEnableHeight == 0 {
216-
return errors.New("vote extensions cannot be disabled once enabled")
232+
// 4 & 5
233+
if params.ABCI.VoteExtensionsEnableHeight > 0 && updated.Abci.VoteExtensionsEnableHeight == 0 {
234+
// 4
235+
if params.ABCI.VoteExtensionsEnableHeight <= h {
236+
return fmt.Errorf("vote extensions cannot be disabled once enabled"+
237+
"old enable height: %d, current height %d",
238+
params.ABCI.VoteExtensionsEnableHeight, h)
239+
}
240+
// 5
241+
return nil
217242
}
243+
// 6 (implicit: updated.Abci.VoteExtensionsEnableHeight > 0)
218244
if updated.Abci.VoteExtensionsEnableHeight <= h {
219-
return fmt.Errorf("VoteExtensionsEnableHeight cannot be updated to a past height, "+
220-
"initial height: %d, current height %d",
221-
params.ABCI.VoteExtensionsEnableHeight, h)
245+
return fmt.Errorf("vote extensions cannot be updated to a past or current height, "+
246+
"enable height: %d, current height %d",
247+
updated.Abci.VoteExtensionsEnableHeight, h)
248+
}
249+
// 7 (implicit: updated.Abci.VoteExtensionsEnableHeight > h)
250+
if params.ABCI.VoteExtensionsEnableHeight <= 0 {
251+
return nil
222252
}
253+
// 8 (implicit: params.ABCI.VoteExtensionsEnableHeight > 0 && updated.Abci.VoteExtensionsEnableHeight > h)
223254
if params.ABCI.VoteExtensionsEnableHeight <= h {
224-
return fmt.Errorf("VoteExtensionsEnableHeight cannot be modified once"+
225-
"the initial height has occurred, "+
226-
"initial height: %d, current height %d",
255+
return fmt.Errorf("vote extensions cannot be modified once enabled"+
256+
"enable height: %d, current height %d",
227257
params.ABCI.VoteExtensionsEnableHeight, h)
228258
}
259+
// 9 (implicit: params.ABCI.VoteExtensionsEnableHeight > h && updated.Abci.VoteExtensionsEnableHeight > h)
229260
return nil
230261
}
231262

‎types/params_test.go‎

Lines changed: 72 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -154,63 +154,78 @@ func TestConsensusParamsUpdate_AppVersion(t *testing.T) {
154154
}
155155

156156
func TestConsensusParamsUpdate_VoteExtensionsEnableHeight(t *testing.T) {
157-
t.Run("set to height but initial height already run", func(*testing.T) {
158-
initialParams := makeParams(1, 0, 2, 0, valEd25519, 1)
159-
update := &cmtproto.ConsensusParams{
160-
Abci: &cmtproto.ABCIParams{
161-
VoteExtensionsEnableHeight: 10,
162-
},
163-
}
164-
require.Error(t, initialParams.ValidateUpdate(update, 1))
165-
require.Error(t, initialParams.ValidateUpdate(update, 5))
166-
})
167-
t.Run("reset to 0", func(t *testing.T) {
168-
initialParams := makeParams(1, 0, 2, 0, valEd25519, 1)
169-
update := &cmtproto.ConsensusParams{
170-
Abci: &cmtproto.ABCIParams{
171-
VoteExtensionsEnableHeight: 0,
172-
},
173-
}
174-
require.Error(t, initialParams.ValidateUpdate(update, 1))
175-
})
176-
t.Run("set to height before current height run", func(*testing.T) {
177-
initialParams := makeParams(1, 0, 2, 0, valEd25519, 100)
178-
update := &cmtproto.ConsensusParams{
179-
Abci: &cmtproto.ABCIParams{
180-
VoteExtensionsEnableHeight: 10,
181-
},
182-
}
183-
require.Error(t, initialParams.ValidateUpdate(update, 11))
184-
require.Error(t, initialParams.ValidateUpdate(update, 99))
185-
})
186-
t.Run("set to height after current height run", func(*testing.T) {
187-
initialParams := makeParams(1, 0, 2, 0, valEd25519, 300)
188-
update := &cmtproto.ConsensusParams{
189-
Abci: &cmtproto.ABCIParams{
190-
VoteExtensionsEnableHeight: 99,
191-
},
192-
}
193-
require.NoError(t, initialParams.ValidateUpdate(update, 11))
194-
require.NoError(t, initialParams.ValidateUpdate(update, 98))
195-
})
196-
t.Run("no error when unchanged", func(*testing.T) {
197-
initialParams := makeParams(1, 0, 2, 0, valEd25519, 100)
198-
update := &cmtproto.ConsensusParams{
199-
Abci: &cmtproto.ABCIParams{
200-
VoteExtensionsEnableHeight: 100,
201-
},
202-
}
203-
require.NoError(t, initialParams.ValidateUpdate(update, 500))
204-
})
205-
t.Run("updated from 0 to 0", func(t *testing.T) {
206-
initialParams := makeParams(1, 0, 2, 0, valEd25519, 0)
207-
update := &cmtproto.ConsensusParams{
208-
Abci: &cmtproto.ABCIParams{
209-
VoteExtensionsEnableHeight: 0,
210-
},
211-
}
212-
require.NoError(t, initialParams.ValidateUpdate(update, 100))
213-
})
157+
const nilTest = -10000000
158+
testCases := []struct {
159+
name string
160+
current int64
161+
from int64
162+
to int64
163+
expectedErr bool
164+
}{
165+
// no change
166+
{"current: 3, 0 -> 0", 3, 0, 0, false},
167+
{"current: 3, 100 -> 100, ", 3, 100, 100, false},
168+
{"current: 100, 100 -> 100, ", 100, 100, 100, true},
169+
{"current: 300, 100 -> 100, ", 300, 100, 100, true},
170+
// set for the first time
171+
{"current: 3, 0 -> 5, ", 3, 0, 5, false},
172+
{"current: 4, 0 -> 5, ", 4, 0, 5, false},
173+
{"current: 5, 0 -> 5, ", 5, 0, 5, true},
174+
{"current: 6, 0 -> 5, ", 6, 0, 5, true},
175+
{"current: 50, 0 -> 5, ", 50, 0, 5, true},
176+
// reset to 0
177+
{"current: 4, 5 -> 0, ", 4, 5, 0, false},
178+
{"current: 5, 5 -> 0, ", 5, 5, 0, true},
179+
{"current: 6, 5 -> 0, ", 6, 5, 0, true},
180+
{"current: 10, 5 -> 0, ", 10, 5, 0, true},
181+
// modify backwards
182+
{"current: 1, 10 -> 5, ", 1, 10, 5, false},
183+
{"current: 4, 10 -> 5, ", 4, 10, 5, false},
184+
{"current: 5, 10 -> 5, ", 5, 10, 5, true},
185+
{"current: 6, 10 -> 5, ", 6, 10, 5, true},
186+
{"current: 9, 10 -> 5, ", 9, 10, 5, true},
187+
{"current: 10, 10 -> 5, ", 10, 10, 5, true},
188+
{"current: 11, 10 -> 5, ", 11, 10, 5, true},
189+
{"current: 100, 10 -> 5, ", 100, 10, 5, true},
190+
// modify forward
191+
{"current: 3, 10 -> 15, ", 3, 10, 15, false},
192+
{"current: 9, 10 -> 15, ", 9, 10, 15, false},
193+
{"current: 10, 10 -> 15, ", 10, 10, 15, true},
194+
{"current: 11, 10 -> 15, ", 11, 10, 15, true},
195+
{"current: 14, 10 -> 15, ", 14, 10, 15, true},
196+
{"current: 15, 10 -> 15, ", 15, 10, 15, true},
197+
{"current: 16, 10 -> 15, ", 16, 10, 15, true},
198+
{"current: 100, 10 -> 15, ", 100, 10, 15, true},
199+
// negative values
200+
{"current: 3, 0 -> -5", 3, 0, -5, true},
201+
{"current: 3, -5 -> 100, ", 3, -5, 100, false},
202+
{"current: 3, -10 -> 3, ", 3, -10, 3, true},
203+
{"current: 3, -3 -> -3", 3, -3, -3, true},
204+
{"current: 100, -8 -> -9, ", 100, -8, -9, true},
205+
{"current: 300, -10 -> -8, ", 300, -10, -8, true},
206+
// test for nil
207+
{"current: 300, 400 -> nil, ", 300, 400, nilTest, false},
208+
{"current: 300, 200 -> nil, ", 300, 200, nilTest, false},
209+
}
210+
211+
for _, tc := range testCases {
212+
t.Run(tc.name, func(*testing.T) {
213+
initialParams := makeParams(1, 0, 2, 0, valEd25519, tc.from)
214+
update := &cmtproto.ConsensusParams{}
215+
if tc.to == nilTest {
216+
update.Abci = nil
217+
} else {
218+
update.Abci = &cmtproto.ABCIParams{
219+
VoteExtensionsEnableHeight: tc.to,
220+
}
221+
}
222+
if tc.expectedErr {
223+
require.Error(t, initialParams.ValidateUpdate(update, tc.current))
224+
} else {
225+
require.NoError(t, initialParams.ValidateUpdate(update, tc.current))
226+
}
227+
})
228+
}
214229
}
215230

216231
func TestProto(t *testing.T) {

0 commit comments

Comments
 (0)