diff --git a/crates/sandlock-core/src/cow/dispatch.rs b/crates/sandlock-core/src/cow/dispatch.rs index f25e3d34..b65a789e 100644 --- a/crates/sandlock-core/src/cow/dispatch.rs +++ b/crates/sandlock-core/src/cow/dispatch.rs @@ -33,7 +33,7 @@ use tokio::sync::Mutex as AsyncMutex; use crate::arch; use crate::cow::result::link_result; -use crate::cow::seccomp::SeccompCowBranch; +use crate::cow::seccomp::{branch_errno, SeccompCowBranch}; use crate::procfs::{build_dirent64, DT_DIR, DT_LNK, DT_REG}; use crate::seccomp::notif::{ decode_open_args, read_child_mem, write_child_mem, write_child_mem_force, NotifAction, @@ -248,11 +248,8 @@ pub(crate) async fn handle_cow_open( let plan = match cow.prepare_open(&path, flags) { Ok(plan) => plan, - Err(crate::error::BranchError::QuotaExceeded) => { - return NotifAction::Errno(libc::ENOSPC) - } - Err(crate::error::BranchError::Exists) => return NotifAction::Errno(libc::EEXIST), - Err(_) => return NotifAction::Continue, + Err(crate::error::BranchError::NotOwned) => return NotifAction::Continue, + Err(e) => return NotifAction::Errno(branch_errno(e)), }; (plan, upper_root, workdir_root) }; @@ -292,12 +289,17 @@ pub(crate) async fn handle_cow_open( upper } Ok(Err(_)) | Err(_) => { - // Copy failed — roll back quota and let kernel handle it + // Continuing would let the kernel open the lower file + // itself, so a write would land outside the branch. let mut st = cow_state.lock().await; if let Some(cow) = st.branch.as_mut() { cow.rollback_copy(file_size); } - return NotifAction::Continue; + let errno = match copy_result { + Ok(Err(e)) => e.raw_os_error().unwrap_or(libc::EIO), + _ => libc::EIO, + }; + return NotifAction::Errno(errno); } } } @@ -320,7 +322,8 @@ pub(crate) async fn handle_cow_open( create_mode, ) { Ok(fd) => fd, - Err(_) => return NotifAction::Continue, + // The kernel cannot see the upper layer, so its answer would be wrong. + Err(errno) => return NotifAction::Errno(errno), }; // Preserve O_CLOEXEC from the original openat flags. diff --git a/crates/sandlock-core/src/cow/seccomp.rs b/crates/sandlock-core/src/cow/seccomp.rs index d2dbe94e..81101d4d 100644 --- a/crates/sandlock-core/src/cow/seccomp.rs +++ b/crates/sandlock-core/src/cow/seccomp.rs @@ -32,7 +32,7 @@ fn parent_rel(rel: &str) -> Option<&str> { /// Errno for a branch failure, for handlers that answer the child directly /// (the `handle_unlink` convention) rather than returning a `BranchError`. -fn branch_errno(e: BranchError) -> i32 { +pub(super) fn branch_errno(e: BranchError) -> i32 { match e { BranchError::QuotaExceeded => libc::ENOSPC, BranchError::Denied => libc::EPERM, @@ -41,6 +41,7 @@ fn branch_errno(e: BranchError) -> i32 { // A kernel object cannot be staged in the upper; EXDEV makes mv and // ln fall back to mknod plus unlink, which the branch virtualizes. BranchError::NotOwned => libc::EXDEV, + BranchError::Io { source, .. } => source.raw_os_error().unwrap_or(libc::EIO), BranchError::Operation(_) | BranchError::Conflict(_) => libc::EIO, } } @@ -1185,18 +1186,31 @@ impl SeccompCowBranch { self.check_quota(0)?; return Ok(CowCopyPlan::Ready(upper_file)); } - Err(e) => return Err(BranchError::Operation(format!("stat lower: {}", e))), + Err(e) => { + return Err(BranchError::Io { + operation: "stat lower", + source: std::io::Error::from_raw_os_error(e), + }) + } }; let kind = st.st_mode & libc::S_IFMT; // Symlink — copy verbatim (tiny, not worth deferring) if kind == libc::S_IFLNK { self.check_quota(256)?; - let target = crate::sys::fs::readlink_in_root(&self.workdir, rel_path) - .map_err(|e| BranchError::Operation(format!("readlink: {}", e)))?; + let target = + crate::sys::fs::readlink_in_root(&self.workdir, rel_path).map_err(|e| { + BranchError::Io { + operation: "readlink", + source: std::io::Error::from_raw_os_error(e), + } + })?; let target = std::path::PathBuf::from(std::ffi::OsString::from_vec(target)); crate::sys::fs::symlinkat_in_root(&self.upper, rel_path, &target.to_string_lossy()) - .map_err(|e| BranchError::Operation(format!("symlink: {}", e)))?; + .map_err(|e| BranchError::Io { + operation: "symlink", + source: std::io::Error::from_raw_os_error(e), + })?; self.disk_used += 256; return Ok(CowCopyPlan::Ready(upper_file)); } @@ -1204,8 +1218,12 @@ impl SeccompCowBranch { // Directory — create immediately (no data copy) if kind == libc::S_IFDIR { self.check_quota(4096)?; - crate::sys::fs::mkdirp_in_root(&self.upper, rel_path, st.st_mode & 0o7777) - .map_err(|e| BranchError::Operation(format!("create dir: {}", e)))?; + crate::sys::fs::mkdirp_in_root(&self.upper, rel_path, st.st_mode & 0o7777).map_err( + |e| BranchError::Io { + operation: "create dir", + source: std::io::Error::from_raw_os_error(e), + }, + )?; let _ = crate::sys::fs::chmod_in_root(&self.upper, rel_path, st.st_mode & 0o7777); self.disk_used += 4096; return Ok(CowCopyPlan::Ready(upper_file)); @@ -4503,6 +4521,22 @@ mod tests { assert!(matches!(err, BranchError::Deleted)); } + #[test] + fn test_prepare_open_preserves_path_errors() { + let (workdir, storage) = setup_workdir(); + fs::write(workdir.path().join("file"), "original").unwrap(); + std::os::unix::fs::symlink("loop", workdir.path().join("loop")).unwrap(); + let mut branch = SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap(); + + for (path, errno) in [("file/child", libc::ENOTDIR), ("loop/child", libc::ELOOP)] { + let path = workdir.path().join(path); + let err = branch + .prepare_open(path.to_str().unwrap(), libc::O_WRONLY as u64) + .unwrap_err(); + assert_eq!(branch_errno(err), errno); + } + } + #[test] fn test_prepare_open_write_existing_needs_copy() { let (workdir, storage) = setup_workdir(); diff --git a/crates/sandlock-core/src/error.rs b/crates/sandlock-core/src/error.rs index 3d9ea5da..4db885d0 100644 --- a/crates/sandlock-core/src/error.rs +++ b/crates/sandlock-core/src/error.rs @@ -115,6 +115,13 @@ pub enum BranchError { #[error("branch operation failed: {0}")] Operation(String), + #[error("branch operation failed: {operation}: {source}")] + Io { + operation: &'static str, + #[source] + source: std::io::Error, + }, + #[error("branch conflict: {0}")] Conflict(String), diff --git a/crates/sandlock-core/src/seccomp/notif.rs b/crates/sandlock-core/src/seccomp/notif.rs index 5717dd96..b93c4915 100644 --- a/crates/sandlock-core/src/seccomp/notif.rs +++ b/crates/sandlock-core/src/seccomp/notif.rs @@ -6,7 +6,7 @@ use std::collections::{HashMap, HashSet}; use std::future::Future; use std::io; use std::net::IpAddr; -use std::os::unix::io::{AsRawFd, IntoRawFd, OwnedFd, RawFd}; +use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, IntoRawFd, OwnedFd, RawFd}; use std::pin::Pin; use std::sync::Arc; @@ -736,11 +736,12 @@ pub(crate) fn decode_open_args(notif: &SeccompNotif, notif_fd: RawFd) -> Option< let a = ¬if.data.args; let nr = notif.data.nr as i64; if nr == libc::SYS_openat { + let (flags, mode) = openat_how(a[2], a[3]); Some(OpenArgs { dirfd: a[0] as i64, path_ptr: a[1], - flags: a[2], - mode: a[3], + flags, + mode, resolve: 0, }) } else if nr == arch::SYS_OPENAT2 { @@ -771,16 +772,61 @@ pub(crate) fn decode_open_args(notif: &SeccompNotif, notif_fd: RawFd) -> Option< }) } else { // legacy open(path, flags, mode) — AT_FDCWD implied. + let (flags, mode) = openat_how(a[1], a[2]); Some(OpenArgs { dirfd: libc::AT_FDCWD as i64, path_ptr: a[0], - flags: a[1], - mode: a[2], + flags, + mode, resolve: 0, }) } } +/// The `open_how` the kernel builds for an `open`/`openat` caller. Handlers +/// serve opens with `openat2`, which rejects what `openat` silently drops: +/// unknown flag bits, stray flags beside `O_PATH` (glibc adds `O_LARGEFILE`), +/// and a mode without `O_CREAT`/`O_TMPFILE`. +fn openat_how(flags: u64, mode: u64) -> (u64, u64) { + // libc exposes O_LARGEFILE as zero on some 64-bit targets, unlike the syscall ABI. + const O_LARGEFILE: u64 = if cfg!(target_arch = "aarch64") { + 0o400000 + } else { + 0o100000 + }; + const VALID_OPEN_FLAGS: u64 = (libc::O_ACCMODE + | libc::O_CREAT + | libc::O_EXCL + | libc::O_NOCTTY + | libc::O_TRUNC + | libc::O_APPEND + | libc::O_NDELAY + | libc::O_NONBLOCK + | libc::O_SYNC + | libc::O_DSYNC + | libc::O_ASYNC + | libc::O_DIRECT + | libc::O_DIRECTORY + | libc::O_NOFOLLOW + | libc::O_NOATIME + | libc::O_CLOEXEC + | libc::O_PATH + | libc::O_TMPFILE) as u64 + | O_LARGEFILE; + const O_PATH_FLAGS: u64 = + (libc::O_DIRECTORY | libc::O_NOFOLLOW | libc::O_PATH | libc::O_CLOEXEC) as u64; + let flags = flags & VALID_OPEN_FLAGS; + let flags = if flags & libc::O_PATH as u64 != 0 { + flags & O_PATH_FLAGS + } else { + flags + }; + let creates = flags & libc::O_CREAT as u64 != 0 + || flags & libc::O_TMPFILE as u64 == libc::O_TMPFILE as u64; + let mode = if creates { mode & 0o7777 } else { 0 }; + (flags, mode) +} + /// An open-family request, decoded once per notification and shared by every /// handler in the chain. Each handler used to read the string again, which /// added nothing: a handler is safe by how it answers, not by rereading. @@ -1469,16 +1515,23 @@ fn respond_value(fd: RawFd, id: u64, val: i64) -> io::Result<()> { /// Fail-closed response used when fd injection fails. /// -/// Denies the syscall with `EACCES` rather than letting it continue: a +/// Denies the syscall rather than letting it continue: a /// `SECCOMP_USER_NOTIF_FLAG_CONTINUE` here would let the child's original /// syscall run unmediated against the host path, silently bypassing /// chroot/file confinement. (Regression guard: this must never be a CONTINUE -/// response.) -fn inject_failure_resp(id: u64) -> SeccompNotifResp { +/// response.) Kernels whose ADDFD takes the source with `fget()` refuse an +/// `O_PATH` file with `EBADF`; the child hears `EOPNOTSUPP`, not a denial. +fn inject_failure_resp(id: u64, srcfd: BorrowedFd<'_>, err: io::Error) -> SeccompNotifResp { + let o_path = unsafe { libc::fcntl(srcfd.as_raw_fd(), libc::F_GETFL) } & libc::O_PATH != 0; + let errno = if o_path && err.raw_os_error() == Some(libc::EBADF) { + libc::EOPNOTSUPP + } else { + libc::EACCES + }; SeccompNotifResp { id, val: 0, - error: -libc::EACCES, + error: -errno, flags: 0, } } @@ -2040,7 +2093,7 @@ fn send_response(fd: RawFd, id: u64, action: NotifAction) -> io::Result<()> { // srcfd (OwnedFd) is dropped at end of this arm, closing the fd. match inject_fd_and_send(fd, id, srcfd.as_raw_fd(), newfd_flags) { Ok(_new_fd) => Ok(()), - Err(_) => send_resp_raw(fd, &inject_failure_resp(id)), + Err(e) => send_resp_raw(fd, &inject_failure_resp(id, srcfd.as_fd(), e)), } } NotifAction::InjectFdSendTracked { @@ -2052,7 +2105,7 @@ fn send_response(fd: RawFd, id: u64, action: NotifAction) -> io::Result<()> { (on_success.0)(new_fd); Ok(()) } - Err(_) => send_resp_raw(fd, &inject_failure_resp(id)), + Err(e) => send_resp_raw(fd, &inject_failure_resp(id, srcfd.as_fd(), e)), }, NotifAction::ReturnValue(val) => respond_value(fd, id, val), NotifAction::Hold => Ok(()), // Don't send a response. @@ -3390,6 +3443,21 @@ pub(crate) async fn cleanup_pid(ctx: &super::ctx::SupervisorCtx, key: super::sta mod tests { use super::*; + #[test] + fn openat_how_matches_what_openat2_accepts() { + let o_path = (libc::O_PATH | libc::O_DIRECTORY) as u64; + assert_eq!(openat_how(o_path | libc::O_NONBLOCK as u64, 0), (o_path, 0)); + assert_eq!(openat_how(o_path | libc::O_RDWR as u64, 0), (o_path, 0)); + assert_eq!(openat_how(libc::O_RDONLY as u64, 0o755), (0, 0)); + let creat = (libc::O_WRONLY | libc::O_CREAT) as u64; + assert_eq!(openat_how(creat, 0o100644), (creat, 0o644)); + let tmpfile = (libc::O_RDWR | libc::O_TMPFILE) as u64; + assert_eq!(openat_how(tmpfile, 0o600), (tmpfile, 0o600)); + assert_eq!(openat_how(libc::O_DIRECTORY as u64, 0o600).1, 0); + assert_eq!(openat_how(creat | 0x4000_0000, 0o644), (creat, 0o644)); + assert_eq!(openat_how(creat | (1 << 63), 0o644), (creat, 0o644)); + } + #[test] fn fifo_writer_probe_sees_a_writer_and_consumes_nothing() { use std::os::unix::io::FromRawFd; @@ -3601,7 +3669,9 @@ mod tests { // When fd injection fails, the supervisor must fail closed: deny the // syscall instead of letting it continue unmediated against the host // path (which would silently bypass chroot/file confinement). - let resp = inject_failure_resp(123); + let file = std::fs::File::open("/").unwrap(); + let resp = + inject_failure_resp(123, file.as_fd(), io::Error::from_raw_os_error(libc::EBADF)); assert_eq!(resp.id, 123); assert_eq!( resp.flags & SECCOMP_USER_NOTIF_FLAG_CONTINUE, @@ -3612,6 +3682,23 @@ mod tests { assert_eq!(resp.error, -libc::EACCES); } + #[test] + fn inject_failure_of_an_o_path_source_is_eopnotsupp() { + let raw = unsafe { libc::open(c"/".as_ptr(), libc::O_PATH | libc::O_CLOEXEC) }; + assert!(raw >= 0); + let fd = unsafe { OwnedFd::from_raw_fd(raw) }; + let ebadf = || io::Error::from_raw_os_error(libc::EBADF); + assert_eq!( + inject_failure_resp(1, fd.as_fd(), ebadf()).error, + -libc::EOPNOTSUPP + ); + let enomem = io::Error::from_raw_os_error(libc::ENOMEM); + assert_eq!( + inject_failure_resp(1, fd.as_fd(), enomem).error, + -libc::EACCES + ); + } + #[test] fn tgid_of_main_thread_is_own_pid() { // The main thread's tid equals the process pid, and its Tgid is the pid. diff --git a/crates/sandlock-core/tests/integration/test_cow.rs b/crates/sandlock-core/tests/integration/test_cow.rs index 59822e8b..fb32ac24 100644 --- a/crates/sandlock-core/tests/integration/test_cow.rs +++ b/crates/sandlock-core/tests/integration/test_cow.rs @@ -266,6 +266,107 @@ async fn test_seccomp_cow_open_directory() { let _ = fs::remove_dir_all(&workdir); } +/// An openat whose mode register is set without O_CREAT must still reach a +/// directory that exists only in the upper layer (issue #325). openat ignores +/// that mode; the openat2 serving the request rejected it. +#[tokio::test] +async fn test_seccomp_cow_openat_ignores_mode_without_creat() { + let workdir = temp_dir("seccomp-openat-mode"); + let out_file = workdir.join("result.txt"); + + let policy = Sandbox::builder() + .fs_read("/usr") + .fs_read("/lib") + .fs_read_if_exists("/lib64") + .fs_read("/bin") + .fs_read("/etc") + .fs_read("/proc") + .fs_read("/dev") + .fs_write(&workdir) + .workdir(&workdir) + .cwd(&workdir) + .on_exit(BranchAction::Commit) + .build() + .unwrap(); + + let script = format!( + concat!( + "import ctypes, os, platform\n", + "libc = ctypes.CDLL('libc.so.6', use_errno=True)\n", + "nr = 257 if platform.machine() == 'x86_64' else 56\n", + "os.mkdir('subdir')\n", + "fd = libc.syscall(nr, -100, b'subdir', os.O_RDONLY | os.O_DIRECTORY, 0o755)\n", + "err = ctypes.get_errno()\n", + "open('{out}', 'w').write('ok' if fd >= 0 else f'errno={{err}}')\n", + ), + out = out_file.display() + ); + let result = policy + .clone() + .run(&["python3", "-c", &script]) + .await + .unwrap(); + assert!( + result.success(), + "stderr: {}", + result.stderr_str().unwrap_or("") + ); + assert_eq!(fs::read_to_string(&out_file).unwrap(), "ok"); + + let _ = fs::remove_dir_all(&workdir); +} + +/// An O_PATH open of an upper-only directory either gets a descriptor or, on +/// kernels whose SECCOMP_IOCTL_NOTIF_ADDFD refuses O_PATH sources, an honest +/// EOPNOTSUPP. It must never be the EACCES of a denial (issue #325). +#[tokio::test] +async fn test_seccomp_cow_o_path_open_of_created_dir() { + let workdir = temp_dir("seccomp-o-path"); + let out_file = workdir.join("result.txt"); + + let policy = Sandbox::builder() + .fs_read("/usr") + .fs_read("/lib") + .fs_read_if_exists("/lib64") + .fs_read("/bin") + .fs_read("/etc") + .fs_read("/proc") + .fs_read("/dev") + .fs_write(&workdir) + .workdir(&workdir) + .cwd(&workdir) + .on_exit(BranchAction::Commit) + .build() + .unwrap(); + + let script = format!( + concat!( + "import errno, os\n", + "os.mkdir('subdir')\n", + "try:\n", + " os.close(os.open('subdir', os.O_PATH | os.O_DIRECTORY))\n", + " res = 'ok'\n", + "except OSError as e:\n", + " res = 'ok' if e.errno == errno.EOPNOTSUPP else f'errno={{e.errno}}'\n", + "open('{out}', 'w').write(res)\n", + ), + out = out_file.display() + ); + let result = policy + .clone() + .run(&["python3", "-c", &script]) + .await + .unwrap(); + assert!( + result.success(), + "stderr: {}", + result.stderr_str().unwrap_or("") + ); + assert_eq!(fs::read_to_string(&out_file).unwrap(), "ok"); + + let _ = fs::remove_dir_all(&workdir); +} + /// Test that chdir works for directories created inside COW. /// /// When a directory is created via COW (only exists in the upper layer),