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

Allow CompactSize to be used for non-length fields (addrv2 and ZSEs) #2173

Closed
teor2345 opened this issue May 20, 2021 · 3 comments · Fixed by #3014
Closed

Allow CompactSize to be used for non-length fields (addrv2 and ZSEs) #2173

teor2345 opened this issue May 20, 2021 · 3 comments · Fixed by #3014
Assignees
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-bug Category: This is a bug C-security Category: Security issues I-consensus Zebra breaks a Zcash consensus rule I-panic Zebra panics with an internal error message

Comments

@teor2345
Copy link
Contributor

teor2345 commented May 20, 2021

Motivation

In #2155, we reject CompactSizes greater than the maximum network message length, as a memory DoS defence-in-depth.

But some ZIPs use CompactSize for counts and ids that are not array lengths.

We might want different compact size types for array lengths, and for non-length values.
We could split the CompactSize into:

  • CompactSizeMessage(u32): limited to the maximum protocol message length (currently 2MB), and
  • CompactSize64(u64): limited to 8 bytes

Specifications

The following ZIPs use CompactSize for counts and ids that are not array lengths:

Alternative Implementations

We could just use librustzcash for these ZIPs, treat its data as opaque, and inherit its CompactSize limits.

Some parts of librustzcash and zcashd currently have a 32 MB limit, inherited from bitcoin.

@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code S-needs-triage Status: A bug report needs triage P-Medium C-security Category: Security issues I-consensus Zebra breaks a Zcash consensus rule I-panic Zebra panics with an internal error message A-network Area: Network protocol updates or fixes labels May 20, 2021
@dconnolly
Copy link
Contributor

never have more than 2 million blocks

This is likely to happen within the next year or so 😬

@dconnolly
Copy link
Contributor

We should split the CompactSize into:

  • CompactSizeLength(u32): limited to the maximum protocol message length (currently 2MB), and
  • CompactSize64(u64): limited to 8 bytes

Could we make these variants of a CompactSize enum?

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label May 31, 2021
@teor2345 teor2345 changed the title Security: Create CompactSizeLength and CompactSize64 types Allow CompactSize to be used for Zcash Shielded Extensions Jul 6, 2021
@teor2345 teor2345 changed the title Allow CompactSize to be used for Zcash Shielded Extensions Allow CompactSize to be used for non-length fields (addrv2 and ZSEs) Nov 1, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Nov 1, 2021

never have more than 2 million blocks

This is likely to happen within the next year or so 😬

Around September 2022 on testnet, if there is no testnet rollback.

We should split the CompactSize into:

  • CompactSizeLength(u32): limited to the maximum protocol message length (currently 2MB), and
  • CompactSize64(u64): limited to 8 bytes

Could we make these variants of a CompactSize enum?

An enum isn't type-safe, so we could accidentally:

  • use a large invalid size for a message length
  • panic because we're checking the protocol message length limit, but the field isn't a message length

So we need separate structs for each type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-bug Category: This is a bug C-security Category: Security issues I-consensus Zebra breaks a Zcash consensus rule I-panic Zebra panics with an internal error message
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants