Security: stop gossiping temporary inbound remote addresses to peers

- stop putting inbound addresses in the address book
- drop address book entries that can't be used for outbound connections
  - distinguish between temporary inbound and permanent outbound peer
    addresses
  - also create variants to handle proxy connections
    (but don't use them yet)
  - avoid tracking connection state for isolated connections
- document security constraints for the address book and peer set

zebra-network/src/address_book.rs

@@ -1,4 +1,4 @@
-//! The addressbook manages information about what peers exist, when they were
+//! The `AddressBook` manages information about what peers exist, when they were
 //! seen, and what services they provide.
 
 use std::{
@@ -13,8 +13,39 @@ use tracing::Span;
 
 use crate::{constants, types::MetaAddr, PeerAddrState};
 
-/// A database of peers, their advertised services, and information on when they
-/// were last seen.
+/// A database of peer listener addresses, their advertised services, and
+/// information on when they were last seen.
+///
+/// # Security
+///
+/// Address book state must be based on outbound connections to peers.
+///
+/// If the address book is updated incorrectly:
+/// - malicious peers can interfere with other peers' `AddressBook` state,
+///   or
+/// - Zebra can advertise unreachable addresses to its own peers.
+///
+/// ## Adding Addresses
+///
+/// The address book should only contain Zcash listener port addresses from peers
+/// on the configured network. These addresses can come from:
+/// - DNS seeders
+/// - addresses gossiped by other peers
+/// - the canonical address (`Version.address_from`) provided by each peer,
+///   particularly peers on inbound connections.
+///
+/// The remote addresses of inbound connections must not be added to the address
+/// book, because they contain ephemeral outbound ports, not listener ports.
+///
+/// Isolated connections must not add addresses or update the address book.
+///
+/// ## Updating Address State
+///
+/// Updates to address state must be based on outbound connections to peers.
+///
+/// Updates must not be based on:
+/// - the remote addresses of inbound connections, or
+/// - the canonical address of any connection.
 #[derive(Clone, Debug)]
 pub struct AddressBook {
     /// Each known peer address has a matching `MetaAddr`
@@ -33,8 +64,11 @@ pub struct AddressMetrics {
     /// The number of addresses in the `Responded` state.
     responded: usize,
 
-    /// The number of addresses in the `NeverAttempted` state.
-    never_attempted: usize,
+    /// The number of addresses in the `NeverAttemptedGossiped` state.
+    never_attempted_gossiped: usize,
+
+    /// The number of addresses in the `NeverAttemptedAlternate` state.
+    never_attempted_alternate: usize,
 
     /// The number of addresses in the `Failed` state.
     failed: usize,
@@ -93,9 +127,10 @@ impl AddressBook {
     /// Add `new` to the address book, updating the previous entry if `new` is
     /// more recent or discarding `new` if it is stale.
     ///
-    /// ## Note
+    /// # Correctness
     ///
-    /// All changes should go through `update` or `take`, to ensure accurate metrics.
+    /// All new addresses should go through `update`, so that the address book
+    /// only contains valid outbound addresses.
     pub fn update(&mut self, new: MetaAddr) {
         let _guard = self.span.enter();
         trace!(
@@ -104,6 +139,14 @@ impl AddressBook {
             recent_peers = self.recently_live_peers().count(),
         );
 
+        // Drop any unspecified or client addresses.
+        //
+        // Communication with these addresses can be monitored via Zebra's
+        // metrics. (The address book is for valid peer addresses.)
+        if !new.is_valid_for_outbound() {
+            return;
+        }
+
         if let Some(prev) = self.get_by_addr(new.addr) {
             if prev.get_last_seen() > new.get_last_seen() {
                 return;
@@ -117,9 +160,10 @@ impl AddressBook {
 
     /// Removes the entry with `addr`, returning it if it exists
     ///
-    /// ## Note
+    /// # Note
     ///
-    /// All changes should go through `update` or `take`, to ensure accurate metrics.
+    /// All address removals should go through `take`, so that the address
+    /// book metrics are accurate.
     fn take(&mut self, removed_addr: SocketAddr) -> Option<MetaAddr> {
         let _guard = self.span.enter();
         trace!(
@@ -254,7 +298,12 @@ impl AddressBook {
     /// Returns metrics for the addresses in this address book.
     pub fn address_metrics(&self) -> AddressMetrics {
         let responded = self.state_peers(PeerAddrState::Responded).count();
-        let never_attempted = self.state_peers(PeerAddrState::NeverAttempted).count();
+        let never_attempted_gossiped = self
+            .state_peers(PeerAddrState::NeverAttemptedGossiped)
+            .count();
+        let never_attempted_alternate = self
+            .state_peers(PeerAddrState::NeverAttemptedAlternate)
+            .count();
         let failed = self.state_peers(PeerAddrState::Failed).count();
         let attempt_pending = self.state_peers(PeerAddrState::AttemptPending).count();
 
@@ -265,7 +314,8 @@ impl AddressBook {
 
         AddressMetrics {
             responded,
-            never_attempted,
+            never_attempted_gossiped,
+            never_attempted_alternate,
             failed,
             attempt_pending,
             recently_live,
@@ -281,7 +331,11 @@ impl AddressBook {
 
         // TODO: rename to address_book.[state_name]
         metrics::gauge!("candidate_set.responded", m.responded as f64);
-        metrics::gauge!("candidate_set.gossiped", m.never_attempted as f64);
+        metrics::gauge!("candidate_set.gossiped", m.never_attempted_gossiped as f64);
+        metrics::gauge!(
+            "candidate_set.alternate",
+            m.never_attempted_alternate as f64
+        );
         metrics::gauge!("candidate_set.failed", m.failed as f64);
         metrics::gauge!("candidate_set.pending", m.attempt_pending as f64);
 
@@ -327,7 +381,12 @@ impl AddressBook {
 
         self.last_address_log = Some(Instant::now());
         // if all peers have failed
-        if m.responded + m.attempt_pending + m.never_attempted == 0 {
+        if m.responded
+            + m.attempt_pending
+            + m.never_attempted_gossiped
+            + m.never_attempted_alternate
+            == 0
+        {
             warn!(
                 address_metrics = ?m,
                 "all peer addresses have failed. Hint: check your network connection"

zebra-network/src/isolated.rs

@@ -15,6 +15,7 @@ use tower::{
 };
 
 use crate::{peer, BoxError, Config, Request, Response};
+use peer::ConnectedAddr;
 
 /// Use the provided TCP connection to create a Zcash connection completely
 /// isolated from all other node state.
@@ -57,13 +58,11 @@ pub fn connect_isolated(
         .finish()
         .expect("provided mandatory builder parameters");
 
-    // We can't get the remote addr from conn, because it might be a tcp
-    // connection through a socks proxy, not directly to the remote. But it
-    // doesn't seem like zcashd cares if we give a bogus one, and Zebra doesn't
-    // touch it at all.
-    let remote_addr = "0.0.0.0:8233".parse().unwrap();
+    // Don't send any metadata about the connection
+    let connected_addr = ConnectedAddr::new_isolated();
 
-    Oneshot::new(handshake, (conn, remote_addr)).map_ok(|client| BoxService::new(Wrapper(client)))
+    Oneshot::new(handshake, (conn, connected_addr))
+        .map_ok(|client| BoxService::new(Wrapper(client)))
 }
 
 // This can be deleted when a new version of Tower with map_err is released.

zebra-network/src/meta_addr.rs

@@ -44,7 +44,13 @@ pub enum PeerAddrState {
 
     /// The peer's address has just been fetched from a DNS seeder, or via peer
     /// gossip, but we haven't attempted to connect to it yet.
-    NeverAttempted,
+    NeverAttemptedGossiped,
+
+    /// The peer's address has just been received as part of a `Version` message,
+    /// so we might already be connected to this peer.
+    ///
+    /// Alternate addresses are attempted after gossiped addresses.
+    NeverAttemptedAlternate,
 
     /// The peer's TCP connection failed, or the peer sent us an unexpected
     /// Zcash protocol message, so we failed the connection.
@@ -54,9 +60,11 @@ pub enum PeerAddrState {
     AttemptPending,
 }
 
+// non-test code should explicitly specify the peer address state
+#[cfg(test)]
 impl Default for PeerAddrState {
     fn default() -> Self {
-        NeverAttempted
+        NeverAttemptedGossiped
     }
 }
 
@@ -66,19 +74,23 @@ impl Ord for PeerAddrState {
     ///
     /// See [`CandidateSet`] and [`MetaAddr::cmp`] for more details.
     fn cmp(&self, other: &Self) -> Ordering {
+        use Ordering::*;
         match (self, other) {
             (Responded, Responded)
-            | (NeverAttempted, NeverAttempted)
             | (Failed, Failed)
-            | (AttemptPending, AttemptPending) => Ordering::Equal,
+            | (NeverAttemptedGossiped, NeverAttemptedGossiped)
+            | (NeverAttemptedAlternate, NeverAttemptedAlternate)
+            | (AttemptPending, AttemptPending) => Equal,
             // We reconnect to `Responded` peers that have stopped sending messages,
             // then `NeverAttempted` peers, then `Failed` peers
-            (Responded, _) => Ordering::Less,
-            (_, Responded) => Ordering::Greater,
-            (NeverAttempted, _) => Ordering::Less,
-            (_, NeverAttempted) => Ordering::Greater,
-            (Failed, _) => Ordering::Less,
-            (_, Failed) => Ordering::Greater,
+            (Responded, _) => Less,
+            (_, Responded) => Greater,
+            (NeverAttemptedGossiped, _) => Less,
+            (_, NeverAttemptedGossiped) => Greater,
+            (NeverAttemptedAlternate, _) => Less,
+            (_, NeverAttemptedAlternate) => Greater,
+            (Failed, _) => Less,
+            (_, Failed) => Greater,
             // AttemptPending is covered by the other cases
         }
     }
@@ -124,8 +136,8 @@ pub struct MetaAddr {
 }
 
 impl MetaAddr {
-    /// Create a new `MetaAddr` from the deserialized fields in an `Addr`
-    /// message.
+    /// Create a new `MetaAddr` from the deserialized fields in a gossiped
+    /// peer `Addr` message.
     pub fn new_gossiped(
         addr: &SocketAddr,
         services: &PeerServices,
@@ -136,11 +148,19 @@ impl MetaAddr {
             services: *services,
             last_seen: *last_seen,
             // the state is Zebra-specific, it isn't part of the Zcash network protocol
-            last_connection_state: NeverAttempted,
+            last_connection_state: NeverAttemptedGossiped,
         }
     }
 
     /// Create a new `MetaAddr` for a peer that has just `Responded`.
+    ///
+    /// # Security
+    ///
+    /// This address must be the remote address from an outbound connection.
+    /// Otherwise:
+    /// - malicious peers could interfere with other peers' `AddressBook` state,
+    ///   or
+    /// - Zebra could advertise unreachable addresses to its own peers.
     pub fn new_responded(addr: &SocketAddr, services: &PeerServices) -> MetaAddr {
         MetaAddr {
             addr: *addr,
@@ -160,6 +180,17 @@ impl MetaAddr {
         }
     }
 
+    /// Create a new `MetaAddr` for a peer's alternate address, received via a
+    /// `Version` message.
+    pub fn new_alternate(addr: &SocketAddr, services: &PeerServices) -> MetaAddr {
+        MetaAddr {
+            addr: *addr,
+            services: *services,
+            last_seen: Utc::now(),
+            last_connection_state: NeverAttemptedAlternate,
+        }
+    }
+
     /// Create a new `MetaAddr` for a peer that has just had an error.
     pub fn new_errored(addr: &SocketAddr, services: &PeerServices) -> MetaAddr {
         MetaAddr {
@@ -195,6 +226,13 @@ impl MetaAddr {
         self.last_seen
     }
 
+    /// Is this address valid for outbound connections?
+    pub fn is_valid_for_outbound(&self) -> bool {
+        self.services.contains(PeerServices::NODE_NETWORK)
+            && !self.addr.ip().is_unspecified()
+            && self.addr.port() != 0
+    }
+
     /// Return a sanitized version of this `MetaAddr`, for sending to a remote peer.
     pub fn sanitize(&self) -> MetaAddr {
         let interval = crate::constants::TIMESTAMP_TRUNCATION_SECONDS;
@@ -207,7 +245,7 @@ impl MetaAddr {
             services: self.services,
             last_seen,
             // the state isn't sent to the remote peer, but sanitize it anyway
-            last_connection_state: Default::default(),
+            last_connection_state: NeverAttemptedGossiped,
         }
     }
 }
@@ -222,29 +260,31 @@ impl Ord for MetaAddr {
     /// See [`CandidateSet`] for more details.
     fn cmp(&self, other: &Self) -> Ordering {
         use std::net::IpAddr::{V4, V6};
+        use Ordering::*;
 
         let oldest_first = self.get_last_seen().cmp(&other.get_last_seen());
         let newest_first = oldest_first.reverse();
 
         let connection_state = self.last_connection_state.cmp(&other.last_connection_state);
         let reconnection_time = match self.last_connection_state {
             Responded => oldest_first,
-            NeverAttempted => newest_first,
+            NeverAttemptedGossiped => newest_first,
+            NeverAttemptedAlternate => newest_first,
             Failed => oldest_first,
             AttemptPending => oldest_first,
         };
         let ip_numeric = match (self.addr.ip(), other.addr.ip()) {
             (V4(a), V4(b)) => a.octets().cmp(&b.octets()),
             (V6(a), V6(b)) => a.octets().cmp(&b.octets()),
-            (V4(_), V6(_)) => Ordering::Less,
-            (V6(_), V4(_)) => Ordering::Greater,
+            (V4(_), V6(_)) => Less,
+            (V6(_), V4(_)) => Greater,
         };
 
         connection_state
             .then(reconnection_time)
             // The remainder is meaningless as an ordering, but required so that we
             // have a total order on `MetaAddr` values: self and other must compare
-            // as Ordering::Equal iff they are equal.
+            // as Equal iff they are equal.
             .then(ip_numeric)
             .then(self.addr.port().cmp(&other.addr.port()))
             .then(self.services.bits().cmp(&other.services.bits()))

zebra-network/src/peer.rs

@@ -21,4 +21,4 @@ pub use client::Client;
 pub use connection::Connection;
 pub use connector::Connector;
 pub use error::{HandshakeError, PeerError, SharedPeerError};
-pub use handshake::Handshake;
+pub use handshake::{ConnectedAddr, Handshake, HandshakeRequest};

zebra-network/src/peer/connector.rs

@@ -11,7 +11,7 @@ use tower::{discover::Change, Service, ServiceExt};
 
 use crate::{BoxError, Request, Response};
 
-use super::{Client, Handshake};
+use super::{Client, ConnectedAddr, Handshake};
 
 /// A wrapper around [`peer::Handshake`] that opens a TCP connection before
 /// forwarding to the inner handshake service. Writing this as its own
@@ -53,7 +53,8 @@ where
         async move {
             let stream = TcpStream::connect(addr).await?;
             hs.ready_and().await?;
-            let client = hs.call((stream, addr)).await?;
+            let connected_addr = ConnectedAddr::new_outbound_direct(addr);
+            let client = hs.call((stream, connected_addr)).await?;
             Ok(Change::Insert(addr, client))
         }
         .boxed()