From 04263688d01fb8426f9f9ced369aaec23d312632 Mon Sep 17 00:00:00 2001 From: Christian Duerr Date: Mon, 30 Oct 2023 21:14:09 +0100 Subject: [PATCH 1/4] Fix symlink/canonical path exception conflicts This fixes an issue on Linux where adding an exception for a path would override all previously set permissions for the canonical path if the old and new uncanonicalized paths did not match. --- Cargo.toml | 5 + src/linux/mod.rs | 164 ++++++++++++++++++++----- src/linux/namespaces.rs | 112 ++--------------- tests/fs_symlink_dir_separate_perms.rs | 30 +++++ 4 files changed, 179 insertions(+), 132 deletions(-) create mode 100644 tests/fs_symlink_dir_separate_perms.rs diff --git a/Cargo.toml b/Cargo.toml index f34faf4..570cbd8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -74,6 +74,11 @@ name = "fs_broken_symlink" path = "tests/fs_broken_symlink.rs" harness = false +[[test]] +name = "fs_symlink_dir_separate_perms" +path = "tests/fs_symlink_dir_separate_perms.rs" +harness = false + [[test]] name = "fs_null" path = "tests/fs_null.rs" diff --git a/src/linux/mod.rs b/src/linux/mod.rs index fe04997..8dbbedc 100644 --- a/src/linux/mod.rs +++ b/src/linux/mod.rs @@ -2,7 +2,9 @@ use std::collections::HashMap; use std::io::Error as IoError; -use std::path::PathBuf; +use std::os::unix::ffi::OsStrExt; +use std::path::{Component, Path, PathBuf}; +use std::{env, io}; use crate::error::{Error, Result}; use crate::linux::namespaces::MountAttrFlags; @@ -15,34 +17,12 @@ mod seccomp; /// Linux sandboxing. #[derive(Default)] pub struct LinuxSandbox { - bind_mounts: HashMap, env_exceptions: Vec, + path_exceptions: PathExceptions, allow_networking: bool, full_env: bool, } -impl LinuxSandbox { - /// Add or modify a bind mount. - /// - /// This will add a new bind mount with the specified permission if it does - /// not exist already. - /// - /// If the bind mount already exists, it will *ADD* the additional - /// permissions. - fn update_bind_mount(&mut self, path: PathBuf, write: bool, execute: bool) { - let flags = - self.bind_mounts.entry(path).or_insert(MountAttrFlags::RDONLY | MountAttrFlags::NOEXEC); - - if write { - flags.remove(MountAttrFlags::RDONLY); - } - - if execute { - flags.remove(MountAttrFlags::NOEXEC); - } - } -} - impl Sandbox for LinuxSandbox { fn new() -> Self { Self::default() @@ -60,9 +40,9 @@ impl Sandbox for LinuxSandbox { } match exception { - Exception::Read(path) => self.update_bind_mount(path, false, false), - Exception::WriteAndRead(path) => self.update_bind_mount(path, true, false), - Exception::ExecuteAndRead(path) => self.update_bind_mount(path, false, true), + Exception::Read(path) => self.path_exceptions.update(path, false, false), + Exception::WriteAndRead(path) => self.path_exceptions.update(path, true, false), + Exception::ExecuteAndRead(path) => self.path_exceptions.update(path, false, true), Exception::Environment(key) => self.env_exceptions.push(key), Exception::FullEnvironment => self.full_env = true, Exception::Networking => self.allow_networking = true, @@ -78,7 +58,7 @@ impl Sandbox for LinuxSandbox { } // Setup namespaces. - namespaces::create_namespaces(self.allow_networking, self.bind_mounts)?; + namespaces::create_namespaces(self.allow_networking, self.path_exceptions)?; // Setup system call filters. SyscallFilter::apply()?; @@ -93,6 +73,62 @@ impl Sandbox for LinuxSandbox { } } +/// Path permissions required for the sandbox. +#[derive(Default)] +pub(crate) struct PathExceptions { + bind_mounts: HashMap, + symlinks: Vec<(PathBuf, PathBuf)>, +} + +impl PathExceptions { + /// Add or modify a path's exceptions. + /// + /// This will add a new bind mount for the canonical path with the specified + /// permission if it does not exist already. + /// + /// If the bind mount already exists, it will *ADD* the additional + /// permissions. + fn update(&mut self, path: PathBuf, write: bool, execute: bool) { + // Use canonical path for indexing. + // + // This ensures that a symlink and its target are treated like the same path for + // exceptions. + // + // If the home path cannot be accessed, we ignore the exception. + let canonical_path = match path.canonicalize() { + Ok(path) => path, + Err(_) => return, + }; + + // Store original symlink path to create it if necessary. + if path_has_symlinks(&path) { + // Normalize symlink's path. + let absolute = match absolute(&path) { + Ok(absolute) => absolute, + Err(_) => return, + }; + let normalized = normalize_path(&absolute); + + self.symlinks.push((normalized, canonical_path.clone())); + } + + // Update bind mount's permission flags. + + let flags = self + .bind_mounts + .entry(canonical_path) + .or_insert(MountAttrFlags::RDONLY | MountAttrFlags::NOEXEC); + + if write { + flags.remove(MountAttrFlags::RDONLY); + } + + if execute { + flags.remove(MountAttrFlags::NOEXEC); + } + } +} + /// Prevent suid/sgid. fn no_new_privs() -> Result<()> { let result = unsafe { libc::prctl(libc::PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) }; @@ -102,3 +138,75 @@ fn no_new_privs() -> Result<()> { _ => Err(IoError::last_os_error().into()), } } + +// Copied from Rust's STD: +// https://github.com/rust-lang/rust/blob/42faef503f3e765120ca0ef06991337668eafc32/library/std/src/sys/unix/path.rs#L23C1-L63C2 +// +// Licensed under MIT: +// https://github.com/rust-lang/rust/blob/master/LICENSE-MIT +// +/// Make a POSIX path absolute without changing its semantics. +fn absolute(path: &Path) -> io::Result { + // This is mostly a wrapper around collecting `Path::components`, with + // exceptions made where this conflicts with the POSIX specification. + // See 4.13 Pathname Resolution, IEEE Std 1003.1-2017 + // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13 + + // Get the components, skipping the redundant leading "." component if it + // exists. + let mut components = path.strip_prefix(".").unwrap_or(path).components(); + let path_os = path.as_os_str().as_bytes(); + + let mut normalized = if path.is_absolute() { + // "If a pathname begins with two successive characters, the + // first component following the leading characters may be + // interpreted in an implementation-defined manner, although more than + // two leading characters shall be treated as a single + // character." + if path_os.starts_with(b"//") && !path_os.starts_with(b"///") { + components.next(); + PathBuf::from("//") + } else { + PathBuf::new() + } + } else { + env::current_dir()? + }; + normalized.extend(components); + + // "Interfaces using pathname resolution may specify additional constraints + // when a pathname that does not name an existing directory contains at + // least one non- character and contains one or more trailing + // characters". + // A trailing is also meaningful if "a symbolic link is + // encountered during pathname resolution". + if path_os.ends_with(b"/") { + normalized.push(""); + } + + Ok(normalized) +} + +/// Normalize path components, stripping out `.` and `..`. +fn normalize_path(path: &Path) -> PathBuf { + let mut normalized = PathBuf::new(); + + for component in path.components() { + match component { + Component::Prefix(_) => unreachable!("impl does not consider windows"), + Component::RootDir => normalized.push("/"), + Component::CurDir => continue, + Component::ParentDir => { + normalized.pop(); + }, + Component::Normal(segment) => normalized.push(segment), + } + } + + normalized +} + +/// Check if a path contains any symlinks. +fn path_has_symlinks(path: &Path) -> bool { + path.ancestors().any(|path| path.read_link().is_ok()) +} diff --git a/src/linux/namespaces.rs b/src/linux/namespaces.rs index 382f130..f74bfee 100644 --- a/src/linux/namespaces.rs +++ b/src/linux/namespaces.rs @@ -1,18 +1,18 @@ //! Linux namespaces. use std::cmp::Ordering; -use std::collections::HashMap; use std::ffi::{CStr, CString}; use std::fs::{self, File}; use std::io::Error as IoError; use std::os::unix::ffi::OsStrExt; use std::os::unix::fs as unixfs; -use std::path::{Component, Path, PathBuf}; -use std::{env, io, mem, ptr}; +use std::path::{Path, PathBuf, Component}; +use std::{env, mem, ptr}; use bitflags::bitflags; use crate::error::Result; +use crate::linux::PathExceptions; /// Path for mount namespace's new root. const NEW_ROOT: &str = "/tmp/birdcage-root"; @@ -23,10 +23,7 @@ const NEW_ROOT: &str = "/tmp/birdcage-root"; /// /// Additionally it will isolate network access if `allow_networking` is /// `false`. -pub fn create_namespaces( - allow_networking: bool, - bind_mounts: HashMap, -) -> Result<()> { +pub fn create_namespaces(allow_networking: bool, exceptions: PathExceptions) -> Result<()> { // Get EUID/EGID outside of the namespace. let uid = unsafe { libc::geteuid() }; let gid = unsafe { libc::getegid() }; @@ -37,7 +34,7 @@ pub fn create_namespaces( } // Isolate filesystem and procfs. - create_mount_namespace(bind_mounts)?; + create_mount_namespace(exceptions)?; // Drop root user mapping and ensure abstract namespace is cleared. create_user_namespace(uid, gid, Namespaces::empty())?; @@ -49,7 +46,7 @@ pub fn create_namespaces( /// /// This will deny access to any path which isn't part of `bind_mounts`. Allowed /// paths are mounted according to their bind mount flags. -fn create_mount_namespace(bind_mounts: HashMap) -> Result<()> { +fn create_mount_namespace(exceptions: PathExceptions) -> Result<()> { // Create mount namespace to allow creation of new mounts. create_user_namespace(0, 0, Namespaces::MOUNT)?; @@ -68,29 +65,8 @@ fn create_mount_namespace(bind_mounts: HashMap) -> Resu // aren't created outside the sandbox. mount_tmpfs(&new_root_c)?; - // Canonicalize paths and resolve symlinks. - // - // If the working directory cannot be accessed, we ignore relative paths. - let mut symlinks = Vec::new(); - let mut bind_mounts = bind_mounts - .into_iter() - .filter_map(|(path, exception)| { - let canonicalized = path.canonicalize().ok()?; - - // Store original symlink path to create it if necessary. - if path_has_symlinks(&path) { - // Normalize symlink's path. - let absolute = absolute(&path).ok()?; - let normalized = normalize_path(&absolute); - - symlinks.push((normalized, canonicalized.clone())); - } - - Some((canonicalized, exception)) - }) - .collect::>(); - // Sort bind mounts by shortest length, to create parents before their children. + let mut bind_mounts: Vec<_> = exceptions.bind_mounts.into_iter().collect(); bind_mounts.sort_unstable_by(|(a_path, a_flags), (b_path, b_flags)| { match a_path.components().count().cmp(&b_path.components().count()) { Ordering::Equal => (a_path, a_flags).cmp(&(b_path, b_flags)), @@ -118,7 +94,7 @@ fn create_mount_namespace(bind_mounts: HashMap) -> Resu } // Ensure original symlink paths are available. - create_symlinks(&new_root, symlinks)?; + create_symlinks(&new_root, exceptions.symlinks)?; // Bind mount old procfs. let old_proc_c = CString::new("/proc").unwrap(); @@ -457,75 +433,3 @@ bitflags! { const SYSVSEM = libc::CLONE_SYSVSEM; } } - -// Copied from Rust's STD: -// https://github.com/rust-lang/rust/blob/42faef503f3e765120ca0ef06991337668eafc32/library/std/src/sys/unix/path.rs#L23C1-L63C2 -// -// Licensed under MIT: -// https://github.com/rust-lang/rust/blob/master/LICENSE-MIT -// -/// Make a POSIX path absolute without changing its semantics. -fn absolute(path: &Path) -> io::Result { - // This is mostly a wrapper around collecting `Path::components`, with - // exceptions made where this conflicts with the POSIX specification. - // See 4.13 Pathname Resolution, IEEE Std 1003.1-2017 - // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13 - - // Get the components, skipping the redundant leading "." component if it - // exists. - let mut components = path.strip_prefix(".").unwrap_or(path).components(); - let path_os = path.as_os_str().as_bytes(); - - let mut normalized = if path.is_absolute() { - // "If a pathname begins with two successive characters, the - // first component following the leading characters may be - // interpreted in an implementation-defined manner, although more than - // two leading characters shall be treated as a single - // character." - if path_os.starts_with(b"//") && !path_os.starts_with(b"///") { - components.next(); - PathBuf::from("//") - } else { - PathBuf::new() - } - } else { - env::current_dir()? - }; - normalized.extend(components); - - // "Interfaces using pathname resolution may specify additional constraints - // when a pathname that does not name an existing directory contains at - // least one non- character and contains one or more trailing - // characters". - // A trailing is also meaningful if "a symbolic link is - // encountered during pathname resolution". - if path_os.ends_with(b"/") { - normalized.push(""); - } - - Ok(normalized) -} - -/// Normalize path components, stripping out `.` and `..`. -fn normalize_path(path: &Path) -> PathBuf { - let mut normalized = PathBuf::new(); - - for component in path.components() { - match component { - Component::Prefix(_) => unreachable!("impl does not consider windows"), - Component::RootDir => normalized.push("/"), - Component::CurDir => continue, - Component::ParentDir => { - normalized.pop(); - }, - Component::Normal(segment) => normalized.push(segment), - } - } - - normalized -} - -/// Check if a path contains any symlinks. -fn path_has_symlinks(path: &Path) -> bool { - path.ancestors().any(|path| path.read_link().is_ok()) -} diff --git a/tests/fs_symlink_dir_separate_perms.rs b/tests/fs_symlink_dir_separate_perms.rs new file mode 100644 index 0000000..d652856 --- /dev/null +++ b/tests/fs_symlink_dir_separate_perms.rs @@ -0,0 +1,30 @@ +use std::fs; +use std::os::unix::fs as unixfs; + +use birdcage::{Birdcage, Exception, Sandbox}; +use tempfile::TempDir; + +fn main() { + const FILE_CONTENT: &str = "expected content"; + + // Setup our test directories. + let tempdir = TempDir::new().unwrap().into_path(); + let symlink_src = tempdir.join("src"); + fs::create_dir(&symlink_src).unwrap(); + let symlink_dst = tempdir.join("dst"); + unixfs::symlink(&symlink_src, &symlink_dst).unwrap(); + + // Add read+write for src, but also add readonly for dst. + let mut birdcage = Birdcage::new(); + birdcage.add_exception(Exception::WriteAndRead(symlink_src.clone())).unwrap(); + birdcage.add_exception(Exception::Read(symlink_dst.clone())).unwrap(); + birdcage.lock().unwrap(); + + // Ensure writing works. + let testfile = symlink_src.join("file"); + fs::write(&testfile, FILE_CONTENT).unwrap(); + + // Ensure reading works. + let content = fs::read_to_string(&testfile).unwrap(); + assert_eq!(content, FILE_CONTENT); +} From 6b5bc2d9fbef53db9f9ebaa70172e27c2d2f631c Mon Sep 17 00:00:00 2001 From: Christian Duerr Date: Mon, 30 Oct 2023 22:13:28 +0100 Subject: [PATCH 2/4] Fix weird Linux build error --- CHANGELOG.md | 1 + Cargo.toml | 5 ----- src/linux/namespaces.rs | 4 ++-- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8c96cc..f1de3f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - (Linux) Sandbox exceptions for symbolic links - (macOS) Modifying exceptions for paths affected by existing exceptions +- (Linux) Symlink/Canonical path's exceptions overriding each other ## [v0.5.0] - 2023-10-13 diff --git a/Cargo.toml b/Cargo.toml index 570cbd8..719376b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,11 +49,6 @@ 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" diff --git a/src/linux/namespaces.rs b/src/linux/namespaces.rs index f74bfee..2d62ac3 100644 --- a/src/linux/namespaces.rs +++ b/src/linux/namespaces.rs @@ -6,7 +6,7 @@ use std::fs::{self, File}; use std::io::Error as IoError; use std::os::unix::ffi::OsStrExt; use std::os::unix::fs as unixfs; -use std::path::{Path, PathBuf, Component}; +use std::path::{Component, Path, PathBuf}; use std::{env, mem, ptr}; use bitflags::bitflags; @@ -23,7 +23,7 @@ const NEW_ROOT: &str = "/tmp/birdcage-root"; /// /// Additionally it will isolate network access if `allow_networking` is /// `false`. -pub fn create_namespaces(allow_networking: bool, exceptions: PathExceptions) -> Result<()> { +pub(crate) fn create_namespaces(allow_networking: bool, exceptions: PathExceptions) -> Result<()> { // Get EUID/EGID outside of the namespace. let uid = unsafe { libc::geteuid() }; let gid = unsafe { libc::getegid() }; From 0fca8c2c767e1ee5520cefcf4529ce18f2b072d4 Mon Sep 17 00:00:00 2001 From: Christian Duerr Date: Wed, 8 Nov 2023 21:17:52 +0100 Subject: [PATCH 3/4] Simplify error handling --- src/linux/mod.rs | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/linux/mod.rs b/src/linux/mod.rs index 8dbbedc..8e8e05f 100644 --- a/src/linux/mod.rs +++ b/src/linux/mod.rs @@ -29,20 +29,10 @@ 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::WriteAndRead(path) - | Exception::ExecuteAndRead(path) = &exception - { - if !path.exists() { - return Err(Error::InvalidPath(path.into())); - } - } - match exception { - Exception::Read(path) => self.path_exceptions.update(path, false, false), - Exception::WriteAndRead(path) => self.path_exceptions.update(path, true, false), - Exception::ExecuteAndRead(path) => self.path_exceptions.update(path, false, true), + Exception::Read(path) => self.path_exceptions.update(path, false, false)?, + Exception::WriteAndRead(path) => self.path_exceptions.update(path, true, false)?, + Exception::ExecuteAndRead(path) => self.path_exceptions.update(path, false, true)?, Exception::Environment(key) => self.env_exceptions.push(key), Exception::FullEnvironment => self.full_env = true, Exception::Networking => self.allow_networking = true, @@ -88,7 +78,7 @@ impl PathExceptions { /// /// If the bind mount already exists, it will *ADD* the additional /// permissions. - fn update(&mut self, path: PathBuf, write: bool, execute: bool) { + fn update(&mut self, path: PathBuf, write: bool, execute: bool) -> Result<()> { // Use canonical path for indexing. // // This ensures that a symlink and its target are treated like the same path for @@ -97,16 +87,13 @@ impl PathExceptions { // If the home path cannot be accessed, we ignore the exception. let canonical_path = match path.canonicalize() { Ok(path) => path, - Err(_) => return, + Err(_) => return Err(Error::InvalidPath(path)), }; // Store original symlink path to create it if necessary. if path_has_symlinks(&path) { // Normalize symlink's path. - let absolute = match absolute(&path) { - Ok(absolute) => absolute, - Err(_) => return, - }; + let absolute = absolute(&path)?; let normalized = normalize_path(&absolute); self.symlinks.push((normalized, canonical_path.clone())); @@ -126,6 +113,8 @@ impl PathExceptions { if execute { flags.remove(MountAttrFlags::NOEXEC); } + + Ok(()) } } From 00f0808f014f77c5dc310ca160c407acc56b2fc2 Mon Sep 17 00:00:00 2001 From: Christian Duerr Date: Fri, 10 Nov 2023 14:10:39 +0100 Subject: [PATCH 4/4] Revert test removal --- Cargo.toml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 719376b..570cbd8 100644 --- a/Cargo.toml +++ b/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"