From 3260a39ab454dabebe0ecee4318f44ff5f40c9da Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 13 Sep 2026 10:06:04 +0200 Subject: [PATCH] refactor(shared): single owner for authorized_root state --- src/server/server.c | 34 +++++++-------------------------- src/shared/file.c | 46 +++++++++++++++++---------------------------- src/shared/file.h | 3 --- src/shared/utils.c | 24 ++++++++++++++++------- src/shared/utils.h | 7 +++++++ tests/test_file.c | 16 ++++++++-------- 6 files changed, 56 insertions(+), 74 deletions(-) diff --git a/src/server/server.c b/src/server/server.c index 494a840..1eb1e8d 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -29,8 +29,6 @@ #include #include -static char* authorized_root; -static int authorized_root_fd = -1; static bool allow_delete; static bool trust_sender; static bool allow_unauthenticated; @@ -187,13 +185,10 @@ static bool tls_client_identity_allowed(SSL* ssl) { } static void release_authorization(void) { - file_set_authorized_root(-1, NULL); - utils_set_authorized_root_fd(-1); - if (authorized_root_fd >= 0) - close(authorized_root_fd); - authorized_root_fd = -1; - free(authorized_root); - authorized_root = NULL; + int root_fd = utils_get_authorized_root_fd(); + utils_set_authorized_root(-1, NULL); + if (root_fd >= 0) + close(root_fd); } static bool path_is_within(const char* root, const char* path) { @@ -219,13 +214,11 @@ static bool ensure_receive_root(const Config* config) { static bool configure_authorization(const char* root) { char resolved[PATH_MAX]; if (!root) { - file_set_authorized_root(-1, NULL); utils_set_authorized_root(-1, NULL); return false; } int root_fd = open(root, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); if (root_fd < 0) { - file_set_authorized_root(-1, NULL); utils_set_authorized_root(-1, NULL); return false; } @@ -234,26 +227,12 @@ static bool configure_authorization(const char* root) { if (fd_path_length < 0 || (size_t)fd_path_length >= sizeof(fd_path) || !realpath(fd_path, resolved)) { close(root_fd); - file_set_authorized_root(-1, NULL); utils_set_authorized_root(-1, NULL); return false; } - authorized_root = str_dup(resolved); - if (!authorized_root) { + if (!utils_set_authorized_root(root_fd, resolved)) { + utils_set_authorized_root(-1, NULL); close(root_fd); - file_set_authorized_root(-1, NULL); - utils_set_authorized_root(-1, NULL); - return false; - } - authorized_root_fd = root_fd; - if (!file_set_authorized_root(authorized_root_fd, authorized_root) || - !utils_set_authorized_root(authorized_root_fd, authorized_root)) { - file_set_authorized_root(-1, NULL); - utils_set_authorized_root(-1, NULL); - close(authorized_root_fd); - authorized_root_fd = -1; - free(authorized_root); - authorized_root = NULL; return false; } return true; @@ -615,6 +594,7 @@ void handler(int file_descriptor) { * in effect. A client's --timeout tightens only that client's own protocol * I/O and the server's socket read/write timeout is the transport default. */ protocol_session_set_io_timeout(&session, config->timeout); + const char* authorized_root = utils_get_authorized_root_path(); if (!authorized_root) { log_message(LOG_LEVEL_ERROR, "No server-side destination root configured"); goto done; diff --git a/src/shared/file.c b/src/shared/file.c index b85e988..9d5aa23 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -284,23 +284,6 @@ size_t file_content_to_buffer(File* file) { /* ---- Secure filesystem primitives ---- */ -static int authorized_root_fd = -1; -static char* authorized_root_path; - -bool file_set_authorized_root(int fd, const char* canonical_path) { - char* path_copy = canonical_path ? str_dup(canonical_path) : NULL; - if (canonical_path && !path_copy) { - authorized_root_fd = -1; - free(authorized_root_path); - authorized_root_path = NULL; - return false; - } - authorized_root_fd = fd; - free(authorized_root_path); - authorized_root_path = path_copy; - return true; -} - bool file_path_exists_secure(const char* path) { if (!path) return false; @@ -483,7 +466,10 @@ static int open_dir_beneath_root(const char* resolved, const char* root) { rel++; if (*rel == '\0') return -1; - int fd = dup(authorized_root_fd); + int root_fd = utils_get_authorized_root_fd(); + if (root_fd < 0) + return -1; + int fd = dup(root_fd); if (fd < 0) return -1; char* copy = str_dup(rel); @@ -525,20 +511,21 @@ int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs) return -1; } int fd; - if (authorized_root_fd >= 0) { - if (!authorized_root_path || path[0] != '/' || - !path_is_within_root(authorized_root_path, path)) { + int root_fd = utils_get_authorized_root_fd(); + const char* root_path = utils_get_authorized_root_path(); + if (root_fd >= 0) { + if (!root_path || path[0] != '/' || !path_is_within_root(root_path, path)) { free(copy); free(leaf); return -1; } - fd = dup(authorized_root_fd); + fd = dup(root_fd); if (fd < 0) { free(copy); free(leaf); return -1; } - size_t root_len = strlen(authorized_root_path); + size_t root_len = strlen(root_path); char* relative = str_dup(path + root_len); if (!relative) { free(copy); @@ -602,15 +589,14 @@ int file_open_secure_parent(const char* path, char** leaf_out, bool create_dirs) O_NOFOLLOW walk. Only honoured when the symlink resolves to a directory that stays beneath the authorized root, so a malicious link can never redirect the write outside it. */ - if (next < 0 && file_keep_dirlinks && authorized_root_path != NULL && + if (next < 0 && file_keep_dirlinks && root_path != NULL && (errno == ELOOP || errno == ENOTDIR || errno == EACCES)) { struct stat lst; if (fstatat(fd, component, &lst, AT_SYMLINK_NOFOLLOW) == 0 && S_ISLNK(lst.st_mode)) { char candidate[PATH_MAX]; char root[PATH_MAX]; - if (realpath(authorized_root_path, root) && - snprintf(candidate, sizeof(candidate), "%s%s/%s", root, rel_buf, component) < - (int)sizeof(candidate)) { + if (realpath(root_path, root) && snprintf(candidate, sizeof(candidate), "%s%s/%s", root, + rel_buf, component) < (int)sizeof(candidate)) { char resolved[PATH_MAX]; if (realpath(candidate, resolved) && strcmp(resolved, root) != 0 && strncmp(root, resolved, strlen(root)) == 0 && @@ -685,8 +671,9 @@ bool file_ensure_directory_secure(const char* path) { return false; /* The authorized root is already an open directory, and the filesystem root is always present: there is no final component left to create for them. */ + const char* root_path = utils_get_authorized_root_path(); bool root_is_open = - authorized_root_fd >= 0 && authorized_root_path && strcmp(norm, authorized_root_path) == 0; + utils_get_authorized_root_fd() >= 0 && root_path && strcmp(norm, root_path) == 0; if (root_is_open || strcmp(norm, "/") == 0) { free(norm); return true; @@ -733,8 +720,9 @@ bool file_directory_exists_secure(const char* path) { char* norm = normalize_directory_path(path); if (!norm) return false; + const char* root_path = utils_get_authorized_root_path(); bool root_is_open = - authorized_root_fd >= 0 && authorized_root_path && strcmp(norm, authorized_root_path) == 0; + utils_get_authorized_root_fd() >= 0 && root_path && strcmp(norm, root_path) == 0; if (root_is_open || strcmp(norm, "/") == 0) { free(norm); return true; diff --git a/src/shared/file.h b/src/shared/file.h index 570d703..3ccc2aa 100644 --- a/src/shared/file.h +++ b/src/shared/file.h @@ -62,9 +62,6 @@ void file_set_keep_dirlinks(bool enable); void file_set_trust_sender(bool enable); bool file_get_trust_sender(void); -/* A configured fd without a canonical identity deliberately rejects paths. */ -bool file_set_authorized_root(int fd, const char* canonical_path); - /* Secure path/filesystem primitives (symlink-safe, O_NOFOLLOW, root-confined). */ bool file_path_exists_secure(const char* path); bool file_stat_secure(const char* path, struct stat* st); diff --git a/src/shared/utils.c b/src/shared/utils.c index d790172..6ab725a 100644 --- a/src/shared/utils.c +++ b/src/shared/utils.c @@ -36,6 +36,14 @@ void utils_set_authorized_root_fd(int fd) { (void)utils_set_authorized_root(fd, NULL); } +int utils_get_authorized_root_fd(void) { + return authorized_root_fd; +} + +const char* utils_get_authorized_root_path(void) { + return authorized_root_path; +} + bool path_is_within_root(const char* root, const char* path) { size_t root_len = strlen(root); return strncmp(root, path, root_len) == 0 && (path[root_len] == '\0' || path[root_len] == '/'); @@ -50,15 +58,16 @@ bool path_is_within_root(const char* root, const char* path) { * in the extra receiver policies they apply, so they are intentionally kept * separate. Both rely on the shared lexical path_is_within_root check. */ static int open_authorized_destination(const char* dest_root) { - if (authorized_root_fd < 0 || !authorized_root_path || !dest_root || - !path_is_within_root(authorized_root_path, dest_root)) + int root_fd = utils_get_authorized_root_fd(); + const char* root_path = utils_get_authorized_root_path(); + if (root_fd < 0 || !root_path || !dest_root || !path_is_within_root(root_path, dest_root)) return -1; - int dirfd = dup(authorized_root_fd); + int dirfd = dup(root_fd); if (dirfd < 0) return -1; - const char* relative_path = dest_root + strlen(authorized_root_path); + const char* relative_path = dest_root + strlen(root_path); while (*relative_path == '/') relative_path++; char* relative = str_dup(*relative_path ? relative_path : "."); @@ -689,11 +698,12 @@ DeleteWalkResult delete_extras_limited(const char* dest_root, const ArrayList* m if (!build_keep_index(manifest, &keep)) return DELETE_WALK_ERROR; int rootfd; - if (authorized_root_fd >= 0) { - if (authorized_root_path) + int root_fd = utils_get_authorized_root_fd(); + if (root_fd >= 0) { + if (utils_get_authorized_root_path()) rootfd = open_authorized_destination(dest_root); else if (dest_root == NULL) - rootfd = dup(authorized_root_fd); + rootfd = dup(root_fd); else rootfd = -1; } else { diff --git a/src/shared/utils.h b/src/shared/utils.h index f4a5bd6..7ac9059 100644 --- a/src/shared/utils.h +++ b/src/shared/utils.h @@ -127,6 +127,13 @@ bool utils_set_authorized_root(int fd, const char* canonical_path); /* The fd-only compatibility form is fail-closed for path-based operations; * callers should use utils_set_authorized_root with the canonical identity. */ void utils_set_authorized_root_fd(int fd); +/* Read accessors for the process-wide authorized root, so every secure-walk + * site consumes the single shared state instead of keeping its own copy. The + * fd is caller-owned (see the setters): it is returned verbatim, never dup'd, + * and the caller that opened it is responsible for closing it. With no root + * configured the fd accessor returns -1 and the path accessor returns NULL. */ +int utils_get_authorized_root_fd(void); +const char* utils_get_authorized_root_path(void); /* True when `path` is `root` itself or lies directly beneath it: a lexical * prefix test requiring the byte after `root` to be '\0' or '/'. Both `root` * and `path` must be absolute canonical paths free of "."/".." components (the diff --git a/tests/test_file.c b/tests/test_file.c index 4561f1a..73c3431 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -1180,7 +1180,7 @@ static void test_trust_sender_authorized_root_confinement() { rmdir(sibling); return; } - EXPECT_TRUE(file_set_authorized_root(root_fd, root_abs)); + EXPECT_TRUE(utils_set_authorized_root(root_fd, root_abs)); file_set_trust_sender(true); struct stat st; @@ -1204,7 +1204,7 @@ static void test_trust_sender_authorized_root_confinement() { free(outside_link); free(inside_link); - file_set_authorized_root(-1, NULL); + utils_set_authorized_root(-1, NULL); close(root_fd); unlink("test_trust_sender_outside_link"); rmdir(sibling); @@ -1220,7 +1220,7 @@ void test_trust_sender() { test_trust_sender_confines_hostile_paths(); test_trust_sender_authorized_root_confinement(); file_set_trust_sender(false); - file_set_authorized_root(-1, NULL); + utils_set_authorized_root(-1, NULL); } /* --sparse/-S hole preservation: a buffer with a long zero run written via @@ -1341,7 +1341,7 @@ static void test_file_write_to_disk_partial_retention() { static void test_dir_time_list() { const char* root = "test_dir_time_root"; const char* sub = "test_dir_time_root/sub"; - file_set_authorized_root(-1, NULL); + utils_set_authorized_root(-1, NULL); rmdir(sub); rmdir(root); EXPECT_EQ_INT(mkdir(root, 0755), 0); @@ -1485,7 +1485,7 @@ static void test_keep_dirlinks_secure_open_impl() { rmdir(outside); return; } - EXPECT_TRUE(file_set_authorized_root(root_fd, root_abs)); + EXPECT_TRUE(utils_set_authorized_root(root_fd, root_abs)); file_set_keep_dirlinks(true); struct stat real_st; @@ -1538,7 +1538,7 @@ static void test_keep_dirlinks_secure_open_impl() { free(leaf); file_set_keep_dirlinks(false); - file_set_authorized_root(-1, NULL); + utils_set_authorized_root(-1, NULL); close(root_fd); unlink(link); unlink(abslink); @@ -1552,10 +1552,10 @@ static void test_keep_dirlinks_secure_open_impl() { * cleared even when an EXPECT inside the body returns early (a failing EXPECT * returns from its own function, so the body's trailing resets may be skipped). */ static void test_keep_dirlinks_secure_open() { - file_set_authorized_root(-1, NULL); + utils_set_authorized_root(-1, NULL); file_set_keep_dirlinks(false); test_keep_dirlinks_secure_open_impl(); - file_set_authorized_root(-1, NULL); + utils_set_authorized_root(-1, NULL); file_set_keep_dirlinks(false); }