Skip to content

Commit

Permalink
Fix Linux sandbox error handling
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
cd-work committed Oct 11, 2023
1 parent b4e61dc commit 545df0b
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 50 deletions.
5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
43 changes: 3 additions & 40 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::error::Error as StdError;
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")]
Expand All @@ -21,8 +22,7 @@ pub enum Error {
Seccomp(SeccompError),

/// Invalid sandbox exception path.
#[cfg(target_os = "macos")]
InvalidPath(InvalidPathError),
InvalidPath(PathBuf),

/// I/O error.
Io(IoError),
Expand All @@ -38,8 +38,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}")
Expand All @@ -62,44 +61,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())
}
}
11 changes: 10 additions & 1 deletion src/linux/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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),
Expand Down
19 changes: 10 additions & 9 deletions src/macos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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))?;

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" {
Expand Down
20 changes: 20 additions & 0 deletions tests/missing_exception.rs
Original file line number Diff line number Diff line change
@@ -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();
}

0 comments on commit 545df0b

Please sign in to comment.