[pve-devel] [RFC PATCH pve-installer 6/6] closes #5757: common: add checks for valid IPv4 address within subnet

[Michael Köppl]

Implement check if the address entered by the user is valid within the
given subnet, i.e. not a network address or broadcast address.

Partially closes [0].

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=5757

Signed-off-by: Michael Köppl <m.koeppl at proxmox.com>
---
Some input / discussion would be much appreciated here, since this might
again be considered too restrictive. Multiple questions came up during
in-person discussion:
* Is check for broadcast address desired or is it considered a valid
configuration for PVE?
* Is IPv6 check necessary and if so, is allowing to set the address to a
broadcast address a valid setting for IPv6?

 proxmox-installer-common/src/utils.rs | 47 +++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 49f1c9f..3d79749 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -16,6 +16,10 @@ pub enum CidrAddressParseError {
     InvalidAddr(AddrParseError),
     /// The mask could not be parsed.
     InvalidMask(Option<ParseIntError>),
+    /// The IP address is a network address.
+    IsNetworkAddr,
+    /// The IP address is a broadcast address.
+    IsBroadcastAddr,
 }
 
 impl fmt::Display for CidrAddressParseError {
@@ -26,6 +30,10 @@ impl fmt::Display for CidrAddressParseError {
             }
             CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
             CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
+            CidrAddressParseError::IsNetworkAddr => String::from("Address is a network address"),
+            CidrAddressParseError::IsBroadcastAddr => {
+                String::from("Address is a broadcast address")
+            }
         };
 
         write!(f, "Invalid CIDR: {msg}")
@@ -62,10 +70,12 @@ impl CidrAddress {
         let addr = addr.into();
 
         if mask > mask_limit(&addr) {
-            Err(CidrAddressParseError::InvalidMask(None))
-        } else {
-            Ok(Self { addr, mask })
+            return Err(CidrAddressParseError::InvalidMask(None));
         }
+
+        check_addr_valid_in_subnet(&addr, mask)?;
+
+        Ok(Self { addr, mask })
     }
 
     /// Returns only the IP address part of the address.
@@ -104,10 +114,12 @@ impl FromStr for CidrAddress {
             .map_err(|err| CidrAddressParseError::InvalidMask(Some(err)))?;
 
         if mask > mask_limit(&addr) {
-            Err(CidrAddressParseError::InvalidMask(None))
-        } else {
-            Ok(Self { addr, mask })
+            return Err(CidrAddressParseError::InvalidMask(None));
         }
+
+        check_addr_valid_in_subnet(&addr, mask)?;
+
+        Ok(Self { addr, mask })
     }
 }
 
@@ -134,6 +146,29 @@ fn mask_limit(addr: &IpAddr) -> usize {
     if addr.is_ipv4() { 32 } else { 128 }
 }
 
+fn check_addr_valid_in_subnet(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+    match &addr {
+        IpAddr::V4(addr_v4) => {
+            let num_host_bits = 32 - mask;
+            let host_part_mask = (1u32 << num_host_bits) - 1;
+
+            let ip_bits = u32::from_be_bytes(addr_v4.octets());
+
+            let network_addr = ip_bits & !host_part_mask;
+            let broadcast_addr = network_addr | host_part_mask;
+
+            if ip_bits == network_addr {
+                Err(CidrAddressParseError::IsNetworkAddr)
+            } else if ip_bits == broadcast_addr {
+                Err(CidrAddressParseError::IsBroadcastAddr)
+            } else {
+                Ok(())
+            }
+        }
+        IpAddr::V6(_) => Ok(()),
+    }
+}
+
 /// Possible errors that might occur when parsing FQDNs.
 #[derive(Debug, Eq, PartialEq)]
 pub enum FqdnParseError {
-- 
2.39.5