diff --git a/src/worker/include/xcecd_tmpfile.h b/src/worker/include/xcecd_tmpfile.h new file mode 100644 index 00000000..b6e6624c --- /dev/null +++ b/src/worker/include/xcecd_tmpfile.h @@ -0,0 +1,69 @@ +#pragma once +/* + * xcecd_tmpfile.h — root-owned files at fixed names in shared directories. + * + * xcecd runs as root and keeps a few files at fixed, well-known names in + * world-writable /tmp (the xcecd_worker log, the planned-restart flag). + * Opening, creating or chmod'ing such a name by path would follow a symlink + * (or write through a hardlink) a local user planted there first, letting + * them append to, create or chmod 0666 any file on the host (bug-1869). + * These helpers only ever act on a regular, single-link file owned by us. + */ +#include +#include +#include +#include +#include +#include + +/* Open `path` for writing (`extra_flags` e.g. O_APPEND; O_TRUNC is not + * meaningful here), creating it with `mode` if absent. An existing file is + * reused only if it is our own regular, single-link file; anything else at + * the name -- a symlink (dangling or not), a hardlink to some other file, a + * FIFO, another user's file -- is removed (root may unlink in a sticky dir) + * and replaced with a fresh file created O_EXCL|O_NOFOLLOW, which never + * follows a link and fails if something reappears in between. Opening is + * O_NONBLOCK so a planted FIFO cannot hang the caller. Change the mode with + * fchmod() on the returned fd, never chmod() on the path. Returns an fd or + * -errno. */ +static inline int xcecd_open_owned_file(const char* path, int extra_flags, + mode_t mode) +{ + const int base = O_WRONLY | O_NOFOLLOW | O_NONBLOCK | O_CLOEXEC + | extra_flags; + struct stat st; + int attempt; + int fd; + + for (attempt = 0; attempt < 3; attempt++) { + fd = open(path, base | O_CREAT, mode); + if (fd >= 0) { + if (fstat(fd, &st) == 0 && S_ISREG(st.st_mode) + && st.st_nlink == 1 && st.st_uid == geteuid()) + return fd; + close(fd); + } else if (errno != ELOOP && errno != ENXIO && errno != EISDIR) { + /* ELOOP: symlink; ENXIO: FIFO without a reader; EISDIR: dir */ + return -errno; + } + + if (unlink(path) < 0 && errno != ENOENT) + return -errno; + fd = open(path, base | O_CREAT | O_EXCL, mode); + if (fd >= 0) + return fd; + if (errno != EEXIST) + return -errno; + } + return -EEXIST; +} + +/* True iff `path` is our own regular file (lstat, so a symlink planted at + * the name never counts). */ +static inline bool xcecd_owned_file_exists(const char* path) +{ + struct stat st; + + return lstat(path, &st) == 0 && S_ISREG(st.st_mode) + && st.st_uid == geteuid(); +} diff --git a/src/worker/src/main.c b/src/worker/src/main.c index 3184e516..bd9919e6 100644 --- a/src/worker/src/main.c +++ b/src/worker/src/main.c @@ -1,5 +1,6 @@ #include "sqlite3.h" #include "xcecd.h" +#include "xcecd_tmpfile.h" #include "xce/cluster.h" #include #include @@ -591,15 +592,18 @@ int main(int argc, char* argv[]) } #ifndef VIRTUAL_CLUSTER - /* create worker log file */ - int worker_log_fd = open(worker_log, O_WRONLY | O_CREAT | O_APPEND, 0644); - if (worker_log_fd != -1) { + /* create worker log file -- bug-1869: never by path (a symlink planted + * at /xcecd_worker.log would get chmod 0666'd) */ + int worker_log_fd = xcecd_open_owned_file(worker_log, O_APPEND, 0644); + if (worker_log_fd >= 0) { XCECD_LOG(XCE_LOG_INFO, "Worker log file created: %s", worker_log); + /* close it and wait real worker to use */ + fchmod(worker_log_fd, 0666); + close(worker_log_fd); + } else { + XCECD_LOG(XCE_LOG_ERROR, "Failed to create worker log %s: %s", + worker_log, strerror(-worker_log_fd)); } - - /* close it and wait real worker to use */ - close(worker_log_fd); - chmod(worker_log, 0666); #endif if (xcecd_config_init(&config) != 0) { diff --git a/src/worker/src/process_worker_manager.c b/src/worker/src/process_worker_manager.c index a119da0c..04f50c1e 100644 --- a/src/worker/src/process_worker_manager.c +++ b/src/worker/src/process_worker_manager.c @@ -6,6 +6,7 @@ * by default. */ #include "process_worker_manager.h" +#include "xcecd_tmpfile.h" #include "xcelog.h" #include #include @@ -64,7 +65,10 @@ int process_manager_start_worker(process_manager_t* manager) proc->command = strdup("xcecd_worker"); proc->log_file = strdup("/tmp/xcecd_worker.log"); - proc->output_fd = open(proc->log_file, O_WRONLY | O_CREAT | O_APPEND, 0644); + /* bug-1869: fixed name in /tmp, opened as root */ + proc->output_fd = xcecd_open_owned_file(proc->log_file, O_APPEND, 0644); + if (proc->output_fd < 0) + proc->output_fd = -1; if (proc->output_fd == -1) { XCECD_LOG(XCE_LOG_ERROR, "Failed to open log file: %s", strerror(errno)); diff --git a/src/worker/src/xcecd.c b/src/worker/src/xcecd.c index de70d45c..6ae86b25 100644 --- a/src/worker/src/xcecd.c +++ b/src/worker/src/xcecd.c @@ -1,6 +1,7 @@ #include "xcecd.h" #include "xcecd_cgroups.h" #include "xcecd_utils.h" +#include "xcecd_tmpfile.h" #include #include #include @@ -612,8 +613,9 @@ void xcecd_cleanup(void) void xcecd_prepare_restart() { - int fd = open(RESTART_FLAG_FILE, O_CREAT | O_WRONLY, 0644); - if (fd != -1) { + /* bug-1869: fixed name in /tmp, created as root */ + int fd = xcecd_open_owned_file(RESTART_FLAG_FILE, 0, 0644); + if (fd >= 0) { close(fd); } else { XCECD_LOG(XCE_LOG_ERROR, "Failed to create restart flag file."); @@ -622,11 +624,22 @@ void xcecd_prepare_restart() bool xcecd_check_restart_type(void) { - if (access(RESTART_FLAG_FILE, F_OK) == 0) { + struct stat st; + + /* bug-1869: only our own regular file is the flag -- anyone can create + * /tmp/.xcecd.restarting and would otherwise make an unplanned restart + * look planned. Something else at the name is removed and ignored. */ + if (xcecd_owned_file_exists(RESTART_FLAG_FILE)) { XCECD_LOG(XCE_LOG_DEBUG, "XCECD daemon is performing planned restart."); unlink(RESTART_FLAG_FILE); return true; } else { + if (lstat(RESTART_FLAG_FILE, &st) == 0) { + XCECD_LOG(XCE_LOG_WARN, + "ignoring and removing %s: not a root-owned regular " + "file", RESTART_FLAG_FILE); + unlink(RESTART_FLAG_FILE); + } if (access(xcecd_ctx.db_path, F_OK) == 0) { XCECD_LOG(XCE_LOG_DEBUG, "XCECD daemon is performing unplanned restart."); diff --git a/src/worker/testsuite/meson.build b/src/worker/testsuite/meson.build index edb7f9a5..bc4bcc1a 100644 --- a/src/worker/testsuite/meson.build +++ b/src/worker/testsuite/meson.build @@ -193,3 +193,16 @@ test('worker job_output_path test', job_output_path_test, suite: 'worker', timeout: 60, env: ['ASAN_OPTIONS=detect_leaks=0']) + +# bug-1869: xcecd's own fixed-name /tmp files (xcecd_worker.log, the +# restart flag) must not follow links planted at those names. +xcecd_tmpfile_test = executable('xcecd_tmpfile_test', + 'test_xcecd_tmpfile.c', + include_directories: [worker_test_incdir], + dependencies: worker_test_deps, + install: false) + +test('worker xcecd_tmpfile test', xcecd_tmpfile_test, + suite: 'worker', + timeout: 60, + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/worker/testsuite/test_xcecd_tmpfile.c b/src/worker/testsuite/test_xcecd_tmpfile.c new file mode 100644 index 00000000..46f573c2 --- /dev/null +++ b/src/worker/testsuite/test_xcecd_tmpfile.c @@ -0,0 +1,194 @@ +/* + * test_xcecd_tmpfile.c — bug-1869: xcecd (root) keeps files at fixed names + * in world-writable /tmp. Opening them must never follow a symlink or write + * through a hardlink planted at the name, must not block on a FIFO, and must + * keep appending to its own legitimate file; the restart flag must only + * count when it is our own regular file. + */ +#include +#include +#include +#include +#include +#include +#include + +#include + +#include "xcecd_tmpfile.h" + +static char g_dir[64]; +static char g_path[128]; +static char g_victim[128]; + +void setUp(void) +{ + int fd; + + strcpy(g_dir, "/tmp/xce_tmpfile_XXXXXX"); + TEST_ASSERT_NOT_NULL(mkdtemp(g_dir)); + snprintf(g_path, sizeof(g_path), "%s/fixed.log", g_dir); + snprintf(g_victim, sizeof(g_victim), "%s/victim", g_dir); + + fd = open(g_victim, O_WRONLY | O_CREAT | O_EXCL, 0600); + 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); + rmdir(g_dir); +} + +static void assert_victim_untouched(void) +{ + char buf[32] = { 0 }; + struct stat st; + 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, + "a planted link's target must not be written"); + TEST_ASSERT_EQUAL_INT(0, stat(g_victim, &st)); + TEST_ASSERT_EQUAL_INT_MESSAGE(0600, (int)(st.st_mode & 0777), + "a planted link's target must not be chmod'ed"); +} + +/* the name must now hold our own fresh regular file */ +static void assert_own_regular_file(int fd) +{ + struct stat st; + + TEST_ASSERT_EQUAL_INT(0, lstat(g_path, &st)); + TEST_ASSERT_TRUE(S_ISREG(st.st_mode)); + TEST_ASSERT_EQUAL_INT(1, (int)st.st_nlink); + TEST_ASSERT_EQUAL_INT((int)geteuid(), (int)st.st_uid); + TEST_ASSERT_EQUAL_INT(2, (int)write(fd, "ok", 2)); +} + +static void test_open_creates_file(void) +{ + int fd = xcecd_open_owned_file(g_path, O_APPEND, 0644); + + TEST_ASSERT_TRUE(fd >= 0); + assert_own_regular_file(fd); + close(fd); +} + +static void test_open_appends_to_own_existing_file(void) +{ + char buf[32] = { 0 }; + int fd = open(g_path, O_WRONLY | O_CREAT, 0644); + + TEST_ASSERT_TRUE(fd >= 0); + TEST_ASSERT_EQUAL_INT(4, (int)write(fd, "old\n", 4)); + close(fd); + + fd = xcecd_open_owned_file(g_path, O_APPEND, 0644); + TEST_ASSERT_TRUE(fd >= 0); + TEST_ASSERT_EQUAL_INT(4, (int)write(fd, "new\n", 4)); + close(fd); + + fd = open(g_path, O_RDONLY); + TEST_ASSERT_EQUAL_INT(8, (int)read(fd, buf, sizeof(buf) - 1)); + close(fd); + TEST_ASSERT_EQUAL_STRING_MESSAGE("old\nnew\n", buf, + "a legitimate existing log must be appended to, not replaced"); +} + +static void test_open_replaces_planted_symlink(void) +{ + int fd; + + TEST_ASSERT_EQUAL_INT(0, symlink(g_victim, g_path)); + fd = xcecd_open_owned_file(g_path, O_APPEND, 0644); + TEST_ASSERT_TRUE(fd >= 0); + TEST_ASSERT_EQUAL_INT(0, fchmod(fd, 0666)); + assert_own_regular_file(fd); + close(fd); + assert_victim_untouched(); +} + +static void test_open_replaces_planted_dangling_symlink(void) +{ + char target[160]; + struct stat st; + int fd; + + snprintf(target, sizeof(target), "%s/created-by-root", g_dir); + TEST_ASSERT_EQUAL_INT(0, symlink(target, g_path)); + fd = xcecd_open_owned_file(g_path, 0, 0644); + TEST_ASSERT_TRUE(fd >= 0); + assert_own_regular_file(fd); + close(fd); + TEST_ASSERT_TRUE_MESSAGE(lstat(target, &st) < 0, + "a dangling symlink's target must not be created"); +} + +static void test_open_replaces_planted_hardlink(void) +{ + int fd; + + TEST_ASSERT_EQUAL_INT(0, link(g_victim, g_path)); + fd = xcecd_open_owned_file(g_path, O_APPEND, 0644); + TEST_ASSERT_TRUE(fd >= 0); + TEST_ASSERT_EQUAL_INT(0, fchmod(fd, 0666)); + assert_own_regular_file(fd); + close(fd); + assert_victim_untouched(); +} + +static void test_open_does_not_block_on_planted_fifo(void) +{ + int fd; + + TEST_ASSERT_EQUAL_INT(0, mkfifo(g_path, 0666)); + alarm(5); /* a blocking open() would hang the suite; fail loudly */ + fd = xcecd_open_owned_file(g_path, O_APPEND, 0644); + alarm(0); + TEST_ASSERT_TRUE(fd >= 0); + assert_own_regular_file(fd); + close(fd); +} + +static void test_flag_exists_only_for_own_regular_file(void) +{ + int fd; + + TEST_ASSERT_FALSE(xcecd_owned_file_exists(g_path)); + + TEST_ASSERT_EQUAL_INT(0, symlink(g_victim, g_path)); + TEST_ASSERT_FALSE_MESSAGE(xcecd_owned_file_exists(g_path), + "a symlink at the flag name is not the flag"); + unlink(g_path); + + TEST_ASSERT_EQUAL_INT(0, mkfifo(g_path, 0600)); + TEST_ASSERT_FALSE_MESSAGE(xcecd_owned_file_exists(g_path), + "a FIFO at the flag name is not the flag"); + unlink(g_path); + + fd = open(g_path, O_WRONLY | O_CREAT, 0644); + TEST_ASSERT_TRUE(fd >= 0); + close(fd); + TEST_ASSERT_TRUE(xcecd_owned_file_exists(g_path)); +} + +int main(void) +{ + /* unbuffered, so a hang in the FIFO test still shows earlier results */ + setvbuf(stdout, NULL, _IONBF, 0); + UNITY_BEGIN(); + RUN_TEST(test_open_creates_file); + RUN_TEST(test_open_appends_to_own_existing_file); + RUN_TEST(test_open_replaces_planted_symlink); + RUN_TEST(test_open_replaces_planted_dangling_symlink); + RUN_TEST(test_open_replaces_planted_hardlink); + RUN_TEST(test_open_does_not_block_on_planted_fifo); + RUN_TEST(test_flag_exists_only_for_own_regular_file); + return UNITY_END(); +}