From 11b5a3365181fd905e1d6c1928aeb6528498f7a8 Mon Sep 17 00:00:00 2001 From: Janito Vaqueiro Ferreira Filho Date: Wed, 10 Nov 2021 20:51:22 -0300 Subject: [PATCH] Security: Avoid reconnecting to peers that are likely unreachable (#3030) * Add a `Duration32::from_days` constructor Make it simpler to construct a `Duration32` representing a certain number of days. * Add `MetaAddr::was_not_recently_seen` method A helper method to check if a peer was never seen before or if it was last seen a long time ago. This will be one of the conditions to consider a peer as unreachable. * Add `MetaAddr::is_probably_unreachable` method A helper method to check if a peer should be considered unreachable. It is considered unreachable if recent connection attempts have failed and it was not recently seen. If a peer is considered unreachable, Zebra shouldn't attempt to connect to it again. * Do not keep trying to connect to unreachable peer A peer is probably unreachable if it was last seen a long time ago and if it's last connection attempt failed. * Test `was_not_recently_seen` Redo the calculation on arbitrary `MetaAddr`s. * Test `is_probably_unreachable` Redo the calculation on arbitrary `MetaAddr`s. * Test if probably unreachable peers are ignored Given an `AddressBook` with a list of arbitrary `MetaAddr`s, check that none of the peers listed for a reconnection is probably unreachable. * Rename unit test to improve clarity Remove the double negative from the name. Co-authored-by: teor * Rename constant to `MAX_RECENT_PEER_AGE` Make the purpose of the constant clearer. Co-authored-by: teor * Rename method to `last_seen_is_recent` Remove the double negative from the name. * Rename method to `is_probably_reachable` Avoid having to negate the result of the method in security critical filter. * Move check into `is_ready_for_connection_attempt` Make sure the check is used in any place that requires a peer that's ready for a connection attempt. * Improve test documention Describe the goal of the test better. Co-authored-by: teor * Improve `is_probably_reachable` documentation List the conditions as bullet points. Co-authored-by: teor * Document what happens when peers have no last seen time Co-authored-by: teor --- zebra-chain/src/serialization/date_time.rs | 8 ++++ zebra-network/src/address_book/tests/prop.rs | 15 +++++++ zebra-network/src/constants.rs | 9 +++++ zebra-network/src/meta_addr.rs | 33 ++++++++++++++++ zebra-network/src/meta_addr/tests/prop.rs | 41 +++++++++++++++++++- 5 files changed, 105 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/serialization/date_time.rs b/zebra-chain/src/serialization/date_time.rs index d5de36acfbc..bfd7228209c 100644 --- a/zebra-chain/src/serialization/date_time.rs +++ b/zebra-chain/src/serialization/date_time.rs @@ -138,6 +138,14 @@ impl Duration32 { Duration32::from_minutes(hours.saturating_mul(60)) } + /// Creates a new [`Duration32`] to represent the given amount of days. + /// + /// If the resulting number of seconds does not fit in a [`u32`], [`Duration32::MAX`] is + /// returned. + pub const fn from_days(days: u32) -> Self { + Duration32::from_hours(days.saturating_mul(24)) + } + /// Returns the number of seconds in this duration. pub fn seconds(&self) -> u32 { self.seconds diff --git a/zebra-network/src/address_book/tests/prop.rs b/zebra-network/src/address_book/tests/prop.rs index 21c6b3ed53d..8e0c2a008e6 100644 --- a/zebra-network/src/address_book/tests/prop.rs +++ b/zebra-network/src/address_book/tests/prop.rs @@ -33,4 +33,19 @@ proptest! { prop_assert!(duration_since_last_seen <= MAX_PEER_ACTIVE_FOR_GOSSIP); } } + + /// Test that only peers that are reachable are listed for reconnection attempts. + #[test] + fn only_reachable_addresses_are_attempted( + local_listener in any::(), + addresses in vec(any::(), 0..MAX_META_ADDR), + ) { + zebra_test::init(); + + let address_book = AddressBook::new_with_addrs(local_listener, Span::none(), addresses); + + for peer in address_book.reconnection_peers() { + prop_assert!(peer.is_probably_reachable(), "peer: {:?}", peer); + } + } } diff --git a/zebra-network/src/constants.rs b/zebra-network/src/constants.rs index 9c59e9d9358..9753073d56f 100644 --- a/zebra-network/src/constants.rs +++ b/zebra-network/src/constants.rs @@ -78,6 +78,15 @@ pub const MIN_PEER_RECONNECTION_DELAY: Duration = Duration::from_secs(60 + 20 + /// within the last three hours." pub const MAX_PEER_ACTIVE_FOR_GOSSIP: Duration32 = Duration32::from_hours(3); +/// The maximum duration since a peer was last seen to consider reconnecting to it. +/// +/// Peers that haven't been seen for more than three days and that had its last connection attempt +/// fail are considered to be offline and Zebra will stop trying to connect to them. +/// +/// This is to ensure that Zebra can't have a denial-of-service as a consequence of having too many +/// offline peers that it constantly and uselessly retries to connect to. +pub const MAX_RECENT_PEER_AGE: Duration32 = Duration32::from_days(3); + /// Regular interval for sending keepalive `Ping` messages to each /// connected peer. pub const HEARTBEAT_INTERVAL: Duration = Duration::from_secs(60); diff --git a/zebra-network/src/meta_addr.rs b/zebra-network/src/meta_addr.rs index d0ed86fd952..0378869ee16 100644 --- a/zebra-network/src/meta_addr.rs +++ b/zebra-network/src/meta_addr.rs @@ -501,6 +501,7 @@ impl MetaAddr { && !self.has_connection_recently_responded() && !self.was_connection_recently_attempted() && !self.has_connection_recently_failed() + && self.is_probably_reachable() } /// Is the [`SocketAddr`] we have for this peer valid for outbound @@ -528,6 +529,38 @@ impl MetaAddr { is_node && self.address_is_valid_for_outbound() } + /// Should this peer considered reachable? + /// + /// A peer is probably reachable if: + /// - it has never been attempted, or + /// - the last connection attempt was successful, or + /// - the last successful connection was less than 3 days ago. + /// + /// # Security + /// + /// This is used by [`Self::is_ready_for_connection_attempt`] so that Zebra stops trying to + /// connect to peers that are likely unreachable. + /// + /// The `untrusted_last_seen` time is used as a fallback time if the local node has never + /// itself seen the peer. If the reported last seen time is a long time ago or `None`, then the local + /// node will attempt to connect the peer once, and if that attempt fails it won't + /// try to connect ever again. (The state can't be `Failed` until after the first connection attempt.) + pub fn is_probably_reachable(&self) -> bool { + self.last_connection_state != PeerAddrState::Failed || self.last_seen_is_recent() + } + + /// Was this peer last seen recently? + /// + /// Returns `true` if this peer was last seen at most + /// [`MAX_RECENT_PEER_AGE`][constants::MAX_RECENT_PEER_AGE] ago. + /// Returns false if the peer is outdated, or it has no last seen time. + pub fn last_seen_is_recent(&self) -> bool { + match self.last_seen() { + Some(last_seen) => last_seen.saturating_elapsed() <= constants::MAX_RECENT_PEER_AGE, + None => false, + } + } + /// Return a sanitized version of this `MetaAddr`, for sending to a remote peer. /// /// Returns `None` if this `MetaAddr` should not be sent to remote peers. diff --git a/zebra-network/src/meta_addr/tests/prop.rs b/zebra-network/src/meta_addr/tests/prop.rs index 821b693d940..bd9ad0809ba 100644 --- a/zebra-network/src/meta_addr/tests/prop.rs +++ b/zebra-network/src/meta_addr/tests/prop.rs @@ -15,8 +15,10 @@ use tokio::{runtime, time::Instant}; use tower::service_fn; use tracing::Span; +use zebra_chain::serialization::DateTime32; + use crate::{ - constants::MIN_PEER_RECONNECTION_DELAY, + constants::{MAX_RECENT_PEER_AGE, MIN_PEER_RECONNECTION_DELAY}, meta_addr::{ arbitrary::{MAX_ADDR_CHANGE, MAX_META_ADDR}, MetaAddr, MetaAddrChange, @@ -356,4 +358,41 @@ proptest! { prop_assert!(max_attempts - min_attempts <= 1); } } + + /// Make sure check if a peer was recently seen is correct. + #[test] + fn last_seen_is_recent_is_correct(peer in any::()) { + let time_since_last_seen = peer + .last_seen() + .map(|last_seen| last_seen.saturating_elapsed()); + + let recently_seen = time_since_last_seen + .map(|elapsed| elapsed <= MAX_RECENT_PEER_AGE) + .unwrap_or(false); + + prop_assert_eq!( + peer.last_seen_is_recent(), + recently_seen, + "last seen: {:?}, now: {:?}", + peer.last_seen(), + DateTime32::now(), + ); + } + + /// Make sure a peer is correctly determined to be probably reachable. + #[test] + fn probably_rechable_is_determined_correctly(peer in any::()) { + let last_attempt_failed = peer.last_connection_state == Failed; + let not_recently_seen = !peer.last_seen_is_recent(); + + let probably_unreachable = last_attempt_failed && not_recently_seen; + + prop_assert_eq!( + peer.is_probably_reachable(), + !probably_unreachable, + "last_connection_state: {:?}, last_seen: {:?}", + peer.last_connection_state, + peer.last_seen() + ); + } }