diff --git a/.gitignore b/.gitignore index 243f2d008a0..6dee1dfe72f 100644 --- a/.gitignore +++ b/.gitignore @@ -84,3 +84,4 @@ tps-results*.json # Custom golangcilint build custom-gcl tools/custom-gcl +ai-notes/ diff --git a/module/validation/seal_validator.go b/module/validation/seal_validator.go index c6c5341a97e..229464afb13 100644 --- a/module/validation/seal_validator.go +++ b/module/validation/seal_validator.go @@ -8,6 +8,7 @@ import ( "github.com/onflow/flow-go/engine" "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/module" + "github.com/onflow/flow-go/module/irrecoverable" "github.com/onflow/flow-go/module/signature" "github.com/onflow/flow-go/state/fork" "github.com/onflow/flow-go/state/protocol" @@ -241,18 +242,39 @@ func (s *sealValidator) Validate(candidate *flow.Block) (*flow.Seal, error) { return latestSeal, nil } -// validateSeal performs integrity checks of single seal. To be valid, we -// require that seal: -// 1) Contains correct number of approval signatures, one aggregated sig for each chunk. -// 2) Every aggregated signature contains valid signer ids. module.ChunkAssigner is used to perform this check. -// 3) Every aggregated signature contains valid signatures. +// validateSeal performs integrity checks of single seal. To be valid, we require that the seal: +// 1. `Seal.FinalState` is equal to the final state as committed to by the sealed execution result. +// 2. Contains correct number of approval signatures, one aggregated sig for each chunk. +// 3. Every aggregated signature contains valid signer ids. `module.ChunkAssigner` is used to perform this check. +// 4. Every aggregated signature contains valid signatures. +// // Returns: -// * nil - in case of success -// * engine.InvalidInputError - in case of malformed seal -// * exception - in case of unexpected error +// - nil - in case of success +// - engine.InvalidInputError - in case of malformed seal +// - exception - in case of unexpected error func (s *sealValidator) validateSeal(seal *flow.Seal, incorporatedResult *flow.IncorporatedResult) error { executionResult := incorporatedResult.Result + // Check that `Seal.FinalState` equals the final state of the execution result, i.e. the last chunk's `EndState`, returned by + // `executionResult.FinalStateCommitment()`. `Seal.FinalState` is a redundant copy of that value, included so that callers such as + // `Snapshot.Commit` can read the sealed state commitment directly from the seal without looking up the execution result. The verifier + // signatures are computed over the fields `{chunk.BlockID, ExecutionResultID, chunk.Index}` for each chunk in the execution result, + // so tampering with those fields in the seal would invalidate verifier signatures in seal, which is caught when checking those + // verifier signatures. Verifiers check the `IncorporatedResult` while consensus nodes check the correct construction of the seal for + // the `IncorporatedResult`. So the only remaining field that a Byzantine proposer could tamper with is the `Seal.FinalState`. We check + // this field first before the per-chunk work, because the check is cheap. But formally, there is no order dependence of the checks. + expectedFinalState, err := executionResult.FinalStateCommitment() + if err != nil { + // `FinalStateCommitment` is documented to fail only with `flow.ErrNoChunks`. That error cannot occur here: `incorporatedResult.Result` + // was loaded from local storage after being incorporated in an ancestor of the candidate block (not the candidate itself), so + // `ReceiptValidator.verifyChunksFormat` has already enforced `Chunks.Len() ≥ 1` (the system chunk is mandatory). Any error observed here + // therefore indicates a bug or storage corruption. + return irrecoverable.NewExceptionf("could not derive final state commitment for execution result %x: %w", executionResult.ID(), err) + } + if seal.FinalState != expectedFinalState { + return engine.NewInvalidInputErrorf("seal's final state %x does not match execution result (final state %x)", seal.FinalState, expectedFinalState) + } + // check that each chunk has an AggregatedSignature if len(seal.AggregatedApprovalSigs) != executionResult.Chunks.Len() { return engine.NewInvalidInputErrorf("mismatching signatures, expected: %d, got: %d", diff --git a/module/validation/seal_validator_test.go b/module/validation/seal_validator_test.go index f88d98f4e21..e396418aaa5 100644 --- a/module/validation/seal_validator_test.go +++ b/module/validation/seal_validator_test.go @@ -272,6 +272,49 @@ func (s *SealValidationSuite) TestSealDuplicatedApproval() { s.Require().True(engine.IsInvalidInputError(err)) } +// TestSealInvalidFinalState verifies that we reject a seal whose `FinalState` does not equal the +// final state from the sealed execution result (i.e. the last chunk's `EndState`). +// +// Background: `Seal.FinalState` is an auxiliary field whose value is supposed to copied from the +// sealed `ExecutionResult` (specifically `executionResult.FinalStateCommitment()`). The verifier +// attestations only cover `{BlockID, ExecutionResultID, ChunkIndex}` from the execution result. +// Verifiers do _not_ commit to `Seal.FinalState`. Without an explicit check by consensus nodes in +// the seal validator, a Byzantine consensus node could propose a block payload with valid aggregated +// approvals for a real incorporated execution result but set `Seal.FinalState` to an arbitrary non-empty +// value. The downstream `Snapshot.Commit` would then expose the poisoned commitment as the sealed state. +// +// We test with the following fork: +// +// ... <- LatestSealedBlock <- B0 <- B1{ Result[B0], Receipt[B0] } <- B2 <- ░newBlock{ Seal[B0]}░ +// +// The gap of 1 block, i.e. B2, is required to avoid a sealing edge-case +// (see test `TestSeal_EnforceGap` for more details). +func (s *SealValidationSuite) TestSealInvalidFinalState() { + _, _, newBlock, receipt, seal := s.generateBasicTestFork() + + // Sanity check for correct testing setup: the fixture's seal must agree with the execution result's final state. + // This guarantees that the subsequent mutation below is the _only_ deviation between the seal and the result. + expectedFinalState, err := receipt.ExecutionResult.FinalStateCommitment() + s.Require().NoError(err) + s.Require().Equal(expectedFinalState, seal.FinalState) + + // Attack Details: The Byzantine proposer publishes `newBlock` containing a seal for B0. The byzantine proposer complies with the + // protocol in that it includes the required number of valid verifier signatures in the seal. It truthfully sets all fields in the + // seal that are covered by the verifier signatures (BlockID, ResultID, ChunkIndex, AggregatedApprovalSigs), to not invalidate the + // aggregated verifier signatures. However, while the protocol mandates that `Seal.FinalState` is set to the `FinalStateCommitment` + // of the execution result previously incorporated in the fork, the byzantine proposer chooses a conflicting value. + // To mount such an attack, the proposer would build (and sign) their proposal with the poisoned `Seal.FinalState` from the start. + // The test takes the shortcut of mutating `Seal.FinalState` in place because `sealValidator.Validate` only inspects the block + // payload; proposer signatures will have been checked in production by the compliance layer before (which we omit here). + seal.FinalState[0] ^= 0xff // bitwise XOR with `0xFF`: inverts all bits + s.Require().NotEqual(flow.EmptyStateCommitment, seal.FinalState) // remains non-empty (NewSeal precondition) + s.Require().NotEqual(expectedFinalState, seal.FinalState) + + _, err = s.sealValidator.Validate(newBlock) + s.Require().Error(err) + s.Require().True(engine.IsInvalidInputError(err), err) +} + // TestSealInvalidChunkAssignment tests that we reject seal with invalid signerID of approval signature for // submitted seal. We test with the following fork: //