Fix modifying exceptions on macOS (#61)

The macOS sandbox is whitelist-based, denying everything and then step
by step removing restrictions with every exception added. As a result,
it wasn't possible to impose additional restrictions after a resource
was made available.

To allow restricting a specific resource even if a parent resource was
already granted full access, we now deny all path access for every
resource individually before granting new exceptions. As a result each
new access always starts out with a clean fully denied slate.

CHANGELOG.md

@@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Fixed
 
 - (Linux) Sandbox exceptions for symbolic links
+- (macOS) Modifying exceptions for paths affected by existing exceptions
 
 ## [v0.5.0] - 2023-10-13
 

Cargo.toml

@@ -49,6 +49,11 @@ name = "fs_readonly"
 path = "tests/fs_readonly.rs"
 harness = false
 
+[[test]]
+name = "fs_restrict_child"
+path = "tests/fs_restrict_child.rs"
+harness = false
+
 [[test]]
 name = "fs_write_also_read"
 path = "tests/fs_write_also_read.rs"

src/lib.rs

@@ -60,10 +60,6 @@ pub trait Sandbox: Sized {
 
     /// Add a new exception to the sandbox.
     ///
-    /// This exception opens up the sandbox to allow access for the specified
-    /// operation. Once an exception is added, it is **not** possible to
-    /// prohibit access to this resource without creating a new sandbox.
-    ///
     /// Exceptions added for symlinks will also automatically apply to the
     /// symlink's target.
     fn add_exception(&mut self, exception: Exception) -> Result<&mut Self>;

src/macos.rs

@@ -2,12 +2,14 @@
 //!
 //! This module implements sandboxing on macOS using `sandbox_init`.
 
+use std::collections::HashMap;
 use std::ffi::{CStr, CString};
-use std::io::Write;
-use std::path::PathBuf;
-use std::result::Result as StdResult;
+use std::io::{Result as IoResult, Write};
+use std::path::{Path, PathBuf};
 use std::{fs, ptr};
 
+use bitflags::bitflags;
+
 use crate::error::{Error, Result};
 use crate::{Exception, Sandbox};
 
@@ -28,48 +30,29 @@ static DEFAULT_RULE: &[u8] = b"\
 ";
 
 /// macOS sandboxing based on Seatbelt.
+#[derive(Default)]
 pub struct MacSandbox {
+    path_exceptions: HashMap<String, PathException>,
     env_exceptions: Vec<String>,
-    profile: Vec<u8>,
+    net_exception: bool,
     full_env: bool,
 }
 
 impl Sandbox for MacSandbox {
     fn new() -> Self {
-        Self { profile: DEFAULT_RULE.to_vec(), env_exceptions: Vec::new(), full_env: false }
+        Self::default()
     }
 
     fn add_exception(&mut self, exception: Exception) -> Result<&mut Self> {
-        // Temporary buffer to hold intermediate writes.
-        // Prevents errors from breaking the whole sandbox profile.
-        let mut buffer = Vec::new();
-
         match exception {
-            Exception::Read(path) => {
-                buffer.write_all(b"(allow file-read* (subpath ")?;
-                let escaped_path = escape_path(path)?;
-                buffer.write_all(escaped_path.as_bytes())?;
-                buffer.write_all(b"))\n")?;
-            },
+            Exception::Read(path) => self.update_path_exceptions(path, PathException::READ)?,
             Exception::WriteAndRead(path) => {
-                self.add_exception(Exception::Read(path.clone()))?;
-
-                buffer.write_all(b"(allow file-write* (subpath ")?;
-                let escaped_path = escape_path(path)?;
-                buffer.write_all(escaped_path.as_bytes())?;
-                buffer.write_all(b"))\n")?;
+                self.update_path_exceptions(path, PathException::WRITE | PathException::READ)?
             },
             Exception::ExecuteAndRead(path) => {
-                self.add_exception(Exception::Read(path.clone()))?;
-
-                buffer.write_all(b"(allow process-exec (subpath ")?;
-                let escaped_path = escape_path(path)?;
-                buffer.write_all(escaped_path.as_bytes())?;
-                buffer.write_all(b"))\n")?;
-            },
-            Exception::Networking => {
-                buffer.write_all(b"(allow network*)\n")?;
+                self.update_path_exceptions(path, PathException::EXECUTE | PathException::READ)?
             },
+            Exception::Networking => self.net_exception = true,
             Exception::Environment(key) => {
                 self.env_exceptions.push(key);
                 return Ok(self);
@@ -79,7 +62,6 @@ impl Sandbox for MacSandbox {
                 return Ok(self);
             },
         }
-        self.profile.write_all(&buffer)?;
         Ok(self)
     }
 
@@ -89,8 +71,10 @@ impl Sandbox for MacSandbox {
             crate::restrict_env_variables(&self.env_exceptions);
         }
 
-        let profile = CString::new(self.profile)
-            .map_err(|_| Error::ActivationFailed("invalid profile".into()))?;
+        // Create the seatbelt sandbox profile.
+        let profile = self.create_profile()?;
+        let profile =
+            CString::new(profile).map_err(|_| Error::ActivationFailed("invalid profile".into()))?;
 
         let mut error = ptr::null_mut();
         let result = unsafe { sandbox_init(profile.as_ptr(), 0, &mut error) };
@@ -111,14 +95,135 @@ impl Sandbox for MacSandbox {
     }
 }
 
+impl MacSandbox {
+    /// Add or modify a path's exceptions.
+    fn update_path_exceptions(&mut self, path: PathBuf, exceptions: PathException) -> Result<()> {
+        // Canonicalize all exception paths.
+        //
+        // Since the macOS sandbox only cares about permissions for symlink targets, due
+        // to the `(allow file-read-metadata)` rule, we don't need to bother with
+        // keeping the original paths.
+        let escaped_path = escape_path(&path)?;
+
+        let exception = self.path_exceptions.entry(escaped_path).or_insert(PathException::empty());
+        exception.insert(exceptions);
+
+        Ok(())
+    }
+
+    /// Create a seatbelt profile for the requested sandbox configuration.
+    fn create_profile(&self) -> Result<Vec<u8>> {
+        let mut profile = DEFAULT_RULE.to_vec();
+
+        // Sort by component count to ensure parent paths appear before descendants.
+        let mut path_exceptions: Vec<_> = self.path_exceptions.iter().collect();
+        path_exceptions.sort_unstable_by(|a, b| a.0.len().cmp(&b.0.len()));
+
+        for (path, exception) in path_exceptions {
+            // Deny all access to clear existing permission grants.
+            Self::revoke_path_access(&mut profile, path)?;
+
+            if exception.contains(PathException::READ) {
+                let rule = PathRule::new(RuleMode::Allow, "file-read*", path.into());
+                rule.write_to(&mut profile)?;
+            }
+            if exception.contains(PathException::WRITE) {
+                let rule = PathRule::new(RuleMode::Allow, "file-write*", path.into());
+                rule.write_to(&mut profile)?;
+            }
+            if exception.contains(PathException::EXECUTE) {
+                let rule = PathRule::new(RuleMode::Allow, "process-exec", path.into());
+                rule.write_to(&mut profile)?;
+            }
+        }
+
+        if self.net_exception {
+            profile.write_all(b"(allow network*)\n")?;
+        }
+
+        Ok(profile)
+    }
+
+    /// Revoke all access permisisons for a path.
+    ///
+    /// This is necessary to grant more restrictive permissions to a child of a
+    /// directory which was previously granted permissions.
+    fn revoke_path_access(buffer: &mut Vec<u8>, path: &str) -> Result<()> {
+        let rule = PathRule::new(RuleMode::Deny, "file-read*", path.into());
+        rule.write_to(buffer)?;
+
+        let rule = PathRule::new(RuleMode::Deny, "file-write*", path.into());
+        rule.write_to(buffer)?;
+
+        let rule = PathRule::new(RuleMode::Deny, "process-exec", path.into());
+        rule.write_to(buffer)?;
+
+        Ok(())
+    }
+}
+
+struct PathRule {
+    mode: RuleMode,
+    access_type: &'static str,
+    path: String,
+}
+
+impl PathRule {
+    fn new(mode: RuleMode, access_type: &'static str, path: String) -> Self {
+        Self { mode, access_type, path }
+    }
+
+    /// Write this rule to a profile.
+    fn write_to(&self, buffer: &mut Vec<u8>) -> IoResult<()> {
+        buffer.write_all(b"(")?;
+        buffer.write_all(self.mode.as_str().as_bytes())?;
+        buffer.write_all(b" ")?;
+
+        buffer.write_all(self.access_type.as_bytes())?;
+
+        buffer.write_all(b" (subpath ")?;
+        buffer.write_all(self.path.as_bytes())?;
+        buffer.write_all(b"))\n")?;
+
+        Ok(())
+    }
+}
+
+/// Mode for a seatbelt rule.
+enum RuleMode {
+    Allow,
+    Deny,
+}
+
+impl RuleMode {
+    fn as_str(&self) -> &str {
+        match self {
+            Self::Allow => "allow",
+            Self::Deny => "deny",
+        }
+    }
+}
+
+bitflags! {
+    /// Types of sandbox filesystem exceptions.
+    struct PathException: u8 {
+        const EXECUTE = 0b0001;
+        const WRITE   = 0b0010;
+        const READ    = 0b0100;
+    }
+}
+
 /// Escape a path: /tt/in\a"x -> "/tt/in\\a\"x"
-fn escape_path(path: PathBuf) -> StdResult<String, Error> {
+fn escape_path(path: &Path) -> Result<String> {
     // Canonicalize the incoming path to support relative paths.
     // The `subpath` action only allows absolute paths.
-    let canonical_path = fs::canonicalize(&path).map_err(|_| Error::InvalidPath(path.clone()))?;
+    let canonical_path =
+        fs::canonicalize(&path).map_err(|_| Error::InvalidPath(path.to_path_buf()))?;
 
-    let mut path_str =
-        canonical_path.into_os_string().into_string().map_err(|_| Error::InvalidPath(path))?;
+    let mut path_str = canonical_path
+        .into_os_string()
+        .into_string()
+        .map_err(|_| Error::InvalidPath(path.to_path_buf()))?;
     // Paths in `subpath` expressions must not end with /.
     while path_str.ends_with('/') && path_str != "/" {
         String::pop(&mut path_str);

tests/fs_restrict_child.rs

@@ -0,0 +1,29 @@
+use std::fs;
+
+use birdcage::{Birdcage, Exception, Sandbox};
+
+fn main() {
+    const FILE_CONTENT: &str = "expected content";
+
+    // Setup our test tree.
+    let tempdir = tempfile::tempdir().unwrap().into_path();
+    let tempfile = tempdir.join("target-file");
+    fs::write(&tempfile, FILE_CONTENT.as_bytes()).unwrap();
+
+    // Setup sandbox, allowing read/write to dir, but only read for the file.
+    let mut birdcage = Birdcage::new();
+    birdcage.add_exception(Exception::WriteAndRead(tempdir.clone())).unwrap();
+    birdcage.add_exception(Exception::Read(tempfile.clone())).unwrap();
+    birdcage.lock().unwrap();
+
+    // Write access to directory works.
+    fs::create_dir(tempdir.join("boop")).unwrap();
+
+    // Read access to file works.
+    let content = fs::read_to_string(&tempfile).unwrap();
+    assert_eq!(content, FILE_CONTENT);
+
+    // Write access to file is denied.
+    let result = fs::write(&tempfile, "no");
+    assert!(result.is_err());
+}