fix(worker): fetchfile only reads the job's own output file (bug-1508)

handle_fetchfile() runs as root in xcecd and open()ed whatever filepath
the request carried. Any libxce client could read any file on a worker
(/etc/shadow, keys) with a fetchfile RPC, and so could any REST user: the
apiserver resolves the path from the job's user-supplied out_file/err_file,
so submitting a job with out_file=/etc/shadow and then reading its output
passed the apiserver's ownership check and read the file as root.

- job_output_path.h: the per-task output name uv_proc.c writes,
  <dir>/job_<jobid>_task<N>.out, now shared by the writer (xcecd_worker)
  and the reader (xcecd), plus xcecd_is_job_output_path(), an exact
  string match for that job's names only.
- fetchfile_open_checked(): rejects any other path with -EACCES, opens
  with O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC and requires a regular file with a
  single link, so a symlink, hardlink, FIFO or directory planted at the
  output name is refused.

Three new tests in test_controller_connector.c (2 RED before the fix);
worker suite 10/10. Sequential HA soak (5 rounds, xcecd_virtual) PASS.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

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 <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+
+#include "utils.h"
+
+#ifdef AUTOSCALER_TEST
+#define XCECD_JOB_OUTPUT_DIR "/shared/log"
+#else
+#define XCECD_JOB_OUTPUT_DIR "/tmp"
+#endif
+
+/* <XCECD_JOB_OUTPUT_DIR>/job_<fmt_jobid>_task<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;
+}

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;
     }
 

src/worker/src/uv_proc.c

@@ -8,26 +8,9 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
+#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;

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_<jobid>_task<N>.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();
 }