From 994a61bfb340e4822e1177ae49a36acb79d20d2e Mon Sep 17 00:00:00 2001 From: Age Manning Date: Tue, 8 Oct 2024 14:26:11 +1100 Subject: [PATCH] Respond to requests from peers with invalid ENRs (#265) * Respond to requests from peers with invalid ENRs * Remove old test function * Remove uncontactable peers from routing table --- README.md | 12 +---- src/handler/mod.rs | 99 ++++++++---------------------------------- src/handler/session.rs | 8 ---- src/handler/tests.rs | 53 +--------------------- src/service.rs | 8 ++++ 5 files changed, 29 insertions(+), 151 deletions(-) diff --git a/README.md b/README.md index 3ce88a19d..039f06916 100644 --- a/README.md +++ b/README.md @@ -80,11 +80,6 @@ A simple example of creating this service is as follows: # Addresses in ENRs -This protocol will drop messages (i.e not respond to requests) from peers that -advertise non-contactable address in their ENR (e.g `127.0.0.1` when connecting -to non-local nodes). This section -explains the rationale behind this design decision. - An ENR is a signed record which is primarily used in this protocol for identifying and connecting to peers. ENRs have **OPTIONAL** `ip` and `port` fields. @@ -127,8 +122,5 @@ This is done in the following way: 3. If a peer connects to us with an ENR that specifies an IP address that does not match the src socket it connects to us on (e.g `127.0.0.1`, or potentially some internal subnet IP that is unreachable from our current - network) we consider this peer malicious/faulty - and drop all packets. This way we can efficiently drop peers that may try to - get us to send messages to arbitrary remote IPs, and we can be sure that all - ENRs in our routing table are contactable (at least by our local node at - some point in time). + network) we consider this ENR invalid and will not add it to our routing + table. However we will still respond to discovery requests. diff --git a/src/handler/mod.rs b/src/handler/mod.rs index f67f8c3c4..d79c0c0c9 100644 --- a/src/handler/mod.rs +++ b/src/handler/mod.rs @@ -74,12 +74,6 @@ use session::Session; // seconds). const BANNED_NODES_CHECK: u64 = 300; // Check every 5 minutes. -// The one-time session timeout. -const ONE_TIME_SESSION_TIMEOUT: u64 = 30; - -// The maximum number of established one-time sessions to maintain. -const ONE_TIME_SESSION_CACHE_CAPACITY: usize = 100; - /// Messages sent from the application layer to `Handler`. #[derive(Debug, Clone, PartialEq)] #[allow(clippy::large_enum_variant)] @@ -216,8 +210,6 @@ pub struct Handler { active_challenges: HashMapDelay, /// Established sessions with peers. sessions: LruTimeCache, - /// Established sessions with peers for a specific request, stored just one per node. - one_time_sessions: LruTimeCache, /// The channel to receive messages from the application layer. service_recv: mpsc::UnboundedReceiver, /// The channel to send messages to the application layer. @@ -308,10 +300,6 @@ impl Handler { config.session_timeout, Some(config.session_cache_capacity), ), - one_time_sessions: LruTimeCache::new( - Duration::from_secs(ONE_TIME_SESSION_TIMEOUT), - Some(ONE_TIME_SESSION_CACHE_CAPACITY), - ), active_challenges: HashMapDelay::new(config.request_timeout), service_recv, service_send, @@ -549,9 +537,6 @@ impl Handler { // Check for an established session let packet = if let Some(session) = self.sessions.get_mut(&node_address) { session.encrypt_message::

(self.node_id, &response.encode()) - } else if let Some(mut session) = self.remove_one_time_session(&node_address, &response.id) - { - session.encrypt_message::

(self.node_id, &response.encode()) } else { // Either the session is being established or has expired. We simply drop the // response in this case. @@ -847,7 +832,7 @@ impl Handler { ephem_pubkey, enr_record, ) { - Ok((mut session, enr)) => { + Ok((session, enr)) => { // Remove the expected response for the challenge. self.remove_expected_response(node_address.socket_addr); // Receiving an AuthResponse must give us an up-to-date view of the node ENR. @@ -868,28 +853,17 @@ impl Handler { { warn!(error = %e, "Failed to inform of established session") } - // When (re-)establishing a session from an outgoing challenge, we do not need - // to filter out this request from active requests, so we do not pass - // the message nonce on to `new_session`. - self.new_session::

(node_address.clone(), session, None) - .await; - self.handle_message( - node_address.clone(), - message_nonce, - message, - authenticated_data, - ) - .await; } else { - // IP's or NodeAddress don't match. Drop the session. + // IP's or NodeAddress don't match. + // + // We still handle the request, but we do not add the ENR to our routing + // table or consider the ENR valid. warn!( udp4_socket = ?enr.udp4_socket(), udp6_socket = ?enr.udp6_socket(), expected = %node_address, "Session has invalid ENR", ); - self.fail_session(&node_address, RequestError::InvalidRemoteEnr, true) - .await; // The ENR doesn't verify. Notify application. self.notify_unverifiable_enr( @@ -898,39 +872,20 @@ impl Handler { node_address.node_id, ) .await; - - // Respond to PING request even if the ENR or NodeAddress don't match - // so that the source node can notice its external IP address has been changed. - let maybe_ping_request = match session.decrypt_message( - message_nonce, - message, - authenticated_data, - ) { - Ok(m) => match Message::decode(&m) { - Ok(Message::Request(request)) if request.msg_type() == 1 => { - Some(request) - } - _ => None, - }, - _ => None, - }; - if let Some(request) = maybe_ping_request { - debug!( - %node_address, - "Responding to a PING request using a one-time session.", - ); - self.one_time_sessions - .insert(node_address.clone(), (request.id.clone(), session)); - if let Err(e) = self - .service_send - .send(HandlerOut::Request(node_address.clone(), Box::new(request))) - .await - { - warn!(error = %e, "Failed to report request to application"); - self.one_time_sessions.remove(&node_address); - } - } } + + // When (re-)establishing a session from an outgoing challenge, we do not need + // to filter out this request from active requests, so we do not pass + // the message nonce on to `new_session`. + self.new_session::

(node_address.clone(), session, None) + .await; + self.handle_message( + node_address.clone(), + message_nonce, + message, + authenticated_data, + ) + .await; } Err(Error::InvalidChallengeSignature(challenge)) => { warn!( @@ -1294,24 +1249,6 @@ impl Handler { } } - /// Remove one-time session by the given NodeAddress and RequestId if exists. - fn remove_one_time_session( - &mut self, - node_address: &NodeAddress, - request_id: &RequestId, - ) -> Option { - match self.one_time_sessions.peek(node_address) { - Some((id, _)) if id == request_id => { - let (_, session) = self - .one_time_sessions - .remove(node_address) - .expect("one-time session must exist"); - Some(session) - } - _ => None, - } - } - /// A request has failed. async fn fail_request( &mut self, diff --git a/src/handler/session.rs b/src/handler/session.rs index f0dc56409..7f3a0ef4c 100644 --- a/src/handler/session.rs +++ b/src/handler/session.rs @@ -265,11 +265,3 @@ impl Session { Ok((packet, session)) } } - -#[cfg(test)] -pub(crate) fn build_dummy_session() -> Session { - Session::new(Keys { - encryption_key: [0; 16], - decryption_key: [0; 16], - }) -} diff --git a/src/handler/tests.rs b/src/handler/tests.rs index a2bc95659..7ff37f1ee 100644 --- a/src/handler/tests.rs +++ b/src/handler/tests.rs @@ -15,10 +15,7 @@ use std::{ ops::Add, }; -use crate::{ - handler::{session::build_dummy_session, HandlerOut::RequestFailed}, - RequestError::SelfRequest, -}; +use crate::{handler::HandlerOut::RequestFailed, RequestError::SelfRequest}; use active_requests::ActiveRequests; use std::time::Duration; use tokio::time::sleep; @@ -78,10 +75,6 @@ async fn build_handler( pending_requests: HashMap::new(), filter_expected_responses, sessions: LruTimeCache::new(config.session_timeout, Some(config.session_cache_capacity)), - one_time_sessions: LruTimeCache::new( - Duration::from_secs(ONE_TIME_SESSION_TIMEOUT), - Some(ONE_TIME_SESSION_CACHE_CAPACITY), - ), active_challenges: HashMapDelay::new(config.request_timeout), service_recv, service_send, @@ -580,50 +573,6 @@ async fn test_self_request_ipv6() { ); } -#[tokio::test] -async fn remove_one_time_session() { - let config = ConfigBuilder::new(ListenConfig::default()).build(); - let key = CombinedKey::generate_secp256k1(); - let enr = Enr::builder() - .ip4(Ipv4Addr::LOCALHOST) - .udp4(9000) - .build(&key) - .unwrap(); - let (_, _, _, mut handler) = build_handler::(enr, key, config).await; - - let enr = { - let key = CombinedKey::generate_secp256k1(); - Enr::builder() - .ip4(Ipv4Addr::LOCALHOST) - .udp4(9000) - .build(&key) - .unwrap() - }; - let node_address = NodeAddress::new("127.0.0.1:9000".parse().unwrap(), enr.node_id()); - let request_id = RequestId::random(); - let session = build_dummy_session(); - handler - .one_time_sessions - .insert(node_address.clone(), (request_id.clone(), session)); - - let other_request_id = RequestId::random(); - assert!(handler - .remove_one_time_session(&node_address, &other_request_id) - .is_none()); - assert_eq!(1, handler.one_time_sessions.len()); - - let other_node_address = NodeAddress::new("127.0.0.1:9001".parse().unwrap(), enr.node_id()); - assert!(handler - .remove_one_time_session(&other_node_address, &request_id) - .is_none()); - assert_eq!(1, handler.one_time_sessions.len()); - - assert!(handler - .remove_one_time_session(&node_address, &request_id) - .is_some()); - assert_eq!(0, handler.one_time_sessions.len()); -} - // Tests replaying active requests. // // In this test, Receiver's session expires and Receiver returns WHOAREYOU. diff --git a/src/service.rs b/src/service.rs index e7cbcf55d..c28eaa20b 100644 --- a/src/service.rs +++ b/src/service.rs @@ -410,6 +410,14 @@ impl Service { self.rpc_failure(request_id, error); } HandlerOut::UnverifiableEnr{enr, socket, node_id} => { + // We have received an ENR that has incorrect socket address fields + // (given the source of the pakcet we received. + // If this node exists in our routing table, remove it, as it may have shifted + // ip addresses and is now no longer contactable. + let key = kbucket::Key::from(node_id); + if self.kbuckets.write().remove(&key) { + debug!(?node_id, "Uncontactable node removed from routing table"); + } self.send_event(Event::UnverifiableEnr{enr, socket, node_id}); } }