From 7213a0a8811940a372076ba8e3dcbdd084ae7eb3 Mon Sep 17 00:00:00 2001 From: "Leo Zhang (zhangchiqing)" Date: Wed, 26 Aug 2026 11:43:29 -0700 Subject: [PATCH 1/3] add nil-element validation to flow models --- consensus/hotstuff/model/timeout.go | 12 ++++++--- consensus/hotstuff/model/timeout_test.go | 28 ++++++++++++++++++-- model/flow/block_test.go | 33 +++++++++++++++++++++--- model/flow/chunk.go | 13 +++++++++- model/flow/chunk_test.go | 12 +++++++++ model/flow/execution_result.go | 5 ++++ model/flow/execution_result_test.go | 13 ++++++++++ model/flow/header.go | 23 +++++++++++++++-- model/flow/header_test.go | 16 ++++++++++++ model/flow/payload.go | 5 ++++ model/flow/payload_test.go | 15 +++++++++++ 11 files changed, 163 insertions(+), 12 deletions(-) diff --git a/consensus/hotstuff/model/timeout.go b/consensus/hotstuff/model/timeout.go index 5482069c7f9..4b59447ee8e 100644 --- a/consensus/hotstuff/model/timeout.go +++ b/consensus/hotstuff/model/timeout.go @@ -94,13 +94,19 @@ func NewTimeoutObject(untrusted UntrustedTimeoutObject) (*TimeoutObject, error) // If a TC is included, the TC must be for the past round, no matter whether a QC // for the last round is also included. In some edge cases, a node might observe // _both_ QC and TC for the previous round, in which case it can include both. + var lastViewTC *flow.TimeoutCertificate if untrusted.LastViewTC != nil { if untrusted.View != untrusted.LastViewTC.View+1 { return nil, fmt.Errorf("invalid TC for non-previous view, expected view %d, got view %d", untrusted.View-1, untrusted.LastViewTC.View) } - if untrusted.NewestQC.View < untrusted.LastViewTC.NewestQC.View { - return nil, fmt.Errorf("timeout.NewestQC is older (view=%d) than the QC in timeout.LastViewTC (view=%d)", untrusted.NewestQC.View, untrusted.LastViewTC.NewestQC.View) + tc, err := flow.NewTimeoutCertificate(flow.UntrustedTimeoutCertificate(*untrusted.LastViewTC)) + if err != nil { + return nil, fmt.Errorf("invalid LastViewTC: %w", err) } + if untrusted.NewestQC.View < tc.NewestQC.View { + return nil, fmt.Errorf("timeout.NewestQC is older (view=%d) than the QC in timeout.LastViewTC (view=%d)", untrusted.NewestQC.View, tc.NewestQC.View) + } + lastViewTC = tc } // The TO must contain a proof that sender legitimately entered View. Transitioning // to round timeout.View is possible either by observing a QC or a TC for the previous round. @@ -118,7 +124,7 @@ func NewTimeoutObject(untrusted UntrustedTimeoutObject) (*TimeoutObject, error) return &TimeoutObject{ View: untrusted.View, NewestQC: untrusted.NewestQC, - LastViewTC: untrusted.LastViewTC, + LastViewTC: lastViewTC, SignerID: untrusted.SignerID, SigData: untrusted.SigData, TimeoutTick: untrusted.TimeoutTick, diff --git a/consensus/hotstuff/model/timeout_test.go b/consensus/hotstuff/model/timeout_test.go index 157761ab4fb..62968f1d935 100644 --- a/consensus/hotstuff/model/timeout_test.go +++ b/consensus/hotstuff/model/timeout_test.go @@ -195,13 +195,15 @@ func TestNewTimeoutObject(t *testing.T) { }) t.Run("invalid input when TimeoutObject's QC is older than TC's QC", func(t *testing.T) { + // TC must be valid: TC.View (199) >= TC.NewestQC.View (150). + // TO.NewestQC.View (80) < TC.NewestQC.View (150) triggers the error. tcQC := helper.MakeQC(helper.WithQCView(150)) - tc := helper.MakeTC(helper.WithTCNewestQC(tcQC), helper.WithTCView(99)) + tc := helper.MakeTC(helper.WithTCNewestQC(tcQC), helper.WithTCView(199)) res, err := model.NewTimeoutObject( model.UntrustedTimeoutObject( *helper.TimeoutObjectFixture( - helper.WithTimeoutObjectView(100), + helper.WithTimeoutObjectView(200), // must be TC.View+1 helper.WithTimeoutLastViewTC(tc), helper.WithTimeoutNewestQC(helper.MakeQC(helper.WithQCView(80))), // older than TC.NewestQC ), @@ -212,6 +214,28 @@ func TestNewTimeoutObject(t *testing.T) { assert.Contains(t, err.Error(), "timeout.NewestQC is older") }) + t.Run("invalid input when LastViewTC has nil NewestQC", func(t *testing.T) { + // A non-nil TC whose nested NewestQC is nil must be rejected; this is the + // exact attack vector from the security audit (CBOR null → nil pointer). + // Build a TC that is initially valid (TC.View=199 >= TC.NewestQC.View=150), + // then poison the nested NewestQC pointer to nil. + tc := helper.MakeTC(helper.WithTCNewestQC(helper.MakeQC(helper.WithQCView(150))), helper.WithTCView(199)) + tc.NewestQC = nil // poison the nested pointer + + res, err := model.NewTimeoutObject( + model.UntrustedTimeoutObject( + *helper.TimeoutObjectFixture( + helper.WithTimeoutObjectView(200), // TO.View == TC.View+1 + helper.WithTimeoutLastViewTC(tc), + helper.WithTimeoutNewestQC(helper.MakeQC(helper.WithQCView(100))), // TO.NewestQC.View < 200 + ), + ), + ) + require.Error(t, err) + require.Nil(t, res) + assert.Contains(t, err.Error(), "invalid LastViewTC") + }) + t.Run("invalid input when no QC for previous round and TC is missing", func(t *testing.T) { qc := helper.MakeQC(helper.WithQCView(90)) diff --git a/model/flow/block_test.go b/model/flow/block_test.go index 14cb137faa2..b4bf2420cfe 100644 --- a/model/flow/block_test.go +++ b/model/flow/block_test.go @@ -118,18 +118,43 @@ func TestBlock_Status(t *testing.T) { } } +// blockWithLastViewTC returns a FullBlockFixture that is guaranteed to have a non-nil LastViewTC. +// HeaderBodyWithParentFixture omits LastViewTC when view == parent.View+1 (1-in-10 chance). +// NewHeaderBody now validates LastViewTC via NewTimeoutCertificate, so mutating a nil TC to a +// zero-value struct would cause hashModel() to panic in the malleability checker. +func blockWithLastViewTC() *flow.Block { + for { + if b := unittest.FullBlockFixture(); b.LastViewTC != nil { + return b + } + } +} + // TestBlockMalleability checks that flow.Block is not malleable: any change in its data // should result in a different ID. -// Because our NewHeaderBody constructor enforces ParentView < View we use -// WithFieldGenerator to safely pass it. +// NewHeaderBody enforces ParentView < View and validates LastViewTC via NewTimeoutCertificate, +// so WithFieldGenerator is used for both to keep those constraints intact. func TestBlockMalleability(t *testing.T) { - block := unittest.FullBlockFixture() + block := blockWithLastViewTC() unittest.RequireEntityNonMalleable( t, - unittest.FullBlockFixture(), + blockWithLastViewTC(), unittest.WithFieldGenerator("HeaderBody.ParentView", func() uint64 { return block.View - 1 // ParentView must stay below View, so set it to View-1 }), + // The field generator for LastViewTC must return the struct value (not a pointer): + // isModelMalleable dereferences *TimeoutCertificate before invoking the generator, + // so modelOrField is flow.TimeoutCertificate at that point. + unittest.WithFieldGenerator("HeaderBody.LastViewTC", func() flow.TimeoutCertificate { + qc := unittest.QuorumCertificateFixture() + return flow.TimeoutCertificate{ + View: qc.View + 1, + NewestQCViews: []uint64{qc.View}, + NewestQC: qc, + SignerIndices: unittest.SignerIndicesFixture(4), + SigData: unittest.SignatureFixture(), + } + }), unittest.WithFieldGenerator("Payload.Results", func() flow.ExecutionResultList { return flow.ExecutionResultList{unittest.ExecutionResultFixture()} }), diff --git a/model/flow/chunk.go b/model/flow/chunk.go index 76064753847..5309abbf47c 100644 --- a/model/flow/chunk.go +++ b/model/flow/chunk.go @@ -230,11 +230,22 @@ func NewChunkDataPack(untrusted UntrustedChunkDataPack) (*ChunkDataPack, error) return nil, fmt.Errorf("ExecutionDataRoot.ChunkExecutionDataIDs must not be empty") } + var collection *Collection + if untrusted.Collection != nil { + c, err := NewCollection(UntrustedCollection{ + Transactions: untrusted.Collection.Transactions, + }) + if err != nil { + return nil, fmt.Errorf("invalid collection: %w", err) + } + collection = c + } + return &ChunkDataPack{ ChunkID: untrusted.ChunkID, StartState: untrusted.StartState, Proof: untrusted.Proof, - Collection: untrusted.Collection, + Collection: collection, ExecutionDataRoot: untrusted.ExecutionDataRoot, }, nil } diff --git a/model/flow/chunk_test.go b/model/flow/chunk_test.go index 6455f1c79bd..72d8602035a 100644 --- a/model/flow/chunk_test.go +++ b/model/flow/chunk_test.go @@ -267,6 +267,18 @@ func TestFromUntrustedChunkDataPack(t *testing.T) { assert.Nil(t, pack) assert.Contains(t, err.Error(), "ExecutionDataRoot.ChunkExecutionDataIDs") }) + + t.Run("Collection with nil transaction element rejected", func(t *testing.T) { + untrusted := baseChunkDataPack + untrusted.Collection = &flow.Collection{ + Transactions: []*flow.TransactionBody{nil}, + } + + pack, err := flow.NewChunkDataPack(untrusted) + assert.Error(t, err) + assert.Nil(t, pack) + assert.Contains(t, err.Error(), "invalid collection") + }) } // TestNewChunk verifies that NewChunk constructs a valid Chunk when given diff --git a/model/flow/execution_result.go b/model/flow/execution_result.go index 7317a55e554..34190abb975 100644 --- a/model/flow/execution_result.go +++ b/model/flow/execution_result.go @@ -47,6 +47,11 @@ func NewExecutionResult(untrusted UntrustedExecutionResult) (*ExecutionResult, e if len(untrusted.Chunks) == 0 { return nil, fmt.Errorf("Chunks must not be empty") } + for i, ch := range untrusted.Chunks { + if ch == nil { + return nil, fmt.Errorf("chunk at index %d is nil", i) + } + } if untrusted.ExecutionDataID == ZeroID { return nil, fmt.Errorf("ExecutionDataID must not be empty") diff --git a/model/flow/execution_result_test.go b/model/flow/execution_result_test.go index c7b4169a6d5..fbad5d84bab 100644 --- a/model/flow/execution_result_test.go +++ b/model/flow/execution_result_test.go @@ -244,6 +244,19 @@ func TestNewExecutionResult(t *testing.T) { assert.Nil(t, res) assert.Contains(t, err.Error(), "ExecutionDataID") }) + + t.Run("nil Chunk element rejected", func(t *testing.T) { + u := flow.UntrustedExecutionResult{ + PreviousResultID: validPrevID, + BlockID: validBlockID, + Chunks: flow.ChunkList{nil}, + ExecutionDataID: validExecDataID, + } + res, err := flow.NewExecutionResult(u) + assert.Error(t, err) + assert.Nil(t, res) + assert.Contains(t, err.Error(), "chunk at index 0 is nil") + }) } // TestNewRootExecutionResult verifies the behavior of the NewRootExecutionResult constructor. diff --git a/model/flow/header.go b/model/flow/header.go index 927855537c0..7d2c3b5147e 100644 --- a/model/flow/header.go +++ b/model/flow/header.go @@ -111,8 +111,27 @@ func NewHeaderBody(untrusted UntrustedHeaderBody) (*HeaderBody, error) { return nil, fmt.Errorf("Timestamp must not be zero-value") } - hb := HeaderBody(untrusted) - return &hb, nil + var lastViewTC *TimeoutCertificate + if untrusted.LastViewTC != nil { + tc, err := NewTimeoutCertificate(UntrustedTimeoutCertificate(*untrusted.LastViewTC)) + if err != nil { + return nil, fmt.Errorf("invalid LastViewTC: %w", err) + } + lastViewTC = tc + } + + return &HeaderBody{ + ChainID: untrusted.ChainID, + ParentID: untrusted.ParentID, + Height: untrusted.Height, + Timestamp: untrusted.Timestamp, + View: untrusted.View, + ParentView: untrusted.ParentView, + ParentVoterIndices: untrusted.ParentVoterIndices, + ParentVoterSigData: untrusted.ParentVoterSigData, + ProposerID: untrusted.ProposerID, + LastViewTC: lastViewTC, + }, nil } // NewRootHeaderBody creates a new instance of root HeaderBody. diff --git a/model/flow/header_test.go b/model/flow/header_test.go index a5bea15d9ba..3988ae083c6 100644 --- a/model/flow/header_test.go +++ b/model/flow/header_test.go @@ -314,6 +314,22 @@ func TestNewHeaderBody(t *testing.T) { assert.Nil(t, hb) assert.Contains(t, err.Error(), "Timestamp must not be zero-value") }) + + t.Run("non-nil LastViewTC with nil NewestQC rejected", func(t *testing.T) { + u := UntrustedHeaderBodyFixture(func(u *flow.UntrustedHeaderBody) { + u.LastViewTC = &flow.TimeoutCertificate{ + View: u.View - 1, + NewestQCViews: []uint64{u.View - 2}, + NewestQC: nil, // nil nested pointer — the attack vector + SignerIndices: unittest.SignerIndicesFixture(4), + SigData: unittest.SignatureFixture(), + } + }) + hb, err := flow.NewHeaderBody(u) + assert.Error(t, err) + assert.Nil(t, hb) + assert.Contains(t, err.Error(), "invalid LastViewTC") + }) } // TestHeaderBodyBuilder_PresenceChecks verifies that HeaderBodyBuilder.Build diff --git a/model/flow/payload.go b/model/flow/payload.go index 5e95ffd6c97..76711546899 100644 --- a/model/flow/payload.go +++ b/model/flow/payload.go @@ -64,6 +64,11 @@ func NewPayload(untrusted UntrustedPayload) (*Payload, error) { if r == nil { return nil, fmt.Errorf("result at index %d is nil", i) } + for j, ch := range r.Chunks { + if ch == nil { + return nil, fmt.Errorf("chunk at index %d in result at index %d is nil", j, i) + } + } } return &Payload{ diff --git a/model/flow/payload_test.go b/model/flow/payload_test.go index c4ef10897c6..028a581c751 100644 --- a/model/flow/payload_test.go +++ b/model/flow/payload_test.go @@ -133,4 +133,19 @@ func TestNewPayload(t *testing.T) { require.Nil(t, res) require.Contains(t, err.Error(), "result at index 0 is nil") }) + + t.Run("nil Chunk element in Result rejected", func(t *testing.T) { + er := unittest.ExecutionResultFixture() + er.Chunks = flow.ChunkList{nil} + + untrusted := flow.UntrustedPayload(unittest.PayloadFixture( + unittest.WithProtocolStateID(unittest.IdentifierFixture()), + )) + untrusted.Results = flow.ExecutionResultList{er} + + res, err := flow.NewPayload(untrusted) + require.Error(t, err) + require.Nil(t, res) + require.Contains(t, err.Error(), "chunk at index 0 in result at index 0 is nil") + }) } From dc3b59026fd459e75e569dddf91df4288f848a78 Mon Sep 17 00:00:00 2001 From: Leo Zhang Date: Thu, 27 Aug 2026 08:40:32 -0700 Subject: [PATCH 2/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- model/flow/block_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/model/flow/block_test.go b/model/flow/block_test.go index b4bf2420cfe..7eda219f482 100644 --- a/model/flow/block_test.go +++ b/model/flow/block_test.go @@ -123,11 +123,13 @@ func TestBlock_Status(t *testing.T) { // NewHeaderBody now validates LastViewTC via NewTimeoutCertificate, so mutating a nil TC to a // zero-value struct would cause hashModel() to panic in the malleability checker. func blockWithLastViewTC() *flow.Block { - for { + const maxAttempts = 1000 + for i := 0; i < maxAttempts; i++ { if b := unittest.FullBlockFixture(); b.LastViewTC != nil { return b } } + panic("failed to generate FullBlockFixture with non-nil LastViewTC") } // TestBlockMalleability checks that flow.Block is not malleable: any change in its data From 0f35865765e21b1f44b587f9067568dcc57b5c42 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:46:44 +0000 Subject: [PATCH 3/3] Use same block fixture in malleability test Co-authored-by: zhangchiqing <811374+zhangchiqing@users.noreply.github.com> --- model/flow/block_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/flow/block_test.go b/model/flow/block_test.go index 7eda219f482..a5d276e46e5 100644 --- a/model/flow/block_test.go +++ b/model/flow/block_test.go @@ -140,7 +140,7 @@ func TestBlockMalleability(t *testing.T) { block := blockWithLastViewTC() unittest.RequireEntityNonMalleable( t, - blockWithLastViewTC(), + block, unittest.WithFieldGenerator("HeaderBody.ParentView", func() uint64 { return block.View - 1 // ParentView must stay below View, so set it to View-1 }),