From deee62d7a416330866953fd17d8799f73133f7f7 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 27 Mar 2023 12:06:34 +1000 Subject: [PATCH 1/7] Close the new connection if Zebra unexpectedly generates a duplicate random nonce --- zebra-network/src/peer/error.rs | 6 +++++- zebra-network/src/peer/handshake.rs | 16 +++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/zebra-network/src/peer/error.rs b/zebra-network/src/peer/error.rs index cc0abb271e3..0180c377d6b 100644 --- a/zebra-network/src/peer/error.rs +++ b/zebra-network/src/peer/error.rs @@ -226,7 +226,11 @@ pub enum HandshakeError { UnexpectedMessage(Box), /// The peer connector detected handshake nonce reuse, possibly indicating self-connection. #[error("Detected nonce reuse, possible self-connection")] - NonceReuse, + RemoteNonceReuse, + /// The peer connector created a duplicate random nonce. This is very unlikely, + /// because the range of the data type is 2^64. + #[error("Unexpectedly created a duplicate random local nonce")] + LocalDuplicateNonce, /// The remote peer closed the connection. #[error("Peer closed connection")] ConnectionClosed, diff --git a/zebra-network/src/peer/handshake.rs b/zebra-network/src/peer/handshake.rs index cb4793e95a7..90ffbd4ab6d 100644 --- a/zebra-network/src/peer/handshake.rs +++ b/zebra-network/src/peer/handshake.rs @@ -595,13 +595,15 @@ where // It is ok to wait for the lock here, because handshakes have a short // timeout, and the async mutex will be released when the task times // out. - // - // Duplicate nonces don't matter here, because 64-bit random collisions are very rare. - // If they happen, we're probably replacing a leftover nonce from a failed connection, - // which wasn't cleaned up when it closed. { let mut locked_nonces = nonces.lock().await; - locked_nonces.insert(local_nonce); + + // Duplicate nonces are very rare, because they require a 64-bit random number collision, + // and the nonce set is limited to a few hundred entries. + let is_unique_nonce = locked_nonces.insert(local_nonce); + if !is_unique_nonce { + return Err(HandshakeError::LocalDuplicateNonce); + } // # Security // @@ -615,7 +617,7 @@ where // - avoiding memory denial of service attacks which make large numbers of connections, // for example, 100 failed inbound connections takes 1 second. // - memory usage: 16 bytes per `Nonce`, 3.2 kB for 200 nonces - // - collision probability: 2^32 has ~50% collision probability, so we use a lower limit + // - collision probability: two hundred 64-bit nonces have a very low collision probability // while locked_nonces.len() > config.peerset_total_connection_limit() { locked_nonces.shift_remove_index(0); @@ -731,7 +733,7 @@ where nonce_reuse }; if nonce_reuse { - Err(HandshakeError::NonceReuse)?; + Err(HandshakeError::RemoteNonceReuse)?; } // SECURITY: Reject connections to peers on old versions, because they might not know about all From cc6adde44676c9e64ae0e5fa2ccaf5a2fc17de31 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 27 Mar 2023 13:37:37 +1000 Subject: [PATCH 2/7] Add a missing test module comment --- zebra-network/src/peer_set/set/tests/vectors.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/zebra-network/src/peer_set/set/tests/vectors.rs b/zebra-network/src/peer_set/set/tests/vectors.rs index 742db317d7d..1f58f0f0b0f 100644 --- a/zebra-network/src/peer_set/set/tests/vectors.rs +++ b/zebra-network/src/peer_set/set/tests/vectors.rs @@ -1,3 +1,5 @@ +//! Fixed test vectors for the peer set. + use std::{iter, time::Duration}; use tokio::time::timeout; From 2f32893264563c37766d868c7a53a0439fd6fb20 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 27 Mar 2023 13:37:55 +1000 Subject: [PATCH 3/7] Avoid peer attacks that replay self-connection nonces to manipulate the nonce set (needs tests) --- zebra-network/src/peer/handshake.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/zebra-network/src/peer/handshake.rs b/zebra-network/src/peer/handshake.rs index 90ffbd4ab6d..424fd504e32 100644 --- a/zebra-network/src/peer/handshake.rs +++ b/zebra-network/src/peer/handshake.rs @@ -595,6 +595,8 @@ where // It is ok to wait for the lock here, because handshakes have a short // timeout, and the async mutex will be released when the task times // out. + // + // START CRITICAL SECTION { let mut locked_nonces = nonces.lock().await; @@ -624,7 +626,8 @@ where } std::mem::drop(locked_nonces); - }; + } + // END CRITICAL SECTION // Don't leak our exact clock skew to our peers. On the other hand, // we can't deviate too much, or zcashd will get confused. @@ -723,20 +726,16 @@ where // // # Security // - // Connections that get a `Version` message will remove their nonces here, - // but connections that fail before this point can leave their nonces in the nonce set. - let nonce_reuse = { - let mut locked_nonces = nonces.lock().await; - let nonce_reuse = locked_nonces.contains(&remote.nonce); - // Regardless of whether we observed nonce reuse, remove our own nonce from the nonce set. - locked_nonces.remove(&local_nonce); - nonce_reuse - }; + // We don't remove the nonce here, because peers that observe our network traffic could + // maliciously remove nonces, and force us to make self-connections. + let nonce_reuse = nonces.lock().await.contains(&remote.nonce); if nonce_reuse { Err(HandshakeError::RemoteNonceReuse)?; } - // SECURITY: Reject connections to peers on old versions, because they might not know about all + // # Security + // + // Reject connections to peers on old versions, because they might not know about all // network upgrades and could lead to chain forks or slower block propagation. let min_version = minimum_peer_version.current(); if remote.version < min_version { From c273db20a79248df85c5b9b2915e7841844b2bb5 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 19 Apr 2023 12:23:42 +1000 Subject: [PATCH 4/7] Add a test that makes sure network self-connections fail --- zebra-network/src/address_book.rs | 6 + .../src/peer_set/initialize/tests/vectors.rs | 160 ++++++++++++++++-- 2 files changed, 153 insertions(+), 13 deletions(-) diff --git a/zebra-network/src/address_book.rs b/zebra-network/src/address_book.rs index e6cfb9d2d4e..b5c9e97364c 100644 --- a/zebra-network/src/address_book.rs +++ b/zebra-network/src/address_book.rs @@ -209,6 +209,12 @@ impl AddressBook { self.address_metrics_tx.subscribe() } + /// Set the local listener address. Only for use in tests. + #[cfg(any(test, feature = "proptest-impl"))] + pub fn set_local_listener(&mut self, addr: SocketAddr) { + self.local_listener = addr; + } + /// Get the local listener address. /// /// This address contains minimal state, but it is not sanitized. diff --git a/zebra-network/src/peer_set/initialize/tests/vectors.rs b/zebra-network/src/peer_set/initialize/tests/vectors.rs index ff249feef7c..c285de67804 100644 --- a/zebra-network/src/peer_set/initialize/tests/vectors.rs +++ b/zebra-network/src/peer_set/initialize/tests/vectors.rs @@ -32,7 +32,7 @@ use zebra_test::net::random_known_port; use crate::{ address_book_updater::AddressBookUpdater, constants, init, - meta_addr::MetaAddr, + meta_addr::{MetaAddr, PeerAddrState}, peer::{self, ClientTestHarness, HandshakeRequest, OutboundConnectorRequest}, peer_set::{ initialize::{ @@ -180,7 +180,8 @@ async fn peer_limit_zero_mainnet() { let unreachable_inbound_service = service_fn(|_| async { unreachable!("inbound service should never be called") }); - let address_book = init_with_peer_limit(0, unreachable_inbound_service, Mainnet).await; + let address_book = + init_with_peer_limit(0, unreachable_inbound_service, Mainnet, None, None).await; assert_eq!( address_book.lock().unwrap().peers().count(), 0, @@ -201,7 +202,8 @@ async fn peer_limit_zero_testnet() { let unreachable_inbound_service = service_fn(|_| async { unreachable!("inbound service should never be called") }); - let address_book = init_with_peer_limit(0, unreachable_inbound_service, Testnet).await; + let address_book = + init_with_peer_limit(0, unreachable_inbound_service, Testnet, None, None).await; assert_eq!( address_book.lock().unwrap().peers().count(), 0, @@ -221,7 +223,7 @@ async fn peer_limit_one_mainnet() { let nil_inbound_service = service_fn(|_| async { Ok(Response::Nil) }); - let _ = init_with_peer_limit(1, nil_inbound_service, Mainnet).await; + let _ = init_with_peer_limit(1, nil_inbound_service, Mainnet, None, None).await; // Let the crawler run for a while. tokio::time::sleep(CRAWLER_TEST_DURATION).await; @@ -240,7 +242,7 @@ async fn peer_limit_one_testnet() { let nil_inbound_service = service_fn(|_| async { Ok(Response::Nil) }); - let _ = init_with_peer_limit(1, nil_inbound_service, Testnet).await; + let _ = init_with_peer_limit(1, nil_inbound_service, Testnet, None, None).await; // Let the crawler run for a while. tokio::time::sleep(CRAWLER_TEST_DURATION).await; @@ -259,7 +261,7 @@ async fn peer_limit_two_mainnet() { let nil_inbound_service = service_fn(|_| async { Ok(Response::Nil) }); - let _ = init_with_peer_limit(2, nil_inbound_service, Mainnet).await; + let _ = init_with_peer_limit(2, nil_inbound_service, Mainnet, None, None).await; // Let the crawler run for a while. tokio::time::sleep(CRAWLER_TEST_DURATION).await; @@ -278,7 +280,7 @@ async fn peer_limit_two_testnet() { let nil_inbound_service = service_fn(|_| async { Ok(Response::Nil) }); - let _ = init_with_peer_limit(2, nil_inbound_service, Testnet).await; + let _ = init_with_peer_limit(2, nil_inbound_service, Testnet, None, None).await; // Let the crawler run for a while. tokio::time::sleep(CRAWLER_TEST_DURATION).await; @@ -1095,6 +1097,128 @@ async fn add_initial_peers_is_rate_limited() { ); } +/// Test that self-connections fail. +// +// TODO: +// - add a unit test that makes sure the error is a nonce reuse error +// - add a unit test that makes sure connections that replay nonces also get rejected +#[tokio::test] +async fn self_connections_should_fail() { + let _init_guard = zebra_test::init(); + + // This test requires an IPv4 network stack with 127.0.0.1 as localhost. + if zebra_test::net::zebra_skip_network_tests() { + return; + } + + const TEST_PEERSET_INITIAL_TARGET_SIZE: usize = 3; + const TEST_CRAWL_NEW_PEER_INTERVAL: Duration = Duration::from_secs(1); + + // If we get an inbound request from a peer, the test has a bug, + // because self-connections should fail at the handshake stage. + let unreachable_inbound_service = + service_fn(|_| async { unreachable!("inbound service should never be called") }); + + let force_listen_addr: SocketAddr = "127.0.0.1:0".parse().unwrap(); + + let no_initial_peers_config = Config { + crawl_new_peer_interval: TEST_CRAWL_NEW_PEER_INTERVAL, + + initial_mainnet_peers: IndexSet::new(), + initial_testnet_peers: IndexSet::new(), + + ..Config::default() + }; + + let address_book = init_with_peer_limit( + TEST_PEERSET_INITIAL_TARGET_SIZE, + unreachable_inbound_service, + Mainnet, + force_listen_addr, + no_initial_peers_config, + ) + .await; + + // Insert our own address into the address book, and make sure it works + // BEGIN CRITICAL SECTION + let real_self_listener = { + let mut unlocked_address_book = address_book + .lock() + .expect("unexpected panic in address book"); + + let real_self_listener = unlocked_address_book.local_listener_meta_addr(); + + // Set a fake listener to get past the check for adding our own address + unlocked_address_book.set_local_listener("192.168.0.0:1".parse().unwrap()); + + let updated_addr = unlocked_address_book.update( + real_self_listener + .new_gossiped_change() + .expect("change is valid"), + ); + + std::mem::drop(unlocked_address_book); + // END CRITICAL SECTION + + // Make sure we modified the address book correctly + assert!( + updated_addr.is_some(), + "inserting our own address into the address book failed: {real_self_listener:?}" + ); + assert_eq!( + updated_addr.unwrap().addr(), + real_self_listener.addr(), + "wrong address inserted into address book" + ); + assert_ne!( + updated_addr.unwrap().addr().ip(), + Ipv4Addr::UNSPECIFIED, + "invalid address inserted into address book: ip must be valid for inbound connections" + ); + assert_ne!( + updated_addr.unwrap().addr().port(), + 0, + "invalid address inserted into address book: port must be valid for inbound connections" + ); + + real_self_listener + }; + + // Wait until the crawler has tried at least one self-connection + tokio::time::sleep(TEST_CRAWL_NEW_PEER_INTERVAL * 3).await; + + // Check that the self-connection failed + // BEGIN CRITICAL SECTION + let self_connection_status = { + let mut unlocked_address_book = address_book + .lock() + .expect("unexpected panic in address book"); + + let self_connection_status = unlocked_address_book + .get(&real_self_listener.addr()) + .expect("unexpected dropped listener address in address book"); + + std::mem::drop(unlocked_address_book); + // END CRITICAL SECTION + + self_connection_status + }; + + // Make sure we fetched from the address book correctly + assert_eq!( + self_connection_status.addr(), + real_self_listener.addr(), + "wrong address fetched from address book" + ); + + // Make sure the self-connection failed + assert_eq!( + self_connection_status.last_connection_state, + PeerAddrState::Failed, + "self-connection should have failed" + ); +} + /// Test that the number of nonces is limited when peers send an invalid response or /// if handshakes time out and are dropped. #[tokio::test] @@ -1103,7 +1227,10 @@ async fn remnant_nonces_from_outbound_connections_are_limited() { let _init_guard = zebra_test::init(); - // This test should not require network access. + // This test requires an IPv4 network stack with 127.0.0.1 as localhost. + if zebra_test::net::zebra_skip_network_tests() { + return; + } const TEST_PEERSET_INITIAL_TARGET_SIZE: usize = 3; @@ -1273,14 +1400,19 @@ async fn local_listener_port_with(listen_addr: SocketAddr, network: Network) { ); } -/// Initialize a peer set with `peerset_initial_target_size` and `inbound_service` on `network`. -/// Returns the newly created [`AddressBook`] for testing. +/// Initialize a peer set with`peerset_initial_target_size`, `inbound_service`, and `network`. /// -/// Binds the network listener to an unused port on all network interfaces. +/// If `force_listen_addr` is set, binds the network listener to that address. +/// Otherwise, binds the network listener to an unused port on all network interfaces. +/// Uses `default_config` or Zebra's defaults for the rest of the configuration. +/// +/// Returns the newly created [`AddressBook`] for testing. async fn init_with_peer_limit( peerset_initial_target_size: usize, inbound_service: S, network: Network, + force_listen_addr: impl Into>, + default_config: impl Into>, ) -> Arc> where S: Service + Clone + Send + 'static, @@ -1290,13 +1422,15 @@ where // (localhost should be enough). let unused_v4 = "0.0.0.0:0".parse().unwrap(); + let default_config = default_config.into().unwrap_or_default(); + let config = Config { peerset_initial_target_size, network, - listen_addr: unused_v4, + listen_addr: force_listen_addr.into().unwrap_or(unused_v4), - ..Config::default() + ..default_config }; let (_peer_service, address_book) = init(config, inbound_service, NoChainTip).await; From 2f29a3da4ee7e44f3ddb52781ef280758a91b1b4 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 19 Apr 2023 12:32:32 +1000 Subject: [PATCH 5/7] Log an info level when self-connections fail (this should be rare) --- zebra-network/src/peer/handshake.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra-network/src/peer/handshake.rs b/zebra-network/src/peer/handshake.rs index 424fd504e32..ead83fad185 100644 --- a/zebra-network/src/peer/handshake.rs +++ b/zebra-network/src/peer/handshake.rs @@ -730,6 +730,7 @@ where // maliciously remove nonces, and force us to make self-connections. let nonce_reuse = nonces.lock().await.contains(&remote.nonce); if nonce_reuse { + info!(?connected_addr, "rejecting self-connection attempt"); Err(HandshakeError::RemoteNonceReuse)?; } From b281cb6d0770d1d74408b5af59acd4bcec780627 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 24 Apr 2023 09:42:34 +1000 Subject: [PATCH 6/7] Just use plain blocks for mutex critical sections --- zebra-network/src/peer/handshake.rs | 3 -- .../src/peer_set/initialize/tests/vectors.rs | 50 +++++++++---------- 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/zebra-network/src/peer/handshake.rs b/zebra-network/src/peer/handshake.rs index ead83fad185..9dd338ae565 100644 --- a/zebra-network/src/peer/handshake.rs +++ b/zebra-network/src/peer/handshake.rs @@ -595,8 +595,6 @@ where // It is ok to wait for the lock here, because handshakes have a short // timeout, and the async mutex will be released when the task times // out. - // - // START CRITICAL SECTION { let mut locked_nonces = nonces.lock().await; @@ -627,7 +625,6 @@ where std::mem::drop(locked_nonces); } - // END CRITICAL SECTION // Don't leak our exact clock skew to our peers. On the other hand, // we can't deviate too much, or zcashd will get confused. diff --git a/zebra-network/src/peer_set/initialize/tests/vectors.rs b/zebra-network/src/peer_set/initialize/tests/vectors.rs index c285de67804..c55cbbd3e49 100644 --- a/zebra-network/src/peer_set/initialize/tests/vectors.rs +++ b/zebra-network/src/peer_set/initialize/tests/vectors.rs @@ -1140,8 +1140,7 @@ async fn self_connections_should_fail() { .await; // Insert our own address into the address book, and make sure it works - // BEGIN CRITICAL SECTION - let real_self_listener = { + let (real_self_listener, updated_addr) = { let mut unlocked_address_book = address_book .lock() .expect("unexpected panic in address book"); @@ -1158,37 +1157,35 @@ async fn self_connections_should_fail() { ); std::mem::drop(unlocked_address_book); - // END CRITICAL SECTION - // Make sure we modified the address book correctly - assert!( - updated_addr.is_some(), - "inserting our own address into the address book failed: {real_self_listener:?}" - ); - assert_eq!( - updated_addr.unwrap().addr(), - real_self_listener.addr(), - "wrong address inserted into address book" - ); - assert_ne!( - updated_addr.unwrap().addr().ip(), - Ipv4Addr::UNSPECIFIED, - "invalid address inserted into address book: ip must be valid for inbound connections" - ); - assert_ne!( - updated_addr.unwrap().addr().port(), - 0, - "invalid address inserted into address book: port must be valid for inbound connections" - ); - - real_self_listener + (real_self_listener, updated_addr) }; + // Make sure we modified the address book correctly + assert!( + updated_addr.is_some(), + "inserting our own address into the address book failed: {real_self_listener:?}" + ); + assert_eq!( + updated_addr.unwrap().addr(), + real_self_listener.addr(), + "wrong address inserted into address book" + ); + assert_ne!( + updated_addr.unwrap().addr().ip(), + Ipv4Addr::UNSPECIFIED, + "invalid address inserted into address book: ip must be valid for inbound connections" + ); + assert_ne!( + updated_addr.unwrap().addr().port(), + 0, + "invalid address inserted into address book: port must be valid for inbound connections" + ); + // Wait until the crawler has tried at least one self-connection tokio::time::sleep(TEST_CRAWL_NEW_PEER_INTERVAL * 3).await; // Check that the self-connection failed - // BEGIN CRITICAL SECTION let self_connection_status = { let mut unlocked_address_book = address_book .lock() @@ -1199,7 +1196,6 @@ async fn self_connections_should_fail() { .expect("unexpected dropped listener address in address book"); std::mem::drop(unlocked_address_book); - // END CRITICAL SECTION self_connection_status }; From 23253116acd622de11765bed46a9d2e12ad1340b Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 24 Apr 2023 09:42:58 +1000 Subject: [PATCH 7/7] Add a missing space --- zebra-network/src/peer_set/initialize/tests/vectors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-network/src/peer_set/initialize/tests/vectors.rs b/zebra-network/src/peer_set/initialize/tests/vectors.rs index c55cbbd3e49..62955c61bea 100644 --- a/zebra-network/src/peer_set/initialize/tests/vectors.rs +++ b/zebra-network/src/peer_set/initialize/tests/vectors.rs @@ -1396,7 +1396,7 @@ async fn local_listener_port_with(listen_addr: SocketAddr, network: Network) { ); } -/// Initialize a peer set with`peerset_initial_target_size`, `inbound_service`, and `network`. +/// Initialize a peer set with `peerset_initial_target_size`, `inbound_service`, and `network`. /// /// If `force_listen_addr` is set, binds the network listener to that address. /// Otherwise, binds the network listener to an unused port on all network interfaces.