feat(consensus): additional sanity checks for the size of proposed blocks by cason · Pull Request #1408 · cometbft/cometbft

cason · 2023-09-27T16:36:40Z

Hardens tests regarding the size of proposed blocks, namely:

The byte size of a proposal block Part should be constant (== types.BlockPartSizeBytes), except for the last part of a PartSet (<= types.BlockPartSizeBytes)
A valid Proposal should not enclose a PartSet enabling the building of a ProposalBlock with size larger than the configured ConsensusParams.Block.MaxBytes. Notice that building a ProposalBlock larger than the allowed would fail in any case, but the proposed changes also invalidate the associated Proposal.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

types/part_set.go

sergio-mena · 2023-09-28T12:06:47Z

types/part_set.go

+		return ErrPartTooBig
+	}
+	// All parts except the last one should have the same constant size.
+	if int64(part.Index) < part.Proof.Total-1 && len(part.Bytes) != int(BlockPartSizeBytes) {


We're still using part.Proof.Total here, are we 100% sure this total will always be in sync with the total number of parts?

Yes, this is a good point. But we don't have the Total field in each Part, we only have it on the PartSet.

The point here is that wrong values for Proof.Total and Proof.Index should make the Merkle-tree verification to fail. But I am not sure whether we test this at this point. Notice that the Part tests have nothing on the arrays used to fill the Part and the Part.Proof bytes.

Is there a possibility that Part is used without populating part.Proof? (I guess no, but double-checking).

It should not be possible, as the next check (line 40) would fail. This check, however, is also a valid check: integer fields are >= 0, hashes have the right length.

So, status regarding this:

we can trust Proof.Index, as change it causes the proof verification to fail

we can't trust Proof.Total, as it is irrelevant for the proof verification provided that Proof.Index < Proof.Total.

Ok, now Proof.Total should be correct, otherwise the Part is not added. Lets check if something breaks due to this additional check.

consensus/state.go

sergio-mena · 2023-09-28T15:11:58Z

consensus/state_test.go

+			maxBlockParts := maxBytes / int64(types.BlockPartSizeBytes)
+			if maxBytes > maxBlockParts*int64(types.BlockPartSizeBytes) {
+				maxBlockParts += 1
+			}
+			numBlockParts := int64(propBlockParts.Total())


Suggested change

maxBlockParts := maxBytes / int64(types.BlockPartSizeBytes)

if maxBytes > maxBlockParts*int64(types.BlockPartSizeBytes) {

maxBlockParts += 1

}

numBlockParts := int64(propBlockParts.Total())

maxBlockParts := (maxBytes-1) / int64(types.BlockPartSizeBytes) + 1

numBlockParts := int64(propBlockParts.Total())

How about this instead?

I prefer using a different method, even if it is less efficient/smart, when testing.

github-actions · 2023-10-09T00:55:45Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sergio-mena

I was convinced I had already approved this (sorry for delay)

thanethomson · 2023-10-12T13:29:23Z

Is this going to be backported? If so, to which version(s)?

cason · 2023-10-12T13:49:38Z

Is this going to be backported? If so, to which version(s)?

I would say all releases. Why?

thanethomson · 2023-10-12T14:03:52Z

I would say all releases. Why?

There are currently no backport labels on the PR.

Let's get this over the finish line please.

cason · 2023-10-12T14:48:40Z

There are currently no backport labels on the PR.

Sorry, my bad. @sergio-mena, can you confirm we are backporting this to all releases?

sergio-mena · 2023-10-16T16:52:55Z

As this comes from our work in a security advisory, I'd say we should backport it to all releases

…ocks (backport cometbft#1408) (cometbft#2139) This is an automatic backport of pull request cometbft#1408 done by [Mergify](https://mergify.com). Cherry-pick of 28ad4d2 has failed: ``` On branch mergify/bp/v0.38.x/pr-1408 Your branch is up to date with 'origin/v0.38.x'. You are currently cherry-picking commit 28ad4d2. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) Changes to be committed: modified: consensus/state.go modified: consensus/state_test.go modified: crypto/merkle/proof.go modified: evidence/pool_test.go modified: state/execution_test.go modified: types/event_bus_test.go modified: types/part_set.go modified: types/part_set_test.go Unmerged paths: (use "git add/rm <file>..." as appropriate to mark resolution) deleted by us: internal/consensus/errors.go both modified: state/store_test.go both modified: store/store_test.go ``` To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details> --------- Co-authored-by: Daniel <daniel.cason@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

* feat(consensus): additional sanity checks for the size of proposed blocks (backport cometbft#1408) (cometbft#2139) This is an automatic backport of pull request cometbft#1408 done by [Mergify](https://mergify.com). Cherry-pick of 28ad4d2 has failed: ``` On branch mergify/bp/v0.38.x/pr-1408 Your branch is up to date with 'origin/v0.38.x'. You are currently cherry-picking commit 28ad4d2. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) Changes to be committed: modified: consensus/state.go modified: consensus/state_test.go modified: crypto/merkle/proof.go modified: evidence/pool_test.go modified: state/execution_test.go modified: types/event_bus_test.go modified: types/part_set.go modified: types/part_set_test.go Unmerged paths: (use "git add/rm <file>..." as appropriate to mark resolution) deleted by us: internal/consensus/errors.go both modified: state/store_test.go both modified: store/store_test.go ``` To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details> --------- Co-authored-by: Daniel <daniel.cason@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Andy Nogueira <me@andynogueira.dev> * Merge commit from fork --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Daniel <daniel.cason@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Andy Nogueira <me@andynogueira.dev> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

…ocks (backport cometbft#1408) (cometbft#2139) This is an automatic backport of pull request cometbft#1408 done by [Mergify](https://mergify.com). Cherry-pick of 28ad4d2 has failed: ``` On branch mergify/bp/v0.38.x/pr-1408 Your branch is up to date with 'origin/v0.38.x'. You are currently cherry-picking commit 28ad4d2. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) Changes to be committed: modified: consensus/state.go modified: consensus/state_test.go modified: crypto/merkle/proof.go modified: evidence/pool_test.go modified: state/execution_test.go modified: types/event_bus_test.go modified: types/part_set.go modified: types/part_set_test.go Unmerged paths: (use "git add/rm <file>..." as appropriate to mark resolution) deleted by us: internal/consensus/errors.go both modified: state/store_test.go both modified: store/store_test.go ``` To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details> --------- Co-authored-by: Daniel <daniel.cason@informal.systems> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

Daniel Cason and others added 4 commits September 27, 2023 18:32

types: PartSet validates size of inner block parts

b937160

consensus: check Proposal's block part set size

ffaefb4

Fix block parts condition

4d306d0

Fixed unit tests

f61b160

sergio-mena reviewed Sep 27, 2023

View reviewed changes

types/part_set.go Outdated Show resolved Hide resolved

Small cosmetic fix in UT

04a0cf2

sergio-mena reviewed Sep 27, 2023

View reviewed changes

types/part_set.go Outdated Show resolved Hide resolved

types: added suggested fixes in Part sanity check

7f1c0b2

cason marked this pull request as ready for review September 28, 2023 03:38

cason requested a review from a team as a code owner September 28, 2023 03:38

cason requested a review from a team September 28, 2023 03:38

sergio-mena reviewed Sep 28, 2023

View reviewed changes

lasarojc reviewed Sep 28, 2023

View reviewed changes

consensus/state.go Outdated Show resolved Hide resolved

Daniel Cason added 2 commits September 28, 2023 16:44

Test unit fails due to error reported by reviewer

e8379d2

Fixing logic error spotted by @lasarojc

af282e1

sergio-mena reviewed Sep 28, 2023

View reviewed changes

Daniel Cason added 3 commits September 28, 2023 17:48

Fixing minor lint issue

2624f81

Added further verification of part proof lenght

8d70a3f

Merge branch 'main' into cason/block-size-checks

42a4b52

github-actions bot added the stale For use by stalebot label Oct 9, 2023

sergio-mena removed the stale For use by stalebot label Oct 10, 2023

sergio-mena approved these changes Oct 10, 2023

View reviewed changes

cason self-assigned this Oct 17, 2023

cason added the consensus label Oct 17, 2023

This was referenced Jul 22, 2024

add empty v9 upgrade burnt-labs/xion#231

Merged

Chore/protoc swagger gen burnt-labs/xion#232

Merged

fix typo in ./proto/xion/jwk/v1/query.proto burnt-labs/xion#234

Merged

This was referenced Aug 9, 2024

Chore/docker multiplatform burnt-labs/xion#246

Merged

Chore/devnet burnt-labs/xion#249

Merged

This was referenced Aug 19, 2024

buf generate must run from within proto dir, do not need to gen cosmo… burnt-labs/xion#250

Merged

Fix/gosec errors burnt-labs/xion#253

Merged

faddat mentioned this pull request Oct 15, 2024

chore: use latest cometbft-db in v0.38.x #4296

Closed

3 tasks

haiyizxx mentioned this pull request Feb 5, 2025

chore!: update dependencies axelarnetwork/axelar-core#2238

Merged

6 tasks

Conversation

cason commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Uh oh!

Uh oh!

sergio-mena Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 9, 2023

Uh oh!

sergio-mena left a comment

Choose a reason for hiding this comment

Uh oh!

thanethomson commented Oct 12, 2023

Uh oh!

cason commented Oct 12, 2023

Uh oh!

thanethomson commented Oct 12, 2023

Uh oh!

cason commented Oct 12, 2023

Uh oh!

sergio-mena commented Oct 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cason commented Sep 27, 2023 •

edited

Loading

sergio-mena Sep 28, 2023 •

edited

Loading