Fix Linux sandbox error handling (#51)

This fixes an issue with the Linux sandbox where invalid paths would
only cause an error during `Birdcage::lock`, at which point the consumer
cannot handle or ignore this error anymore since the sandbox creation
has already been attempted.

CHANGELOG.md

@@ -6,6 +6,12 @@ The sections should follow the order `Packaging`, `Added`, `Changed`, `Fixed` an
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
+## [Unreleased]
+
+### Changed
+
+- (Linux) Report invalid paths when adding exceptions
+
 ## [0.4.0] - 2023-10-09
 
 ### Added

Cargo.toml

@@ -54,6 +54,11 @@ name = "seccomp"
 path = "tests/seccomp.rs"
 harness = false
 
+[[test]]
+name = "missing_exception"
+path = "tests/missing_exception.rs"
+harness = false
+
 [target.'cfg(target_os = "linux")'.dependencies]
 seccompiler = "0.3.0"
 libc = "0.2.132"

src/error.rs

@@ -1,10 +1,9 @@
 //! Sandboxing errors.
 
 use std::error::Error as StdError;
-#[cfg(target_os = "macos")]
-use std::ffi::OsString;
 use std::fmt::{self, Display, Formatter};
 use std::io::Error as IoError;
+use std::path::PathBuf;
 use std::result::Result as StdResult;
 
 #[cfg(target_os = "linux")]
@@ -21,8 +20,7 @@ pub enum Error {
     Seccomp(SeccompError),
 
     /// Invalid sandbox exception path.
-    #[cfg(target_os = "macos")]
-    InvalidPath(InvalidPathError),
+    InvalidPath(PathBuf),
 
     /// I/O error.
     Io(IoError),
@@ -38,8 +36,7 @@ impl Display for Error {
         match self {
             #[cfg(target_os = "linux")]
             Self::Seccomp(error) => write!(f, "seccomp error: {error}"),
-            #[cfg(target_os = "macos")]
-            Self::InvalidPath(error) => write!(f, "invalid path: {error:?}"),
+            Self::InvalidPath(path) => write!(f, "invalid path: {path:?}"),
             Self::Io(error) => write!(f, "input/output error: {error}"),
             Self::ActivationFailed(error) => {
                 write!(f, "failed to initialize a sufficient sandbox: {error}")
@@ -62,44 +59,8 @@ impl From<BackendError> for Error {
     }
 }
 
-#[cfg(target_os = "macos")]
-impl From<InvalidPathError> for Error {
-    fn from(error: InvalidPathError) -> Self {
-        Self::InvalidPath(error)
-    }
-}
-
 impl From<IoError> for Error {
     fn from(error: IoError) -> Self {
         Self::Io(error)
     }
 }
-
-/// Invalid sandbox exception path.
-#[cfg(target_os = "macos")]
-#[derive(Debug)]
-pub struct InvalidPathError(String);
-
-#[cfg(target_os = "macos")]
-impl StdError for InvalidPathError {}
-
-#[cfg(target_os = "macos")]
-impl Display for InvalidPathError {
-    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        write!(f, "invalid path: {}", self.0)
-    }
-}
-
-#[cfg(target_os = "macos")]
-impl From<IoError> for InvalidPathError {
-    fn from(error: IoError) -> Self {
-        InvalidPathError(error.to_string())
-    }
-}
-
-#[cfg(target_os = "macos")]
-impl From<OsString> for InvalidPathError {
-    fn from(error: OsString) -> Self {
-        InvalidPathError(error.to_string_lossy().into_owned())
-    }
-}

src/linux/mod.rs

@@ -4,7 +4,7 @@ use std::collections::HashMap;
 use std::io::Error as IoError;
 use std::path::PathBuf;
 
-use crate::error::Result;
+use crate::error::{Error, Result};
 use crate::linux::namespaces::MountFlags;
 use crate::linux::seccomp::SyscallFilter;
 use crate::{Exception, Sandbox};
@@ -49,6 +49,15 @@ impl Sandbox for LinuxSandbox {
     }
 
     fn add_exception(&mut self, exception: Exception) -> Result<&mut Self> {
+        // Report error if exception is added for an invalid path.
+        if let Exception::Read(path) | Exception::Write(path) | Exception::ExecuteAndRead(path) =
+            &exception
+        {
+            if !path.exists() {
+                return Err(Error::InvalidPath(path.into()));
+            }
+        }
+
         match exception {
             Exception::Read(path) => self.update_bind_mount(path, false, false),
             Exception::Write(path) => self.update_bind_mount(path, true, false),

src/macos.rs

@@ -8,7 +8,7 @@ use std::path::PathBuf;
 use std::result::Result as StdResult;
 use std::{fs, ptr};
 
-use crate::error::{Error, InvalidPathError, Result};
+use crate::error::{Error, Result};
 use crate::{Exception, Sandbox};
 
 /// Deny-all fallback rule.
@@ -110,19 +110,20 @@ impl Sandbox for MacSandbox {
 }
 
 /// Escape a path: /tt/in\a"x -> "/tt/in\\a\"x"
-fn escape_path(path: PathBuf) -> StdResult<String, InvalidPathError> {
+fn escape_path(path: PathBuf) -> StdResult<String, Error> {
     // Canonicalize the incoming path to support relative paths.
     // The `subpath` action only allows absolute paths.
-    let path = fs::canonicalize(path)?;
+    let canonical_path = fs::canonicalize(&path).map_err(|_| Error::InvalidPath(path.clone()))?;
 
-    let mut path = path.into_os_string().into_string()?;
+    let mut path_str =
+        canonical_path.into_os_string().into_string().map_err(|_| Error::InvalidPath(path))?;
     // Paths in `subpath` expressions must not end with /.
-    while path.ends_with('/') && path != "/" {
-        String::pop(&mut path);
+    while path_str.ends_with('/') && path_str != "/" {
+        String::pop(&mut path_str);
     }
-    path = path.replace('"', r#"\""#);
-    path = path.replace('\\', r#"\\"#);
-    Ok(format!("\"{path}\""))
+    path_str = path_str.replace('"', r#"\""#);
+    path_str = path_str.replace('\\', r#"\\"#);
+    Ok(format!("\"{path_str}\""))
 }
 
 extern "C" {

tests/missing_exception.rs

@@ -0,0 +1,20 @@
+use std::path::PathBuf;
+
+use birdcage::error::Error;
+use birdcage::{Birdcage, Exception, Sandbox};
+
+fn main() {
+    let mut birdcage = Birdcage::new();
+
+    // Add a path that doesn't exist.
+    let result = birdcage.add_exception(Exception::Read("/does/not/exist".into()));
+
+    // Ensure it is appropriately reported that exception was NOT added.
+    match result {
+        Err(Error::InvalidPath(path)) => assert_eq!(path, PathBuf::from("/does/not/exist")),
+        _ => panic!("expected path error"),
+    }
+
+    // Ensure locking is always successful.
+    birdcage.lock().unwrap();
+}