Skip to content

Refactor MetaAddr fields to enable security fixes #1849

Closed

Description

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

Labels

A-rustArea: Updates to Rust codeC-bugCategory: This is a bugC-securityCategory: Security issuesI-heavyProblems with excessive memory, disk, or CPU usageI-slowProblems with performance or responsivenessI-unbounded-growthZebra keeps using resources, without any limit

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions