From 3dba12cd81d0a961f95df3b135418706d36d6362 Mon Sep 17 00:00:00 2001 From: Markus Rudy Date: Thu, 19 Mar 2026 17:07:06 +0100 Subject: [PATCH] agent: constrain copy_file to shared directory In confidential mode, the host is not trusted and might try to create malicious files in the guest VM. This commit changes the implementation of the CopyFile method to use pathrs, which is designed to prevent accidental writes to locations outside the shared directory (/run/kata-containers/shared/containers). It also adds a detailed description of what the method does, and test cases for intended and malicious use cases. This was originally considered as a mitigation candidate for https://github.com/kata-containers/kata-containers/security/advisories/GHSA-q49m-57vm-c8cc, but we decided to go with the simpler change to address the immediate risk. It's still desirable to lock down CopyFile writes for defense in depth, thus the resubmission outside the GHSA. Signed-off-by: Markus Rudy --- Cargo.lock | 19 + Cargo.toml | 1 + src/agent/Cargo.toml | 1 + src/agent/src/rpc.rs | 585 +++++++++++++++--- .../api-tests/test_copy_file.bats | 39 +- .../api-tests/test_set_policy.bats | 2 +- 6 files changed, 550 insertions(+), 97 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 03d1818d24..924a2b11d1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3565,6 +3565,7 @@ dependencies = [ "nix 0.31.3", "oci-spec 0.8.4", "opentelemetry 0.17.0", + "pathrs", "procfs 0.12.0", "prometheus", "protobuf", @@ -5298,6 +5299,24 @@ dependencies = [ "once_cell", ] +[[package]] +name = "pathrs" +version = "0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2fb2311801201fc6fd2e8a9f4841b41eee565e992fbe713731e29e367b8e3f17" +dependencies = [ + "bitflags 2.11.1", + "itertools 0.14.0", + "libc", + "memchr", + "once_cell", + "rustix 1.1.4", + "rustversion", + "static_assertions", + "tempfile", + "thiserror 2.0.18", +] + [[package]] name = "pci-ids" version = "0.2.6" diff --git a/Cargo.toml b/Cargo.toml index 15cd5e9d11..f55cc665b5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -182,6 +182,7 @@ netns-rs = "0.1.0" nix = { version = "0.31.3", features = ["fs", "mount", "sched", "process", "ioctl", "signal", "socket", "feature", "user", "hostname", "term", "event", "mman", "reboot"] } oci-spec = { version = "0.8.1", features = ["runtime"] } opentelemetry = { version = "0.17.0", features = ["rt-tokio"] } +pathrs = "0.2.4" procfs = "0.12.0" prometheus = { version = "0.14.0", features = ["process"] } protobuf = "3.7.2" diff --git a/src/agent/Cargo.toml b/src/agent/Cargo.toml index c4b4be5fd8..00786d6d9d 100644 --- a/src/agent/Cargo.toml +++ b/src/agent/Cargo.toml @@ -81,6 +81,7 @@ safe-path.workspace = true # to be modified at runtime. logging.workspace = true vsock-exporter.workspace = true +pathrs.workspace = true # Initdata base64 = { workspace = true } diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index 816df3abcc..fdc3c902c2 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -6,6 +6,7 @@ use async_trait::async_trait; #[cfg(feature = "agent-policy")] use kata_agent_policy::policy::PolicyCopyFileRequest; +use pathrs::flags::OpenFlags; use rustjail::{pipestream::PipeStream, process::StreamType}; use tokio::io::{AsyncReadExt, AsyncWriteExt, ReadHalf}; use tokio::sync::Mutex; @@ -64,7 +65,6 @@ use nix::unistd::{self, Pid}; use rustjail::process::ProcessOperations; #[cfg(all(test, not(target_arch = "powerpc64")))] use std::os::fd::AsRawFd; -use std::os::fd::BorrowedFd; #[cfg(target_arch = "s390x")] use crate::ccw; @@ -113,7 +113,7 @@ use std::os::unix::prelude::PermissionsExt; use std::process::{Command, Stdio}; use nix::unistd::{Gid, Uid}; -use std::fs::{File, OpenOptions}; +use std::fs::File; use std::io::{BufRead, BufReader, Write}; use std::os::unix::fs::FileExt; use std::path::PathBuf; @@ -146,6 +146,14 @@ const ERR_NO_SANDBOX_PIDNS: &str = "Sandbox does not have sandbox_pidns"; // not available. const IPTABLES_RESTORE_WAIT_SEC: u64 = 5; +/// This mask is applied to parent directories implicitly created for CopyFile requests. +const IMPLICIT_DIRECTORY_PERMISSION_MASK: u32 = 0o777; + +/// This mask is applied to files and directories created for CopyFile requests. +/// In addition to the permissions, it allows setuid/setgid/sticky bits. +/// Note that the setuid bit does not have an effect on Linux, though. +const FILE_PERMISSION_MASK: u32 = 0o7777; + // Convenience function to obtain the scope logger. fn sl() -> slog::Logger { slog_scope::logger() @@ -1680,7 +1688,9 @@ impl agent_ttrpc::AgentService for AgentService { #[cfg(not(feature = "agent-policy"))] is_allowed(&req).await?; - do_copy_file(&req).map_ttrpc_err(same)?; + // Potentially untrustworthy data from the host needs to go into the shared dir. + let root_path = PathBuf::from(KATA_GUEST_SHARE_DIR); + do_copy_file(&req, &root_path).map_ttrpc_err(same)?; Ok(Empty::new()) } @@ -2240,127 +2250,173 @@ fn do_set_guest_date_time(sec: i64, usec: i64) -> Result<()> { Ok(()) } -fn do_copy_file(req: &CopyFileRequest) -> Result<()> { - let path = PathBuf::from(req.path.as_str()); +/// do_copy_file creates a file, directory or symlink beneath the provided directory. +/// +/// The function guarantees that no content is written outside of the directory. However, a symlink +/// created by this function might point outside the shared directory. Other users of that +/// directory need to consider whether they trust the host, or handle the directory with the same +/// care as do_copy_file. +/// +/// Parent directories are created, if they don't exist already. For these implicit operations, the +/// permissions are set with req.dir_mode. The actual target is created with permissions from +/// req.file_mode, even if it's a directory. +/// +/// If req.file_mode requests a symbolic link, the link is created pointing to the path in +/// req.data. In that case, req.file_mode is ignored because symlinks don't have permissions on +/// Linux. +/// +/// If this function returns an error, the filesystem may be in an unexpected state. This is not +/// significant for the caller, since errors are almost certainly not retriable. The runtime should +/// abandon this VM instead. +fn do_copy_file(req: &CopyFileRequest, shared_dir: &PathBuf) -> Result<()> { + let insecure_full_path = PathBuf::from(req.path.as_str()); + let path = insecure_full_path + .strip_prefix(shared_dir) + .context(format!( + "removing {:?} prefix from {}", + shared_dir, req.path + ))?; - if !path.starts_with(CONTAINER_BASE) { - return Err(anyhow!( - "Path {:?} does not start with {}", - path, - CONTAINER_BASE - )); - } + // The shared directory might not exist yet, but we need to create it in order to open the root. + std::fs::create_dir_all(shared_dir)?; + let root = pathrs::Root::open(shared_dir)?; // Create parent directories if missing if let Some(parent) = path.parent() { - if !parent.exists() { - let dir = parent.to_path_buf(); - // Attempt to create directory, ignore AlreadyExists errors - if let Err(e) = fs::create_dir_all(&dir) { - if e.kind() != std::io::ErrorKind::AlreadyExists { - return Err(e.into()); - } - } + let dir = root + .mkdir_all( + parent, + &std::fs::Permissions::from_mode(req.dir_mode & IMPLICIT_DIRECTORY_PERMISSION_MASK), + ) + .context("mkdir_all parent")? + .reopen(OpenFlags::O_DIRECTORY) + .context("reopen parent")?; - // Set directory permissions and ownership - std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(req.dir_mode))?; - unistd::chown( - &dir, - Some(Uid::from_raw(req.uid as u32)), - Some(Gid::from_raw(req.gid as u32)), - )?; - } + // TODO(burgerdev): why are we only applying this to the immediate parent? + unistd::fchown( + dir, + Some(Uid::from_raw(req.uid as u32)), + Some(Gid::from_raw(req.gid as u32)), + ) + .context("fchown parent")? } let sflag = stat::SFlag::from_bits_truncate(req.file_mode); if sflag.contains(stat::SFlag::S_IFDIR) { - // Remove existing non-directory file if present - if path.exists() && !path.is_dir() { - fs::remove_file(&path)?; - } - - fs::create_dir(&path).or_else(|e| { - if e.kind() != std::io::ErrorKind::AlreadyExists { - return Err(e); + // Directories are somewhat special: for backwards compatibility, we need to preserve an + // existing directory at path, so we can't just remove_all. Instead, we try to remove a + // file and just don't propagate the error if it's a directory or doesn't exist. + root.remove_file(path).or_else(|e| match e.kind() { + pathrs::error::ErrorKind::OsError(Some(errno)) + if errno == libc::ENOENT || errno == libc::EISDIR => + { + Ok(()) } - Ok(()) + _ => Err(e), })?; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(req.file_mode))?; + // mkdir_all does not support the setuid/setgid/sticky bits, so we first create the + // directory with the stricter mask and then change permissions with the correct mask. + let dir = root + .mkdir_all( + path, + &std::fs::Permissions::from_mode( + req.file_mode & IMPLICIT_DIRECTORY_PERMISSION_MASK, + ), + ) + .context("mkdir_all dir")? + .reopen(OpenFlags::O_DIRECTORY) + .context("reopen dir")?; + dir.set_permissions(std::fs::Permissions::from_mode( + req.file_mode & FILE_PERMISSION_MASK, + ))?; - unistd::chown( - &path, + unistd::fchown( + dir, Some(Uid::from_raw(req.uid as u32)), Some(Gid::from_raw(req.gid as u32)), - )?; + ) + .context("fchown dir")?; return Ok(()); } + // Remove any existing file if we're not resuming a chunked upload. + if req.offset == 0 { + // Remove anything that might already exist at the target location. + // This is safe even for a symlink leaf, remove_all removes the named inode in its parent dir. + root.remove_all(path).or_else(|e| match e.kind() { + pathrs::error::ErrorKind::OsError(Some(errno)) if errno == libc::ENOENT => Ok(()), + _ => Err(e), + })?; + } + // Handle symlink creation if sflag.contains(stat::SFlag::S_IFLNK) { - // Clean up existing path (whether symlink, dir, or file) - if path.exists() || path.is_symlink() { - // Use appropriate removal method based on path type - if path.is_symlink() { - unistd::unlink(&path)?; - } else if path.is_dir() { - fs::remove_dir_all(&path)?; - } else { - fs::remove_file(&path)?; - } - } - // Create new symbolic link let symlink_target = PathBuf::from(OsStr::from_bytes(&req.data)); - // Use BorrowedFd to wrap AT_FDCWD for symlinkat - let cwd_fd = unsafe { BorrowedFd::borrow_raw(libc::AT_FDCWD) }; - unistd::symlinkat(&symlink_target, cwd_fd, &path)?; + root.create(path, &pathrs::InodeType::Symlink(symlink_target)) + .context("create symlink")?; - // Set symlink ownership (permissions not supported for symlinks) - let path_str = CString::new(path.as_os_str().as_bytes())?; - - let ret = unsafe { libc::lchown(path_str.as_ptr(), req.uid as u32, req.gid as u32) }; - Errno::result(ret).map(drop)?; + // Set symlink ownership. + // At the time of writing this, there was no API for creating the symlink and opening a + // handle to the created inode. Best we can do is to resolve it again under the root and + // hope that its still the same inode, but at least we guarantee that we're changing + // ownership only within the shared directory. + nix::unistd::fchownat( + root, + path, + Some(Uid::from_raw(req.uid as u32)), + Some(Gid::from_raw(req.gid as u32)), + nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW, + ) + .context("fchownat")?; + // Symlinks don't have permissions on Linux! return Ok(()); } - let mut tmpfile = path.clone(); + let mut tmpfile = path.to_path_buf(); tmpfile.set_extension("tmp"); - let file = OpenOptions::new() - .write(true) - .create(true) - .truncate(req.offset == 0) // Only truncate when offset is 0 - .open(&tmpfile)?; + // Write file content. + let flags = if req.offset == 0 { + OpenFlags::O_RDWR | OpenFlags::O_CREAT | OpenFlags::O_TRUNC + } else { + OpenFlags::O_RDWR | OpenFlags::O_CREAT + }; + let file = root + .create_file( + &tmpfile, + flags, + &std::fs::Permissions::from_mode(req.file_mode & FILE_PERMISSION_MASK), + ) + .context("create_file")?; + file.write_all_at(req.data.as_slice(), req.offset as u64) + .context("write_all_at")?; - file.write_all_at(req.data.as_slice(), req.offset as u64)?; - let st = stat::stat(&tmpfile)?; + // Check whether we're waiting for more data. + let st = nix::sys::stat::fstat(&file).context("fstat")?; if st.st_size != req.file_size { return Ok(()); } - file.set_permissions(std::fs::Permissions::from_mode(req.file_mode))?; + // Things like umask can change the permissions after create, make sure that they stay + file.set_permissions(std::fs::Permissions::from_mode( + req.file_mode & FILE_PERMISSION_MASK, + )) + .context("set_permissions")?; - unistd::chown( - &tmpfile, + unistd::fchown( + file, Some(Uid::from_raw(req.uid as u32)), Some(Gid::from_raw(req.gid as u32)), - )?; + ) + .context("fchown")?; - // Remove existing target path before rename - if path.exists() || path.is_symlink() { - if path.is_dir() { - fs::remove_dir_all(&path)?; - } else { - fs::remove_file(&path)?; - } - } - - fs::rename(tmpfile, path)?; + nix::fcntl::renameat(&root, &tmpfile, &root, path).context("renameat")?; Ok(()) } @@ -2651,6 +2707,7 @@ mod tests { use super::*; use crate::{namespace::Namespace, protocols::agent_ttrpc_async::AgentService as _}; + use anyhow::{bail, ensure}; use nix::mount; use nix::sched::{unshare, CloneFlags}; use oci::{ @@ -3745,4 +3802,366 @@ COMMIT ids.sort(); assert_eq!(ids, vec!["container-1", "container-2"]); } + + #[tokio::test] + async fn test_do_copy_file() { + let temp_dir = tempdir().expect("creating temp dir failed"); + // We start one directory deeper such that we catch problems when the shared directory does + // not exist yet. + let base = temp_dir.path().join("shared"); + + type Assertions = Box Result<()>>; + struct TestCase { + name: String, + request: CopyFileRequest, + assertions: Assertions, + should_fail: bool, + } + + // Attention: these test cases depend on each other and can't be reordered. + // The first few cases build up a directory structure that the subsequent tests then rely + // on or try to exploit. + // TODO(burgerdev): define a common directory structure for all tests up front. + let tests = [ + TestCase { + name: "Create a top-level file".into(), + request: CopyFileRequest { + path: base.join("f").to_string_lossy().into(), + file_mode: 0o644 | libc::S_IFREG, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + let f = base.join("f"); + let f_stat = fs::metadata(&f).context("stat ./f failed")?; + ensure!(f_stat.is_file()); + ensure!(0o644 == f_stat.permissions().mode() & 0o777); + let content = std::fs::read_to_string(&f).context("read ./f failed")?; + ensure!(content.is_empty()); + Ok(()) + }), + }, + TestCase { + name: "Writing a file onto an existing file replaces it".into(), + request: CopyFileRequest { + path: base.join("f").to_string_lossy().into(), + file_mode: 0o600 | libc::S_IFREG, + data: b"Hello!".to_vec(), + file_size: 6, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + let f = base.join("f"); + let f_stat = fs::metadata(&f).context("stat ./f failed")?; + ensure!(f_stat.is_file()); + ensure!(0o600 == f_stat.permissions().mode() & 0o777); + let content = std::fs::read_to_string(&f).context("read ./f failed")?; + ensure!("Hello!" == content); + Ok(()) + }), + }, + TestCase { + name: "Creating a file implicitly creates parent directories".into(), + request: CopyFileRequest { + path: base.join("a/b").to_string_lossy().into(), + dir_mode: 0o755 | libc::S_IFDIR, + file_mode: 0o644 | libc::S_IFREG, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?; + ensure!(a_stat.is_dir()); + ensure!(0o755 == a_stat.permissions().mode() & 0o777); + let b_stat = fs::metadata(base.join("a/b")).context("stat ./a/b failed")?; + ensure!(b_stat.is_file()); + ensure!(0o644 == b_stat.permissions().mode() & 0o777); + Ok(()) + }), + }, + TestCase { + name: "Create a file within an existing directory".into(), + request: CopyFileRequest { + path: base.join("a/c").to_string_lossy().into(), + dir_mode: 0o700 | libc::S_IFDIR, // Test that existing directories are not touched - we expect this to stay 0o755. + file_mode: 0o621 | libc::S_IFREG, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?; + ensure!(a_stat.is_dir()); + ensure!(0o755 == a_stat.permissions().mode() & 0o777); + let c_stat = fs::metadata(base.join("a/c")).context("stat ./a/c failed")?; + ensure!(c_stat.is_file()); + ensure!(0o621 == c_stat.permissions().mode() & 0o777); + Ok(()) + }), + }, + TestCase { + name: "Create a directory".into(), + request: CopyFileRequest { + path: base.join("a/d").to_string_lossy().into(), + dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode. + file_mode: 0o755 | libc::S_IFDIR, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?; + ensure!(a_stat.is_dir()); + ensure!(0o755 == a_stat.permissions().mode() & 0o777); + let d_stat = fs::metadata(base.join("a/d")).context("stat ./a/d failed")?; + ensure!(d_stat.is_dir()); + ensure!(0o755 == d_stat.permissions().mode() & 0o777); + Ok(()) + }), + }, + TestCase { + name: "Creating a dir onto an existing file replaces the file".into(), + request: CopyFileRequest { + path: base.join("a/b").to_string_lossy().into(), + dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode. + file_mode: 0o755 | libc::S_IFDIR, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + let b_stat = fs::metadata(base.join("a/b")).context("stat ./a/b failed")?; + ensure!(b_stat.is_dir()); + ensure!(0o755 == b_stat.permissions().mode() & 0o777); + Ok(()) + }), + }, + TestCase { + name: "Creating a file onto an existing dir replaces the dir".into(), + request: CopyFileRequest { + path: base.join("a/b").to_string_lossy().into(), + dir_mode: 0o755 | libc::S_IFDIR, + file_mode: 0o644 | libc::S_IFREG, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + let b_stat = fs::metadata(base.join("a/b")).context("stat ./a/b failed")?; + ensure!(b_stat.is_file()); + ensure!(0o644 == b_stat.permissions().mode() & 0o777); + Ok(()) + }), + }, + TestCase { + name: "Creating a dir onto an existing dir does not replace that dir".into(), + request: CopyFileRequest { + path: base.join("a").to_string_lossy().into(), + dir_mode: 0o755 | libc::S_IFDIR, + file_mode: 0o751 | libc::S_IFDIR, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + // Check that a/b still exists + let b_stat = fs::metadata(base.join("a/b")).context("stat ./a/b failed")?; + ensure!(b_stat.is_file()); + let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?; + ensure!(0o751 == a_stat.permissions().mode() & 0o777); + Ok(()) + }), + }, + TestCase { + name: "Create a symlink".into(), + request: CopyFileRequest { + path: base.join("a/link").to_string_lossy().into(), + dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode. + file_mode: 0o755 | libc::S_IFLNK, + data: b"/etc/passwd".to_vec(), + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + let link = base.join("a/link"); + let link_stat = nix::sys::stat::lstat(&link).context("stat ./a/link failed")?; + // Linux symlinks have no permissions! + ensure!(0o777 | libc::S_IFLNK == link_stat.st_mode); + let target = fs::read_link(&link).context("read_link ./a/link failed")?; + ensure!(target.to_string_lossy() == "/etc/passwd"); + Ok(()) + }), + }, + TestCase { + name: "Create a directory with setgid and sticky bit".into(), + request: CopyFileRequest { + path: base.join("x/y").to_string_lossy().into(), + dir_mode: 0o3755 | libc::S_IFDIR, + file_mode: 0o3770 | libc::S_IFDIR, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + // Implicitly created directories should not get a sticky bit. + let x_stat = fs::metadata(base.join("x")).context("stat ./x failed")?; + ensure!(x_stat.is_dir()); + ensure!(0o755 == x_stat.permissions().mode() & 0o7777); + // Explicitly created directories should. + let y_stat = fs::metadata(base.join("x/y")).context("stat ./x/y failed")?; + ensure!(y_stat.is_dir()); + ensure!(0o3770 == y_stat.permissions().mode() & 0o7777); + Ok(()) + }), + }, + TestCase { + name: "Chunked upload 1".into(), + request: CopyFileRequest { + path: base.join("x/chunked").to_string_lossy().into(), + dir_mode: 0o755 | libc::S_IFDIR, + file_mode: 0o644 | libc::S_IFREG, + offset: 0, + file_size: 11, + data: b"Hello ".to_vec(), + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + ensure!( + !(fs::exists(base.join("x/chunked")) + .context("exists ./x/chunked failed")?) + ); + Ok(()) + }), + }, + TestCase { + name: "Chunked upload 2".into(), + request: CopyFileRequest { + path: base.join("x/chunked").to_string_lossy().into(), + dir_mode: 0o755 | libc::S_IFDIR, + file_mode: 0o644 | libc::S_IFREG, + offset: 6, + file_size: 11, + data: b"World".to_vec(), + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + let content = std::fs::read(base.join("x/chunked"))?; + println!("{:?}", content); + ensure!(b"Hello World".to_vec() == content); + Ok(()) + }), + }, + // ================================= + // Below are some adversarial tests. + // ================================= + TestCase { + name: "Malicious intermediate directory is a symlink".into(), + request: CopyFileRequest { + path: base + .join("a/link/this-could-just-be-shadow-but-I-am-not-risking-it") + .to_string_lossy() + .into(), + dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode. + file_mode: 0o755 | libc::S_IFLNK, + data: b"root:password:19000:0:99999:7:::\n".to_vec(), + file_size: 33, + ..Default::default() + }, + should_fail: true, + assertions: Box::new(|base| -> Result<()> { + let link_stat = nix::sys::stat::lstat(&base.join("a/link")) + .context("stat ./a/link failed")?; + ensure!(0o777 | libc::S_IFLNK == link_stat.st_mode); + Ok(()) + }), + }, + TestCase { + name: "Creating a symlink onto an existing symlink should replace the symlink, not follow it".into(), + request: CopyFileRequest { + path: base.join("a/link").to_string_lossy().into(), + dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode. + file_mode: 0o755 | libc::S_IFLNK, + data: b"/etc".to_vec(), + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + // The symlink should be created at the same place (not followed), with the new content. + let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?; + ensure!(a_stat.is_dir()); + ensure!(0o751 == a_stat.permissions().mode() & 0o777); + let link = base.join("a/link"); + let link_stat = nix::sys::stat::lstat(&link).context("stat ./a/link failed")?; + // Linux symlinks have no permissions! + ensure!(0o777 | libc::S_IFLNK == link_stat.st_mode); + let target = fs::read_link(&link).context("read_link ./a/link failed")?; + ensure!(target.to_string_lossy() == "/etc"); + Ok(()) + }), + }, + TestCase { + name: "Creating a file at an existing symlink replaces the link and does not follow it".into(), + request: CopyFileRequest { + path: base.join("a/link").to_string_lossy().into(), + file_mode: 0o600 | libc::S_IFREG, + data: b"Hello!".to_vec(), + file_size: 6, + ..Default::default() + }, + should_fail: false, + assertions: Box::new(|base| -> Result<()> { + // The symlink itself should be replaced with the file, not followed. + let link = base.join("a/link"); + let link_stat = nix::sys::stat::lstat(&link).context("stat ./a/link failed")?; + ensure!(0o600 | libc::S_IFREG == link_stat.st_mode); + let content = std::fs::read_to_string(&link).context("read ./a/link failed")?; + ensure!("Hello!" == content); + Ok(()) + }), + }, + TestCase { + name: "Writing outside the shared directory is rejected".into(), + request: CopyFileRequest { + path: base.parent().unwrap().join("not-shared").to_string_lossy().into(), + file_mode: 0o600 | libc::S_IFREG, + ..Default::default() + }, + should_fail: true, + assertions: Box::new(|base| -> Result<()> { + match fs::metadata(base.parent().unwrap().join("not-shared")) { + Ok(_) => bail!("successful write outside shared directory"), + Err(_) => Ok(()) + } + }), + }, + TestCase { + name: "Traversal outside shared directory is rejected".into(), + request: CopyFileRequest { + path: base.join("../not-shared").to_string_lossy().into(), + file_mode: 0o600 | libc::S_IFREG, + ..Default::default() + }, + should_fail: true, + assertions: Box::new(|base| -> Result<()> { + match fs::metadata(base.join("../not-shared")) { + Ok(_) => bail!("successful write outside shared directory"), + Err(_) => Ok(()) + } + }), + }, + ]; + + let uid = unistd::getuid().as_raw() as i32; + let gid = unistd::getgid().as_raw() as i32; + + for mut tc in tests { + println!("Running test case: {}", tc.name); + // Since we're in a unit test, using root ownership causes issues with cleaning the temp dir. + tc.request.uid = uid; + tc.request.gid = gid; + + let res = do_copy_file(&tc.request, &base); + if tc.should_fail != res.is_err() { + panic!("{}: unexpected do_copy_file result: {:?}", tc.name, res) + } + (tc.assertions)(&base).context(tc.name).unwrap() + } + } } diff --git a/tests/functional/kata-agent-apis/api-tests/test_copy_file.bats b/tests/functional/kata-agent-apis/api-tests/test_copy_file.bats index d9875683a2..c05c0097d4 100755 --- a/tests/functional/kata-agent-apis/api-tests/test_copy_file.bats +++ b/tests/functional/kata-agent-apis/api-tests/test_copy_file.bats @@ -11,32 +11,32 @@ setup_file() { info "setup" } -@test "Test CopyFile API: Copy a file to /run/kata-containers" { - info "Copy file to /run/kata-containers" +@test "Test CopyFile API: Copy a file to /run/kata-containers/shared/containers" { + info "Copy file to /run/kata-containers/shared/containers" src_file=$(mktemp) local cmds=() - cmds+=("-c 'CopyFile json://{\"src\": \"$src_file\", \"dest\":\"/run/kata-containers/foo\"}'") + cmds+=("-c 'CopyFile json://{\"src\": \"$src_file\", \"dest\":\"/run/kata-containers/shared/containers/foo\"}'") run_agent_ctl "${cmds[@]}" rm $src_file } -@test "Test CopyFile API: Copy a symlink to /run/kata-containers" { - info "Copy symlink to /run/kata-containers" +@test "Test CopyFile API: Copy a symlink to /run/kata-containers/shared/containers" { + info "Copy symlink to /run/kata-containers/shared/containers" src_file=$(mktemp) link_file="/tmp/link" ln -s $src_file $link_file local cmds=() - cmds+=("-c 'CopyFile json://{\"src\": \"$link_file\", \"dest\":\"/run/kata-containers/link\"}'") + cmds+=("-c 'CopyFile json://{\"src\": \"$link_file\", \"dest\":\"/run/kata-containers/shared/containers/link\"}'") run_agent_ctl "${cmds[@]}" unlink $link_file rm $src_file } -@test "Test CopyFile API: Copy a directory to /run/kata-containers" { - info "Copy directory to /run/kata-containers" +@test "Test CopyFile API: Copy a directory to /run/kata-containers/shared/containers" { + info "Copy directory to /run/kata-containers/shared/containers" src_dir=$(mktemp -d) local cmds=() - cmds+=("-c 'CopyFile json://{\"src\": \"$src_dir\", \"dest\":\"/run/kata-containers/dir\"}'") + cmds+=("-c 'CopyFile json://{\"src\": \"$src_dir\", \"dest\":\"/run/kata-containers/shared/containers/dir\"}'") run_agent_ctl "${cmds[@]}" rmdir $src_dir } @@ -51,17 +51,30 @@ setup_file() { rm $src_file } -@test "Test CopyFile API: Copy a large file to /run/kata-containers" { - info "Copy large file to /run/kata-containers" +@test "Test CopyFile API: Copy a file to an unallowed destination beneath /run/kata-containers" { + # This is a regression test, copying files to /run/kata-containers used to be allowed, but the + # implementation is more strict now and requires paths to be under + # /run/kata-containers/shared/containers. + info "Copy file to /tmp" + src_file=$(mktemp) + local cmds=() + cmds+=("-c 'CopyFile json://{\"src\": \"$src_file\", \"dest\":\"/run/kata-containers/shared/foo\"}'") + run run_agent_ctl "${cmds[@]}" + [ "$status" -ne 0 ] + rm $src_file +} + +@test "Test CopyFile API: Copy a large file to /run/kata-containers/shared/containers" { + info "Copy large file to /run/kata-containers/shared/containers" src_file="/tmp/large_file_2M.txt" truncate -s 2M $src_file local cmds=() - cmds+=("-c 'CopyFile json://{\"src\": \"$src_file\", \"dest\":\"/run/kata-containers/large_file_2M.txt\"}'") + cmds+=("-c 'CopyFile json://{\"src\": \"$src_file\", \"dest\":\"/run/kata-containers/shared/containers/large_file_2M.txt\"}'") run_agent_ctl "${cmds[@]}" rm $src_file } teardown_file() { info "teardown" - sudo rm -r /run/kata-containers/ || echo "Failed to clean /run/kata-containers" + sudo rm -r /run/kata-containers/shared/containers/ || echo "Failed to clean /run/kata-containers/shared/containers" } diff --git a/tests/functional/kata-agent-apis/api-tests/test_set_policy.bats b/tests/functional/kata-agent-apis/api-tests/test_set_policy.bats index 6d2f5df7ff..ed15f6369d 100755 --- a/tests/functional/kata-agent-apis/api-tests/test_set_policy.bats +++ b/tests/functional/kata-agent-apis/api-tests/test_set_policy.bats @@ -30,7 +30,7 @@ setup_file() { src_file=$(mktemp) local cmds=() - cmds+=("-c 'CopyFile json://{\"src\": \"$src_file\", \"dest\":\"/run/kata-containers/foo\"}'") + cmds+=("-c 'CopyFile json://{\"src\": \"$src_file\", \"dest\":\"/run/kata-containers/shared/containers/foo\"}'") run run_agent_ctl "${cmds[@]}" [ "$status" -ne 0 ]