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

security(state): Limit the number of non-finalized chains tracked by Zebra #6447

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Apr 2, 2023

Motivation

There is no limit on the number of non-finalized chains that Zebra can track.

Theoretically, this allows unbounded use of memory. But this bug is difficult to exploit on mainnet, because it would require mining hundreds of side-chain blocks at mainnet difficulty.

Specifications

The number of chains tracked by a node implementation is a standard rule, not a consensus rule. For zcashd, it is currently 1 in memory, and some number on disk.

For Zebra, after this PR it will be 10 in memory. (Zebra doesn't store any unverified or side-chain blocks on disk.)

Complex Code or Requirements

This is concurrent code, the non-finalized state is cloned before reading it concurrently. We ensure that the limit of 10 is kept by making the list of chains into a private field, and only accessing them via a method.

Solution

  • Limit the number of non-finalized chains to 10
    • If the limit is reached, drop the chain with the least work
    • Make the chain list private, and provide iterator, count, and insert chain methods

Review

This is a low priority security fix.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added C-bug Category: This is a bug P-Low ❄️ C-security Category: Security issues I-unbounded-growth Zebra keeps using resources, without any limit A-state Area: State / database changes A-concurrency Area: Async code, needs extra work to make it work properly. labels Apr 2, 2023
@teor2345 teor2345 requested a review from a team as a code owner April 2, 2023 23:38
@teor2345 teor2345 self-assigned this Apr 2, 2023
@teor2345 teor2345 requested review from arya2 and removed request for a team April 2, 2023 23:38
@teor2345 teor2345 changed the title Security: Limit the number of non-finalized chains tracked by Zebra security(state): Limit the number of non-finalized chains tracked by Zebra Apr 2, 2023
@teor2345 teor2345 force-pushed the limit-chain-forks branch from 43c1b9d to 4822ab0 Compare April 3, 2023 00:55
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #6447 (de1269f) into main (5db2243) will increase coverage by 0.07%.
The diff coverage is 90.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6447      +/-   ##
==========================================
+ Coverage   77.78%   77.86%   +0.07%     
==========================================
  Files         304      304              
  Lines       39637    39659      +22     
==========================================
+ Hits        30833    30879      +46     
+ Misses       8804     8780      -24     

Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks excellent!

zebra-state/src/service/read/find.rs Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

teor2345 commented Apr 3, 2023

Temporary network errors:

warning: spurious network error (2 tries remaining): early EOF; class=Net (12); code=Eof (-20)
Warning: warning: spurious network error (1 tries remaining): early EOF; class=Net (12); code=Eof (-20)
error: Unable to update registry crates-io
Error: Unable to update registry crates-io
Caused by:
failed to fetch [https://github.com/rust-lang/crates.io-index](https://github.com/rust-lang/crates.io-index%60)

https://github.com/ZcashFoundation/zebra/actions/runs/4601125572/jobs/8128642182#step:5:15

Error: -03 20:40:13 [ERROR] cargo metadata exited with an error: Updating crates.io index
warning: spurious network error (2 tries remaining): early EOF; class=Net (12); code=Eof (-20)
warning: spurious network error (1 tries remaining): early EOF; class=Net (12); code=Eof (-20)
error: Unable to update registry crates-io

Caused by:
failed to fetch [https://github.com/rust-lang/crates.io-index](https://github.com/rust-lang/crates.io-index%60)

https://github.com/ZcashFoundation/zebra/actions/runs/4601125562/jobs/8128592665?pr=6447#step:4:14

mergify bot added a commit that referenced this pull request Apr 3, 2023
@mergify mergify bot merged commit 00d46e1 into main Apr 4, 2023
@mergify mergify bot deleted the limit-chain-forks branch April 4, 2023 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-state Area: State / database changes C-bug Category: This is a bug C-security Category: Security issues I-unbounded-growth Zebra keeps using resources, without any limit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants