From 6d47169a5cee6319ebf419e375635e1fac2d48fc Mon Sep 17 00:00:00 2001
From: Karolin Varner <karo@cupdev.net>
Date: Sun, 4 Aug 2024 21:16:09 +0200
Subject: [PATCH] feat: Set CLOEXEC flag on claimed fds and mask them
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Masking the file descriptors (by replaying them with a file descriptor pointing towards /dev/null)
mitigates use after free (on file descriptor) attacks. In case some
piece of code still holds a reference to the file descriptor, that
file descriptor now merely holds a reference to /dev/null.

Otherwise, the file descriptor might be reused and the reference
could now mistakenly point to all sorts of – potentially more harmful – files, such as memfd_secret
file descriptors, storing our secret keys.
---
 Cargo.toml      |  2 +-
 util/src/fd.rs  | 54 +++++++++++++++++++++++++++++++++++--------
 util/src/mem.rs | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 10 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index ab829ff..555af42 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -83,4 +83,4 @@ procspawn = {version = "1.0.0", features= ["test-support"]}
 #Broker dependencies (might need cleanup or changes)
 wireguard-uapi = { version = "3.0.0", features = ["xplatform"] }
 command-fds = "0.2.3"
-rustix = { version = "0.38.27", features = ["net"] }
+rustix = { version = "0.38.27", features = ["net", "fs"] }
diff --git a/util/src/fd.rs b/util/src/fd.rs
index cf5eaf9..0268fa3 100644
--- a/util/src/fd.rs
+++ b/util/src/fd.rs
@@ -1,12 +1,48 @@
-use std::os::fd::{OwnedFd, RawFd};
+use rustix::{
+    fd::{AsFd, BorrowedFd, FromRawFd, OwnedFd, RawFd},
+    io::{fcntl_dupfd_cloexec, DupFlags},
+};
 
-/// Clone some file descriptor
+use crate::mem::Forgetting;
+
+/// Prepare a file descriptor for use in Rust code.
 ///
-/// If the file descriptor is invalid, an error will be raised.
-pub fn claim_fd(fd: RawFd) -> anyhow::Result<OwnedFd> {
-    use rustix::{fd::BorrowedFd, io::dup};
-
-    // This is safe since [dup] will simply raise
-    let fd = unsafe { dup(BorrowedFd::borrow_raw(fd))? };
-    Ok(fd)
+/// Checks if the file descriptor is valid and duplicates it to a new file descriptor.
+/// The old file descriptor is masked to avoid potential use after free (on file descriptor)
+/// in case the given file descriptor is still used somewhere
+pub fn claim_fd(fd: RawFd) -> rustix::io::Result<OwnedFd> {
+    let new = clone_fd_cloexec(unsafe { BorrowedFd::borrow_raw(fd) })?;
+    mask_fd(fd)?;
+    Ok(new)
+}
+
+pub fn mask_fd(fd: RawFd) -> rustix::io::Result<()> {
+    let mut owned = Forgetting::new(unsafe { OwnedFd::from_raw_fd(fd) });
+    clone_fd_to_cloexec(open_nullfd()?, &mut owned)
+}
+
+pub fn clone_fd_cloexec<Fd: AsFd>(fd: Fd) -> rustix::io::Result<OwnedFd> {
+    const MINFD: RawFd = 3; // Avoid stdin, stdout, and stderr
+    fcntl_dupfd_cloexec(fd, MINFD)
+}
+
+#[cfg(target_os = "linux")]
+pub fn clone_fd_to_cloexec<Fd: AsFd>(fd: Fd, new: &mut OwnedFd) -> rustix::io::Result<()> {
+    use rustix::io::dup3;
+    dup3(fd, new, DupFlags::CLOEXEC)
+}
+
+#[cfg(not(target_os = "linux"))]
+pub fn clone_fd_to_cloexec<Fd: AsFd>(fd: Fd, new: &mut OwnedFd) -> rustix::io::Result<()> {
+    use rustix::io::{dup2, fcntl_setfd, FdFlags};
+    dup2(&fd, new)?;
+    fcntl_setfd(&new, FdFlags::CLOEXEC)
+}
+
+/// Open a "blocked" file descriptor. I.e. a file descriptor that is neither meant for reading nor
+/// writing
+pub fn open_nullfd() -> rustix::io::Result<OwnedFd> {
+    use rustix::fs::{open, Mode, OFlags};
+    // TODO: Add tests showing that this will throw errors on use
+    open("/dev/null", OFlags::CLOEXEC, Mode::empty())
 }
diff --git a/util/src/mem.rs b/util/src/mem.rs
index be3d0e8..620e3d8 100644
--- a/util/src/mem.rs
+++ b/util/src/mem.rs
@@ -1,5 +1,7 @@
 use std::borrow::{Borrow, BorrowMut};
 use std::cmp::min;
+use std::mem::{forget, swap};
+use std::ops::{Deref, DerefMut};
 
 /// Concatenate two byte arrays
 // TODO: Zeroize result?
@@ -31,3 +33,62 @@ pub fn cpy_min<T: BorrowMut<[u8]> + ?Sized, F: Borrow<[u8]> + ?Sized>(src: &F, d
     let len = min(src.len(), dst.len());
     dst[..len].copy_from_slice(&src[..len]);
 }
+
+/// Wrapper type to inhibit calling [std::mem::Drop] when the underlying variable is freed
+#[derive(PartialEq, Eq, PartialOrd, Ord, Debug, Clone, Default)]
+pub struct Forgetting<T> {
+    value: Option<T>,
+}
+
+impl<T> Forgetting<T> {
+    pub fn new(value: T) -> Self {
+        let value = Some(value);
+        Self { value }
+    }
+
+    pub fn extract(mut self) -> T {
+        let mut value = None;
+        swap(&mut value, &mut self.value);
+        value.unwrap()
+    }
+}
+
+impl<T> From<T> for Forgetting<T> {
+    fn from(value: T) -> Self {
+        Self::new(value)
+    }
+}
+
+impl<T> Deref for Forgetting<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        self.value.as_ref().unwrap()
+    }
+}
+
+impl<T> DerefMut for Forgetting<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        self.value.as_mut().unwrap()
+    }
+}
+
+impl<T> Borrow<T> for Forgetting<T> {
+    fn borrow(&self) -> &T {
+        self.deref()
+    }
+}
+
+impl<T> BorrowMut<T> for Forgetting<T> {
+    fn borrow_mut(&mut self) -> &mut T {
+        self.deref_mut()
+    }
+}
+
+impl<T> Drop for Forgetting<T> {
+    fn drop(&mut self) {
+        let mut value = None;
+        swap(&mut self.value, &mut value);
+        forget(value)
+    }
+}