From 7864a3dab7d9870f7ab38648255eadce4d4a2f76 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 26 Sep 2026 07:55:23 +0000 Subject: [PATCH] fix(worker): don't follow links planted at a job's output file name (bug-1868) uv_proc_exec() created /tmp/job__task.out as root with O_WRONLY|O_CREAT|O_TRUNC. The name is predictable and /tmp is world- writable, so a local user could plant a symlink (or hardlink) there before the job started and make the root worker truncate and write job output into the target. Only the host's fs.protected_* sysctls stood in the way. xcecd_create_job_output() (job_output_path.h) now unlinks whatever is at the name and creates a fresh file with O_EXCL|O_NOFOLLOW|O_CLOEXEC, mode 0600 as before; O_EXCL never follows a symlink and fails if anything reappears, so a race can only fail the create, never redirect it (retried 3 times). A re-run job still starts with an empty file, as O_TRUNC gave. New test_job_output_path.c (4 tests, 2 RED before the fix); worker suite 11/11. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/worker/include/job_output_path.h | 32 +++++ src/worker/src/uv_proc.c | 12 +- src/worker/testsuite/meson.build | 13 ++ src/worker/testsuite/test_job_output_path.c | 130 ++++++++++++++++++++ 4 files changed, 179 insertions(+), 8 deletions(-) create mode 100644 src/worker/testsuite/test_job_output_path.c diff --git a/src/worker/include/job_output_path.h b/src/worker/include/job_output_path.h index fb0e27d1..19c907af 100644 --- a/src/worker/include/job_output_path.h +++ b/src/worker/include/job_output_path.h @@ -6,12 +6,18 @@ * name; handle_fetchfile() (controller_connector.c, xcecd) serves reads of it * and must accept nothing else (bug-1508). The two live in different * executables, so the format is shared here rather than in either .c file. + * xcecd_create_job_output() is the only way the file should be created + * (bug-1868). */ +#include +#include #include #include #include #include #include +#include +#include #include "utils.h" @@ -69,3 +75,29 @@ static inline bool xcecd_is_job_output_path(uint64_t jobid, const char* path) return strcmp(p, ".out") == 0; } + +/* bug-1868: create (or re-create, for a re-run job) a task's output file. + * The name is predictable and lives in world-writable /tmp while the worker + * runs as root, so a plain O_CREAT|O_TRUNC open would truncate and write + * through a symlink or hardlink a local user planted there beforehand. + * Instead, remove whatever is at the name (root may unlink in a sticky dir) + * and create a new file with O_EXCL|O_NOFOLLOW: O_EXCL never follows a + * symlink and fails if anything reappears in between, so a racing attacker + * can at worst make this fail, never redirect the write. Retries a few + * times on that race. Returns an fd or -errno. */ +static inline int xcecd_create_job_output(const char* path) +{ + int attempt; + int fd = -1; + + for (attempt = 0; attempt < 3; attempt++) { + if (unlink(path) < 0 && errno != ENOENT) + return -errno; + fd = open(path, + O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC, + S_IRUSR | S_IWUSR); + if (fd >= 0 || errno != EEXIST) + break; + } + return (fd < 0) ? -errno : fd; +} diff --git a/src/worker/src/uv_proc.c b/src/worker/src/uv_proc.c index 83a9b1ea..8029b277 100644 --- a/src/worker/src/uv_proc.c +++ b/src/worker/src/uv_proc.c @@ -248,13 +248,10 @@ int uv_proc_exec(uv_proc_manager_t* manager, const char* command, manager->job_id, i); uv_stdio_container_t stdio[3]; - uv_fs_t file_req; - int fd - = uv_fs_open(manager->loop, &file_req, job_output, - O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR, NULL); + int fd = xcecd_create_job_output(job_output); if (fd < 0) { - XCECD_WORKER_LOG(XCE_LOG_ERROR, "uv_fs_open failed: %s", - uv_strerror(fd)); + XCECD_WORKER_LOG(XCE_LOG_ERROR, "create %s failed: %s", job_output, + strerror(-fd)); free(process); for (int j = 0; j < manager->proc_count; j++) { @@ -290,8 +287,7 @@ int uv_proc_exec(uv_proc_manager_t* manager, const char* command, int result = uv_spawn(manager->loop, process, &options); - uv_fs_close(manager->loop, &file_req, fd, NULL); - uv_fs_req_cleanup(&file_req); + close(fd); if (result != 0) { XCECD_WORKER_LOG(XCE_LOG_ERROR, "uv_spawn failed: %s", diff --git a/src/worker/testsuite/meson.build b/src/worker/testsuite/meson.build index 75c9eb36..edb7f9a5 100644 --- a/src/worker/testsuite/meson.build +++ b/src/worker/testsuite/meson.build @@ -180,3 +180,16 @@ test('worker xcecd_cgroups rusage test', xcecd_cgroups_rusage_test, suite: 'worker', timeout: 60, env: ['ASAN_OPTIONS=detect_leaks=0']) + +# bug-1868: the root worker's per-task output file creation must not follow +# a symlink/hardlink planted at its predictable /tmp name (job_output_path.h). +job_output_path_test = executable('job_output_path_test', + 'test_job_output_path.c', + include_directories: [worker_test_incdir], + dependencies: worker_test_deps, + install: false) + +test('worker job_output_path test', job_output_path_test, + suite: 'worker', + timeout: 60, + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/worker/testsuite/test_job_output_path.c b/src/worker/testsuite/test_job_output_path.c new file mode 100644 index 00000000..636f45ae --- /dev/null +++ b/src/worker/testsuite/test_job_output_path.c @@ -0,0 +1,130 @@ +/* + * test_job_output_path.c — bug-1868: the root worker creates each task's + * output file at a predictable name in world-writable /tmp + * (job_output_path.h). xcecd_create_job_output() must never write through a + * symlink or hardlink planted at that name, while still giving a re-run job + * a fresh, empty file. + */ +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "job_output_path.h" + +#define JOP_JOBID ((uint64_t)918680001ULL) + +static char g_path[256]; +static char g_victim[64]; + +void setUp(void) +{ + int fd; + + xcecd_job_output_path(g_path, sizeof(g_path), JOP_JOBID, 0); + unlink(g_path); + + strcpy(g_victim, "/tmp/xce_jop_victim_XXXXXX"); + fd = mkstemp(g_victim); + TEST_ASSERT_TRUE(fd >= 0); + TEST_ASSERT_EQUAL_INT(8, (int)write(fd, "PRECIOUS", 8)); + close(fd); +} + +void tearDown(void) +{ + unlink(g_path); + unlink(g_victim); +} + +static void assert_victim_untouched(void) +{ + char buf[16] = { 0 }; + int fd = open(g_victim, O_RDONLY); + + TEST_ASSERT_TRUE(fd >= 0); + TEST_ASSERT_EQUAL_INT(8, (int)read(fd, buf, sizeof(buf) - 1)); + close(fd); + TEST_ASSERT_EQUAL_STRING_MESSAGE("PRECIOUS", buf, + "the file a planted link pointed at must not be truncated/written"); +} + +static void assert_fresh_private_regular_file(int fd) +{ + struct stat st; + + TEST_ASSERT_EQUAL_INT(0, lstat(g_path, &st)); + TEST_ASSERT_TRUE_MESSAGE(S_ISREG(st.st_mode), + "the output name must now be a regular file, not a link"); + TEST_ASSERT_EQUAL_INT(1, (int)st.st_nlink); + TEST_ASSERT_EQUAL_INT(0, (int)st.st_size); + TEST_ASSERT_EQUAL_INT(S_IRUSR | S_IWUSR, (int)(st.st_mode & 0777)); + + TEST_ASSERT_EQUAL_INT(2, (int)write(fd, "ok", 2)); +} + +static void test_creates_new_output_file(void) +{ + int fd = xcecd_create_job_output(g_path); + + TEST_ASSERT_TRUE(fd >= 0); + assert_fresh_private_regular_file(fd); + close(fd); +} + +static void test_rerun_gets_empty_file(void) +{ + int fd = open(g_path, O_WRONLY | O_CREAT, 0600); + + TEST_ASSERT_TRUE(fd >= 0); + TEST_ASSERT_EQUAL_INT(3, (int)write(fd, "old", 3)); + close(fd); + + fd = xcecd_create_job_output(g_path); + TEST_ASSERT_TRUE(fd >= 0); + assert_fresh_private_regular_file(fd); + close(fd); +} + +static void test_planted_symlink_is_not_followed(void) +{ + int fd; + + TEST_ASSERT_EQUAL_INT(0, symlink(g_victim, g_path)); + + fd = xcecd_create_job_output(g_path); + if (fd >= 0) { + assert_fresh_private_regular_file(fd); + close(fd); + } + assert_victim_untouched(); +} + +static void test_planted_hardlink_is_not_written_through(void) +{ + int fd; + + TEST_ASSERT_EQUAL_INT(0, link(g_victim, g_path)); + + fd = xcecd_create_job_output(g_path); + if (fd >= 0) { + assert_fresh_private_regular_file(fd); + close(fd); + } + assert_victim_untouched(); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_creates_new_output_file); + RUN_TEST(test_rerun_gets_empty_file); + RUN_TEST(test_planted_symlink_is_not_followed); + RUN_TEST(test_planted_hardlink_is_not_written_through); + return UNITY_END(); +}