diff --git a/Cargo.lock b/Cargo.lock index 8feb22cb5d..8ccb51dcd5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3575,6 +3575,7 @@ dependencies = [ "nix 0.31.3", "oci-spec 0.10.0", "opentelemetry 0.17.0", + "pathrs", "procfs 0.12.0", "prometheus", "protobuf", @@ -5237,6 +5238,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 162dfe4549..5ee2657c46 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -183,6 +183,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.10.0", 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 b09f8600df..a1a2318986 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 765a01665c..279a8f1528 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::{ @@ -3744,4 +3801,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 ]