Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialization for validator::Set and related proto structs is inconsistent, lacks validation #1348

Closed
mzabaluev opened this issue Sep 7, 2023 · 1 comment · Fixed by #1350
Assignees
Labels
domain-types Anything relating to the creation, modification or removal of domain types enhancement New feature or request security serialization

Comments

@mzabaluev
Copy link
Contributor

mzabaluev commented Sep 7, 2023

#1340 attempted to fix an issue with deserializing ValidatorSet data without the total_voting_power field being present in the serialized message.
The field is actually redundant, its value is computable as the sum of voting powers of the validators listed in the message. If the field is not present in the deserialized JSON or protobuf, its value should be recomputed. There is also an issue of trust with impications on protocol security: if we accept the total voting power value without checking it against the sum of listed validators' powers, this means validator::Set cannot be assumed correct by construction.

In the serialization bolted onto the tendermint-proto types generated for ValidatorSet, we skipped the field to match the serialization schema of misbehavior evidence, where it is not present in at least one place, in #1292.

Unfortunately, validator::Set has also had Serialize and Deserialize derived, the total_voting_power field included with no fallback, and this stricter deserialization schema has probably been in use by parties unknown. That serialization is simply derived, with no validations.

Definition of "done"

The following needs to be implemented and backed by tests:

  • In tendermint, the Deserialize impl for validator::Set can tolerate the absence of the total_voting_power field by computing it from validator data.
  • If the the total_voting_power field is present in the deserialized structure, its value is validated against the computed sum of validators' voting powers.
    • A special case can be made for the value of 0, which is easy to fill by default in Go and seems to occur in the wild. This is also the default deserialization for the missing field in protobuf. It should be treated like omission of the field.
  • In tendermint-proto, the deserialization for the generated ValidatorSet structs accepts a present total_voting_power field and applies the same validation as described above represents its value in the struct field. On serialization, the field is omitted to maintain backward compatibility; alternatively (in a technically breaking change), the field is included, but serializations of compound types with validator set fields may bypass this serialization by e.g. using a custom helper.
  • The sum computation is protected against integer overflows.
  • Validator set serializations where the total voting power exceeds the protocol-defined maximum limit (u64::MAX / 8) are rejected as invalid.

Originally posted by @mzabaluev in #1340 (comment)

@mzabaluev mzabaluev added enhancement New feature or request serialization domain-types Anything relating to the creation, modification or removal of domain types security labels Sep 7, 2023
@mzabaluev
Copy link
Contributor Author

Corrected in the DoD: the tendermint-proto structs are raw protocol data, they should not normally be validated.

@mzabaluev mzabaluev self-assigned this Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain-types Anything relating to the creation, modification or removal of domain types enhancement New feature or request security serialization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant