diff options
author | Allen Webb <allenwebb@google.com> | 2021-04-16 09:44:53 -0500 |
---|---|---|
committer | Treehugger Robot <treehugger-gerrit@google.com> | 2021-04-19 23:12:26 +0000 |
commit | c71826854946d4f24e23304b63aab0f2c1639932 (patch) | |
tree | f726d6199eeec0cd64efd992d7ff4168c3c5149b | |
parent | 2a227b73e82376757297b484f79f86eabc3be52a (diff) | |
download | minijail-c71826854946d4f24e23304b63aab0f2c1639932.tar.gz |
Add overlap handling to redirect_fds().
Previously redirect_fds would clobber mapping source fds if the
destination of a previous mapping collided. This change adds detection
of these collisions and handles it by mapping them ahead of time when
possible or mapping to a temporary otherwise.
It also cleans up some badness in the Rust wrapper that was closing
file descriptors already closed by libminijail, and updates the
fork_remap test to make it easier to debug.
Bug: 185349327
Test: cargo test -- --test-threads=1
Change-Id: I637846dfbe73b73dbb5d218bcfc8464c0cd7d3b4
-rw-r--r-- | libminijail.c | 77 | ||||
-rw-r--r-- | rust/minijail/src/lib.rs | 10 | ||||
-rw-r--r-- | rust/minijail/tests/fork_remap.rs | 64 |
3 files changed, 122 insertions, 29 deletions
diff --git a/libminijail.c b/libminijail.c index 582554a..e0d9d54 100644 --- a/libminijail.c +++ b/libminijail.c @@ -8,6 +8,7 @@ #define _GNU_SOURCE #include <asm/unistd.h> +#include <assert.h> #include <dirent.h> #include <errno.h> #include <fcntl.h> @@ -26,6 +27,7 @@ #include <sys/param.h> #include <sys/prctl.h> #include <sys/resource.h> +#include <sys/select.h> #include <sys/stat.h> #include <sys/sysmacros.h> #include <sys/types.h> @@ -2592,8 +2594,74 @@ static int close_open_fds(int *inheritable_fds, size_t size) return 0; } +/* Return true if the specified file descriptor is already open. */ +static int fd_is_open(int fd) +{ + return fcntl(fd, F_GETFD) != -1 || errno != EBADF; +} + +static_assert(FD_SETSIZE >= MAX_PRESERVED_FDS * 2 - 1, + "If true, ensure_no_fd_conflict will always find an unused fd."); + +/* If p->parent_fd will be used by a child_fd, move it to an unused fd. */ +static int ensure_no_fd_conflict(const fd_set* child_fds, + struct preserved_fd* p) +{ + if (!FD_ISSET(p->parent_fd, child_fds)){ + return 0; + } + + /* + * If no other parent_fd matches the child_fd then use it instead of a + * temporary. + */ + int fd = p->child_fd; + if (fd_is_open(fd)) { + fd = FD_SETSIZE - 1; + while (FD_ISSET(fd, child_fds) || fd_is_open(fd)) { + --fd; + if (fd < 0) { + die("failed to find an unused fd"); + } + } + } + + int ret = dup2(p->parent_fd, fd); + /* + * warn() opens a file descriptor so it needs to happen after dup2 to + * avoid unintended side effects. This can be avoided by reordering the + * mapping requests so that the source fds with overlap are mapped + * first (unless there are cycles). + */ + warn("mapped fd overlap: moving %d to %d", p->parent_fd, fd); + if (ret == -1) { + return -1; + } + + p->parent_fd = fd; + return 0; +} + static int redirect_fds(struct minijail *j) { + fd_set child_fds; + FD_ZERO(&child_fds); + + /* Relocate parent_fds that would be replaced by a child_fd. */ + for (size_t i = 0; i < j->preserved_fd_count; i++) { + int child_fd = j->preserved_fds[i].child_fd; + if (FD_ISSET(child_fd, &child_fds)) { + die("fd %d is mapped more than once", child_fd); + } + + if (ensure_no_fd_conflict(&child_fds, + &j->preserved_fds[i]) == -1) { + return -1; + } + + FD_SET(child_fd, &child_fds); + } + for (size_t i = 0; i < j->preserved_fd_count; i++) { if (j->preserved_fds[i].parent_fd == j->preserved_fds[i].child_fd) { @@ -2609,13 +2677,10 @@ static int redirect_fds(struct minijail *j) * fds that are *not* child fds. */ for (size_t i = 0; i < j->preserved_fd_count; i++) { - int closeable = true; - for (size_t i2 = 0; i2 < j->preserved_fd_count; i2++) { - closeable &= j->preserved_fds[i].parent_fd != - j->preserved_fds[i2].child_fd; + int parent_fd = j->preserved_fds[i].parent_fd; + if (!FD_ISSET(parent_fd, &child_fds)) { + close(parent_fd); } - if (closeable) - close(j->preserved_fds[i].parent_fd); } return 0; } diff --git a/rust/minijail/src/lib.rs b/rust/minijail/src/lib.rs index 289668e..d4f5787 100644 --- a/rust/minijail/src/lib.rs +++ b/rust/minijail/src/lib.rs @@ -9,7 +9,7 @@ use std::fmt::{self, Display}; use std::fs; use std::io; use std::os::raw::{c_char, c_ulong, c_ushort}; -use std::os::unix::io::{AsRawFd, RawFd}; +use std::os::unix::io::{AsRawFd, IntoRawFd, RawFd}; use std::path::{Path, PathBuf}; use std::ptr::{null, null_mut}; @@ -733,6 +733,10 @@ impl Minijail { /// could cause a lot of trouble if not handled carefully. FDs 0, 1, and 2 /// are overwritten with /dev/null FDs unless they are included in the /// inheritable_fds list. + /// + /// Also, any Rust objects that own fds may try to close them after the fork. If they belong + /// to a fd number that was mapped to, the mapped fd will be closed instead. + /// /// This Function may abort in the child on error because a partially /// entered jail isn't recoverable. pub unsafe fn fork(&self, inheritable_fds: Option<&[RawFd]>) -> Result<pid_t> { @@ -787,6 +791,10 @@ impl Minijail { if ret < 0 { return Err(Error::ForkingMinijail(ret)); } + if ret == 0 { + // Safe because dev_null was remapped. + dev_null.into_raw_fd(); + } Ok(ret as pid_t) } diff --git a/rust/minijail/tests/fork_remap.rs b/rust/minijail/tests/fork_remap.rs index 8ace83b..6cf3415 100644 --- a/rust/minijail/tests/fork_remap.rs +++ b/rust/minijail/tests/fork_remap.rs @@ -10,40 +10,51 @@ use std::fs::{read_link, File, OpenOptions}; use std::io::{self, Read}; -use std::os::unix::io::{AsRawFd, FromRawFd, RawFd}; +use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd}; use std::path::Path; use minijail::Minijail; const DEV_NULL: &str = "/dev/null"; const DEV_ZERO: &str = "/dev/zero"; +const PROC_CMDLINE: &str = "/proc/self/cmdline"; -const DEST_FD1: RawFd = 7; -const DEST_FD2: RawFd = 8; - -fn open_dev_zero() -> Result<File, io::Error> { +fn open_path(path: &str) -> Result<File, io::Error> { OpenOptions::new() .read(true) - .write(true) - .open(Path::new(DEV_ZERO)) + .write(false) + .open(Path::new(path)) } fn main() { - let mut check_file1 = open_dev_zero().unwrap(); - let mut check_file2 = open_dev_zero().unwrap(); + let mut check_file1 = open_path(DEV_ZERO).unwrap(); + let mut check_file2 = open_path(PROC_CMDLINE).unwrap(); let j = Minijail::new().unwrap(); - for p in &[0, 1, check_file1.as_raw_fd(), check_file2.as_raw_fd()] { + let mut stdio_expected = String::new(); + let mut file2_expected = String::new(); + for &p in &[0, 1, 2, check_file1.as_raw_fd(), check_file2.as_raw_fd()] { let path = format!("/proc/self/fd/{}", p); let target = read_link(Path::new(&path)); eprintln!("P: {} -> {:?}", p, &target); + if p == 2 { + stdio_expected = target.unwrap().to_string_lossy().to_string(); + } else if p == check_file2.as_raw_fd() { + file2_expected = target.unwrap().to_string_lossy().to_string(); + } } + // Swap fd1 and fd2. + let dest_fd1: RawFd = check_file2.as_raw_fd(); + let dest_fd2: RawFd = check_file1.as_raw_fd(); + if unsafe { j.fork_remap(&[ - (2, 2), - (check_file1.as_raw_fd(), DEST_FD1), - (check_file2.as_raw_fd(), DEST_FD2), + // fd 0 tests stdio mapped to /dev/null. + (2, 1), // One-to-many. + (2, 2), // Identity. + (check_file1.as_raw_fd(), dest_fd1), // Cross-over. + (check_file2.as_raw_fd(), dest_fd2), // Cross-over. ]) } .unwrap() @@ -54,21 +65,27 @@ fn main() { return; } - // Safe because we are re-taking ownership of a remapped fd after forking. - check_file1 = unsafe { File::from_raw_fd(DEST_FD1) }; - check_file2 = unsafe { File::from_raw_fd(DEST_FD2) }; + // Safe because we are re-taking ownership of remapped fds after forking. + unsafe { + check_file1.into_raw_fd(); + check_file1 = File::from_raw_fd(dest_fd1); + + check_file2.into_raw_fd(); + check_file2 = File::from_raw_fd(dest_fd2); + } for (p, expected) in &[ (0, DEV_NULL), - (1, DEV_NULL), - (DEST_FD1, DEV_ZERO), - (DEST_FD2, DEV_ZERO), + (1, &stdio_expected), + (2, &stdio_expected), + (dest_fd1, DEV_ZERO), + (dest_fd2, &file2_expected), ] { let path = format!("/proc/self/fd/{}", p); let target = read_link(Path::new(&path)); - eprintln!("C: {} -> {:?}", p, &target); + eprintln!(" C: {} -> {:?}", p, &target); if !matches!(&target, Ok(p) if p == Path::new(expected)) { - panic!("C: got {:?}; expected Ok({:?})", target, expected); + panic!(" C: got {:?}; expected Ok({:?})", target, expected); } } @@ -77,5 +94,8 @@ fn main() { check_file1.read_exact(&mut buffer).unwrap(); assert_eq!(&buffer, &[0u8; BUFFER_LEN]); - eprintln!("Child done."); + let mut file2_contents = Vec::<u8>::new(); + check_file2.read_to_end(&mut file2_contents).unwrap(); + + eprintln!(" Child done."); } |