Fix symlink/canonical path exception conflicts (#60)

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.

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
 

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"

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,54 +17,22 @@ mod seccomp;
 /// Linux sandboxing.
 #[derive(Default)]
 pub struct LinuxSandbox {
-    bind_mounts: HashMap<PathBuf, MountAttrFlags>,
     env_exceptions: Vec<String>,
+    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()
     }
 
     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.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 +48,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 +63,61 @@ impl Sandbox for LinuxSandbox {
     }
 }
 
+/// Path permissions required for the sandbox.
+#[derive(Default)]
+pub(crate) struct PathExceptions {
+    bind_mounts: HashMap<PathBuf, MountAttrFlags>,
+    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) -> Result<()> {
+        // 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 Err(Error::InvalidPath(path)),
+        };
+
+        // Store original symlink path to create it if necessary.
+        if path_has_symlinks(&path) {
+            // Normalize symlink's path.
+            let absolute = absolute(&path)?;
+            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);
+        }
+
+        Ok(())
+    }
+}
+
 /// 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 +127,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<PathBuf> {
+    // 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 <slash> characters, the
+        // first component following the leading <slash> characters may be
+        // interpreted in an implementation-defined manner, although more than
+        // two leading <slash> characters shall be treated as a single <slash>
+        // 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- <slash> character and contains one or more trailing
+    // <slash> characters".
+    // A trailing <slash> 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())
+}