Skip to content

Commit

Permalink
Security: Limit address book size to limit memory usage (#3162)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@teor2345
teor2345 authored Dec 6, 2021
1 parent 9416729 commit 332afc1
Show file tree
Hide file tree
Showing 6 changed files with 237 additions and 35 deletions.
50 changes: 42 additions & 8 deletions zebra-network/src/address_book.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ use ordered_map::OrderedMap;
use tracing::Span;

use crate::{
meta_addr::MetaAddrChange, protocol::external::canonical_socket_addr, types::MetaAddr,
PeerAddrState,
constants, meta_addr::MetaAddrChange, protocol::external::canonical_socket_addr,
types::MetaAddr, PeerAddrState,
};

#[cfg(test)]
Expand Down Expand Up @@ -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,

Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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 {
Expand All @@ -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()
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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()
}
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
88 changes: 84 additions & 4 deletions zebra-network/src/address_book/tests/prop.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ use tracing::Span;
use zebra_chain::serialization::Duration32;

use crate::{
constants::MAX_PEER_ACTIVE_FOR_GOSSIP,
meta_addr::{arbitrary::MAX_META_ADDR, MetaAddr},
constants::{MAX_ADDRS_IN_ADDRESS_BOOK, MAX_PEER_ACTIVE_FOR_GOSSIP},
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(
Expand All @@ -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
Expand All @@ -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,
);
}
}
}
}
}
37 changes: 28 additions & 9 deletions zebra-network/src/address_book/tests/vectors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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 =
AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs);
let address_book = AddressBook::new_with_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())
Expand All @@ -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 =
AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs);
let address_book = AddressBook::new_with_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())
Expand All @@ -64,8 +75,12 @@ fn address_book_peer_order() {
meta_addr2.addr = addr1;

let addrs = vec![meta_addr1, meta_addr2];
let address_book =
AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs);
let address_book = AddressBook::new_with_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())
Expand All @@ -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 =
AddressBook::new_with_addrs("0.0.0.0:0".parse().unwrap(), Span::current(), addrs);
let address_book = AddressBook::new_with_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())
Expand Down
57 changes: 57 additions & 0 deletions zebra-network/src/constants.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
);
}
}
Loading

0 comments on commit 332afc1

Please sign in to comment.