From 59ce174d22a9d29a804e6023fd3e9465a120b0e4 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 13 Sep 2026 00:57:09 +0200 Subject: [PATCH 1/5] fix(client-send): UAF in basis preflight and missing_args leak --- src/client/client_send.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/client/client_send.c b/src/client/client_send.c index f4db82e..16ed906 100644 --- a/src/client/client_send.c +++ b/src/client/client_send.c @@ -338,9 +338,10 @@ static bool basis_oversize_preflight(const Config* config) { return false; DirectoryScanner* scanner = directory_scanner_create_with_options(config->send_directory, &prepared.options); - prepared_scanner_destroy(&prepared); - if (!scanner) + if (!scanner) { + prepared_scanner_destroy(&prepared); return false; + } bool ok = true; Chunk* chunk; while ((chunk = directory_scanner_next(scanner)) != NULL) { @@ -364,7 +365,10 @@ static bool basis_oversize_preflight(const Config* config) { } if (directory_scanner_failed(scanner) || directory_scanner_had_io_error(scanner)) ok = false; + /* The scanner borrows prepared.options' base_filters/hardlinks pointers, so + prepared must outlive the scanner. */ directory_scanner_destroy(scanner); + prepared_scanner_destroy(&prepared); return ok; } @@ -1886,6 +1890,8 @@ int send_files(Config* config) { if (config->transport == TRANSPORT_TCP) log_message(LOG_LEVEL_ERROR, "could not connect to server%s", config->use_tls ? " via TLS" : ""); + if (missing_args) + array_list_delete(missing_args); return 1; } ProtocolSession session; From 4557924972a662d8e2440302ad3bbf1f0c131737 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 13 Sep 2026 00:57:32 +0200 Subject: [PATCH 2/5] fix(receiver): cap DirTimeList growth and fix placeholder Data leaks --- src/shared/file_receive.c | 16 +++++++-- src/shared/file_receive.h | 13 ++++++- tests/test_file.c | 72 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 3 deletions(-) diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index ffb72df..f21867c 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -1696,9 +1696,9 @@ File* receive_incremental_check(int fd, const Config* config, bool* skipped) { return NULL; } - if (has_path_traversal(check_path)) { + if (check_path[0] == '\0' || has_path_traversal(check_path)) { char* escaped_path = output_escape(check_path, log_get_8_bit_output()); - log_message(LOG_LEVEL_ERROR, "Path traversal detected: %s", + log_message(LOG_LEVEL_ERROR, "Invalid received check path: %s", escaped_path ? escaped_path : ""); free(escaped_path); free(check_path); @@ -1832,6 +1832,7 @@ File* receive_incremental_check(int fd, const Config* config, bool* skipped) { existing/ignore-existing/update/backup/delay-updates policy. */ File* materialized = file_create(check_path); if (materialized && basis.content) { + data_destroy(materialized->data); materialized->data = basis.content; basis.content = NULL; materialized->metadata = file_metadata_create(NULL, &basis.st, false, false); @@ -2095,6 +2096,7 @@ File* receive_incremental_check(int fd, const Config* config, bool* skipped) { file->metadata = meta; file->xattrs = append_xattrs; append_xattrs = NULL; + data_destroy(file->data); file->data = data_create(full, full_size); if (!file->data) { /* data_create already freed full on failure */ file_destroy(file); @@ -2247,6 +2249,7 @@ void dir_time_list_init(DirTimeList* list) { list->entries = NULL; list->count = 0; list->capacity = 0; + list->bytes = 0; } void dir_time_list_free(DirTimeList* list) { @@ -2260,11 +2263,19 @@ void dir_time_list_free(DirTimeList* list) { list->entries = NULL; list->count = 0; list->capacity = 0; + list->bytes = 0; } bool dir_time_list_add(DirTimeList* list, const char* wire_path, const FileMetadata* metadata) { if (!list || !wire_path || !metadata) return true; /* nothing to remember; never a hard error */ + /* Cumulative, not per-frame: the sender may stream a tree across unbounded + STATUS_DIR_TIMES frames, so bound the TOTAL retained here. Reject before + touching the list, leaving it exactly as it was (the caller fails the + transfer, which becomes a clean protocol error). */ + size_t path_len = strlen(wire_path); + if (list->count >= MAX_DIR_TIME_ENTRIES || path_len > MAX_DIR_TIME_BYTES - list->bytes) + return false; if (list->count == list->capacity) { size_t new_capacity = list->capacity == 0 ? 16 : list->capacity * 2; if (new_capacity < list->capacity) @@ -2291,6 +2302,7 @@ bool dir_time_list_add(DirTimeList* list, const char* wire_path, const FileMetad list->paths[list->count] = copy; list->entries[list->count] = *metadata; list->count++; + list->bytes += path_len; return true; } diff --git a/src/shared/file_receive.h b/src/shared/file_receive.h index 37dc15a..ec6ccfa 100644 --- a/src/shared/file_receive.h +++ b/src/shared/file_receive.h @@ -7,6 +7,15 @@ /* Server-side file receive/save path. */ +/* Cumulative caps for the deferred directory-time accumulator. The sender may + * legitimately split a large tree across repeated STATUS_DIR_TIMES frames, so a + * per-frame bound is not enough: the receiver must bound the TOTAL it retains + * against a hostile sender. Mirror the delete-manifest limits + * (MAX_MANIFEST_ENTRIES / MAX_MANIFEST_BYTES): the entry count bounds the + * metadata array and the byte budget bounds the concatenated path strings. */ +#define MAX_DIR_TIME_ENTRIES (1024 * 1024) +#define MAX_DIR_TIME_BYTES (16ULL * 1024 * 1024) + File* file_receive(const Config* config, int file_descriptor); File* file_receive_directory(int file_descriptor, const Config* config); File* file_receive_dir_time(int file_descriptor, const Config* config); @@ -28,6 +37,7 @@ typedef struct { FileMetadata* entries; /* owned, parallel to paths */ size_t count; size_t capacity; + size_t bytes; /* cumulative strlen of every retained path */ } DirTimeList; /* Capture gate shared by the sender-side and receiver-side sinks: directory @@ -39,7 +49,8 @@ bool dir_times_should_capture(const Config* config); void dir_time_list_init(DirTimeList* list); void dir_time_list_free(DirTimeList* list); /* Deep-copy one directory's path + metadata into the list. Returns false on - * allocation failure (the caller fails the transfer). */ + * allocation failure OR when the cumulative entry/byte caps would be exceeded + * (the caller fails the transfer). */ bool dir_time_list_add(DirTimeList* list, const char* wire_path, const FileMetadata* metadata); /* Apply every accumulated directory's mtime (and atime when captured) beneath * `root_directory`, confined fd-relative. Best-effort per entry: an absent diff --git a/tests/test_file.c b/tests/test_file.c index fd715fc..0f0225d 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -1370,6 +1370,76 @@ static void test_dir_time_list() { rmdir(root); } +/* A hostile sender can stream unbounded STATUS_DIR_TIMES frames; the + * accumulator must bound the CUMULATIVE path bytes (not just one frame) and + * reject the add that would cross the cap, leaving the list untouched. */ +static void test_dir_time_list_cap() { + DirTimeList list; + dir_time_list_init(&list); + EXPECT_EQ_INT((int)list.bytes, 0); + FileMetadata metadata = {.mtime_sec = 1, .mtime_nsec = 0}; + + size_t path_len = MAX_STRING_SIZE - 1; + char* path = malloc(path_len + 1); + EXPECT_NOT_NULL(path); + memset(path, 'a', path_len); + path[path_len] = '\0'; + + bool rejected = false; + for (size_t i = 0; i < MAX_DIR_TIME_ENTRIES + 1 && !rejected; i++) { + size_t before_count = list.count; + size_t before_bytes = list.bytes; + if (!dir_time_list_add(&list, path, &metadata)) { + rejected = true; + /* The rejected add must not have partially mutated the list. */ + EXPECT_TRUE(list.count == before_count); + EXPECT_TRUE(list.bytes == before_bytes); + } else { + EXPECT_TRUE(list.count == before_count + 1); + EXPECT_TRUE(list.bytes == before_bytes + path_len); + } + } + EXPECT_TRUE(rejected); + EXPECT_TRUE(list.count <= MAX_DIR_TIME_ENTRIES); + EXPECT_TRUE(list.bytes <= MAX_DIR_TIME_BYTES); + + /* The retained entries are still intact and freeable after the rejection. */ + EXPECT_TRUE(list.count > 0); + EXPECT_TRUE(strcmp(list.paths[0], path) == 0); + dir_time_list_free(&list); + EXPECT_EQ_INT((int)list.bytes, 0); + free(path); +} + +/* receive_incremental_check must reject an empty check_path; every other + * receive path rejects path[0]=='\0'. Feed the check header (empty wire path + * + size/mtime/nsec) and assert the check is refused without being skipped. */ +static void test_receive_incremental_check_empty_path() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + cfg->checksum = false; + + int p[2]; + EXPECT_EQ_INT(pipe(p), 0); + size_t wire_len = 0; + unsigned long long check_size = 0; + long long check_mtime = 0; + long long check_mtime_nsec = 0; + EXPECT_TRUE(send_n_data(p[1], &wire_len, sizeof(wire_len))); + EXPECT_TRUE(send_n_data(p[1], &check_size, sizeof(check_size))); + EXPECT_TRUE(send_n_data(p[1], &check_mtime, sizeof(check_mtime))); + EXPECT_TRUE(send_n_data(p[1], &check_mtime_nsec, sizeof(check_mtime_nsec))); + + bool skipped = true; + File* file = receive_incremental_check(p[0], cfg, &skipped); + EXPECT_NULL(file); + EXPECT_FALSE(skipped); + + close(p[0]); + close(p[1]); + config_delete(cfg); +} + /* -K/--keep-dirlinks secure open: with an authorized root, a destination path * component that is a symlink to an IN-ROOT directory is used as that directory * (its referent is opened through a relative O_NOFOLLOW walk from the root fd, @@ -1531,6 +1601,8 @@ void test_file() { } test_file_metadata_create(); test_dir_time_list(); + test_dir_time_list_cap(); + test_receive_incremental_check_empty_path(); test_keep_dirlinks_secure_open(); test_inplace_overwrite_clears_special_mode_bits(); test_inplace_overwrite_metadata_strips_special_bits(); From f8252cf3e7c726b6345e10d3652f3d59404ce302 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 13 Sep 2026 00:57:32 +0200 Subject: [PATCH 3/5] fix(protocol): bound pre-auth config string memory --- src/shared/config.c | 106 +++++++++++++++++++++++++++++++------------- src/shared/config.h | 18 ++++++++ tests/test_config.c | 85 +++++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 30 deletions(-) diff --git a/src/shared/config.c b/src/shared/config.c index 4bd72c9..584c61a 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -202,6 +202,51 @@ static bool receive_wire_bool(int fd, bool* value) { return true; } +/* Cumulative budget for the strings retained by one received Config (see + * MAX_CONFIG_STRING_BYTES). Config strings are received once per connection + * before authentication and live for its whole lifetime, so the charge is never + * released. */ +typedef struct { + unsigned long long used; +} ConfigStringBudget; + +/* Charge `bytes` (the retained allocation: string body plus NUL) against the + * aggregate config-string budget. Returns false when the ceiling would be + * exceeded, letting the caller reject the frame with a clear error instead of + * retaining unbounded pre-auth memory. */ +static bool config_string_budget_charge(ConfigStringBudget* budget, size_t bytes) { + if ((unsigned long long)bytes > MAX_CONFIG_STRING_BYTES || + budget->used > MAX_CONFIG_STRING_BYTES - (unsigned long long)bytes) { + log_message(LOG_LEVEL_ERROR, "Config string budget exceeded (%llu + %zu > %llu bytes)", + budget->used, bytes, (unsigned long long)MAX_CONFIG_STRING_BYTES); + return false; + } + budget->used += (unsigned long long)bytes; + return true; +} + +static char* config_receive_str(int fd, ConfigStringBudget* budget) { + char* value = receive_str(fd); + if (!value) + return NULL; + if (!config_string_budget_charge(budget, strlen(value) + 1)) { + free(value); + return NULL; + } + return value; +} + +static char* config_receive_str_redacted(int fd, ConfigStringBudget* budget) { + char* value = receive_str_redacted(fd); + if (!value) + return NULL; + if (!config_string_budget_charge(budget, strlen(value) + 1)) { + free(value); + return NULL; + } + return value; +} + static bool validate_received_config(const Config* config) { return valid_wire_bool(config->save_to_disk) && valid_wire_bool(config->use_multithreading) && valid_wire_bool(config->use_chunk_serialization) && @@ -257,7 +302,7 @@ static bool validate_received_config(const Config* config) { config->delta_block_size <= DELTA_BLOCK_SIZE_MAX && config->delta_max_file_size <= DELTA_MAX_FILE_SIZE && config->modify_window >= 0 && config->max_delete >= -1 && config->skip_compress_count >= 0 && - config->skip_compress_count <= 10000 && config->max_alloc > 0 && + config->skip_compress_count <= MAX_SKIP_COMPRESS_SUFFIXES && config->max_alloc > 0 && (!config->chmod_spec || !*config->chmod_spec || chmod_apply(0, config->chmod_spec, &(mode_t){0})) && /* The received --iconv CONVERT_SPEC is untrusted input that drives @@ -819,7 +864,7 @@ static bool send_checksum_options(int fd, const Config* c) { send_n_data(fd, &c->checksum_seed, sizeof(c->checksum_seed)); } -static bool receive_core_fields(int fd, Config* c) { +static bool receive_core_fields(int fd, Config* c, ConfigStringBudget* budget) { int value; if (!receive_wire_bool(fd, &c->eight_bit_output)) return false; @@ -829,8 +874,8 @@ static bool receive_core_fields(int fd, Config* c) { if (c->max_alloc > MAX_SERVER_ALLOC) c->max_alloc = MAX_SERVER_ALLOC; protocol_session_set_max_alloc(NULL, c->max_alloc); - c->send_directory = receive_str(fd); - c->receive_root_directory = receive_str(fd); + c->send_directory = config_receive_str(fd, budget); + c->receive_root_directory = config_receive_str(fd, budget); if (!c->send_directory || !c->receive_root_directory) return false; if (!receive_wire_bool(fd, &c->save_to_disk) || !receive_wire_bool(fd, &c->use_multithreading) || @@ -863,10 +908,10 @@ static bool receive_delta_fields(int fd, Config* c) { receive_n_data(fd, &c->delta_max_file_size, sizeof(unsigned long long)); } -static bool receive_file_options(int fd, Config* c) { +static bool receive_file_options(int fd, Config* c, ConfigStringBudget* budget) { if (!receive_wire_bool(fd, &c->backup)) return false; - char* backup_dir = receive_str(fd); + char* backup_dir = config_receive_str(fd, budget); if (!backup_dir) return false; if (*backup_dir != '\0') { @@ -920,8 +965,8 @@ static bool receive_selection_options(int fd, Config* c) { return receive_wire_bool(fd, &c->delete_delay); } -static bool receive_resume_options(int fd, Config* c) { - char* temp_dir = receive_str(fd); +static bool receive_resume_options(int fd, Config* c, ConfigStringBudget* budget) { + char* temp_dir = config_receive_str(fd, budget); if (!temp_dir) return false; if (*temp_dir != '\0') { @@ -935,7 +980,7 @@ static bool receive_resume_options(int fd, Config* c) { string for "unset". Canonicalize the empty wire value back to NULL so receivers observe exactly what the client configured (plain --backup, for example, must not look like --backup-dir ""). */ - char* partial_dir = receive_str(fd); + char* partial_dir = config_receive_str(fd, budget); if (!partial_dir) return false; if (*partial_dir != '\0') { @@ -943,7 +988,7 @@ static bool receive_resume_options(int fd, Config* c) { } else { free(partial_dir); } - char* suffix = receive_str(fd); + char* suffix = config_receive_str(fd, budget); if (!suffix) return false; if (*suffix != '\0') { @@ -957,20 +1002,20 @@ static bool receive_resume_options(int fd, Config* c) { return false; if (!receive_n_data(fd, &c->modify_window, sizeof(c->modify_window))) return false; - c->compress_choice = receive_str(fd); + c->compress_choice = config_receive_str(fd, budget); if (!c->compress_choice) return false; - c->chmod_spec = receive_str(fd); + c->chmod_spec = config_receive_str(fd, budget); if (!c->chmod_spec || !receive_wire_bool(fd, &c->skip_compress_set) || !receive_int(fd, &c->skip_compress_count) || c->skip_compress_count < 0 || - c->skip_compress_count > 10000) + c->skip_compress_count > MAX_SKIP_COMPRESS_SUFFIXES) return false; if (c->skip_compress_count > 0) { c->skip_compress_suffixes = calloc((size_t)c->skip_compress_count, sizeof(char*)); if (!c->skip_compress_suffixes) return false; for (int i = 0; i < c->skip_compress_count; i++) { - c->skip_compress_suffixes[i] = receive_str(fd); + c->skip_compress_suffixes[i] = config_receive_str(fd, budget); if (!c->skip_compress_suffixes[i]) return false; } @@ -978,7 +1023,7 @@ static bool receive_resume_options(int fd, Config* c) { return true; } -static bool receive_basis_options(int fd, Config* c) { +static bool receive_basis_options(int fd, Config* c, ConfigStringBudget* budget) { int count; if (!receive_int(fd, &count)) return false; @@ -988,7 +1033,7 @@ static bool receive_basis_options(int fd, Config* c) { int type; if (!receive_int(fd, &type) || type <= BASIS_DEST_NONE || type > BASIS_DEST_LINK) return false; - char* path = receive_str(fd); + char* path = config_receive_str(fd, budget); if (!path) return false; /* config_basis_append validates and canonicalizes the path; a rejected @@ -1119,8 +1164,8 @@ static bool send_daemon_module(int fd, const Config* c) { return send_str(fd, c->module ? c->module : ""); } -static bool receive_daemon_module(int fd, Config* c) { - char* module = receive_str(fd); +static bool receive_daemon_module(int fd, Config* c, ConfigStringBudget* budget) { + char* module = config_receive_str(fd, budget); if (!module) return false; /* Guard against a hostile client flooding the log with an over-long module @@ -1155,14 +1200,14 @@ static bool send_daemon_auth(int fd, const Config* c) { return send_str_redacted(fd, c->auth_user); } -static bool receive_daemon_auth(int fd, Config* c) { +static bool receive_daemon_auth(int fd, Config* c, ConfigStringBudget* budget) { int present; if (!receive_int(fd, &present) || !valid_wire_bool(present)) return false; if (!present) return true; /* Redacted receive: never log the incoming username body. */ - char* user = receive_str_redacted(fd); + char* user = config_receive_str_redacted(fd, budget); if (!user) return false; if (!credentials_username_valid(user)) { @@ -1272,8 +1317,8 @@ static bool send_iconv_spec(int fd, const Config* c) { return send_str(fd, c->iconv_spec ? c->iconv_spec : ""); } -static bool receive_iconv_spec(int fd, Config* c) { - char* spec = receive_str(fd); +static bool receive_iconv_spec(int fd, Config* c, ConfigStringBudget* budget) { + char* spec = config_receive_str(fd, budget); if (!spec) return false; if (*spec == '\0') { @@ -1376,8 +1421,9 @@ Config* config_receive_with_validate(int file_descriptor, ConfigValidateFunc val Config* config = config_create(); if (!config) return NULL; + ConfigStringBudget budget = {0}; free(config->version); - config->version = receive_str(file_descriptor); + config->version = config_receive_str(file_descriptor, &budget); if (!config->version) goto error; if (strcmp(config->version, PROTOCOL_VERSION) != 0) { @@ -1388,21 +1434,21 @@ Config* config_receive_with_validate(int file_descriptor, ConfigValidateFunc val send_status(file_descriptor, STATUS_ERROR); goto error; } - if (!receive_core_fields(file_descriptor, config) || + if (!receive_core_fields(file_descriptor, config, &budget) || !receive_delta_fields(file_descriptor, config) || - !receive_file_options(file_descriptor, config) || + !receive_file_options(file_descriptor, config, &budget) || !receive_selection_options(file_descriptor, config) || - !receive_resume_options(file_descriptor, config) || - !receive_basis_options(file_descriptor, config) || + !receive_resume_options(file_descriptor, config, &budget) || + !receive_basis_options(file_descriptor, config, &budget) || !receive_fuzzy_option(file_descriptor, config) || !receive_checksum_options(file_descriptor, config) || !receive_identity_options(file_descriptor, config) || !receive_metadata_times_options(file_descriptor, config) || !receive_symlink_trust_options(file_descriptor, config) || !receive_phase4_xattr_options(file_descriptor, config) || - !receive_daemon_module(file_descriptor, config) || - !receive_daemon_auth(file_descriptor, config) || - !receive_iconv_spec(file_descriptor, config) || + !receive_daemon_module(file_descriptor, config, &budget) || + !receive_daemon_auth(file_descriptor, config, &budget) || + !receive_iconv_spec(file_descriptor, config, &budget) || !receive_privilege_options(file_descriptor, config) || !receive_copy_as_options(file_descriptor, config)) goto error; diff --git a/src/shared/config.h b/src/shared/config.h index 66fd5eb..d582be2 100644 --- a/src/shared/config.h +++ b/src/shared/config.h @@ -643,6 +643,24 @@ typedef struct Config { /* Upper bound on total basis-dir entries (rsync caps --link-dest at 20). */ #define MAX_BASIS_DIRS 64 +/* Upper bound on the number of --skip-compress suffixes accepted from the wire. + * Each suffix is an independent wire string (up to MAX_STRING_SIZE = 64 KiB), so + * without this a hostile pre-auth client could otherwise retain + * skip_count * MAX_STRING_SIZE bytes on the server before authentication; 256 + * covers any realistic suffix list while keeping the worst case small. */ +#define MAX_SKIP_COMPRESS_SUFFIXES 256 + +/* Aggregate ceiling on the bytes retained by ALL strings in one received config + * frame (version, send/receive roots, backup/temp/partial/suffix, compression + * choice, chmod spec, skip-compress suffixes, basis paths, module, auth user, + * iconv spec, ...). The config frame is parsed BEFORE authentication and every + * one of these strings lives for the whole connection, so this cumulative + * (never released) budget bounds the pre-auth memory a single connection can + * pin. MAX_SKIP_COMPRESS_SUFFIXES / MAX_BASIS_DIRS bound the individual + * repeatable counts; this budget bounds their product and any single oversized + * field. */ +#define MAX_CONFIG_STRING_BYTES (1ULL * 1024 * 1024) + /* Identity-mapping sentinels and bounds (see identity.h for semantics). * IDENTITY_MATCH_ANY is a usermap/groupmap FROM '*' (matches any id); * IDENTITY_CURRENT is a chown / map TO '*' (resolve to the receiver's current diff --git a/tests/test_config.c b/tests/test_config.c index 4ffc49f..003d768 100644 --- a/tests/test_config.c +++ b/tests/test_config.c @@ -6,6 +6,7 @@ #include "queue.h" #include "test_utils.h" #include "utils.h" +#include #include #include #include @@ -1846,6 +1847,89 @@ static void test_config_receive_rejects_copy_as_without_metadata() { config_delete(c); } +/* Like roundtrip_config_ok, but the parent is the RECEIVER so the frame can be + rejected MID-way, before the sender finishes writing it. The sender child + ignores SIGPIPE so the receiver closing early cannot kill it; the parent + waits for the child to exit after observing the rejection. */ +static bool roundtrip_config_rejected(const Config* send_cfg) { + int p[2]; + if (socketpair(AF_UNIX, SOCK_STREAM, 0, p) != 0) + return false; + pid_t pid = fork(); + if (pid == 0) { + (void)signal(SIGPIPE, SIG_IGN); + close(p[0]); + io_set_fds(p[1], p[1]); + config_send(p[1], send_cfg); + close(p[1]); + _exit(0); + } + close(p[1]); + io_set_fds(p[0], p[0]); + Config* recv = config_receive(p[0]); + bool rejected = recv == NULL; + config_delete(recv); + close(p[0]); + int status; + waitpid(pid, &status, 0); + return rejected; +} + +/* Build a Config with `count` --skip-compress suffixes, each `suffix_len` bytes + long, for the pre-auth config-string budget tests. */ +static Config* make_skip_compress_config(int count, size_t suffix_len) { + Config* c = config_create(); + if (!c) + return NULL; + c->send_directory = str_dup("/src"); + c->receive_root_directory = str_dup("/dst"); + c->skip_compress_set = true; + c->skip_compress_count = count; + c->skip_compress_suffixes = calloc((size_t)count, sizeof(char*)); + if (!c->skip_compress_suffixes) { + config_delete(c); + return NULL; + } + char* suffix = malloc(suffix_len + 1); + if (!suffix) { + config_delete(c); + return NULL; + } + memset(suffix, 'x', suffix_len); + suffix[suffix_len] = '\0'; + for (int i = 0; i < count; i++) + c->skip_compress_suffixes[i] = str_dup(suffix); + free(suffix); + return c; +} + +/* Pre-auth memory bound: one connection must not retain unbounded config + strings. An over-limit --skip-compress count is refused, and even an + in-range count cannot exceed the aggregate per-connection string budget. */ +static void test_config_receive_rejects_oversized_string_budget() { + if (is_running_under_valgrind()) + return; + + /* Exactly MAX_SKIP_COMPRESS_SUFFIXES tiny suffixes are accepted. */ + Config* ok = make_skip_compress_config(MAX_SKIP_COMPRESS_SUFFIXES, 1); + EXPECT_NOT_NULL(ok); + EXPECT_TRUE(roundtrip_config_ok(ok)); + config_delete(ok); + + /* One suffix over the count cap is rejected before any suffix is read. */ + Config* over_count = make_skip_compress_config(MAX_SKIP_COMPRESS_SUFFIXES + 1, 1); + EXPECT_NOT_NULL(over_count); + EXPECT_TRUE(roundtrip_config_rejected(over_count)); + config_delete(over_count); + + /* In-range count, but the strings together exceed MAX_CONFIG_STRING_BYTES + (64 suffixes * ~64 KiB > 1 MiB), so the aggregate budget rejects it. */ + Config* over_bytes = make_skip_compress_config(64, MAX_STRING_SIZE - 1); + EXPECT_NOT_NULL(over_bytes); + EXPECT_TRUE(roundtrip_config_rejected(over_bytes)); + config_delete(over_bytes); +} + /* identity_copy_as_refused() is the pure, pre-snapshot refusal predicate: a --copy-as is refused when the receiver is not root OR the effective super mode is OFF (an operator veto), and never when --copy-as is unset. */ @@ -2009,6 +2093,7 @@ void test_config() { test_config_copy_as_wire_roundtrip(); test_config_receive_rejects_negative_copy_as(); test_config_receive_rejects_copy_as_without_metadata(); + test_config_receive_rejects_oversized_string_budget(); test_config_receive_with_validate_rejects(); } test_identity_copy_as_refused(); From a90e234eb3d7c7d4b8c423e7d5defe4f6b1e026d Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 13 Sep 2026 00:59:21 +0200 Subject: [PATCH 4/5] harden: overflow guards, auth-user validation, TLS1.3 policy, build hardening --- .gitea/workflows/ci.yaml | 12 +++++----- CMakeLists.txt | 42 +++++++++++++++++++++++++++++++++- src/shared/array_list.c | 3 +++ src/shared/daemon_conf.c | 7 ++++++ src/shared/file_list.c | 4 +++- src/shared/transport_ssh.c | 2 +- src/shared/transport_tls.c | 12 ++++++++++ tests/test_array_list.c | 20 +++++++++++++++- tests/test_daemon_conf.c | 47 ++++++++++++++++++++++++++++++++++++++ 9 files changed, 139 insertions(+), 10 deletions(-) diff --git a/.gitea/workflows/ci.yaml b/.gitea/workflows/ci.yaml index 6fb81bb..bc08d57 100644 --- a/.gitea/workflows/ci.yaml +++ b/.gitea/workflows/ci.yaml @@ -12,7 +12,7 @@ jobs: container: gitea.tap-tap.win/taptap/fastsync-ci:v10 steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 - name: clang-format check run: find src/ tests/ -name '*.c' -o -name '*.h' | xargs clang-format --dry-run --Werror @@ -30,7 +30,7 @@ jobs: needs: lint steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 - name: Configure run: cmake -B build -S . -DSTRICT_WARNINGS=ON @@ -59,7 +59,7 @@ jobs: sanitizer: [address, undefined] steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 - name: Configure run: cmake -B build-${{ matrix.sanitizer }} -S . -DSANITIZER=${{ matrix.sanitizer }} @@ -77,7 +77,7 @@ jobs: if: github.event_name == 'push' steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 - name: Configure (clang + fuzz) run: CC=clang CXX=clang++ cmake -B build-fuzz -S . -DENABLE_FUZZ=ON @@ -99,7 +99,7 @@ jobs: if: github.event_name == 'push' steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 - name: Configure run: cmake -B build -S . -DENABLE_COVERAGE=ON @@ -123,7 +123,7 @@ jobs: if: github.event_name == 'push' steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 - name: Configure run: cmake -B build -S . -DSTRICT_WARNINGS=ON diff --git a/CMakeLists.txt b/CMakeLists.txt index 3ec3b24..ff92672 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -38,11 +38,24 @@ if(ENABLE_COVERAGE) add_link_options(--coverage) endif() +# --- Build hardening option --- +# Production hardening is applied to the shipping server/client binaries only, +# and only when no sanitizer or coverage instrumentation is active: sanitizers +# carry their own instrumentation, and _FORTIFY_SOURCE requires an optimising +# build (never the -O0 used for coverage). +option(ENABLE_HARDENING "Enable compiler/linker hardening for production targets" ON) +set(HARDENING_ACTIVE OFF) +if(ENABLE_HARDENING AND SANITIZER STREQUAL "none" AND NOT ENABLE_COVERAGE) + set(HARDENING_ACTIVE ON) +endif() + include(FetchContent) FetchContent_Declare( xxhash GIT_REPOSITORY https://github.com/Cyan4973/xxHash - GIT_TAG v0.8.3 + # v0.8.3 is a lightweight tag pointing at this exact commit (no ^{} peel + # entry); pin the commit SHA instead of the mutable tag. + GIT_TAG e626a72bc2321cd320e953a0ccf1584cad60f363 # v0.8.3 SOURCE_SUBDIR cmake_unofficial ) FetchContent_MakeAvailable(xxhash) @@ -73,6 +86,33 @@ add_executable(client ${CLIENT_SRCS} ${SHARED_SRCS} ${FILE_STORE_SRCS} ${SERVER_ target_include_directories(client PRIVATE src/shared src/server src/client) target_link_libraries(client PRIVATE Threads::Threads ${ZSTD_LIBRARY} OpenSSL::SSL OpenSSL::Crypto xxhash) +# --- Production hardening --- +# Each compile flag is probed so a compiler/architecture that lacks it still +# configures cleanly. _FORTIFY_SOURCE is guarded separately because it only +# works in an optimising build. xxHash is a static archive built by +# FetchContent, so it must be position-independent for the -pie link. +if(HARDENING_ACTIVE) + set_target_properties(xxhash PROPERTIES POSITION_INDEPENDENT_CODE ON) + include(CheckCCompilerFlag) + foreach(flag -fstack-protector-strong -fstack-clash-protection -fPIE) + string(MAKE_C_IDENTIFIER "HARDEN_${flag}" _harden_var) + check_c_compiler_flag("${flag}" ${_harden_var}) + endforeach() + check_c_compiler_flag("-D_FORTIFY_SOURCE=2" HARDEN_FORTIFY_SOURCE) + foreach(target server client) + foreach(flag -fstack-protector-strong -fstack-clash-protection -fPIE) + string(MAKE_C_IDENTIFIER "HARDEN_${flag}" _harden_var) + if(${_harden_var}) + target_compile_options(${target} PRIVATE ${flag}) + endif() + endforeach() + if(HARDEN_FORTIFY_SOURCE) + target_compile_options(${target} PRIVATE -D_FORTIFY_SOURCE=2) + endif() + target_link_options(${target} PRIVATE -pie -Wl,-z,relro -Wl,-z,now -Wl,-z,noexecstack) + endforeach() +endif() + # --- Testing --- enable_testing() diff --git a/src/shared/array_list.c b/src/shared/array_list.c index 95799b0..7813e6d 100644 --- a/src/shared/array_list.c +++ b/src/shared/array_list.c @@ -1,6 +1,7 @@ #include "log.h" #include "array_list.h" #include "protocol.h" +#include #include #include #include @@ -39,6 +40,8 @@ void array_list_delete(ArrayList* array_list) { static bool array_list_extend(ArrayList* array_list) { if (array_list == NULL) return false; + if (array_list->capacity > INT_MAX / 2) + return false; int new_capacity = array_list->capacity * 2; if (new_capacity == 0) new_capacity = INITIAL_ARRAY_SIZE; diff --git a/src/shared/daemon_conf.c b/src/shared/daemon_conf.c index e806549..7a1c83d 100644 --- a/src/shared/daemon_conf.c +++ b/src/shared/daemon_conf.c @@ -1,4 +1,5 @@ #include "daemon_conf.h" +#include "credentials.h" #include "utils.h" #include #include @@ -192,6 +193,12 @@ static bool apply_module_key(DaemonModule* module, char* key, char* value, char* const char* user = trim_ws(token); if (*user == '\0') continue; + if (!credentials_username_valid(user)) { + set_error(err, err_size, "module '%s': invalid 'auth users' entry '%s'", module->name, + user); + free(list); + return false; + } char** grown = realloc(module->auth_users, (size_t)(module->auth_user_count + 1) * sizeof(char*)); if (!grown) { diff --git a/src/shared/file_list.c b/src/shared/file_list.c index 8e40c78..528c8d4 100644 --- a/src/shared/file_list.c +++ b/src/shared/file_list.c @@ -2,6 +2,7 @@ #include "log.h" #include "utils.h" #include +#include #include #include #include @@ -51,7 +52,8 @@ static int normalize_entry(const char* raw, size_t len, bool strip_line_endings, if (len == 0) return 0; if (raw[0] == '/') { - snprintf(err, err_size, "absolute path entries are not allowed: '%.*s'", (int)len, raw); + int print_len = len > (size_t)INT_MAX ? INT_MAX : (int)len; + snprintf(err, err_size, "absolute path entries are not allowed: '%.*s'", print_len, raw); return -1; } /* Reject NUL bytes inside a token defensively (NUL-delimited mode splits on diff --git a/src/shared/transport_ssh.c b/src/shared/transport_ssh.c index b5010ac..97eb1ea 100644 --- a/src/shared/transport_ssh.c +++ b/src/shared/transport_ssh.c @@ -128,7 +128,7 @@ char* ssh_build_remote_command(const char* server_path, bool old_args, char* con q++; len++; } - if (len > SIZE_MAX - q * 3 || len + q * 3 + 3 > SIZE_MAX - command_len) + if (q > (SIZE_MAX - len) / 3 || len + q * 3 + 3 > SIZE_MAX - command_len) return NULL; command_len += len + q * 3 + 3; } diff --git a/src/shared/transport_tls.c b/src/shared/transport_tls.c index 85bb5a1..81aa483 100644 --- a/src/shared/transport_tls.c +++ b/src/shared/transport_tls.c @@ -65,6 +65,18 @@ static SSL_CTX* create_ssl_ctx(bool is_server, const char* cert, const char* key SSL_CTX_free(ctx); return NULL; } + /* TLS 1.3 ciphersuites are configured separately from the TLS 1.2 and below + * cipher list above. Pin the three AEAD suites OpenSSL offers, dropping + * TLS_AES_128_CCM_SHA256 and the CCM_8 variant, and fail closed if the + * library rejects the policy. SSL_CTX_set_ciphersuites needs OpenSSL 1.1.1; + * earlier versions have no TLS 1.3, so the call is compile-guarded. */ +#if OPENSSL_VERSION_NUMBER >= 0x10101000L + if (SSL_CTX_set_ciphersuites( + ctx, "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256") != 1) { + SSL_CTX_free(ctx); + return NULL; + } +#endif if (cert && key) { struct stat key_stat; diff --git a/tests/test_array_list.c b/tests/test_array_list.c index 964458d..a917dae 100644 --- a/tests/test_array_list.c +++ b/tests/test_array_list.c @@ -1,6 +1,7 @@ #include "test_array_list.h" #include "array_list.h" #include "test_utils.h" +#include #include static int destroyer_calls = 0; @@ -9,7 +10,7 @@ static void test_destroyer(void* item) { free(item); } -void test_array_list() { +static void test_array_list_basic() { ArrayList* list = array_list_create(free); EXPECT_NOT_NULL(list); EXPECT_EQ_INT(list->size, 0); @@ -54,3 +55,20 @@ void test_array_list() { array_list_delete(list); EXPECT_EQ_INT(destroyer_calls, 106); } + +/* A capacity that would overflow `capacity * 2` must be refused instead of + * wrapping into signed-overflow UB; array_list_add surfaces the failure. */ +static void test_array_list_extend_overflow_guard() { + ArrayList* list = array_list_create(NULL); + EXPECT_NOT_NULL(list); + list->capacity = INT_MAX / 2 + 1; + list->size = list->capacity; + EXPECT_FALSE(array_list_add(list, NULL)); + list->size = 0; + array_list_delete(list); +} + +void test_array_list() { + test_array_list_basic(); + test_array_list_extend_overflow_guard(); +} diff --git a/tests/test_daemon_conf.c b/tests/test_daemon_conf.c index 9580f8b..1f54a2e 100644 --- a/tests/test_daemon_conf.c +++ b/tests/test_daemon_conf.c @@ -1,4 +1,5 @@ #include "test_daemon_conf.h" +#include "credentials.h" #include "daemon_conf.h" #include "test_utils.h" #include @@ -320,6 +321,51 @@ static void test_daemon_conf_dparam_override() { daemon_conf_free(conf); } +/* Each `auth users` entry is validated with the same username rule as the + * credential store, so invisible whitespace/control characters can never make + * an exact strcmp match ambiguous. */ +static void test_daemon_conf_auth_users_validated() { + char* path; + char err[256]; + const DaemonConf* conf; + + EXPECT_EQ_INT(write_conf("[m]\npath = /x\nauth users = alice, bad user\n", &path), 0); + conf = daemon_conf_load(path, err, sizeof(err)); + free(path); + EXPECT_NULL(conf); + EXPECT_TRUE(strstr(err, "invalid 'auth users' entry") != NULL); + + EXPECT_EQ_INT(write_conf("[m]\npath = /x\nauth users = good\tbad\n", &path), 0); + conf = daemon_conf_load(path, err, sizeof(err)); + free(path); + EXPECT_NULL(conf); + EXPECT_TRUE(strstr(err, "invalid 'auth users' entry") != NULL); + + /* An over-long name exceeds CREDENTIAL_MAX_USER_LEN and is rejected. */ + { + char body[CREDENTIAL_MAX_USER_LEN + 128]; + int n = snprintf(body, sizeof(body), "[m]\npath = /x\nauth users = "); + memset(body + n, 'a', CREDENTIAL_MAX_USER_LEN + 1); + body[n + CREDENTIAL_MAX_USER_LEN + 1] = '\n'; + body[n + CREDENTIAL_MAX_USER_LEN + 2] = '\0'; + EXPECT_EQ_INT(write_conf(body, &path), 0); + conf = daemon_conf_load(path, err, sizeof(err)); + free(path); + EXPECT_NULL(conf); + EXPECT_TRUE(strstr(err, "invalid 'auth users' entry") != NULL); + } + + /* Empty entries between commas are skipped, not treated as invalid. */ + EXPECT_EQ_INT(write_conf("[m]\npath = /x\nauth users = alice,, bob\n", &path), 0); + DaemonConf* ok_conf = daemon_conf_load(path, err, sizeof(err)); + free(path); + EXPECT_NOT_NULL(ok_conf); + EXPECT_EQ_INT(ok_conf->modules[0].auth_user_count, 2); + EXPECT_EQ_STR(ok_conf->modules[0].auth_users[0], "alice"); + EXPECT_EQ_STR(ok_conf->modules[0].auth_users[1], "bob"); + daemon_conf_free(ok_conf); +} + static void test_daemon_module_name_valid() { EXPECT_TRUE(daemon_module_name_valid("backup")); EXPECT_TRUE(daemon_module_name_valid("Backup_2")); @@ -351,5 +397,6 @@ void test_daemon_conf() { test_daemon_conf_missing_file_rejected(); test_daemon_conf_find_module(); test_daemon_conf_dparam_override(); + test_daemon_conf_auth_users_validated(); test_daemon_module_name_valid(); } \ No newline at end of file From ea2f76cd7a2144a633e2f4723a9a5e9776b13b2b Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 13 Sep 2026 01:18:54 +0200 Subject: [PATCH 5/5] fix(receiver): charge per-entry DirTimeList cost; cap client --skip-compress --- src/client/client_cli.c | 5 +++++ src/shared/file_receive.c | 8 ++++++-- tests/test_file.c | 2 +- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/client/client_cli.c b/src/client/client_cli.c index 400be1b..6600ec0 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -517,6 +517,11 @@ static int parse_skip_compress(Config* config, const char* value) { token[--len] = '\0'; if (len == 0) continue; + if (config->skip_compress_count >= MAX_SKIP_COMPRESS_SUFFIXES) { + fprintf(stderr, "--skip-compress supports at most %d suffixes\n", MAX_SKIP_COMPRESS_SUFFIXES); + free(list); + return -1; + } if (config_add_pattern(&config->skip_compress_suffixes, &config->skip_compress_count, token, "--skip-compress") != 0) { free(list); diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index f21867c..477fa68 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -2274,7 +2274,11 @@ bool dir_time_list_add(DirTimeList* list, const char* wire_path, const FileMetad touching the list, leaving it exactly as it was (the caller fails the transfer, which becomes a clean protocol error). */ size_t path_len = strlen(wire_path); - if (list->count >= MAX_DIR_TIME_ENTRIES || path_len > MAX_DIR_TIME_BYTES - list->bytes) + /* Charge the whole per-entry cost (path copy + pointer slot + metadata + struct), not just the path, so the array growth is bounded by the same + cumulative budget. */ + size_t entry_cost = path_len + sizeof(FileMetadata) + sizeof(char*); + if (list->count >= MAX_DIR_TIME_ENTRIES || entry_cost > MAX_DIR_TIME_BYTES - list->bytes) return false; if (list->count == list->capacity) { size_t new_capacity = list->capacity == 0 ? 16 : list->capacity * 2; @@ -2302,7 +2306,7 @@ bool dir_time_list_add(DirTimeList* list, const char* wire_path, const FileMetad list->paths[list->count] = copy; list->entries[list->count] = *metadata; list->count++; - list->bytes += path_len; + list->bytes += entry_cost; return true; } diff --git a/tests/test_file.c b/tests/test_file.c index 0f0225d..928f544 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -1396,7 +1396,7 @@ static void test_dir_time_list_cap() { EXPECT_TRUE(list.bytes == before_bytes); } else { EXPECT_TRUE(list.count == before_count + 1); - EXPECT_TRUE(list.bytes == before_bytes + path_len); + EXPECT_TRUE(list.bytes == before_bytes + path_len + sizeof(FileMetadata) + sizeof(char*)); } } EXPECT_TRUE(rejected);