Security: Limit address book size to limit memory usage (#3162)

* Refactor the address response limit

* Limit the number of peers in the address book

* Allow changing the address book limit in tests

* Add tests for the address book length limit

* rustfmt

zebra-network/src/address_book.rs

@@ -8,8 +8,8 @@ use ordered_map::OrderedMap;
 use tracing::Span;
 
 use crate::{
-    meta_addr::MetaAddrChange, protocol::external::canonical_socket_addr, types::MetaAddr,
+    constants, meta_addr::MetaAddrChange, protocol::external::canonical_socket_addr,
-    PeerAddrState,
+    types::MetaAddr, PeerAddrState,
 };
 
 #[cfg(test)]
@@ -59,6 +59,12 @@ pub struct AddressBook {
     /// sorts in ascending order, but [`OrderedMap`] sorts in descending order.
     by_addr: OrderedMap<SocketAddr, MetaAddr, Reverse<MetaAddr>>,
 
+    /// The maximum number of addresses in the address book.
+    ///
+    /// Always set to [`MAX_ADDRS_IN_ADDRESS_BOOK`](constants::MAX_ADDRS_IN_ADDRESS_BOOK),
+    /// in release builds. Lower values are used during testing.
+    addr_limit: usize,
+
     /// The local listener address.
     local_listener: SocketAddr,
 
@@ -107,6 +113,7 @@ impl AddressBook {
 
         let mut new_book = AddressBook {
             by_addr: OrderedMap::new(|meta_addr| Reverse(*meta_addr)),
+            addr_limit: constants::MAX_ADDRS_IN_ADDRESS_BOOK,
             local_listener: canonical_socket_addr(local_listener),
             span,
             last_address_log: None,
@@ -117,7 +124,9 @@ impl AddressBook {
     }
 
     /// Construct an [`AddressBook`] with the given `local_listener`,
-    /// [`tracing::Span`], and addresses.
+    /// `addr_limit`, [`tracing::Span`], and addresses.
+    ///
+    /// `addr_limit` is enforced by this method, and by [`AddressBook::update`].
     ///
     /// If there are multiple [`MetaAddr`]s with the same address,
     /// an arbitrary address is inserted into the address book,
@@ -128,6 +137,7 @@ impl AddressBook {
     #[cfg(any(test, feature = "proptest-impl"))]
     pub fn new_with_addrs(
         local_listener: SocketAddr,
+        addr_limit: usize,
         span: Span,
         addrs: impl IntoIterator<Item = MetaAddr>,
     ) -> AddressBook {
@@ -138,6 +148,7 @@ impl AddressBook {
         let chrono_now = Utc::now();
 
         let mut new_book = AddressBook::new(local_listener, span);
+        new_book.addr_limit = addr_limit;
 
         let addrs = addrs
             .into_iter()
@@ -146,6 +157,7 @@ impl AddressBook {
                 meta_addr
             })
             .filter(MetaAddr::address_is_valid_for_outbound)
+            .take(addr_limit)
             .map(|meta_addr| (meta_addr.addr, meta_addr));
 
         for (socket_addr, meta_addr) in addrs {
@@ -270,6 +282,25 @@ impl AddressBook {
             }
 
             self.by_addr.insert(updated.addr, updated);
+
+            // Security: Limit the number of peers in the address book.
+            //
+            // We only delete outdated peers when we have too many peers.
+            // If we deleted them as soon as they became too old,
+            // then other peers could re-insert them into the address book.
+            // And we would start connecting to those outdated peers again,
+            // ignoring the age limit in [`MetaAddr::is_probably_reachable`].
+            while self.by_addr.len() > self.addr_limit {
+                let surplus_peer = self
+                    .peers()
+                    .next_back()
+                    .expect("just checked there is at least one peer");
+
+                self.by_addr.remove(&surplus_peer.addr);
+            }
+
+            assert!(self.len() <= self.addr_limit);
+
             std::mem::drop(_guard);
             self.update_metrics(instant_now, chrono_now);
         }
@@ -320,7 +351,7 @@ impl AddressBook {
     /// Return an iterator over all peers.
     ///
     /// Returns peers in reconnection attempt order, including recently connected peers.
-    pub fn peers(&'_ self) -> impl Iterator<Item = MetaAddr> + '_ {
+    pub fn peers(&'_ self) -> impl Iterator<Item = MetaAddr> + DoubleEndedIterator + '_ {
         let _guard = self.span.enter();
         self.by_addr.descending_values().cloned()
     }
@@ -331,7 +362,7 @@ impl AddressBook {
         &'_ self,
         instant_now: Instant,
         chrono_now: chrono::DateTime<Utc>,
-    ) -> impl Iterator<Item = MetaAddr> + '_ {
+    ) -> impl Iterator<Item = MetaAddr> + DoubleEndedIterator + '_ {
         let _guard = self.span.enter();
 
         // Skip live peers, and peers pending a reconnect attempt.
@@ -344,7 +375,10 @@ impl AddressBook {
 
     /// Return an iterator over all the peers in `state`,
     /// in reconnection attempt order, including recently connected peers.
-    pub fn state_peers(&'_ self, state: PeerAddrState) -> impl Iterator<Item = MetaAddr> + '_ {
+    pub fn state_peers(
+        &'_ self,
+        state: PeerAddrState,
+    ) -> impl Iterator<Item = MetaAddr> + DoubleEndedIterator + '_ {
         let _guard = self.span.enter();
 
         self.by_addr
@@ -359,7 +393,7 @@ impl AddressBook {
         &'_ self,
         instant_now: Instant,
         chrono_now: chrono::DateTime<Utc>,
-    ) -> impl Iterator<Item = MetaAddr> + '_ {
+    ) -> impl Iterator<Item = MetaAddr> + DoubleEndedIterator + '_ {
         let _guard = self.span.enter();
 
         self.by_addr
@@ -373,7 +407,7 @@ impl AddressBook {
     pub fn recently_live_peers(
         &'_ self,
         now: chrono::DateTime<Utc>,
-    ) -> impl Iterator<Item = MetaAddr> + '_ {
+    ) -> impl Iterator<Item = MetaAddr> + DoubleEndedIterator + '_ {
         let _guard = self.span.enter();
 
         self.by_addr

zebra-network/src/address_book/tests/prop.rs

@@ -9,13 +9,15 @@ use tracing::Span;
 use zebra_chain::serialization::Duration32;
 
 use crate::{
-    constants::MAX_PEER_ACTIVE_FOR_GOSSIP,
+    constants::{MAX_ADDRS_IN_ADDRESS_BOOK, MAX_PEER_ACTIVE_FOR_GOSSIP},
-    meta_addr::{arbitrary::MAX_META_ADDR, MetaAddr},
+    meta_addr::{arbitrary::MAX_META_ADDR, MetaAddr, MetaAddrChange},
     AddressBook,
 };
 
 const TIME_ERROR_MARGIN: Duration32 = Duration32::from_seconds(1);
 
+const MAX_ADDR_CHANGE: usize = 10;
+
 proptest! {
     #[test]
     fn only_recently_reachable_are_gossiped(
@@ -25,7 +27,12 @@ proptest! {
         zebra_test::init();
         let chrono_now = Utc::now();
 
-        let address_book = AddressBook::new_with_addrs(local_listener, Span::none(), addresses);
+        let address_book = AddressBook::new_with_addrs(
+            local_listener,
+            MAX_ADDRS_IN_ADDRESS_BOOK,
+            Span::none(),
+            addresses
+        );
 
         for gossiped_address in address_book.sanitized(chrono_now) {
             let duration_since_last_seen = gossiped_address
@@ -48,10 +55,83 @@ proptest! {
         let instant_now = Instant::now();
         let chrono_now = Utc::now();
 
-        let address_book = AddressBook::new_with_addrs(local_listener, Span::none(), addresses);
+        let address_book = AddressBook::new_with_addrs(
+            local_listener,
+            MAX_ADDRS_IN_ADDRESS_BOOK,
+            Span::none(),
+            addresses
+        );
 
         for peer in address_book.reconnection_peers(instant_now, chrono_now) {
             prop_assert!(peer.is_probably_reachable(chrono_now), "peer: {:?}", peer);
         }
     }
+
+    /// Test that the address book limit is respected for multiple peers.
+    #[test]
+    fn address_book_length_is_limited(
+        local_listener in any::<SocketAddr>(),
+        addr_changes_lists in vec(
+            MetaAddrChange::addr_changes_strategy(MAX_ADDR_CHANGE),
+            2..MAX_ADDR_CHANGE
+        ),
+        addr_limit in 0..=MAX_ADDR_CHANGE,
+        pre_fill in any::<bool>(),
+    ) {
+        zebra_test::init();
+
+        let initial_addrs = if pre_fill {
+            addr_changes_lists
+                .iter()
+                .map(|(addr, _changes)| addr)
+                .cloned()
+                .collect()
+        } else {
+            Vec::new()
+        };
+
+        // sequentially apply changes for one address, then move on to the next
+
+        let mut address_book = AddressBook::new_with_addrs(
+            local_listener,
+            addr_limit,
+            Span::none(),
+            initial_addrs.clone(),
+        );
+
+        for (_addr, changes) in addr_changes_lists.iter() {
+            for change in changes {
+                address_book.update(*change);
+
+                prop_assert!(
+                    address_book.len() <= addr_limit,
+                    "sequential test length: {} was greater than limit: {}",
+                    address_book.len(), addr_limit,
+                );
+            }
+        }
+
+        // interleave changes for different addresses
+
+        let mut address_book = AddressBook::new_with_addrs(
+            local_listener,
+            addr_limit,
+            Span::none(),
+            initial_addrs,
+        );
+
+        for index in 0..MAX_ADDR_CHANGE {
+            for (_addr, changes) in addr_changes_lists.iter() {
+                if let Some(change) = changes.get(index) {
+                    address_book.update(*change);
+
+                    prop_assert!(
+                        address_book.len() <= addr_limit,
+                        "interleave test length: {} was greater than limit: {}",
+                        address_book.len(), addr_limit,
+                    );
+                }
+            }
+        }
+    }
 }

zebra-network/src/address_book/tests/vectors.rs

@@ -7,7 +7,10 @@ use tracing::Span;
 
 use zebra_chain::serialization::{DateTime32, Duration32};
 
-use crate::{meta_addr::MetaAddr, protocol::external::types::PeerServices, AddressBook};
+use crate::{
+    constants::MAX_ADDRS_IN_ADDRESS_BOOK, meta_addr::MetaAddr,
+    protocol::external::types::PeerServices, AddressBook,
+};
 
 /// Make sure an empty address book is actually empty.
 #[test]
@@ -39,8 +42,12 @@ fn address_book_peer_order() {
 
     // Regardless of the order of insertion, the most recent address should be chosen first
     let addrs = vec![meta_addr1, meta_addr2];
-    let address_book =
+    let address_book = AddressBook::new_with_addrs(
-        AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs);
+        "0.0.0.0:0".parse().unwrap(),
+        MAX_ADDRS_IN_ADDRESS_BOOK,
+        Span::current(),
+        addrs,
+    );
     assert_eq!(
         address_book
             .reconnection_peers(Instant::now(), Utc::now())
@@ -50,8 +57,12 @@ fn address_book_peer_order() {
 
     // Reverse the order, check that we get the same result
     let addrs = vec![meta_addr2, meta_addr1];
-    let address_book =
+    let address_book = AddressBook::new_with_addrs(
-        AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs);
+        "0.0.0.0:0".parse().unwrap(),
+        MAX_ADDRS_IN_ADDRESS_BOOK,
+        Span::current(),
+        addrs,
+    );
     assert_eq!(
         address_book
             .reconnection_peers(Instant::now(), Utc::now())
@@ -64,8 +75,12 @@ fn address_book_peer_order() {
     meta_addr2.addr = addr1;
 
     let addrs = vec![meta_addr1, meta_addr2];
-    let address_book =
+    let address_book = AddressBook::new_with_addrs(
-        AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs);
+        "0.0.0.0:0".parse().unwrap(),
+        MAX_ADDRS_IN_ADDRESS_BOOK,
+        Span::current(),
+        addrs,
+    );
     assert_eq!(
         address_book
             .reconnection_peers(Instant::now(), Utc::now())
@@ -75,8 +90,12 @@ fn address_book_peer_order() {
 
     // Reverse the order, check that we get the same result
     let addrs = vec![meta_addr2, meta_addr1];
-    let address_book =
+    let address_book = AddressBook::new_with_addrs(
-        AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs);
+        "0.0.0.0:0".parse().unwrap(),
+        MAX_ADDRS_IN_ADDRESS_BOOK,
+        Span::current(),
+        addrs,
+    );
     assert_eq!(
         address_book
             .reconnection_peers(Instant::now(), Utc::now())

zebra-network/src/constants.rs

@@ -138,6 +138,27 @@ pub const GET_ADDR_FANOUT: usize = 3;
 /// https://zips.z.cash/zip-0155#specification
 pub const MAX_ADDRS_IN_MESSAGE: usize = 1000;
 
+/// The fraction of addresses Zebra sends in response to a `Peers` request.
+///
+/// Each response contains approximately:
+/// `address_book.len() / ADDR_RESPONSE_LIMIT_DENOMINATOR`
+/// addresses, selected at random from the address book.
+///
+/// # Security
+///
+/// This limit makes sure that Zebra does not reveal its entire address book
+/// in a single `Peers` response.
+pub const ADDR_RESPONSE_LIMIT_DENOMINATOR: usize = 3;
+
+/// The maximum number of addresses Zebra will keep in its address book.
+///
+/// This is a tradeoff between:
+/// - revealing the whole address book in a few requests,
+/// - sending the maximum number of peer addresses, and
+/// - making sure the limit code actually gets run.
+pub const MAX_ADDRS_IN_ADDRESS_BOOK: usize =
+    MAX_ADDRS_IN_MESSAGE * (ADDR_RESPONSE_LIMIT_DENOMINATOR + 1);
+
 /// Truncate timestamps in outbound address messages to this time interval.
 ///
 /// ## SECURITY
@@ -261,4 +282,40 @@ mod tests {
         assert!(EWMA_DECAY_TIME > REQUEST_TIMEOUT,
                 "The EWMA decay time should be higher than the request timeout, so timed out peers are penalised by the EWMA.");
     }
+
+    /// Make sure that peer age limits are consistent with each other.
+    #[test]
+    fn ensure_peer_age_limits_consistent() {
+        zebra_test::init();
+
+        assert!(
+            MAX_PEER_ACTIVE_FOR_GOSSIP <= MAX_RECENT_PEER_AGE,
+            "we should only gossip peers we are actually willing to try ourselves"
+        );
+    }
+
+    /// Make sure the address limits are consistent with each other.
+    #[test]
+    #[allow(clippy::assertions_on_constants)]
+    fn ensure_address_limits_consistent() {
+        // Zebra 1.0.0-beta.2 address book metrics in December 2021.
+        const TYPICAL_MAINNET_ADDRESS_BOOK_SIZE: usize = 4_500;
+
+        zebra_test::init();
+
+        assert!(
+            MAX_ADDRS_IN_ADDRESS_BOOK >= GET_ADDR_FANOUT * MAX_ADDRS_IN_MESSAGE,
+            "the address book should hold at least a fanout's worth of addresses"
+        );
+
+        assert!(
+            MAX_ADDRS_IN_ADDRESS_BOOK / ADDR_RESPONSE_LIMIT_DENOMINATOR > MAX_ADDRS_IN_MESSAGE,
+            "the address book should hold enough addresses for a full response"
+        );
+
+        assert!(
+            MAX_ADDRS_IN_ADDRESS_BOOK < TYPICAL_MAINNET_ADDRESS_BOOK_SIZE,
+            "the address book limit should actually be used"
+        );
+    }
 }

zebra-network/src/meta_addr/tests/prop.rs

@@ -19,7 +19,7 @@ use tracing::Span;
 use zebra_chain::serialization::DateTime32;
 
 use crate::{
-    constants::{MAX_RECENT_PEER_AGE, MIN_PEER_RECONNECTION_DELAY},
+    constants::{MAX_ADDRS_IN_ADDRESS_BOOK, MAX_RECENT_PEER_AGE, MIN_PEER_RECONNECTION_DELAY},
     meta_addr::{
         arbitrary::{MAX_ADDR_CHANGE, MAX_META_ADDR},
         MetaAddr, MetaAddrChange,
@@ -154,8 +154,12 @@ proptest! {
 
         let chrono_now = Utc::now();
 
-        let address_book =
+        let address_book = AddressBook::new_with_addrs(
-            AddressBook::new_with_addrs(local_listener, Span::none(), address_book_addrs);
+            local_listener,
+            MAX_ADDRS_IN_ADDRESS_BOOK,
+            Span::none(),
+            address_book_addrs
+        );
         let sanitized_addrs = address_book.sanitized(chrono_now);
 
         let expected_local_listener = address_book.local_listener_meta_addr();
@@ -226,6 +230,7 @@ proptest! {
 
         let address_book = Arc::new(std::sync::Mutex::new(AddressBook::new_with_addrs(
             SocketAddr::from_str("0.0.0.0:0").unwrap(),
+            MAX_ADDRS_IN_ADDRESS_BOOK,
             Span::none(),
             addrs,
         )));
@@ -248,9 +253,13 @@ proptest! {
                     attempt_count += 1;
                     prop_assert!(
                         attempt_count <= 1,
-                        "candidate: {:?}, change: {}, now: {:?}, earliest next attempt: {:?}, \
+                        "candidate: {:?},\n \
-                         attempts: {}, live peer interval limit: {}, test time limit: {:?}, \
+                         change: {},\n \
-                         peer change interval: {:?}, original addr was in address book: {}",
+                         now: {:?},\n \
+                         earliest next attempt: {:?},\n \
+                         attempts: {}, live peer interval limit: {},\n \
+                         test time limit: {:?}, peer change interval: {:?},\n \
+                         original addr was in address book: {}\n",
                         candidate_addr,
                         i,
                         Instant::now(),

zebrad/src/components/inbound.rs

@@ -25,7 +25,10 @@ use zebra_state as zs;
 
 use zebra_chain::block::{self, Block};
 use zebra_consensus::chain::VerifyChainError;
-use zebra_network::{constants::MAX_ADDRS_IN_MESSAGE, AddressBook};
+use zebra_network::{
+    constants::{ADDR_RESPONSE_LIMIT_DENOMINATOR, MAX_ADDRS_IN_MESSAGE},
+    AddressBook,
+};
 
 // Re-use the syncer timeouts for consistency.
 use super::{
@@ -39,10 +42,6 @@ mod tests;
 
 use downloads::Downloads as BlockDownloads;
 
-/// A security parameter to return only 1/3 of available addresses as a
-/// response to a `Peers` request.
-const FRAC_OF_AVAILABLE_ADDRESS: f64 = 1. / 3.;
-
 type BlockDownloadPeerSet =
     Buffer<BoxService<zn::Request, zn::Response, zn::BoxError>, zn::Request>;
 type State = Buffer<BoxService<zs::Request, zs::Response, zs::BoxError>, zs::Request>;
@@ -313,9 +312,13 @@ impl Service<zn::Request> for Inbound {
                 let mut peers = peers.sanitized(now);
 
                 // Truncate the list
-                let truncate_at = MAX_ADDRS_IN_MESSAGE
+                //
-                    .min((peers.len() as f64 * FRAC_OF_AVAILABLE_ADDRESS).ceil() as usize);
+                // TODO: replace with div_ceil once it stabilises
-                peers.truncate(truncate_at);
+                //       https://github.com/rust-lang/rust/issues/88581
+                let address_limit = (peers.len() + ADDR_RESPONSE_LIMIT_DENOMINATOR - 1) / ADDR_RESPONSE_LIMIT_DENOMINATOR;
+                let address_limit = MAX_ADDRS_IN_MESSAGE
+                    .min(address_limit);
+                peers.truncate(address_limit);
 
                 if !peers.is_empty() {
                     async { Ok(zn::Response::Peers(peers)) }.boxed()