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: Zebra's address book can use all available memory #1873

Closed
teor2345 opened this issue Mar 9, 2021 · 1 comment · Fixed by #3162
Closed

Security: Zebra's address book can use all available memory #1873

teor2345 opened this issue Mar 9, 2021 · 1 comment · Fixed by #3162
Assignees
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-slow Problems with performance or responsiveness I-unbounded-growth Zebra keeps using resources, without any limit

Comments

@teor2345
Copy link
Contributor

teor2345 commented Mar 9, 2021

Motivation

Zebra does not limit the size of its address book. Since it accepts up to 1000 addresses from each peer address request, other nodes can fill up Zebra's memory with junk addresses.

Goals

  1. Zebra should limit the address book size.
  2. Zebra should delete peers based on how recently they were successful.
  3. Zebra should replace peers that have never been successful with new gossiped peers.
  4. Zebra should replace old peers that have never been attempted with new gossiped peers.

Edge Cases

  1. In PR Security: Avoid reconnecting to peers that are likely unreachable #3030, we made Zebra try outdated peers once, and if they fail, ignore them forever. When should we delete these peers?

If we delete them immediately, we could get gossiped another copy of their address, and then retry them

  1. We closed ticket Zebra should limit the number of addresses it uses from a single Addrs response, to avoid address book takeover #1869 because it wouldn't work with zcashd. Does deleting peers make address book takeover more likely? How can we prevent that?

  2. What happens if our local clock is a long way ahead, so all gossiped peers seem outdated to us?

  3. Do we need to warn the user when all their peers get deleted? What if all their peers fail the is_probably_reachable check from Security: Avoid reconnecting to peers that are likely unreachable #3030?

Suggestions

Choosing Peers

Zebra could retain peers in the following order, choosing the newest timestamps first within each category:

  • recently successful peers: they will be re-added to the peer set as long as they stay connected
  • request pending peers: they will be re-added to the peer set if they are successful
  • never attempted peers: they have just been gossiped by other peers, so they are untrusted
  • peers that have never been successful

To implement this choice, Zebra could sort peers in recently live order, then reconnection (Ord trait) order, then delete the last peers in the list. AddressBook::update is the only method that increases the address book size, so it is a good place to delete excess peers.

Edge Cases

  • We should not spawn an extra task to make these changes, because that increases the risk of threaded mutex deadlocks
  • We should ignore last_failed_time, so that we don't risk losing all our peers after a network interruption
  • Zebra should minimise the performance impact of this new code:

Choosing the Limit

Alternatives

We could choose a random selection of untrusted_last_seen peers instead, but choosing the newest peers retains peers that are more likely to be available.

Mitigations

This issue is mitigated by:

@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-Critical C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage I-slow Problems with performance or responsiveness I-unbounded-growth Zebra keeps using resources, without any limit I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data labels Mar 9, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Mar 10, 2021
@teor2345
Copy link
Contributor Author

This ticket is not Critical, because #1889 resists this attack. (And zcashd should resist these kinds of attacks anyway.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug C-security Category: Security issues I-heavy Problems with excessive memory, disk, or CPU usage I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-slow Problems with performance or responsiveness I-unbounded-growth Zebra keeps using resources, without any limit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants