fix: cross-validate ExtendedCommit against LastCommit in blocksync (backport #2884)#2930
Merged
rootulp merged 2 commits intov0.39.x-celestiafrom Apr 17, 2026
Merged
Conversation
…2884) Motivated by https://dashboard.hackenproof.com/manager/companies/celestia/celestia/reports/CELESTIA-238 even though that report is N/A because the attack doesn't really work as described. ## Summary - Cross-validate the ExtendedCommit against the validator set during block sync before persisting to the blockstore - Use `state.Validators.VerifyCommit()` on `extCommit.ToCommit()` to validate all signatures and addresses match the validator set, matching how upstream CometBFT handles this - Add tests demonstrating that a corrupted ExtendedCommit with a same-length but wrong ValidatorAddress bypasses existing validation (`ValidateBasic`, `EnsureExtensions`) and causes a panic on consensus restart via `reconstructLastCommit` → `ToExtendedVoteSet` ### Background During block sync, the blocksync reactor validates `second.LastCommit` via `VerifyCommitLight` but saves the separately-received `extCommit` without cross-validating it. A malicious peer could provide a valid block and LastCommit but a corrupted ExtendedCommit (e.g. with wrong ValidatorAddress fields). The corrupted ExtendedCommit would pass `EnsureExtensions` (which only checks extension presence) and be persisted to the blockstore. On consensus restart, `reconstructLastCommit` would load the poisoned ExtendedCommit and panic in `addSigsToVoteSet` due to the address mismatch. ### Approach Call `state.Validators.VerifyCommit(chainID, firstID, first.Height, extCommit.ToCommit())` before persisting the ExtendedCommit. This validates that every non-absent signature's `ValidatorAddress` matches the validator at that index and verifies all cryptographic signatures. On any mismatch, the peer is disconnected. An earlier approach compared `extCommit.ToCommit().Hash()` against `second.LastCommit.Hash()`, but this produced false positives because these two commits can legitimately contain different vote subsets (the proposer of `second` may have collected different 2/3+ signatures than the peer who provided `extCommit`). The `VerifyCommit` approach avoids this by validating the ExtendedCommit against the validator set directly, without comparing against `second.LastCommit`. ## Test plan - [x] `TestReportedAttack_PrependByteToValidatorAddress` — confirms the originally reported 0xFF-prepend attack is already caught during deserialization - [x] `TestSameLengthAddressCorruption_PanicsOnRestart` — proves the real validation gap: a same-length address corruption bypasses all pre-existing checks and causes a panic on vote set reconstruction - [x] `TestVerifyCommitDetectsAddressCorruption` — proves `VerifyCommit` detects the corruption - [x] Existing blocksync tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> (cherry picked from commit f98262f)
mergify
Bot
requested review from
evan-forbes,
mcrakhman,
ninabarbakadze and
rach-id
as code owners
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+1
to
+3
| - `[blocksync]` Cross-validate ExtendedCommit against LastCommit before | ||
| persisting during block sync to prevent a malicious peer from injecting a | ||
| corrupted ExtendedCommit that causes a panic on consensus restart. |
There was a problem hiding this comment.
🟡 Changelog filename missing required issue/PR number prefix
Both CONTRIBUTING.md and CLAUDE.md mandate that changelog entries follow the naming format {issue-or-pr-number}-{description}.md. The file .changelog/unreleased/bug-fixes/blocksync-extcommit-validation.md is missing the PR number prefix — it should be 2930-blocksync-extcommit-validation.md (for PR #2930).
Rule references
From CONTRIBUTING.md: "Every fix, improvement, feature, or breaking change should be made in a pull-request that includes a file .changelog/unreleased/${category}/${issue-or-pr-number}-${description}.md"
From CLAUDE.md: "Every PR needs a changelog entry at .changelog/unreleased/{category}/{issue-or-pr-number}-{description}.md"
Was this helpful? React with 👍 or 👎 to provide feedback.
ninabarbakadze
approved these changes
Apr 17, 2026
mcrakhman
approved these changes
Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivated by https://dashboard.hackenproof.com/manager/companies/celestia/celestia/reports/CELESTIA-238 even though that report is N/A because the attack doesn't really work as described.
Summary
state.Validators.VerifyCommit()onextCommit.ToCommit()to validate all signatures and addresses match the validator set, matching how upstream CometBFT handles thisValidateBasic,EnsureExtensions) and causes a panic on consensus restart viareconstructLastCommit→ToExtendedVoteSetBackground
During block sync, the blocksync reactor validates
second.LastCommitviaVerifyCommitLightbut saves the separately-receivedextCommitwithout cross-validating it. A malicious peer could provide a valid block and LastCommit but a corrupted ExtendedCommit (e.g. with wrong ValidatorAddress fields). The corrupted ExtendedCommit would passEnsureExtensions(which only checks extension presence) and be persisted to the blockstore. On consensus restart,reconstructLastCommitwould load the poisoned ExtendedCommit and panic inaddSigsToVoteSetdue to the address mismatch.Approach
Call
state.Validators.VerifyCommit(chainID, firstID, first.Height, extCommit.ToCommit())before persisting the ExtendedCommit. This validates that every non-absent signature'sValidatorAddressmatches the validator at that index and verifies all cryptographic signatures. On any mismatch, the peer is disconnected.An earlier approach compared
extCommit.ToCommit().Hash()againstsecond.LastCommit.Hash(), but this produced false positives because these two commits can legitimately contain different vote subsets (the proposer ofsecondmay have collected different 2/3+ signatures than the peer who providedextCommit). TheVerifyCommitapproach avoids this by validating the ExtendedCommit against the validator set directly, without comparing againstsecond.LastCommit.Test plan
TestReportedAttack_PrependByteToValidatorAddress— confirms the originally reported 0xFF-prepend attack is already caught during deserializationTestSameLengthAddressCorruption_PanicsOnRestart— proves the real validation gap: a same-length address corruption bypasses all pre-existing checks and causes a panic on vote set reconstructionTestVerifyCommitDetectsAddressCorruption— provesVerifyCommitdetects the corruption🤖 Generated with Claude Code
This is an automatic backport of pull request #2884 done by Mergify.