From 0918663e3ea3ff509e07925fb8d7ad97e7bf807b Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 24 May 2023 06:50:29 +1000 Subject: [PATCH] fix(net): Rate-limit MetaAddrChange::Responded from peers (#6738) * Rate-limit MetaAddrChange::Responded from peers * Document rate-limits on the address book updater channel --- zebra-network/src/peer/handshake.rs | 46 +++++++++++++++------ zebra-network/src/peer_set/candidate_set.rs | 6 +++ zebra-network/src/peer_set/initialize.rs | 6 +++ 3 files changed, 46 insertions(+), 12 deletions(-) diff --git a/zebra-network/src/peer/handshake.rs b/zebra-network/src/peer/handshake.rs index 40c07493d..f6660ac05 100644 --- a/zebra-network/src/peer/handshake.rs +++ b/zebra-network/src/peer/handshake.rs @@ -914,6 +914,10 @@ where // addresses. Otherwise, malicious peers could interfere with the // address book state of other peers by providing their addresses in // `Version` messages. + // + // New alternate peer address and peer responded updates are rate-limited because: + // - opening connections is rate-limited + // - we only send these messages once per handshake let alternate_addrs = connected_addr.get_alternate_addrs(remote_canonical_addr); for alt_addr in alternate_addrs { let alt_addr = MetaAddr::new_alternate(alt_addr, &remote_services); @@ -1010,18 +1014,10 @@ where "addr" => connected_addr.get_transient_addr_label(), ); - if let Some(book_addr) = connected_addr.get_address_book_addr() { - if matches!(msg, Message::Ping(_) | Message::Pong(_)) { - // the collector doesn't depend on network activity, - // so this await should not hang - let _ = inbound_ts_collector - .send(MetaAddr::new_responded( - book_addr, - &remote_services, - )) - .await; - } - } + // # Security + // + // Peer messages are not rate-limited, so we can't send anything + // to a shared channel or do anything expensive here. } Err(err) => { metrics::counter!( @@ -1031,6 +1027,12 @@ where "addr" => connected_addr.get_transient_addr_label(), ); + // # Security + // + // Peer errors are rate-limited because: + // - opening connections is rate-limited + // - the number of connections is limited + // - after the first error, the peer is disconnected if let Some(book_addr) = connected_addr.get_address_book_addr() { let _ = inbound_ts_collector .send(MetaAddr::new_errored(book_addr, remote_services)) @@ -1295,6 +1297,20 @@ async fn send_periodic_heartbeats_run_loop( &remote_services, ) .await?; + + // # Security + // + // Peer heartbeats are rate-limited because: + // - opening connections is rate-limited + // - the number of connections is limited + // - Zebra initiates each heartbeat using a timer + if let Some(book_addr) = connected_addr.get_address_book_addr() { + // the collector doesn't depend on network activity, + // so this await should not hang + let _ = heartbeat_ts_collector + .send(MetaAddr::new_responded(book_addr, &remote_services)) + .await; + } } unreachable!("unexpected IntervalStream termination") @@ -1399,6 +1415,12 @@ where Err(err) => { tracing::debug!(?err, "heartbeat error, shutting down"); + // # Security + // + // Peer errors and shutdowns are rate-limited because: + // - opening connections is rate-limited + // - the number of connections is limited + // - after the first error or shutdown, the peer is disconnected if let Some(book_addr) = connected_addr.get_address_book_addr() { let _ = address_book_updater .send(MetaAddr::new_errored(book_addr, *remote_services)) diff --git a/zebra-network/src/peer_set/candidate_set.rs b/zebra-network/src/peer_set/candidate_set.rs index 042a92b27..76006672c 100644 --- a/zebra-network/src/peer_set/candidate_set.rs +++ b/zebra-network/src/peer_set/candidate_set.rs @@ -316,6 +316,12 @@ where /// Add new `addrs` to the address book. async fn send_addrs(&self, addrs: impl IntoIterator) { + // # Security + // + // New gossiped peers are rate-limited because: + // - Zebra initiates requests for new gossiped peers + // - the fanout is limited + // - the number of addresses per peer is limited let addrs: Vec = addrs .into_iter() .map(MetaAddr::new_gossiped_change) diff --git a/zebra-network/src/peer_set/initialize.rs b/zebra-network/src/peer_set/initialize.rs index f9b002f94..31c5a6625 100644 --- a/zebra-network/src/peer_set/initialize.rs +++ b/zebra-network/src/peer_set/initialize.rs @@ -433,6 +433,12 @@ async fn limit_initial_peers( // Send every initial peer to the address book, in preferred order. // (This treats initial peers the same way we treat gossiped peers.) + // + // # Security + // + // Initial peers are limited because: + // - the number of initial peers is limited + // - this code only runs once at startup for peer in preferred_peers.values().flatten() { let peer_addr = MetaAddr::new_initial_peer(*peer); // `send` only waits when the channel is full.