From 1b52fe90c8825caaf8b4a2ab66c0b631706dcfe8 Mon Sep 17 00:00:00 2001
From: Dan Cline <6798349+Rjected@users.noreply.github.com>
Date: Fri, 24 Feb 2023 18:21:02 -0500
Subject: [PATCH] feat: do not ban non-global IPs (#1549)

---
 crates/net/common/src/ban_list.rs       | 61 ++++++++++++++++++++++---
 crates/net/network/src/peers/manager.rs |  2 +-
 2 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/crates/net/common/src/ban_list.rs b/crates/net/common/src/ban_list.rs
index bf9db48ce1..548dc5d33b 100644
--- a/crates/net/common/src/ban_list.rs
+++ b/crates/net/common/src/ban_list.rs
@@ -2,6 +2,19 @@
 use reth_primitives::PeerId;
 use std::{collections::HashMap, net::IpAddr, time::Instant};
 
+/// Determines whether or not the IP is globally routable.
+/// Should be replaced with [`IpAddr::is_global`](std::net::IpAddr::is_global) once it is stable.
+pub fn is_global(ip: &IpAddr) -> bool {
+    if ip.is_unspecified() || ip.is_loopback() {
+        return false
+    }
+
+    match ip {
+        IpAddr::V4(ip) => !ip.is_private() && !ip.is_link_local(),
+        IpAddr::V6(_) => true,
+    }
+}
+
 /// Stores peers that should be taken out of circulation either indefinitely or until a certain
 /// timestamp
 #[derive(Debug, Clone, Default, PartialEq)]
@@ -14,17 +27,21 @@ pub struct BanList {
 
 impl BanList {
     /// Creates a new ban list that bans the given peers and ips indefinitely.
+    ///
+    /// This will ban non-global IPs if they are included in `banned_ips`
     pub fn new(
         banned_peers: impl IntoIterator<Item = PeerId>,
         banned_ips: impl IntoIterator<Item = IpAddr>,
     ) -> Self {
-        Self {
-            banned_ips: banned_ips.into_iter().map(|ip| (ip, None)).collect(),
-            banned_peers: banned_peers.into_iter().map(|peer| (peer, None)).collect(),
-        }
+        Self::new_with_timeout(
+            banned_peers.into_iter().map(|peer| (peer, None)).collect(),
+            banned_ips.into_iter().map(|ip| (ip, None)).collect(),
+        )
     }
 
     /// Creates a new ban list that bans the given peers and ips with an optional timeout.
+    ///
+    /// This will ban non-global IPs if they are included in `banned_ips`
     pub fn new_with_timeout(
         banned_peers: HashMap<PeerId, Option<Instant>>,
         banned_ips: HashMap<IpAddr, Option<Instant>>,
@@ -100,6 +117,8 @@ impl BanList {
     }
 
     /// Bans the IP until the timestamp.
+    ///
+    /// This does not ban non-global IPs.
     pub fn ban_ip_until(&mut self, ip: IpAddr, until: Instant) {
         self.ban_ip_with(ip, Some(until));
     }
@@ -110,6 +129,8 @@ impl BanList {
     }
 
     /// Bans the IP indefinitely.
+    ///
+    /// This does not ban non-global IPs.
     pub fn ban_ip(&mut self, ip: IpAddr) {
         self.ban_ip_with(ip, None);
     }
@@ -125,8 +146,12 @@ impl BanList {
     }
 
     /// Bans the ip indefinitely or until the given timeout.
+    ///
+    /// This does not ban non-global IPs.
     pub fn ban_ip_with(&mut self, ip: IpAddr, until: Option<Instant>) {
-        self.banned_ips.insert(ip, until);
+        if is_global(&ip) {
+            self.banned_ips.insert(ip, until);
+        }
     }
 }
 
@@ -146,11 +171,35 @@ mod tests {
 
     #[test]
     fn can_ban_unban_ip() {
-        let ip = IpAddr::from([0, 0, 0, 0]);
+        let ip = IpAddr::from([1, 1, 1, 1]);
         let mut banlist = BanList::default();
         banlist.ban_ip(ip);
         assert!(banlist.is_banned_ip(&ip));
         banlist.unban_ip(&ip);
         assert!(!banlist.is_banned_ip(&ip));
     }
+
+    #[test]
+    fn cannot_ban_non_global() {
+        let mut ip = IpAddr::from([0, 0, 0, 0]);
+        let mut banlist = BanList::default();
+        banlist.ban_ip(ip);
+        assert!(!banlist.is_banned_ip(&ip));
+
+        ip = IpAddr::from([10, 0, 0, 0]);
+        banlist.ban_ip(ip);
+        assert!(!banlist.is_banned_ip(&ip));
+
+        ip = IpAddr::from([127, 0, 0, 0]);
+        banlist.ban_ip(ip);
+        assert!(!banlist.is_banned_ip(&ip));
+
+        ip = IpAddr::from([172, 17, 0, 0]);
+        banlist.ban_ip(ip);
+        assert!(!banlist.is_banned_ip(&ip));
+
+        ip = IpAddr::from([172, 16, 0, 0]);
+        banlist.ban_ip(ip);
+        assert!(!banlist.is_banned_ip(&ip));
+    }
 }
diff --git a/crates/net/network/src/peers/manager.rs b/crates/net/network/src/peers/manager.rs
index 791e4eb362..6fba9291cc 100644
--- a/crates/net/network/src/peers/manager.rs
+++ b/crates/net/network/src/peers/manager.rs
@@ -1539,7 +1539,7 @@ mod test {
 
     #[tokio::test]
     async fn test_dropped_incoming() {
-        let socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 1, 2)), 8008);
+        let socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(1, 0, 1, 2)), 8008);
         let ban_duration = Duration::from_millis(500);
         let config = PeersConfig { ban_duration, ..Default::default() };
         let mut peers = PeersManager::new(config);