Validate VM file descriptors before ownership transfer (#177)

* Validate VM file descriptors before ownership transfer

* Handle unnamed macOS Unix sockets

lib/proxy/udp_packet_helper.rs

@@ -17,10 +17,50 @@ impl UdpPacketHelper for UdpPacket<&[u8]> {
     }
 
     fn is_dhcp_request(&self) -> bool {
-        self.src_port() == Self::BOOTPC_PORT || self.dst_port() == Self::BOOTPS_PORT
+        self.src_port() == Self::BOOTPC_PORT && self.dst_port() == Self::BOOTPS_PORT
     }
 
     fn is_dhcp_response(&self) -> bool {
-        self.src_port() == Self::BOOTPS_PORT || self.dst_port() == Self::BOOTPC_PORT
+        self.src_port() == Self::BOOTPS_PORT && self.dst_port() == Self::BOOTPC_PORT
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::UdpPacketHelper;
+    use smoltcp::wire::UdpPacket;
+
+    #[test]
+    fn test_is_dhcp_request_requires_both_standard_ports() {
+        assert!(is_dhcp_request(68, 67));
+        assert!(!is_dhcp_request(68, 9999));
+        assert!(!is_dhcp_request(9999, 67));
+    }
+
+    #[test]
+    fn test_is_dhcp_response_requires_both_standard_ports() {
+        assert!(is_dhcp_response(67, 68));
+        assert!(!is_dhcp_response(67, 9999));
+        assert!(!is_dhcp_response(9999, 68));
+    }
+
+    fn is_dhcp_request(src_port: u16, dst_port: u16) -> bool {
+        let buffer = udp_packet_buffer(src_port, dst_port);
+        let udp_pkt = UdpPacket::new_unchecked(&buffer[..]);
+        udp_pkt.is_dhcp_request()
+    }
+
+    fn is_dhcp_response(src_port: u16, dst_port: u16) -> bool {
+        let buffer = udp_packet_buffer(src_port, dst_port);
+        let udp_pkt = UdpPacket::new_unchecked(&buffer[..]);
+        udp_pkt.is_dhcp_response()
+    }
+
+    fn udp_packet_buffer(src_port: u16, dst_port: u16) -> [u8; 8] {
+        let mut buffer = [0; 8];
+        let mut udp_pkt = UdpPacket::new_unchecked(&mut buffer[..]);
+        udp_pkt.set_src_port(src_port);
+        udp_pkt.set_dst_port(dst_port);
+        buffer
     }
 }

lib/proxy/vm.rs

@@ -1,12 +1,13 @@
+use crate::dhcp_snooper::Lease;
 use crate::proxy::udp_packet_helper::UdpPacketHelper;
 use crate::proxy::{Action, Proxy};
 use anyhow::Context;
 use anyhow::Result;
 use ipnet::Ipv4Net;
 use smoltcp::wire::{
-    ArpPacket, EthernetFrame, EthernetProtocol, IpProtocol, Ipv4Packet, UdpPacket,
+    ArpOperation, ArpPacket, ArpRepr, EthernetFrame, EthernetProtocol, IpProtocol, Ipv4Packet,
+    UdpPacket,
 };
-use std::net::Ipv4Addr;
 
 impl Proxy<'_> {
     pub(crate) fn process_frame_from_vm(&mut self, frame: EthernetFrame<&[u8]>) -> Result<()> {
@@ -40,22 +41,7 @@ impl Proxy<'_> {
     }
 
     fn allowed_from_vm_arp(&self, arp_pkt: ArpPacket<&[u8]>) -> Option<()> {
-        if arp_pkt.source_hardware_addr() != self.vm_mac_address.0 {
-            return None;
-        }
-
-        let source_protocol_addr: [u8; 4] = arp_pkt.source_protocol_addr().try_into().unwrap();
-        let source_protocol_addr = Ipv4Addr::from(source_protocol_addr);
-
-        if let Some(lease) = self.dhcp_snooper.lease() {
-            if lease.valid_ip_source(source_protocol_addr) {
-                return Some(());
-            }
-        } else if source_protocol_addr.is_unspecified() {
-            return Some(());
-        }
-
-        None
+        vm_arp_allowed(arp_pkt, self.vm_mac_address, self.dhcp_snooper.lease())
     }
 
     pub(crate) fn allowed_from_vm_ipv4(&self, ipv4_pkt: Ipv4Packet<&[u8]>) -> Option<()> {
@@ -115,3 +101,147 @@ impl Proxy<'_> {
         None
     }
 }
+
+fn vm_arp_allowed(
+    arp_pkt: ArpPacket<&[u8]>,
+    vm_mac_address: smoltcp::wire::EthernetAddress,
+    lease: &Option<Lease>,
+) -> Option<()> {
+    let (operation, source_hardware_addr, source_protocol_addr) =
+        match ArpRepr::parse(&arp_pkt).ok()? {
+            ArpRepr::EthernetIpv4 {
+                operation,
+                source_hardware_addr,
+                source_protocol_addr,
+                ..
+            } => (operation, source_hardware_addr, source_protocol_addr),
+            _ => return None,
+        };
+
+    if !matches!(operation, ArpOperation::Request | ArpOperation::Reply) {
+        return None;
+    }
+
+    if source_hardware_addr != vm_mac_address {
+        return None;
+    }
+
+    if let Some(lease) = lease {
+        if lease.valid_ip_source(source_protocol_addr) {
+            return Some(());
+        }
+    } else if source_protocol_addr.is_unspecified() {
+        return Some(());
+    }
+
+    None
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::dhcp_snooper::Lease;
+    use smoltcp::wire::{
+        ArpHardware, ArpOperation, ArpPacket, EthernetAddress, EthernetProtocol, Ipv4Address,
+    };
+    use std::collections::HashSet;
+    use std::time::Duration;
+
+    #[test]
+    fn test_allowed_from_vm_arp_allows_unspecified_request_without_lease() {
+        let vm_mac_address = EthernetAddress([0x02, 0x00, 0x00, 0x00, 0x00, 0x01]);
+        let buf = arp_packet(vm_mac_address.0, [0, 0, 0, 0], ArpOperation::Request, 6, 4);
+        let arp_pkt = ArpPacket::new_checked(buf.as_slice()).unwrap();
+
+        assert!(super::vm_arp_allowed(arp_pkt, vm_mac_address, &None).is_some());
+    }
+
+    #[test]
+    fn test_allowed_from_vm_arp_allows_reply_for_leased_ip() {
+        let vm_mac_address = EthernetAddress([0x02, 0x00, 0x00, 0x00, 0x00, 0x01]);
+        let lease_ip = Ipv4Address::new(192, 168, 0, 2);
+        let lease = Some(Lease::new(
+            lease_ip,
+            Duration::from_secs(600),
+            HashSet::new(),
+        ));
+        let buf = arp_packet(
+            vm_mac_address.0,
+            lease_ip.octets(),
+            ArpOperation::Reply,
+            6,
+            4,
+        );
+        let arp_pkt = ArpPacket::new_checked(buf.as_slice()).unwrap();
+
+        assert!(super::vm_arp_allowed(arp_pkt, vm_mac_address, &lease).is_some());
+    }
+
+    #[test]
+    fn test_allowed_from_vm_arp_rejects_unknown_operation() {
+        let vm_mac_address = EthernetAddress([0x02, 0x00, 0x00, 0x00, 0x00, 0x01]);
+        let buf = arp_packet(
+            vm_mac_address.0,
+            [0, 0, 0, 0],
+            ArpOperation::Unknown(3),
+            6,
+            4,
+        );
+        let arp_pkt = ArpPacket::new_checked(buf.as_slice()).unwrap();
+
+        assert!(super::vm_arp_allowed(arp_pkt, vm_mac_address, &None).is_none());
+    }
+
+    #[test]
+    fn test_allowed_from_vm_arp_rejects_non_ethernet_hardware_type() {
+        let vm_mac_address = EthernetAddress([0x02, 0x00, 0x00, 0x00, 0x00, 0x01]);
+        let mut buf = arp_packet(vm_mac_address.0, [0, 0, 0, 0], ArpOperation::Request, 6, 4);
+        let mut arp_pkt = ArpPacket::new_unchecked(buf.as_mut_slice());
+        arp_pkt.set_hardware_type(ArpHardware::Unknown(2));
+        let arp_pkt = ArpPacket::new_checked(buf.as_slice()).unwrap();
+
+        assert!(super::vm_arp_allowed(arp_pkt, vm_mac_address, &None).is_none());
+    }
+
+    #[test]
+    fn test_allowed_from_vm_arp_rejects_non_ipv4_protocol_type() {
+        let vm_mac_address = EthernetAddress([0x02, 0x00, 0x00, 0x00, 0x00, 0x01]);
+        let mut buf = arp_packet(vm_mac_address.0, [0, 0, 0, 0], ArpOperation::Request, 6, 4);
+        let mut arp_pkt = ArpPacket::new_unchecked(buf.as_mut_slice());
+        arp_pkt.set_protocol_type(EthernetProtocol::Ipv6);
+        let arp_pkt = ArpPacket::new_checked(buf.as_slice()).unwrap();
+
+        assert!(super::vm_arp_allowed(arp_pkt, vm_mac_address, &None).is_none());
+    }
+
+    #[test]
+    fn test_allowed_from_vm_arp_rejects_non_ipv4_protocol_length() {
+        let vm_mac_address = EthernetAddress([0x02, 0x00, 0x00, 0x00, 0x00, 0x01]);
+        let buf = arp_packet(vm_mac_address.0, [0, 0, 0], ArpOperation::Request, 6, 3);
+        let arp_pkt = ArpPacket::new_checked(buf.as_slice()).unwrap();
+
+        assert!(super::vm_arp_allowed(arp_pkt, vm_mac_address, &None).is_none());
+    }
+
+    fn arp_packet(
+        source_hardware_addr: [u8; 6],
+        source_protocol_addr: impl AsRef<[u8]>,
+        operation: ArpOperation,
+        hardware_len: u8,
+        protocol_len: u8,
+    ) -> Vec<u8> {
+        let source_protocol_addr = source_protocol_addr.as_ref();
+        let payload_len = 8 + 2 * (hardware_len as usize + protocol_len as usize);
+        let mut buf = vec![0; payload_len];
+        let mut arp_pkt = ArpPacket::new_unchecked(buf.as_mut_slice());
+        arp_pkt.set_hardware_type(ArpHardware::Ethernet);
+        arp_pkt.set_protocol_type(EthernetProtocol::Ipv4);
+        arp_pkt.set_hardware_len(hardware_len);
+        arp_pkt.set_protocol_len(protocol_len);
+        arp_pkt.set_operation(operation);
+        arp_pkt.set_source_hardware_addr(&source_hardware_addr[..hardware_len as usize]);
+        arp_pkt.set_source_protocol_addr(source_protocol_addr);
+        arp_pkt.set_target_hardware_addr(&[0; 6][..hardware_len as usize]);
+        arp_pkt.set_target_protocol_addr(&vec![0; protocol_len as usize]);
+        buf
+    }
+}

lib/vm.rs

@@ -1,5 +1,7 @@
-use anyhow::Result;
-use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
+use anyhow::{Context, Result, bail};
+use std::io;
+use std::mem::{size_of, zeroed};
+use std::os::fd::{AsRawFd, FromRawFd, RawFd};
 use std::os::unix::net::UnixDatagram;
 
 pub struct VM {
@@ -8,6 +10,9 @@ pub struct VM {
 
 impl VM {
     pub fn new(vm_fd: RawFd) -> Result<VM> {
+        let vm_fd = duplicate_vm_fd(vm_fd)?;
+
+        // SAFETY: duplicate_vm_fd only returns a valid descriptor that it owns.
         let sock = unsafe { UnixDatagram::from_raw_fd(vm_fd) };
         sock.set_nonblocking(true)?;
 
@@ -23,8 +28,163 @@ impl VM {
     }
 }
 
+fn duplicate_vm_fd(vm_fd: RawFd) -> Result<RawFd> {
+    if vm_fd < 0 {
+        bail!("invalid VM file descriptor {vm_fd}: value must be non-negative");
+    }
+
+    // SAFETY: fcntl duplicates the descriptor without transferring ownership of vm_fd.
+    let duplicated_fd = unsafe { libc::fcntl(vm_fd, libc::F_DUPFD_CLOEXEC, 0) };
+    if duplicated_fd == -1 {
+        return Err(io::Error::last_os_error())
+            .with_context(|| format!("failed to duplicate VM file descriptor {vm_fd}"));
+    }
+
+    if let Err(error) = validate_vm_fd(duplicated_fd) {
+        // SAFETY: duplicated_fd is an open descriptor owned by this function.
+        unsafe { libc::close(duplicated_fd) };
+        return Err(error);
+    }
+
+    Ok(duplicated_fd)
+}
+
+fn validate_vm_fd(vm_fd: RawFd) -> Result<()> {
+    // SAFETY: fcntl only reads descriptor state and does not take ownership.
+    if unsafe { libc::fcntl(vm_fd, libc::F_GETFD) } == -1 {
+        return Err(io::Error::last_os_error())
+            .with_context(|| format!("failed to inspect VM file descriptor {vm_fd}"));
+    }
+
+    let mut socket_type = 0;
+    let mut socket_type_len = size_of::<libc::c_int>() as libc::socklen_t;
+
+    // SAFETY: socket_type and socket_type_len are valid writable buffers of the sizes given.
+    if unsafe {
+        libc::getsockopt(
+            vm_fd,
+            libc::SOL_SOCKET,
+            libc::SO_TYPE,
+            (&mut socket_type as *mut libc::c_int).cast(),
+            &mut socket_type_len,
+        )
+    } == -1
+    {
+        return Err(io::Error::last_os_error())
+            .with_context(|| format!("VM file descriptor {vm_fd} is not a socket"));
+    }
+
+    if socket_type != libc::SOCK_DGRAM {
+        bail!("VM file descriptor {vm_fd} is not a Unix datagram socket");
+    }
+
+    let mut address: libc::sockaddr_storage = unsafe { zeroed() };
+    let mut address_len = size_of::<libc::sockaddr_storage>() as libc::socklen_t;
+
+    // SAFETY: address and address_len are valid writable buffers of the sizes given.
+    if unsafe {
+        libc::getsockname(
+            vm_fd,
+            (&mut address as *mut libc::sockaddr_storage).cast(),
+            &mut address_len,
+        )
+    } == -1
+    {
+        return Err(io::Error::last_os_error()).with_context(|| {
+            format!("failed to inspect the address family of VM file descriptor {vm_fd}")
+        });
+    }
+
+    // macOS returns a zero-length address for unnamed UNIX-domain sockets,
+    // including socketpair descriptors. Other socket families return their
+    // address family when getsockname succeeds.
+    let is_unix_socket = address_len == 0 || address.ss_family as libc::c_int == libc::AF_UNIX;
+    if !is_unix_socket {
+        bail!("VM file descriptor {vm_fd} is not a Unix socket");
+    }
+
+    Ok(())
+}
+
 impl AsRawFd for VM {
     fn as_raw_fd(&self) -> RawFd {
         self.sock.as_raw_fd()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::VM;
+    use std::fs::File;
+    use std::net::UdpSocket;
+    use std::os::fd::AsRawFd;
+    use std::os::unix::net::{UnixDatagram, UnixStream};
+
+    #[test]
+    fn test_new_rejects_negative_fd() {
+        let error = VM::new(-1).err().unwrap();
+
+        assert_eq!(
+            error.to_string(),
+            "invalid VM file descriptor -1: value must be non-negative"
+        );
+    }
+
+    #[test]
+    fn test_new_rejects_non_socket_fd_without_taking_ownership() {
+        let file = File::open("/dev/null").unwrap();
+        let error = VM::new(file.as_raw_fd()).err().unwrap();
+
+        assert!(error.to_string().contains("is not a socket"));
+        assert!(file.metadata().is_ok());
+    }
+
+    #[test]
+    fn test_new_rejects_closed_fd() {
+        let (socket, _peer) = UnixDatagram::pair().unwrap();
+        let vm_fd = socket.as_raw_fd();
+        drop(socket);
+
+        let error = VM::new(vm_fd).err().unwrap();
+
+        assert!(
+            error
+                .to_string()
+                .contains("failed to duplicate VM file descriptor")
+        );
+    }
+
+    #[test]
+    fn test_new_rejects_non_datagram_socket() {
+        let (stream, _peer) = UnixStream::pair().unwrap();
+        let error = VM::new(stream.as_raw_fd()).err().unwrap();
+
+        assert!(error.to_string().contains("not a Unix datagram socket"));
+    }
+
+    #[test]
+    fn test_new_rejects_internet_datagram_socket() {
+        let socket = UdpSocket::bind("127.0.0.1:0").unwrap();
+        let error = VM::new(socket.as_raw_fd()).err().unwrap();
+
+        assert!(error.to_string().contains("not a Unix socket"));
+    }
+
+    #[test]
+    fn test_new_does_not_close_original_fd_when_vm_is_dropped() {
+        let (socket, _peer) = UnixDatagram::pair().unwrap();
+        let vm = VM::new(socket.as_raw_fd()).unwrap();
+        drop(vm);
+
+        let socket_fd_is_open = unsafe { libc::fcntl(socket.as_raw_fd(), libc::F_GETFD) != -1 };
+
+        if socket_fd_is_open {
+            drop(socket);
+        } else {
+            // Avoid double-closing the descriptor if this test catches an unsafe implementation.
+            std::mem::forget(socket);
+        }
+
+        assert!(socket_fd_is_open);
+    }
+}

src/main.rs

@@ -26,6 +26,7 @@ use uzers::{get_current_groupname, get_current_username, get_effective_uid};
 struct Args {
     #[clap(
         long,
+        value_parser = parse_vm_fd,
         help = "FD number to use for communicating with the VM's networking stack"
     )]
     vm_fd: c_int,
@@ -217,6 +218,18 @@ fn try_main() -> anyhow::Result<()> {
     proxy.run()
 }
 
+fn parse_vm_fd(value: &str) -> Result<c_int, String> {
+    let vm_fd = value
+        .parse::<c_int>()
+        .map_err(|err| format!("invalid file descriptor: {err}"))?;
+
+    if vm_fd < 0 {
+        return Err("file descriptor must be non-negative".to_string());
+    }
+
+    Ok(vm_fd)
+}
+
 fn sudo_escalation_works() -> bool {
     let exe = std::env::current_exe().unwrap();
     let args = std::env::args().skip(1);
@@ -252,3 +265,25 @@ fn set_bootpd_lease_time(lease_time: u32) {
         SCPreferencesCommitChanges(prefs.as_concrete_TypeRef());
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::Args;
+    use clap::Parser;
+
+    #[test]
+    fn test_cli_rejects_negative_vm_fd_before_startup() {
+        let error = Args::try_parse_from([
+            "softnet",
+            "--vm-fd=-1",
+            "--vm-mac-address=02:00:00:00:00:01",
+        ])
+        .unwrap_err();
+
+        assert!(
+            error
+                .to_string()
+                .contains("file descriptor must be non-negative")
+        );
+    }
+}