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

Refactor MetaAddr fields to enable security fixes #1849

Closed
8 tasks done
teor2345 opened this issue Mar 5, 2021 · 3 comments · Fixed by #2275
Closed
8 tasks done

Refactor MetaAddr fields to enable security fixes #1849

teor2345 opened this issue Mar 5, 2021 · 3 comments · Fixed by #2275
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-slow Problems with performance or responsiveness I-unbounded-growth Zebra keeps using resources, without any limit

Comments

@teor2345
Copy link
Contributor

teor2345 commented Mar 5, 2021

Motivation

Zebra updates the same time field for different peer events, making it hard to:

  • securely handle peer addresses
  • provide accurate timestamps when gossiping peers to other nodes

Solution

Desired Changes

Zebra should:

Zebra should track the following times for each peer:

  • untrusted_last_seen remote time gossiped by the peer that told us about this address
  • last_success local time when we last saw a message from this peer
  • last_attempt local time when we last attempted to connect to this peer
  • last_failed local time when we last failed to connect to this peer

We might want to also refactor the services field into untrusted_indirect_services and untrusted_direct_services as part of this change. (Neither field is trusted, but one of them came directly from the peer, and the other came via other peers.)

This might be easier to implement using an enum with Direct (confirmed addr, claimed services) and Indirect (definitely unconfirmed) variants.

Incidental Changes

It might be difficult to maintain exactly the same next peer address selection behaviour after this change. If we can't keep the same behaviour, we should make the fix in #1876 as part of this change.

Correctness

Correctness:

  • reject updates that move existing peers into the NeverAttempted... states. (Updates can only be AttemptPending, Responded, or Failed.)
  • ensure that all inserts of new peers are in the NeverAttempted... states.
  • keep the oldest timestamps for NeverAttempted... peers.

Alternatives

It would be nice to keep fewer times for each peer, but these times all seem to be required by upcoming network security fixes. We can provide accessor methods on MetaAddr to simplify the interface to these times.

We could also keep an address_entry_creation_time, but after the fix in #1871, we can safely use untrusted_last_seen_time instead, because untrusted_last_seen_time is earlier than or equal to address_entry_creation_time. Using untrusted_last_seen_time is better when the peer is honest, because it contains useful information about the last time that peer was available.

Follow Up

We might want to remove the Ord impl, and just use a custom sort function for each different purpose (next address, removing addresses, etc.).

Context

zcashd does not have this issue.

@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 NU-5 Network Upgrade: NU5 specific tasks P-High 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 labels Mar 5, 2021
@teor2345 teor2345 added this to the 2021 Sprint 4 milestone Mar 5, 2021
@teor2345 teor2345 self-assigned this Mar 8, 2021
@teor2345 teor2345 removed this from the 2021 Sprint 4 milestone Mar 8, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Mar 8, 2021

This is a fairly large ticket for a security fix. So we should try to work out ways to split it up, delay parts of it, or remove parts of it.

If we split it into smaller fixes, it will be easier to review, and less likely to have unintended consequences.

A small design doc might also be helpful here. We could add it to the CandidateSet or AddressBook module docs.

@mpguerra mpguerra added this to the 2021 Sprint 5 milestone Mar 8, 2021
@teor2345 teor2345 changed the title Zebra should eventually stop trying to contact peers that always fail Refactor MetaAddr fields to enable security fixes Mar 9, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Mar 9, 2021

After the split, this is now a small task.

@teor2345
Copy link
Contributor Author

teor2345 commented Mar 9, 2021

After the fix in #1871, we can safely used untrusted_last_seen_time instead of address_entry_creation_time, because untrusted_last_seen_time is earlier than or equal to address_entry_creation_time.

So I've deleted address_entry_creation_time from this ticket.

@teor2345 teor2345 modified the milestones: 2021 Sprint 6, 2021 Sprint 7 Mar 31, 2021
@teor2345 teor2345 removed the NU-5 Network Upgrade: NU5 specific tasks label Apr 26, 2021
@teor2345 teor2345 modified the milestones: 2021 Sprint 8 , 2021 Sprint 9 May 3, 2021
@mpguerra mpguerra removed this from the 2021 Sprint 9 milestone May 13, 2021
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-slow Problems with performance or responsiveness I-unbounded-growth Zebra keeps using resources, without any limit
Projects
None yet
2 participants