Memory access problems

Cargo.toml

@@ -1,8 +1,8 @@
 [package]
 name = "defguard_wireguard_rs"
-version = "0.10.0"
+version = "0.11.0"
 edition = "2024"
-rust-version = "1.87"
+rust-version = "1.91"
 description = "A unified multi-platform high-level API for managing WireGuard interfaces"
 license = "Apache-2.0"
 readme = "README.md"
@@ -16,7 +16,7 @@ base64 = "0.22"
 log = "0.4"
 serde = { version = "1.0", features = ["derive"], optional = true }
 thiserror = "2.0"
-x25519-dalek = { version = "2.0", features = ["getrandom", "static_secrets"] }
+x25519-dalek = { version = "3.0", features = ["getrandom", "static_secrets"] }
 
 [dev-dependencies]
 env_logger = "0.11"
@@ -43,7 +43,7 @@ wireguard-nt = "0.5"
 [target.'cfg(target_os = "linux")'.dependencies]
 netlink-packet-core = "0.8"
 netlink-packet-generic = "0.4"
-netlink-packet-route = "0.30"
+netlink-packet-route = "0.31"
 netlink-packet-utils = "0.6"
 netlink-packet-wireguard = "0.4"
 netlink-sys = "0.8"

src/bsd/ifconfig.rs

@@ -1,6 +1,7 @@
 use std::{
     net::{Ipv4Addr, Ipv6Addr},
     os::fd::AsRawFd,
+    ptr::from_mut,
 };
 
 use libc::{AF_INET, AF_INET6, AF_UNIX, IF_NAMESIZE, IFF_UP, c_ulong, ioctl};
@@ -94,9 +95,9 @@ impl IfReq {
     pub(super) fn create(&mut self) -> Result<(), IoError> {
         let socket = create_socket(AF_UNIX)?;
         #[cfg(target_os = "netbsd")]
-        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCIFCREATE, &*self) };
+        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCIFCREATE, from_mut::<Self>(self)) };
         #[cfg(any(target_os = "freebsd", target_os = "macos"))]
-        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCIFCREATE2, &*self) };
+        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCIFCREATE2, from_mut::<Self>(self)) };
         c_int_to_error(result)?;
 
         Ok(())
@@ -148,7 +149,7 @@ impl IfMtu {
 
     pub(super) fn get_mtu(&mut self) -> Result<u32, IoError> {
         let socket = create_socket(AF_UNIX)?;
-        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCGIFMTU, &*self) };
+        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCGIFMTU, from_mut::<Self>(self)) };
         c_int_to_error(result)?;
 
         Ok(self.ifru_mtu)
@@ -308,7 +309,7 @@ impl IfReqFlags {
         let socket = create_socket(AF_UNIX)?;
 
         // Get current interface flags.
-        let _result = unsafe { ioctl(socket.as_raw_fd(), SIOCGIFFLAGS, &*self) };
+        let _result = unsafe { ioctl(socket.as_raw_fd(), SIOCGIFFLAGS, from_mut::<Self>(self)) };
 
         // Set interface up flag.
         self.ifr_flags |= IFF_UP as u64;

src/bsd/mod.rs

@@ -63,9 +63,15 @@ static NV_CIDR: &str = "cidr";
 static NV_IPV4: &str = "ipv4";
 static NV_IPV6: &str = "ipv6";
 
-/// Cast bytes to `T`.
-unsafe fn cast_ref<T>(bytes: &[u8]) -> &T {
-    unsafe { bytes.as_ptr().cast::<T>().as_ref().unwrap() }
+/// Read a `T` out of a byte buffer that may not be aligned to `T`.
+/// This returns an owned value rather than a reference.
+///
+/// # Safety
+/// `bytes` must contain a valid bit pattern for `T` in its first `size_of::<T>()`
+/// bytes; `T` must be a plain-old-data type (no meaningful `Drop`).
+unsafe fn read_unaligned<T>(bytes: &[u8]) -> T {
+    assert!(bytes.len() >= size_of::<T>());
+    unsafe { bytes.as_ptr().cast::<T>().read_unaligned() }
 }
 
 /// Cast `T' to bytes.
@@ -102,6 +108,8 @@ pub enum IoError {
     Unpack,
     #[error("Failed to load kernel module")]
     KernelModule,
+    #[error("Failed to parse NvList: {0}")]
+    NvList(#[from] nvlist::NvListError),
 }
 
 impl From<io::Error> for IoError {
@@ -291,18 +299,14 @@ pub fn get_host(if_name: &str) -> Result<Host, IoError> {
     wg_data.read_data()?;
 
     let mut nvlist = NvList::new();
-    // FIXME: use proper error, here and above
-    nvlist
-        .unpack(wg_data.as_slice())
-        .map_err(|_| IoError::MemAlloc)?;
+    nvlist.unpack(wg_data.as_slice())?;
 
     Ok(Host::from_nvlist(&nvlist))
 }
 
 pub fn set_host(if_name: &str, host: &Host) -> Result<(), IoError> {
     let nvlist = host.as_nvlist();
-    // FIXME: use proper error, here and above
-    let mut buf = nvlist.pack().map_err(|_| IoError::MemAlloc)?;
+    let mut buf = nvlist.pack()?;
 
     let mut wg_data = WgWriteIo::new(if_name, &mut buf);
     wg_data.write_data()
@@ -311,8 +315,7 @@ pub fn set_host(if_name: &str, host: &Host) -> Result<(), IoError> {
 pub fn set_peer(if_name: &str, peer: &Peer) -> Result<(), IoError> {
     let mut nvlist = NvList::new();
     nvlist.append_nvlist_array(NV_PEERS, vec![peer.as_nvlist()]);
-    // FIXME: use proper error, here and above
-    let mut buf = nvlist.pack().map_err(|_| IoError::MemAlloc)?;
+    let mut buf = nvlist.pack()?;
 
     let mut wg_data = WgWriteIo::new(if_name, &mut buf);
     wg_data.write_data()
@@ -321,8 +324,7 @@ pub fn set_peer(if_name: &str, peer: &Peer) -> Result<(), IoError> {
 pub fn delete_peer(if_name: &str, public_key: &Key) -> Result<(), IoError> {
     let mut nvlist = NvList::new();
     nvlist.append_nvlist_array(NV_PEERS, vec![public_key.as_nvlist_for_removal()]);
-    // FIXME: use proper error, here and above
-    let mut buf = nvlist.pack().map_err(|_| IoError::MemAlloc)?;
+    let mut buf = nvlist.pack()?;
 
     let mut wg_data = WgWriteIo::new(if_name, &mut buf);
     wg_data.write_data()
@@ -420,20 +422,19 @@ pub fn flush_interface(if_name: &str) -> Result<(), IoError> {
                 let name = CStr::from_ptr((*addr).ifa_name);
                 if name == ifname_c.as_c_str() {
                     let ifa_addr = (*addr).ifa_addr;
-                    if ifa_addr.is_null() {
-                        continue;
-                    }
-                    // Convert `ifa_addr` to `IpAddr`.
-                    // Note: `ifa_addr` is actually `sockaddr_in` or `sockaddr_in6` depending on
-                    // `sa_len` and `sa_family`.
-                    if (*ifa_addr).sa_len == SA_IN_SIZE && (*ifa_addr).sa_family == AF_INET {
-                        if let Some(sockaddr) = SockAddrIn::from_raw(ifa_addr) {
-                            addr_to_remove.push(sockaddr.ip_addr());
-                        }
-                    } else if (*ifa_addr).sa_len == SA_IN6_SIZE
-                        && (*ifa_addr).sa_family == libc::AF_INET6 as u8
-                    {
-                        if let Some(sockaddr) = SockAddrIn6::from_raw(ifa_addr) {
+                    // Skip entries without an address, but still advance to the next one.
+                    if !ifa_addr.is_null() {
+                        // Convert `ifa_addr` to `IpAddr`.
+                        // Note: `ifa_addr` is actually `sockaddr_in` or `sockaddr_in6` depending on
+                        // `sa_len` and `sa_family`.
+                        if (*ifa_addr).sa_len == SA_IN_SIZE && (*ifa_addr).sa_family == AF_INET {
+                            if let Some(sockaddr) = SockAddrIn::from_raw(ifa_addr) {
+                                addr_to_remove.push(sockaddr.ip_addr());
+                            }
+                        } else if (*ifa_addr).sa_len == SA_IN6_SIZE
+                            && (*ifa_addr).sa_family == libc::AF_INET6 as u8
+                            && let Some(sockaddr) = SockAddrIn6::from_raw(ifa_addr)
+                        {
                             addr_to_remove.push(sockaddr.ip_addr());
                         }
                     }

src/bsd/nvlist.rs

@@ -143,12 +143,12 @@ impl Error for NvListError {}
 impl fmt::Display for NvListError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            Self::NameTooLong => write!(f, "name is too long"),
-            Self::NotEnoughBytes => write!(f, "not enough bytes"),
-            Self::WrongHeader => write!(f, "wrong header"),
-            Self::WrongName => write!(f, "wrong name"),
-            Self::WrongPair => write!(f, "wrong name-value pair"),
-            Self::WrongPairData => write!(f, "wrong name-value pair data"),
+            Self::NameTooLong => f.write_str("name is too long"),
+            Self::NotEnoughBytes => f.write_str("not enough bytes"),
+            Self::WrongHeader => f.write_str("wrong header"),
+            Self::WrongName => f.write_str("wrong name"),
+            Self::WrongPair => f.write_str("wrong name-value pair"),
+            Self::WrongPairData => f.write_str("wrong name-value pair data"),
         }
     }
 }

src/bsd/route.rs

@@ -9,7 +9,7 @@ use std::{
 use libc::{ESRCH, PF_ROUTE, SHUT_RD, SOCK_RAW, read, shutdown, socket, write};
 
 use super::{
-    IoError, c_int_to_error, cast_bytes, cast_ref,
+    IoError, c_int_to_error, cast_bytes, read_unaligned,
     sockaddr::{SockAddrDl, SocketFromRaw, unpack_sockaddr},
 };
 
@@ -219,10 +219,10 @@ fn if_addr<S: SocketFromRaw>(if_name: &str) -> Option<S> {
         let mut addr = addrs;
         while !addr.is_null() {
             let name = unsafe { CStr::from_ptr((*addr).ifa_name) };
-            if name == ifname_c.as_c_str() {
-                if let Some(sockaddr) = unsafe { S::from_raw((*addr).ifa_addr) } {
-                    return Some(sockaddr);
-                }
+            if name == ifname_c.as_c_str()
+                && let Some(sockaddr) = unsafe { S::from_raw((*addr).ifa_addr) }
+            {
+                return Some(sockaddr);
             }
             addr = unsafe { (*addr).ifa_next };
         }
@@ -370,7 +370,7 @@ impl<Payload> RtMessage<Payload> {
             {
                 // not in table
             } else {
-                return Err(err)?;
+                Err(err)?;
             }
         }
 
@@ -388,7 +388,7 @@ impl<Payload> RtMessage<Payload> {
             {
                 return Ok(None); // not in table
             }
-            return Err(err)?;
+            Err(err)?;
         }
 
         let mut buf = [0u8; 256]; // FIXME: fixed buffer size
@@ -400,7 +400,7 @@ impl<Payload> RtMessage<Payload> {
             return Err(IoError::Unpack);
         }
 
-        let header = unsafe { cast_ref::<RtMsgHdr>(&buf) };
+        let header = unsafe { read_unaligned::<RtMsgHdr>(&buf) };
 
         let mut offset = size_of::<RtMsgHdr>();
         if header.rtm_addrs & RTA_DST != 0 {

src/bsd/sockaddr.rs

@@ -5,7 +5,7 @@ use std::{
     ptr::{copy, from_mut},
 };
 
-use super::{AF_INET, AF_INET6, AF_LINK, SA_IN_SIZE, SA_IN6_SIZE, cast_bytes, cast_ref};
+use super::{AF_INET, AF_INET6, AF_LINK, SA_IN_SIZE, SA_IN6_SIZE, cast_bytes, read_unaligned};
 
 pub(super) trait SocketFromRaw {
     unsafe fn from_raw(addr: *const libc::sockaddr) -> Option<Self>
@@ -212,20 +212,22 @@ pub(super) fn pack_sockaddr(sockaddr: &SocketAddr) -> Vec<u8> {
 
 pub(super) fn unpack_sockaddr(buf: &[u8]) -> Option<SocketAddr> {
     match buf.first() {
-        Some(&SA_IN_SIZE) => {
-            let sockaddr_in = unsafe { cast_ref::<SockAddrIn>(buf) };
+        // The leading byte is `sa_len`; verify the buffer actually holds a whole struct
+        // before reading it, otherwise a truncated buffer would be an out-of-bounds read.
+        Some(&SA_IN_SIZE) if buf.len() >= size_of::<SockAddrIn>() => {
+            let sockaddr_in = unsafe { read_unaligned::<SockAddrIn>(buf) };
             // sanity checks
             if sockaddr_in.family == AF_INET {
-                Some(sockaddr_in.into())
+                Some((&sockaddr_in).into())
             } else {
                 None
             }
         }
-        Some(&SA_IN6_SIZE) => {
-            let sockaddr_in6 = unsafe { cast_ref::<SockAddrIn6>(buf) };
+        Some(&SA_IN6_SIZE) if buf.len() >= size_of::<SockAddrIn6>() => {
+            let sockaddr_in6 = unsafe { read_unaligned::<SockAddrIn6>(buf) };
             // sanity checks
             if sockaddr_in6.family == AF_INET6 {
-                Some(sockaddr_in6.into())
+                Some((&sockaddr_in6).into())
             } else {
                 None
             }

src/bsd/timespec.rs

@@ -3,7 +3,7 @@ use std::{
     time::{Duration, SystemTime},
 };
 
-use super::{cast_bytes, cast_ref};
+use super::{cast_bytes, read_unaligned};
 
 #[repr(C)]
 struct TimeSpec {
@@ -50,8 +50,8 @@ pub(super) fn unpack_timespec(buf: &[u8]) -> Option<SystemTime> {
     const TS_SIZE: usize = size_of::<TimeSpec>();
     match buf.len() {
         TS_SIZE => {
-            let ts = unsafe { cast_ref::<TimeSpec>(buf) };
-            Some(ts.into())
+            let ts = unsafe { read_unaligned::<TimeSpec>(buf) };
+            Some((&ts).into())
         }
         _ => None,
     }

src/bsd/wgio.rs

@@ -1,7 +1,7 @@
 use std::{
     alloc::{Layout, alloc, dealloc},
     os::fd::AsRawFd,
-    ptr::null_mut,
+    ptr::{from_mut, from_ref, null_mut},
     slice::from_raw_parts,
 };
 
@@ -42,33 +42,37 @@ impl WgReadIo {
 
     /// Allocate data buffer.
     fn alloc_data(&mut self) -> Result<(), IoError> {
-        if self.wgd_data.is_null() {
-            if let Ok(layout) = Layout::array::<u8>(self.wgd_size) {
-                unsafe {
-                    self.wgd_data = alloc(layout);
-                }
-                return Ok(());
+        // Note: `alloc` with a zero-sized layout is undefined behaviour.
+        if self.wgd_size != 0
+            && self.wgd_data.is_null()
+            && let Ok(layout) = Layout::array::<u8>(self.wgd_size)
+        {
+            unsafe {
+                self.wgd_data = alloc(layout);
             }
+            Ok(())
+        } else {
+            Err(IoError::MemAlloc)
         }
-        Err(IoError::MemAlloc)
     }
 
     /// Return buffer as slice.
-    pub(super) fn as_slice<'a>(&self) -> &'a [u8] {
+    pub(super) fn as_slice(&self) -> &[u8] {
         unsafe { from_raw_parts(self.wgd_data, self.wgd_size) }
     }
 
     pub(super) fn read_data(&mut self) -> Result<(), IoError> {
         let socket = create_socket(AF_UNIX)?;
 
         // First do ioctl with empty `wg_data` to obtain buffer size.
-        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCGWG, &*self) };
+        // IMPORTANT: Pass a raw mutable pointer, so it's not optimized out..
+        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCGWG, from_mut::<Self>(self)) };
         c_int_to_error(result)?;
 
         // Allocate buffer.
         self.alloc_data()?;
         // Second call to ioctl with allocated buffer.
-        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCGWG, &*self) };
+        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCGWG, from_mut::<Self>(self)) };
         c_int_to_error(result)?;
 
         Ok(())
@@ -113,7 +117,8 @@ impl WgWriteIo {
 
     pub(super) fn write_data(&mut self) -> Result<(), IoError> {
         let socket = create_socket(AF_UNIX)?;
-        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCSWG, &*self) };
+        // `SIOCSWG` only reads the struct, so a shared pointer is correct here.
+        let result = unsafe { ioctl(socket.as_raw_fd(), SIOCSWG, from_ref::<Self>(self)) };
         c_int_to_error(result)?;
 
         Ok(())

src/host.rs

@@ -126,10 +126,10 @@ impl Host {
                         }
                     }
                     "allowed_ip" => {
-                        if let Some(ref mut peer) = peer_ref {
-                            if let Ok(addr) = value.parse() {
-                                peer.allowed_ips.push(addr);
-                            }
+                        if let Some(ref mut peer) = peer_ref
+                            && let Ok(addr) = value.parse()
+                        {
+                            peer.allowed_ips.push(addr);
                         }
                     }
                     "last_handshake_time_sec" => {
@@ -158,11 +158,11 @@ impl Host {
                     }
                     // "errno" ends config
                     "errno" => {
-                        if let Ok(errno) = value.parse::<u32>() {
-                            if errno == 0 {
-                                // Break here, or BufReader will wait for EOF.
-                                break;
-                            }
+                        if let Ok(errno) = value.parse::<u32>()
+                            && errno == 0
+                        {
+                            // Break here, or BufReader will wait for EOF.
+                            break;
                         }
                         return Err(io::Error::other("error reading UAPI"));
                     }