Use canonical addresses for the IP filter

Author: rnijveld

fuzz/fuzz_targets/ipfilter.rs

@@ -22,8 +22,13 @@ impl<'a> Arbitrary<'a> for ASubnet {
             let addr = IpAddr::V4(Ipv4Addr::from(u.arbitrary::<[u8; 4]>()?));
             Ok(ASubnet(IpSubnet { mask, addr }))
         } else {
-            let mask: u8 = u.int_in_range(0..=128)?;
             let addr = IpAddr::V6(Ipv6Addr::from(u.arbitrary::<[u8; 16]>()?));
+            let addr = addr.to_canonical();
+            let mask: u8 = if addr.is_ipv4() {
+                u.int_in_range(0..=32)?
+            } else {
+                u.int_in_range(0..=128)?
+            };
             Ok(ASubnet(IpSubnet { mask, addr }))
         }
     }

ntp-proto/src/ipfilter.rs

@@ -200,19 +200,20 @@ impl IpFilter {
 
     /// Check whether a given ip address is contained in the filter.
     /// Complexity: O(1)
-    pub fn is_in(&self, addr: &IpAddr) -> bool {
+    pub fn is_in(&self, addr: IpAddr) -> bool {
+        let addr = addr.to_canonical();
         match addr {
             IpAddr::V4(addr) => self.is_in4(addr),
             IpAddr::V6(addr) => self.is_in6(addr),
         }
     }
 
-    fn is_in4(&self, addr: &Ipv4Addr) -> bool {
+    fn is_in4(&self, addr: Ipv4Addr) -> bool {
         self.ipv4_filter
             .lookup((u32::from_be_bytes(addr.octets()) as u128) << 96)
     }
 
-    fn is_in6(&self, addr: &Ipv6Addr) -> bool {
+    fn is_in6(&self, addr: Ipv6Addr) -> bool {
         self.ipv6_filter.lookup(u128::from_be_bytes(addr.octets()))
     }
 }
@@ -229,7 +230,7 @@ pub mod fuzz {
     use super::*;
 
     fn contains(subnet: &IpSubnet, addr: &IpAddr) -> bool {
-        match (subnet.addr, addr) {
+        match (subnet.addr, addr.to_canonical()) {
             (IpAddr::V4(net), IpAddr::V4(addr)) => {
                 let net = u32::from_be_bytes(net.octets());
                 let addr = u32::from_be_bytes(addr.octets());
@@ -263,13 +264,15 @@ pub mod fuzz {
         let filter = IpFilter::new(nets);
 
         for addr in addr {
-            assert_eq!(filter.is_in(addr), any_contains(nets, addr));
+            assert_eq!(filter.is_in(*addr), any_contains(nets, addr));
         }
     }
 }
 
 #[cfg(test)]
 mod tests {
+    use crate::SubnetParseError;
+
     use super::*;
 
     #[test]
@@ -295,31 +298,45 @@ mod tests {
     fn test_filter() {
         let filter = IpFilter::new(&[
             "127.0.0.0/24".parse().unwrap(),
-            "::FFFF:0000:0000/96".parse().unwrap(),
+            "::FFFF:192.168.0.0/104".parse().unwrap(),
         ]);
-        assert!(filter.is_in(&"127.0.0.1".parse().unwrap()));
-        assert!(!filter.is_in(&"192.168.1.1".parse().unwrap()));
-        assert!(filter.is_in(&"::FFFF:ABCD:0123".parse().unwrap()));
-        assert!(!filter.is_in(&"::FEEF:ABCD:0123".parse().unwrap()));
+        assert!(filter.is_in("127.0.0.1".parse().unwrap()));
+        assert!(!filter.is_in("10.0.1.1".parse().unwrap()));
+        assert!(filter.is_in("::FFFF:192.168.1.1".parse().unwrap()));
+        assert!(!filter.is_in("::FFFF:10.0.0.5".parse().unwrap()));
+        assert!(!filter.is_in("::FEEF:ABCD:1234".parse().unwrap()));
+    }
+
+    #[test]
+    fn test_subnet_mapped_ipv4_overlap() {
+        let subnet_err = "::FFFF:192.168.0.0/95".parse::<IpSubnet>().unwrap_err();
+        assert_eq!(subnet_err, SubnetParseError::MaskV4Range);
+    }
+
+    #[test]
+    fn test_subnet_mapped_ipv4() {
+        let subnet = "::FFFF:192.168.0.0/120".parse::<IpSubnet>().unwrap();
+        assert_eq!(subnet.addr, "192.168.0.0".parse::<IpAddr>().unwrap());
+        assert_eq!(subnet.mask, 24);
     }
 
     #[test]
     fn test_subnet_edgecases() {
         let filter = IpFilter::new(&["0.0.0.0/0".parse().unwrap(), "::/0".parse().unwrap()]);
 
-        assert!(filter.is_in(&"0.0.0.0".parse().unwrap()));
-        assert!(filter.is_in(&"255.255.255.255".parse().unwrap()));
-        assert!(filter.is_in(&"::".parse().unwrap()));
-        assert!(filter.is_in(&"FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFF".parse().unwrap()));
+        assert!(filter.is_in("0.0.0.0".parse().unwrap()));
+        assert!(filter.is_in("255.255.255.255".parse().unwrap()));
+        assert!(filter.is_in("::".parse().unwrap()));
+        assert!(filter.is_in("FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFF:FFFF".parse().unwrap()));
 
         let filter = IpFilter::new(&[
             "1.2.3.4/32".parse().unwrap(),
             "10:32:54:76:98:BA:DC:FE/128".parse().unwrap(),
         ]);
 
-        assert!(filter.is_in(&"1.2.3.4".parse().unwrap()));
-        assert!(!filter.is_in(&"1.2.3.5".parse().unwrap()));
-        assert!(filter.is_in(&"10:32:54:76:98:BA:DC:FE".parse().unwrap()));
-        assert!(!filter.is_in(&"10:32:54:76:98:BA:DC:FF".parse().unwrap()));
+        assert!(filter.is_in("1.2.3.4".parse().unwrap()));
+        assert!(!filter.is_in("1.2.3.5".parse().unwrap()));
+        assert!(filter.is_in("10:32:54:76:98:BA:DC:FE".parse().unwrap()));
+        assert!(!filter.is_in("10:32:54:76:98:BA:DC:FF".parse().unwrap()));
     }
 }

ntp-proto/src/server.rs

@@ -125,10 +125,10 @@ impl<C> Server<C> {
     }
 
     fn intended_action(&mut self, client_ip: IpAddr) -> (ServerResponse, ServerReason) {
-        if self.denyfilter.is_in(&client_ip) {
+        if self.denyfilter.is_in(client_ip) {
             // First apply denylist
             (self.config.denylist.action.into(), ServerReason::Policy)
-        } else if !self.allowfilter.is_in(&client_ip) {
+        } else if !self.allowfilter.is_in(client_ip) {
             // Then allowlist
             (self.config.allowlist.action.into(), ServerReason::Policy)
         } else if !self.client_cache.is_allowed(
@@ -422,6 +422,7 @@ pub enum SubnetParseError {
     Subnet,
     Ip(AddrParseError),
     Mask,
+    MaskV4Range,
 }
 
 impl std::error::Error for SubnetParseError {}
@@ -432,6 +433,10 @@ impl Display for SubnetParseError {
             Self::Subnet => write!(f, "Invalid subnet syntax"),
             Self::Ip(e) => write!(f, "{e} in subnet"),
             Self::Mask => write!(f, "Invalid subnet mask"),
+            Self::MaskV4Range => write!(
+                f,
+                "Subnet mask overflows the IPv4 range of an IPv4-mapped IPv6 address"
+            ),
         }
     }
 }
@@ -449,13 +454,25 @@ impl std::str::FromStr for IpSubnet {
         let (addr, mask) = s.split_once('/').ok_or(SubnetParseError::Subnet)?;
         let addr: IpAddr = addr.parse()?;
         let mask: u8 = mask.parse().map_err(|_| SubnetParseError::Mask)?;
+
+        // Canonicalize IPv4-mapped IPv6 addresses (e.g. `::ffff:192.168.0.0`)
+        // to their IPv4 form so they match against canonicalized filtered IPs.
+        let (addr, mask) = match (addr, addr.to_canonical()) {
+            (IpAddr::V6(_), canonical @ IpAddr::V4(_)) => {
+                let mask = mask.checked_sub(96).ok_or(SubnetParseError::MaskV4Range)?;
+                (canonical, mask)
+            }
+            _ => (addr, mask),
+        };
+
         let max_mask = match addr {
             IpAddr::V4(_) => 32,
             IpAddr::V6(_) => 128,
         };
         if mask > max_mask {
             return Err(SubnetParseError::Mask);
         }
+
         Ok(IpSubnet { addr, mask })
     }
 }