diff --git a/src/worker/include/job_output_path.h b/src/worker/include/job_output_path.h new file mode 100644 index 00000000..fb0e27d1 --- /dev/null +++ b/src/worker/include/job_output_path.h @@ -0,0 +1,71 @@ +#pragma once +/* + * job_output_path.h — the worker's per-task job output file naming. + * + * uv_proc.c (xcecd_worker) creates each task's stdout/stderr file under this + * 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. + */ +#include +#include +#include +#include +#include + +#include "utils.h" + +#ifdef AUTOSCALER_TEST +#define XCECD_JOB_OUTPUT_DIR "/shared/log" +#else +#define XCECD_JOB_OUTPUT_DIR "/tmp" +#endif + +/* /job__task.out */ +static inline int xcecd_job_output_path(char* buf, size_t bufsize, + uint64_t jobid, int task) +{ + char id[128]; + + if (buf == NULL) + return -1; + fmt_jobid(id, sizeof(id) - 1, jobid); + return snprintf(buf, bufsize, "%s/job_%s_task%d.out", + XCECD_JOB_OUTPUT_DIR, id, task); +} + +/* True iff `path` is exactly xcecd_job_output_path(jobid, N) for some task + * index N >= 0, spelled the way snprintf("%d") spells it (no sign, no + * leading zeros). An exact string match, so "..", "//", other jobs' files + * and any other path are all rejected without touching the filesystem. */ +static inline bool xcecd_is_job_output_path(uint64_t jobid, const char* path) +{ + char prefix[256]; + char id[128]; + const char* p; + int n; + int ndigits = 0; + + if (path == NULL) + return false; + + fmt_jobid(id, sizeof(id) - 1, jobid); + n = snprintf(prefix, sizeof(prefix), "%s/job_%s_task", + XCECD_JOB_OUTPUT_DIR, id); + if (n < 0 || (size_t)n >= sizeof(prefix)) + return false; + if (strncmp(path, prefix, (size_t)n) != 0) + return false; + + p = path + n; + while (*p >= '0' && *p <= '9') { + p++; + ndigits++; + } + if (ndigits == 0 || ndigits > 9) + return false; + if (ndigits > 1 && path[n] == '0') + return false; + + return strcmp(p, ".out") == 0; +} diff --git a/src/worker/src/controller_connector.c b/src/worker/src/controller_connector.c index ee94b6c0..82ffec47 100755 --- a/src/worker/src/controller_connector.c +++ b/src/worker/src/controller_connector.c @@ -1,5 +1,6 @@ #include "client.h" #include "controller_connector.h" +#include "job_output_path.h" #include "netio.h" #include "uv_ipc.h" #include "xcecd_cgroups.h" @@ -1403,6 +1404,37 @@ static int handle_runtasks(xce_request_t* request) return 0; } +/* bug-1508: this daemon runs as root, so a fetchfile request may only read + * the worker's own output file for that job (job_output_path.h), never an + * arbitrary path. The name check is an exact string match; the open then + * refuses a symlink (O_NOFOLLOW), a FIFO that would block the reply + * (O_NONBLOCK) and anything but a regular file with a single link, so a + * link planted at the output name cannot expose another file. Returns an + * fd or -errno (-EACCES for a disallowed path). */ +static int fetchfile_open_checked(uint64_t jobid, const char* path) +{ + struct stat st; + int fd; + + if (!xcecd_is_job_output_path(jobid, path)) + return -EACCES; + + fd = open(path, O_RDONLY | O_NOFOLLOW | O_NONBLOCK | O_CLOEXEC); + if (fd < 0) + return -errno; + + if (fstat(fd, &st) < 0) { + int err = -errno; + close(fd); + return err; + } + if (!S_ISREG(st.st_mode) || st.st_nlink != 1) { + close(fd); + return -EACCES; + } + return fd; +} + /** * @brief Handles the fetchfile request from the controller. * @@ -1442,11 +1474,11 @@ static int handle_fetchfile(xce_request_t* request) ? 4 * 1024 * 1024 : msg->max_bytes; - fd = open(msg->filepath, O_RDONLY); + fd = fetchfile_open_checked(msg->jobid, msg->filepath); if (fd < 0) { XCECD_LOG(XCE_LOG_ERROR, "Fetchfile: open(%s) failed: %s", - msg->filepath, strerror(errno)); - freply.error = -errno; + msg->filepath, strerror(-fd)); + freply.error = fd; goto send; } diff --git a/src/worker/src/uv_proc.c b/src/worker/src/uv_proc.c index 2ddc5aa8..83a9b1ea 100644 --- a/src/worker/src/uv_proc.c +++ b/src/worker/src/uv_proc.c @@ -8,26 +8,9 @@ #include #include +#include "job_output_path.h" #include "utils.h" -static int make_job_output_path(char* buf, size_t bufsize, const char* dir, - uint64_t jobid, int task) -{ - char idstring[128]; - - if (buf == NULL) { - return -EINVAL; - } - - if (dir == NULL) { - dir = "/tmp"; - } - - fmt_jobid(idstring, sizeof(idstring) - 1, jobid); - - return snprintf(buf, bufsize, "%s/job_%s_task%d.out", dir, idstring, task); -} - static void on_process_exit(uv_process_t* process, int64_t exit_status, int term_signal) { @@ -261,13 +244,8 @@ int uv_proc_exec(uv_proc_manager_t* manager, const char* command, } char job_output[256]; -#ifdef AUTOSCALER_TEST - make_job_output_path(job_output, sizeof(job_output) - 1, "/shared/log", - manager->job_id, i); -#else - make_job_output_path(job_output, sizeof(job_output) - 1, "/tmp", - manager->job_id, i); -#endif + xcecd_job_output_path(job_output, sizeof(job_output) - 1, + manager->job_id, i); uv_stdio_container_t stdio[3]; uv_fs_t file_req; diff --git a/src/worker/testsuite/test_controller_connector.c b/src/worker/testsuite/test_controller_connector.c index 33e87ad3..8572bcef 100644 --- a/src/worker/testsuite/test_controller_connector.c +++ b/src/worker/testsuite/test_controller_connector.c @@ -1410,6 +1410,139 @@ test_blocked_connector_thread_does_not_stall_other_liveness_duty(void) ticks, SLOW_REJOIN_BLOCK_MS); } +/* ════════════════════════════════════════════════════════════════════════ + * bug-1508: handle_fetchfile() runs as root and used to open() whatever + * path the request named. fetchfile_open_checked() must only open the + * worker's own output file for that job, /tmp/job__task.out + * (uv_proc.c), as a regular, single-link, non-symlink file. + * ════════════════════════════════════════════════════════════════════════ */ + +#define FF_JOBID ((uint64_t)915080001ULL) +#define FF_OTHER_JOBID ((uint64_t)915080002ULL) + +static void ff_output_path(char* buf, size_t len, uint64_t jobid, int task) +{ + char id[128]; + + fmt_jobid(id, sizeof(id) - 1, jobid); + snprintf(buf, len, "/tmp/job_%s_task%d.out", id, task); +} + +static void ff_write_file(const char* path, const char* content) +{ + FILE* f; + + unlink(path); + f = fopen(path, "w"); + TEST_ASSERT_NOT_NULL(f); + fputs(content, f); + fclose(f); +} + +static void ff_assert_rejected(uint64_t jobid, const char* path) +{ + int fd = fetchfile_open_checked(jobid, path); + + if (fd >= 0) + close(fd); + TEST_ASSERT_TRUE_MESSAGE(fd < 0, path); +} + +static void test_fetchfile_opens_own_job_output(void) +{ + char p0[256], p3[256]; + char buf[16] = { 0 }; + int fd; + ssize_t n; + + ff_output_path(p0, sizeof(p0), FF_JOBID, 0); + ff_output_path(p3, sizeof(p3), FF_JOBID, 3); + ff_write_file(p0, "hello"); + ff_write_file(p3, "task3"); + + fd = fetchfile_open_checked(FF_JOBID, p0); + TEST_ASSERT_TRUE_MESSAGE(fd >= 0, "task0 output of the job must open"); + n = read(fd, buf, sizeof(buf) - 1); + close(fd); + TEST_ASSERT_EQUAL_INT(5, (int)n); + TEST_ASSERT_EQUAL_STRING("hello", buf); + + fd = fetchfile_open_checked(FF_JOBID, p3); + TEST_ASSERT_TRUE_MESSAGE(fd >= 0, "any task index's output must open"); + close(fd); + + unlink(p0); + unlink(p3); +} + +static void test_fetchfile_rejects_arbitrary_paths(void) +{ + char own[256], other[256], buf[512]; + + ff_output_path(own, sizeof(own), FF_JOBID, 0); + ff_output_path(other, sizeof(other), FF_OTHER_JOBID, 0); + ff_write_file(own, "own"); + ff_write_file(other, "other"); + + ff_assert_rejected(FF_JOBID, "/etc/passwd"); + ff_assert_rejected(FF_JOBID, "/etc/hostname"); + /* another job's output, even though it is a real worker output file */ + ff_assert_rejected(FF_JOBID, other); + + /* traversal / non-canonical spellings of an allowed name */ + snprintf(buf, sizeof(buf), "%s/../../etc/passwd", own); + ff_assert_rejected(FF_JOBID, buf); + snprintf(buf, sizeof(buf), "/tmp/../tmp/%s", own + strlen("/tmp/")); + ff_assert_rejected(FF_JOBID, buf); + snprintf(buf, sizeof(buf), "/tmp//%s", own + strlen("/tmp/")); + ff_assert_rejected(FF_JOBID, buf); + + /* malformed task suffixes */ + snprintf(buf, sizeof(buf), "%sx", own); + ff_assert_rejected(FF_JOBID, buf); + snprintf(buf, sizeof(buf), "%.*s.out", (int)(strlen(own) - strlen("0.out")), + own); + ff_assert_rejected(FF_JOBID, buf); /* "..._task.out" */ + snprintf(buf, sizeof(buf), "%.*s-1.out", + (int)(strlen(own) - strlen("0.out")), own); + ff_assert_rejected(FF_JOBID, buf); + + ff_assert_rejected(FF_JOBID, ""); + + unlink(own); + unlink(other); +} + +static void test_fetchfile_rejects_symlink_and_hardlink_at_output_name(void) +{ + char own[256]; + char target[] = "/tmp/xce_ff_hardlink_target_XXXXXX"; + int tfd; + + ff_output_path(own, sizeof(own), FF_JOBID, 0); + + /* a symlink planted at the job's output name must not be followed */ + unlink(own); + TEST_ASSERT_EQUAL_INT(0, symlink("/etc/hostname", own)); + ff_assert_rejected(FF_JOBID, own); + unlink(own); + + /* a hardlink to another file at the output name must not be read */ + tfd = mkstemp(target); + TEST_ASSERT_TRUE(tfd >= 0); + TEST_ASSERT_TRUE(write(tfd, "secret", 6) == 6); + close(tfd); + TEST_ASSERT_EQUAL_INT(0, link(target, own)); + ff_assert_rejected(FF_JOBID, own); + unlink(own); + unlink(target); + + /* a directory at the output name is not a readable output file */ + TEST_ASSERT_EQUAL_INT(0, mkdir(own, 0700)); + ff_assert_rejected(FF_JOBID, own); + rmdir(own); +} + /* ════════════════════════════════════════════════════════════════════════ * main * ════════════════════════════════════════════════════════════════════════ */ @@ -1455,6 +1588,9 @@ int main(void) RUN_TEST(test_xcecd_join_cluster_all_unreachable_reports_every_candidate); RUN_TEST( test_blocked_connector_thread_does_not_stall_other_liveness_duty); + RUN_TEST(test_fetchfile_opens_own_job_output); + RUN_TEST(test_fetchfile_rejects_arbitrary_paths); + RUN_TEST(test_fetchfile_rejects_symlink_and_hardlink_at_output_name); return UNITY_END(); }