From ff189aeef2bbefbcf5706362e0b466f6d942dba4 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 15 Aug 2026 12:34:41 +0200 Subject: [PATCH 1/5] fix: harden network security boundaries --- src/client/client_cli.c | 4 +-- src/server/server.c | 26 ++++++++++++++++--- src/shared/chunk.c | 23 ++++++++++++++++- src/shared/config.c | 48 +++++++++++++++++++++++++++++++++++ src/shared/delta.c | 6 ++++- src/shared/file.c | 33 ++++++++++++++++++------ src/shared/file.h | 1 + src/shared/protocol.c | 5 ++++ src/shared/transport_tls.c | 12 +++++++++ tests/integration/common.py | 4 ++- tests/integration/test_tls.py | 3 +++ 11 files changed, 149 insertions(+), 16 deletions(-) diff --git a/src/client/client_cli.c b/src/client/client_cli.c index c00a9f2..0a345a7 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -400,8 +400,8 @@ static bool validate_config(const Config* config) { return false; } if (config->use_tls) { - if (!config->tls_cert || !config->tls_key) { - fprintf(stderr, "Error: --tls requires --cert and --key\n"); + if (!config->tls_cert || !config->tls_key || !config->tls_ca) { + fprintf(stderr, "Error: --tls requires --cert, --key, and --ca\n"); return false; } } diff --git a/src/server/server.c b/src/server/server.c index ee34957..6ab4e4f 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -23,6 +23,7 @@ static char* authorized_root; static int authorized_root_fd = -1; static bool allow_delete; +static bool allow_unauthenticated; static bool path_is_within(const char* root, const char* path) { size_t n = strlen(root); @@ -34,6 +35,15 @@ static bool valid_batch_path(const char* path) { strchr(path, '\0') == path + strlen(path); } +static bool batch_path_exists_secure(const char* path, const char* root) { + char* full_path = path_cat(root, path); + if (!full_path) + return false; + bool exists = file_path_exists_secure(full_path); + free(full_path); + return exists; +} + static bool __attribute__((unused)) configure_authorization(const char* root) { char resolved[PATH_MAX]; if (!root || !realpath(root, resolved)) @@ -121,7 +131,8 @@ int receive_files(Config* config, int fd) { char* full_path = path_cat(config->receive_root_directory, check_path); struct stat st; bool has_old = full_path && lstat(full_path, &st) == 0; - bool match = has_old && (unsigned long long)st.st_size == check_size && + bool secure_exists = batch_path_exists_secure(check_path, config->receive_root_directory); + bool match = has_old && secure_exists && (unsigned long long)st.st_size == check_size && (long long)st.st_mtime == check_mtime; bool sent = send_status(fd, match ? STATUS_OK : STATUS_NEXT); free(full_path); @@ -179,6 +190,12 @@ void handler(int file_descriptor) { close(file_descriptor); return; } + if (!allow_unauthenticated && ssl == NULL) { + log_message(LOG_LEVEL_ERROR, "Rejected unauthenticated plaintext connection"); + config_delete(config); + close(file_descriptor); + return; + } char resolved_destination[PATH_MAX]; char* canonical_destination = realpath(config->receive_root_directory, NULL); const char* destination = @@ -283,6 +300,7 @@ static void print_server_usage(void) { printf(" --ca TLS CA certificate file (PEM)\n"); printf(" --destination-root Authorized destination root (default: .)\n"); printf(" --allow-delete Permit manifest deletion\n"); + printf(" --allow-unauthenticated Allow plaintext/anonymous network clients\n"); printf(" -v, --verbose Enable debug logging\n"); printf(" --help Show this help\n"); } @@ -317,6 +335,8 @@ int main(int argc, char* argv[]) { destination_root = argv[++i]; } else if (strcmp(argv[i], "--allow-delete") == 0) { allow_delete = true; + } else if (strcmp(argv[i], "--allow-unauthenticated") == 0) { + allow_unauthenticated = true; } else if (strcmp(argv[i], "-p") == 0 && i + 1 < argc) { char* end; long p = strtol(argv[++i], &end, 10); @@ -357,8 +377,8 @@ int main(int argc, char* argv[]) { return 1; } if (use_tls) { - if (!tls_cert || !tls_key) { - fprintf(stderr, "Error: --tls requires --cert and --key\n"); + if (!tls_cert || !tls_key || !tls_ca) { + fprintf(stderr, "Error: --tls requires --cert, --key, and --ca\n"); server_delete(&g_server); return 1; } diff --git a/src/shared/chunk.c b/src/shared/chunk.c index 4ccf0d5..480342e 100644 --- a/src/shared/chunk.c +++ b/src/shared/chunk.c @@ -14,6 +14,7 @@ /* Maximum individual file data size within a chunk (64 MB) */ #define MAX_FILE_DATA_SIZE (64ULL * 1024 * 1024) +#define MAX_FILES_PER_CHUNK 65536U Chunk* chunk_create(File** items, int element_count) { Chunk* chunk = (Chunk*)malloc(sizeof(Chunk)); @@ -88,10 +89,17 @@ Data* chunk_serialize(Chunk* chunk, bool use_metadata) { Chunk* chunk_deserialize(Data* data, bool use_metadata) { ArrayList* files = array_list_create(file_destroy); + if (files == NULL) + return NULL; char* data_pointer = data->data; size_t remaining_size = data->size; while (remaining_size > 0) { + if ((unsigned int)files->size >= MAX_FILES_PER_CHUNK) { + log_message(LOG_LEVEL_ERROR, "Chunk contains too many files"); + array_list_delete(files); + return NULL; + } if (remaining_size < sizeof(size_t)) { log_message(LOG_LEVEL_ERROR, "Invalid chunk format: not enough data for path length"); array_list_delete(files); @@ -109,6 +117,10 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { return NULL; } + if (path_len == SIZE_MAX) { + array_list_delete(files); + return NULL; + } char* path = malloc(path_len + 1); if (path == NULL) { perror("Could not allocate memory for file path"); @@ -117,6 +129,11 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { } memcpy(path, data_pointer, path_len); path[path_len] = '\0'; + if (memchr(path, '\0', path_len) != NULL) { + free(path); + array_list_delete(files); + return NULL; + } data_pointer += path_len; remaining_size -= path_len; @@ -180,7 +197,11 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { data_pointer += file_data_size; remaining_size -= file_data_size; - array_list_add(files, file); + if (!array_list_add(files, file)) { + file_destroy(file); + array_list_delete(files); + return NULL; + } } File** file_array = (File**)array_list_to_array(files); diff --git a/src/shared/config.c b/src/shared/config.c index 4c1a3a0..529512b 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -100,6 +100,33 @@ static void config_set_defaults(Config* config) { config->compress_choice = NULL; } +static bool valid_wire_bool(int value) { + return value == 0 || value == 1; +} + +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) && + valid_wire_bool(config->use_compression) && valid_wire_bool(config->use_metadata) && + valid_wire_bool(config->use_sendfile) && valid_wire_bool(config->use_delete) && + valid_wire_bool(config->use_incremental) && valid_wire_bool(config->use_delta) && + valid_wire_bool(config->backup) && valid_wire_bool(config->follow_symlinks) && + valid_wire_bool(config->copy_links) && valid_wire_bool(config->safe_links) && + valid_wire_bool(config->copy_unsafe_links) && + valid_wire_bool(config->preserve_hard_links) && valid_wire_bool(config->preserve_acls) && + valid_wire_bool(config->preserve_xattrs) && valid_wire_bool(config->preserve_devices) && + valid_wire_bool(config->preserve_sparse) && valid_wire_bool(config->update) && + valid_wire_bool(config->inplace) && valid_wire_bool(config->append) && + valid_wire_bool(config->append_verify) && valid_wire_bool(config->delete_excluded) && + valid_wire_bool(config->delete_after) && valid_wire_bool(config->relative) && + valid_wire_bool(config->prune_empty_dirs) && valid_wire_bool(config->partial) && + config->compression_level >= 1 && config->compression_level <= 22 && + config->chunk_size > 0 && config->chunk_size <= MAX_CHUNK_SIZE && + config->delta_block_size >= DELTA_BLOCK_SIZE_MIN && + config->delta_block_size <= DELTA_BLOCK_SIZE_MAX && + config->delta_max_file_size <= DELTA_MAX_FILE_SIZE && config->max_delete >= 0; +} + Config* config_create(void) { Config* config = malloc(sizeof(Config)); if (!config) @@ -320,18 +347,26 @@ Config* config_receive(int file_descriptor) { int tmp; if (!receive_int(file_descriptor, &tmp)) goto error; + if (!valid_wire_bool(tmp)) + goto error; config->save_to_disk = tmp; if (!receive_int(file_descriptor, &tmp)) goto error; + if (!valid_wire_bool(tmp)) + goto error; config->use_multithreading = tmp; if (!receive_int(file_descriptor, &tmp)) goto error; config->use_chunk_serialization = tmp; if (!receive_int(file_descriptor, &tmp)) goto error; + if (!valid_wire_bool(tmp)) + goto error; config->use_compression = tmp; if (!receive_int(file_descriptor, &tmp)) goto error; + if (!valid_wire_bool(tmp)) + goto error; config->use_metadata = tmp; if (!receive_int(file_descriptor, &tmp)) goto error; @@ -340,15 +375,23 @@ Config* config_receive(int file_descriptor) { goto error; if (!receive_int(file_descriptor, &tmp)) goto error; + if (!valid_wire_bool(tmp)) + goto error; config->use_sendfile = tmp; if (!receive_int(file_descriptor, &tmp)) goto error; + if (!valid_wire_bool(tmp)) + goto error; config->use_delete = tmp; if (!receive_int(file_descriptor, &tmp)) goto error; + if (!valid_wire_bool(tmp)) + goto error; config->use_incremental = tmp; if (!receive_int(file_descriptor, &tmp)) goto error; + if (!valid_wire_bool(tmp)) + goto error; config->use_delta = tmp; if (!receive_n_data(file_descriptor, &config->delta_block_size, sizeof(config->delta_block_size))) goto error; @@ -440,6 +483,11 @@ Config* config_receive(int file_descriptor) { send_status(file_descriptor, STATUS_ERROR); goto error; } + if (!validate_received_config(config)) { + fprintf(stderr, "Invalid configuration received from client\n"); + send_status(file_descriptor, STATUS_ERROR); + goto error; + } if (!send_status(file_descriptor, STATUS_OK)) goto error; return config; diff --git a/src/shared/delta.c b/src/shared/delta.c index 26685db..343c0be 100644 --- a/src/shared/delta.c +++ b/src/shared/delta.c @@ -36,6 +36,9 @@ DeltaSignature* delta_signature_create(const void* old_file_data, uint64_t old_f if (old_file_data == NULL || old_file_size == 0 || block_size == 0) return NULL; + if (old_file_size > DELTA_MAX_FILE_SIZE || old_file_size > UINT32_MAX * (uint64_t)block_size) + return NULL; + uint32_t block_count = (uint32_t)((old_file_size + block_size - 1) / block_size); DeltaSignature* sig = malloc(sizeof(DeltaSignature)); @@ -463,7 +466,8 @@ Delta* delta_deserialize(const Data* data) { void* delta_apply(const void* old_data, uint64_t old_size, const Delta* delta, uint32_t block_size) { if (!old_data || !delta || (delta->new_file_size > 0 && delta->instructions == NULL) || - (delta->instruction_count > 0 && block_size == 0)) + (delta->instruction_count > 0 && block_size == 0) || + delta->new_file_size > DELTA_MAX_FILE_SIZE || delta->new_file_size > SIZE_MAX) return NULL; void* output = malloc((size_t)delta->new_file_size); diff --git a/src/shared/file.c b/src/shared/file.c index b333ce1..3df95bb 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -155,7 +155,24 @@ bool file_send_single_calls(File* file, int file_descriptor, bool use_metadata, static bool to_disk_secure(const char* path, const void* data, unsigned long long data_size, bool inplace, bool sparse, const FileMetadata* metadata); -static int open_secure_parent(const char* path, char** leaf_out); +static int open_secure_parent(const char* path, char** leaf_out, bool create_dirs); + +bool file_path_exists_secure(const char* path) { + if (!path) + return false; + char* leaf = NULL; + int parent_fd = open_secure_parent(path, &leaf, false); + if (parent_fd < 0) + return false; + struct stat st; + int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + bool exists = fd >= 0 && fstat(fd, &st) == 0; + if (fd >= 0) + close(fd); + close(parent_fd); + free(leaf); + return exists; +} static bool rename_secure(const char* old_path, const char* new_path); static int authorized_root_fd = -1; static char* authorized_root_path; @@ -532,7 +549,7 @@ File* receive_incremental_check(int fd, const Config* config, bool* skipped) { int old_fd = -1; if (full_path) { char* leaf = NULL; - int parent_fd = open_secure_parent(full_path, &leaf); + int parent_fd = open_secure_parent(full_path, &leaf, false); if (parent_fd >= 0) { old_fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); free(leaf); @@ -542,7 +559,7 @@ File* receive_incremental_check(int fd, const Config* config, bool* skipped) { } unsigned long long old_size = has_old_file ? (unsigned long long)st.st_size : 0; void* old_data = NULL; - if (has_old_file && old_size > 0) { + if (has_old_file && old_size > 0 && old_size <= DELTA_MAX_FILE_SIZE && old_size <= SIZE_MAX) { old_data = malloc((size_t)old_size); if (old_data) { size_t got = 0; @@ -651,7 +668,7 @@ File* receive_incremental_check(int fd, const Config* config, bool* skipped) { return file; } -static int open_secure_parent(const char* path, char** leaf_out) { +static int open_secure_parent(const char* path, char** leaf_out, bool create_dirs) { char* copy = str_dup(path); if (!copy) return -1; @@ -691,7 +708,7 @@ static int open_secure_parent(const char* path, char** leaf_out) { while (component) { if (strcmp(component, ".") != 0 && strcmp(component, "..") != 0) { int next = openat(fd, component, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); - if (next < 0 && errno == ENOENT && mkdirat(fd, component, 0755) == 0) + if (create_dirs && next < 0 && errno == ENOENT && mkdirat(fd, component, 0755) == 0) next = openat(fd, component, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); if (next < 0) { close(fd); @@ -711,8 +728,8 @@ static int open_secure_parent(const char* path, char** leaf_out) { static bool rename_secure(const char* old_path, const char* new_path) { char *old_leaf = NULL, *new_leaf = NULL; - int old_parent = open_secure_parent(old_path, &old_leaf); - int new_parent = open_secure_parent(new_path, &new_leaf); + int old_parent = open_secure_parent(old_path, &old_leaf, true); + int new_parent = open_secure_parent(new_path, &new_leaf, true); bool ok = old_parent >= 0 && new_parent >= 0 && renameat(old_parent, old_leaf, new_parent, new_leaf) == 0; if (old_parent >= 0) @@ -741,7 +758,7 @@ static bool write_all(int fd, const void* data, unsigned long long size) { static bool to_disk_secure(const char* path, const void* data, unsigned long long data_size, bool inplace, bool sparse, const FileMetadata* metadata) { char* leaf = NULL; - int dirfd = open_secure_parent(path, &leaf); + int dirfd = open_secure_parent(path, &leaf, true); if (dirfd < 0) return false; int fd = -1; diff --git a/src/shared/file.h b/src/shared/file.h index e763f91..33fbd52 100644 --- a/src/shared/file.h +++ b/src/shared/file.h @@ -40,6 +40,7 @@ bool to_disk(const char* path, const void* data, unsigned long long data_size, b bool file_save_to_disk(const char* root_directory, const File* file, const Config* config); void file_set_authorized_root(int fd, const char* canonical_path); File* receive_incremental_check(int fd, const Config* config, bool* skipped); +bool file_path_exists_secure(const char* path); int receive_manifest(int fd, const Config* config, int* next_status); #endif diff --git a/src/shared/protocol.c b/src/shared/protocol.c index a3a3aed..56fb852 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -260,6 +260,11 @@ char* receive_str(int file_descriptor) { free(data); return NULL; } + if (memchr(data, '\0', size) != NULL) { + free(data); + log_message(LOG_LEVEL_ERROR, "Received string contains an embedded NUL"); + return NULL; + } data[size] = '\0'; total_allocated_bytes += size + 1; log_message(LOG_LEVEL_DEBUG, "Received String: %s", data); diff --git a/src/shared/transport_tls.c b/src/shared/transport_tls.c index 49fc554..2b373ef 100644 --- a/src/shared/transport_tls.c +++ b/src/shared/transport_tls.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -43,8 +44,19 @@ static SSL_CTX* create_ssl_ctx(bool is_server, const char* cert, const char* key } SSL_CTX_set_min_proto_version(ctx, TLS1_2_VERSION); + if (SSL_CTX_set_cipher_list(ctx, "HIGH:!aNULL:!eNULL:!MD5:!RC4:!3DES") != 1) { + SSL_CTX_free(ctx); + return NULL; + } if (cert && key) { + struct stat key_stat; + if (stat(key, &key_stat) != 0 || !S_ISREG(key_stat.st_mode) || key_stat.st_uid != geteuid() || + (key_stat.st_mode & (S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH))) { + log_message(LOG_LEVEL_ERROR, "TLS private key must be owned by the current user and private"); + SSL_CTX_free(ctx); + return NULL; + } if (SSL_CTX_use_certificate_file(ctx, cert, SSL_FILETYPE_PEM) <= 0) { log_message(LOG_LEVEL_ERROR, "Failed to load certificate: %s", cert); log_ssl_errors(); diff --git a/tests/integration/common.py b/tests/integration/common.py index 08cc9c9..2e9653a 100644 --- a/tests/integration/common.py +++ b/tests/integration/common.py @@ -25,7 +25,9 @@ class ServerManager: def start(self, extra_args=None): self.stop() self._port = _find_free_port() - cmd = SERVER_CMD + ["-p", str(self._port)] + # Plain TCP is intentionally explicit in the server; integration tests + # exercise that opt-in mode rather than relying on the secure default. + cmd = SERVER_CMD + ["-p", str(self._port), "--allow-unauthenticated"] if extra_args: cmd += extra_args self._proc = subprocess.Popen(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) diff --git a/tests/integration/test_tls.py b/tests/integration/test_tls.py index 06e8caa..d74ef15 100644 --- a/tests/integration/test_tls.py +++ b/tests/integration/test_tls.py @@ -102,6 +102,7 @@ class TestTLSBasic: with ServerManager() as server: server.start(extra_args=[ "--tls", "--cert", certs["server_cert"], "--key", certs["server_key"], + "--ca", certs["ca"], ]) result, dur = run_client( SOURCE_DIR, DEST_DIR, @@ -125,6 +126,7 @@ class TestTLSBasic: with ServerManager() as server: server.start(extra_args=[ "--tls", "--cert", certs["server_cert"], "--key", certs["server_key"], + "--ca", certs["ca"], ]) result, dur = run_client( SOURCE_DIR, DEST_DIR, @@ -149,6 +151,7 @@ class TestTLSBasic: with ServerManager() as server: server.start(extra_args=[ "--tls", "--cert", certs["server_cert"], "--key", certs["server_key"], + "--ca", certs["ca"], ]) result, dur = run_client( SOURCE_DIR, DEST_DIR, -- 2.52.0 From 6f8847899c2d433537f9a5a04e977489ce0af672 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 15 Aug 2026 12:49:30 +0200 Subject: [PATCH 2/5] fix: validate remaining wire booleans --- src/shared/config.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/shared/config.c b/src/shared/config.c index 529512b..833a8b4 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -120,6 +120,7 @@ static bool validate_received_config(const Config* config) { valid_wire_bool(config->append_verify) && valid_wire_bool(config->delete_excluded) && valid_wire_bool(config->delete_after) && valid_wire_bool(config->relative) && valid_wire_bool(config->prune_empty_dirs) && valid_wire_bool(config->partial) && + valid_wire_bool(config->delete_before) && valid_wire_bool(config->checksum) && config->compression_level >= 1 && config->compression_level <= 22 && config->chunk_size > 0 && config->chunk_size <= MAX_CHUNK_SIZE && config->delta_block_size >= DELTA_BLOCK_SIZE_MIN && -- 2.52.0 From 298bfe0d0f7c74ee75e1e8ae56e4b74519b4b2f0 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 15 Aug 2026 13:13:57 +0200 Subject: [PATCH 3/5] fix: address delegated review findings --- README.md | 6 ++--- src/server/server.c | 6 +++-- src/shared/chunk.c | 47 ++++++++++++++++++++++++++++----- src/shared/metadata.c | 6 +++-- src/shared/transport_tls.c | 14 ++++++++-- src/shared/utils.c | 53 ++++++++++++++++++++++++++++++++++++-- src/shared/utils.h | 2 +- 7 files changed, 115 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index d6b7a42..634c8d5 100644 --- a/README.md +++ b/README.md @@ -188,7 +188,7 @@ Config negotiation is sender-driven: the client serializes transfer options and 7. **Metadata restoration** — `chmod()`, `chown()`, `utimensat()` on the receiving side 8. **`--delete`** — sender tracks all sent paths; receiver walks destination tree and removes unlisted files/directories 9. **SSH transport** — `socketpair()` + `fork()` + `execvp("ssh", ...)` with `ControlMaster` and port support -10. **TLS transport** — OpenSSL `SSL_CTX` with TLS 1.2 minimum, optional CA verification, transparent `SSL_read`/`SSL_write` via `io_set_ssl()` +10. **TLS transport** — OpenSSL `SSL_CTX` with TLS 1.2 minimum, mutual CA verification, transparent `SSL_read`/`SSL_write` via `io_set_ssl()` 11. **Path traversal protection** — `has_path_traversal()` rejects any file path containing `..` components, preventing directory escape attacks 12. **Connection limiting** — server tracks active connections and rejects new ones beyond `max_connections` (default 100) 13. **Keep-alive** — idle connections receive periodic `STATUS_KEEPALIVE` to detect half-open TCP connections @@ -202,7 +202,7 @@ Config negotiation is sender-driven: the client serializes transfer options and All received file paths are validated by `has_path_traversal()` before any disk operation. Any path containing `..` components is rejected with `STATUS_ERROR`, preventing directory escape attacks. ### TLS Certificate Verification -When `--ca` is provided, the server performs mutual TLS verification (`SSL_VERIFY_PEER` with depth 4). Without `--ca`, TLS is still encrypted but peer certificates are not verified. +TLS requires `--ca` and performs mutual TLS verification (`SSL_VERIFY_PEER` with depth 4). Connections without certificate verification are rejected. ### Connection Limits The server enforces a maximum of 100 concurrent connections (configurable via `max_connections` in `Server`). When the limit is reached, new connections are immediately rejected and closed. @@ -249,7 +249,7 @@ cmake -B build -S . && cmake --build build -j$(nproc) ### Server with TLS ```bash -./build/server --tls --cert server.pem --key server-key.pem +./build/server --tls --cert server.pem --key server-key.pem --ca ca.pem ``` ### Server via SSH diff --git a/src/server/server.c b/src/server/server.c index 6ab4e4f..c902c21 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -58,7 +58,7 @@ static bool __attribute__((unused)) configure_authorization(const char* root) { return false; } file_set_authorized_root(authorized_root_fd, authorized_root); - utils_set_authorized_root_fd(authorized_root_fd); + utils_set_authorized_root(authorized_root_fd, authorized_root); return true; } @@ -363,10 +363,12 @@ int main(int argc, char* argv[]) { return 1; } if (stdio_mode) { + /* SSH authenticates the stdio transport outside of FastSync. */ + allow_unauthenticated = true; io_set_fds(STDIN_FILENO, STDOUT_FILENO); handler(STDIN_FILENO); file_set_authorized_root(-1, NULL); - utils_set_authorized_root_fd(-1); + utils_set_authorized_root(-1, NULL); close(authorized_root_fd); free(authorized_root); return 0; diff --git a/src/shared/chunk.c b/src/shared/chunk.c index 480342e..2ff008c 100644 --- a/src/shared/chunk.c +++ b/src/shared/chunk.c @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -17,16 +18,26 @@ #define MAX_FILES_PER_CHUNK 65536U Chunk* chunk_create(File** items, int element_count) { + if (element_count < 0 || (element_count > 0 && items == NULL)) + return NULL; Chunk* chunk = (Chunk*)malloc(sizeof(Chunk)); if (chunk == NULL) { perror("ERROR: Could not allocate memory for chunk structure"); return NULL; } - chunk->items = (File**)malloc(element_count * sizeof(File*)); - if (chunk->items == NULL) { - free(chunk); - return NULL; + if (element_count == 0) { + chunk->items = NULL; + } else { + if ((size_t)element_count > SIZE_MAX / sizeof(File*)) { + free(chunk); + return NULL; + } + chunk->items = (File**)malloc((size_t)element_count * sizeof(File*)); + if (chunk->items == NULL) { + free(chunk); + return NULL; + } } for (int i = 0; i < element_count; i++) { @@ -139,18 +150,25 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { File* file = file_create(path); free(path); + if (file == NULL) { + array_list_delete(files); + return NULL; + } if (use_metadata) { if (remaining_size < sizeof(int)) { log_message(LOG_LEVEL_ERROR, "Invalid chunk format: not enough data for metadata"); + file_destroy(file); array_list_delete(files); return NULL; } // Peek at present flag to determine total size needed before reading int present_flag; memcpy(&present_flag, data_pointer, sizeof(int)); - if (present_flag && remaining_size < sizeof(int) + FILE_METADATA_WIRE_SIZE) { + if ((present_flag != 0 && present_flag != 1) || + (present_flag == 1 && remaining_size < sizeof(int) + FILE_METADATA_WIRE_SIZE)) { log_message(LOG_LEVEL_ERROR, "Invalid chunk format: not enough data for metadata body"); + file_destroy(file); array_list_delete(files); return NULL; } @@ -162,6 +180,7 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { if (remaining_size < sizeof(size_t)) { log_message(LOG_LEVEL_ERROR, "Invalid chunk format: not enough data for data size"); + file_destroy(file); array_list_delete(files); return NULL; } @@ -173,6 +192,7 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { if (remaining_size < file_data_size) { log_message(LOG_LEVEL_ERROR, "Invalid chunk format: not enough data for file content"); + file_destroy(file); array_list_delete(files); return NULL; } @@ -181,19 +201,27 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { if (file_data_size > MAX_FILE_DATA_SIZE) { log_message(LOG_LEVEL_ERROR, "File data size %zu exceeds maximum %llu", file_data_size, (unsigned long long)MAX_FILE_DATA_SIZE); + file_destroy(file); array_list_delete(files); return NULL; } - void* file_data = malloc(file_data_size); + size_t allocation_size = file_data_size > 0 ? file_data_size : 1; + void* file_data = malloc(allocation_size); if (file_data == NULL) { perror("Could not allocate memory for file data"); + file_destroy(file); array_list_delete(files); return NULL; } memcpy(file_data, data_pointer, file_data_size); data_destroy(file->data); file->data = data_create(file_data, file_data_size); + if (file->data == NULL) { + file_destroy(file); + array_list_delete(files); + return NULL; + } data_pointer += file_data_size; remaining_size -= file_data_size; @@ -205,10 +233,15 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { } File** file_array = (File**)array_list_to_array(files); + if (files->size > 0 && file_array == NULL) { + array_list_delete(files); + return NULL; + } Chunk* chunk = chunk_create(file_array, files->size); free(file_array); - files->item_destroyer = NULL; + if (chunk != NULL) + files->item_destroyer = NULL; array_list_delete(files); return chunk; diff --git a/src/shared/metadata.c b/src/shared/metadata.c index 49052ca..215327b 100644 --- a/src/shared/metadata.c +++ b/src/shared/metadata.c @@ -175,7 +175,8 @@ FileMetadata* metadata_receive(int file_descriptor, int* ok) { void file_restore_metadata(const char* path, const FileMetadata* metadata) { if (metadata == NULL) return; - if (chmod(path, metadata->mode & 07777 & ~(S_ISUID | S_ISGID)) != 0) + mode_t safe_mode = metadata->mode & 0777 & ~(S_IWGRP | S_IWOTH); + if (chmod(path, safe_mode) != 0) log_message(LOG_LEVEL_WARNING, "Failed to chmod %s: %s", path, strerror(errno)); /* Never apply client-supplied ownership. The descriptor API below is the receiver write path; retain this legacy API only for compatibility. */ @@ -192,7 +193,8 @@ bool file_restore_metadata_fd(int fd, const FileMetadata* metadata) { if (fd < 0 || metadata == NULL) return metadata == NULL; bool ok = true; - if (fchmod(fd, metadata->mode & 07777 & ~(S_ISUID | S_ISGID)) != 0) + mode_t safe_mode = metadata->mode & 0777 & ~(S_IWGRP | S_IWOTH); + if (fchmod(fd, safe_mode) != 0) ok = false; /* Client uid/gid values are deliberately not authoritative. */ struct timespec times[2] = {{.tv_sec = 0, .tv_nsec = UTIME_OMIT}, diff --git a/src/shared/transport_tls.c b/src/shared/transport_tls.c index 2b373ef..15d5727 100644 --- a/src/shared/transport_tls.c +++ b/src/shared/transport_tls.c @@ -35,6 +35,10 @@ static void log_ssl_errors(void) { static SSL_CTX* create_ssl_ctx(bool is_server, const char* cert, const char* key, const char* ca_path) { + if (!is_server && !ca_path) { + log_message(LOG_LEVEL_ERROR, "TLS clients require a CA certificate path"); + return NULL; + } const SSL_METHOD* method = is_server ? TLS_server_method() : TLS_client_method(); SSL_CTX* ctx = SSL_CTX_new(method); if (!ctx) { @@ -43,7 +47,10 @@ static SSL_CTX* create_ssl_ctx(bool is_server, const char* cert, const char* key return NULL; } - SSL_CTX_set_min_proto_version(ctx, TLS1_2_VERSION); + if (SSL_CTX_set_min_proto_version(ctx, TLS1_2_VERSION) != 1) { + SSL_CTX_free(ctx); + return NULL; + } if (SSL_CTX_set_cipher_list(ctx, "HIGH:!aNULL:!eNULL:!MD5:!RC4:!3DES") != 1) { SSL_CTX_free(ctx); return NULL; @@ -103,7 +110,10 @@ static SSL* wrap_fd_with_ssl(int fd, SSL_CTX* ctx, bool is_server, const char* h // Enable hostname verification for client connections when a hostname is provided. // Must be done before SSL_connect to take effect during the handshake. if (!is_server && hostname) { - SSL_set1_host(ssl, hostname); + if (SSL_set1_host(ssl, hostname) != 1) { + SSL_free(ssl); + return NULL; + } } // Retry SSL_accept/SSL_connect on WANT_READ/WANT_WRITE (non-blocking handshake) diff --git a/src/shared/utils.c b/src/shared/utils.c index 9e11988..d0359e0 100644 --- a/src/shared/utils.c +++ b/src/shared/utils.c @@ -11,9 +11,58 @@ #include static int authorized_root_fd = -1; +static char* authorized_root_path; -void utils_set_authorized_root_fd(int fd) { +void utils_set_authorized_root(int fd, const char* canonical_path) { authorized_root_fd = fd; + free(authorized_root_path); + authorized_root_path = canonical_path ? str_dup(canonical_path) : NULL; +} + +static 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] == '/'); +} + +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)) + return -1; + + int dirfd = dup(authorized_root_fd); + if (dirfd < 0) + return -1; + + const char* relative_path = dest_root + strlen(authorized_root_path); + while (*relative_path == '/') + relative_path++; + char* relative = str_dup(*relative_path ? relative_path : "."); + if (!relative) { + close(dirfd); + return -1; + } + + char* saveptr = NULL; + char* component = strtok_r(relative, "/", &saveptr); + while (component) { + if (strcmp(component, ".") == 0 || strcmp(component, "..") == 0) { + free(relative); + close(dirfd); + return -1; + } + int next = openat(dirfd, component, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + if (next < 0) { + free(relative); + close(dirfd); + return -1; + } + close(dirfd); + dirfd = next; + component = strtok_r(NULL, "/", &saveptr); + } + + free(relative); + return dirfd; } bool mkdir_r(const char* path) { @@ -208,7 +257,7 @@ static bool delete_extras_fd(int dirfd, const char* rel_path, ArrayList* manifes bool delete_extras(const char* dest_root, ArrayList* manifest) { int rootfd = authorized_root_fd >= 0 - ? dup(authorized_root_fd) + ? open_authorized_destination(dest_root) : open(dest_root, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); if (rootfd < 0) return false; diff --git a/src/shared/utils.h b/src/shared/utils.h index d04daf5..9eeb17f 100644 --- a/src/shared/utils.h +++ b/src/shared/utils.h @@ -9,7 +9,7 @@ char* str_dup(const char* string); char* path_cat(const char* path1, const char* path2); bool glob_match(const char* pattern, const char* str); bool delete_extras(const char* dest_root, ArrayList* manifest); -void utils_set_authorized_root_fd(int fd); +void utils_set_authorized_root(int fd, const char* canonical_path); bool has_path_traversal(const char* path); #endif -- 2.52.0 From daf662cc2dd80fd47d27021c72622effe5163501 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 15 Aug 2026 13:24:13 +0200 Subject: [PATCH 4/5] fix: address remaining PR review findings --- README.md | 11 +++++++-- src/server/server.c | 46 +++++++++++++++++++++++------------ src/shared/chunk.c | 7 +++--- src/shared/config.c | 3 ++- src/shared/file.c | 26 ++++++++++++++++---- src/shared/file.h | 1 + src/shared/metadata.c | 9 +++++-- src/shared/protocol.c | 16 +----------- src/shared/utils.c | 26 +++++++++++++++++--- src/shared/utils.h | 3 +++ tests/integration/test_tls.py | 6 ++--- 11 files changed, 104 insertions(+), 50 deletions(-) diff --git a/README.md b/README.md index 634c8d5..c1253ed 100644 --- a/README.md +++ b/README.md @@ -140,6 +140,7 @@ Config negotiation is sender-driven: the client serializes transfer options and | `--cert ` | TLS certificate file (PEM) | | `--key ` | TLS private key file (PEM) | | `--ca ` | TLS CA certificate file for verification (PEM) | +| `--client-cn ` | Required TLS client certificate common name | ### Server @@ -151,6 +152,9 @@ Config negotiation is sender-driven: the client serializes transfer options and | `--cert ` | TLS certificate file (PEM) | | `--key ` | TLS private key file (PEM) | | `--ca ` | TLS CA certificate file for verification (PEM) | +| `--destination-root ` | Authorized destination root (default: `.`) | +| `--allow-delete` | Permit manifest deletion | +| `--allow-unauthenticated` | Permit plaintext TCP clients | | `-v, --verbose` | Enable debug logging | | `--help` | Show help | @@ -244,12 +248,12 @@ cmake -B build -S . && cmake --build build -j$(nproc) ### Server (TCP mode) ```bash -./build/server +./build/server --allow-unauthenticated ``` ### Server with TLS ```bash -./build/server --tls --cert server.pem --key server-key.pem --ca ca.pem +./build/server --tls --cert server.pem --key server-key.pem --ca ca.pem --client-cn fastsync-client ``` ### Server via SSH @@ -265,6 +269,9 @@ Place the `fastsync-server` binary in the remote `$PATH`. The client runs `ssh u ./build/client --source-dir /path/to/send --dest-dir /path/to/receive --save-to-disk ``` +Plain TCP requires the explicit `--allow-unauthenticated` server option. Use TLS for +authenticated network connections. + ### Client — TCP with TLS ```bash ./build/client --tls --cert client.pem --key client-key.pem --ca ca.pem \ diff --git a/src/server/server.c b/src/server/server.c index c902c21..26f02b9 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -19,11 +19,28 @@ #include #include #include +#include static char* authorized_root; static int authorized_root_fd = -1; static bool allow_delete; static bool allow_unauthenticated; +static const char* required_client_cn; + +static bool tls_client_identity_allowed(SSL* ssl) { + if (!ssl || !required_client_cn) + return false; + X509* certificate = SSL_get1_peer_certificate(ssl); + if (!certificate) + return false; + char common_name[256]; + int length = X509_NAME_get_text_by_NID(X509_get_subject_name(certificate), NID_commonName, + common_name, sizeof(common_name)); + bool allowed = length >= 0 && (size_t)length < sizeof(common_name) && + strcmp(common_name, required_client_cn) == 0; + X509_free(certificate); + return allowed; +} static bool path_is_within(const char* root, const char* path) { size_t n = strlen(root); @@ -35,15 +52,6 @@ static bool valid_batch_path(const char* path) { strchr(path, '\0') == path + strlen(path); } -static bool batch_path_exists_secure(const char* path, const char* root) { - char* full_path = path_cat(root, path); - if (!full_path) - return false; - bool exists = file_path_exists_secure(full_path); - free(full_path); - return exists; -} - static bool __attribute__((unused)) configure_authorization(const char* root) { char resolved[PATH_MAX]; if (!root || !realpath(root, resolved)) @@ -128,11 +136,10 @@ int receive_files(Config* config, int fd) { send_status(fd, STATUS_ERROR); return -1; } - char* full_path = path_cat(config->receive_root_directory, check_path); struct stat st; - bool has_old = full_path && lstat(full_path, &st) == 0; - bool secure_exists = batch_path_exists_secure(check_path, config->receive_root_directory); - bool match = has_old && secure_exists && (unsigned long long)st.st_size == check_size && + char* full_path = path_cat(config->receive_root_directory, check_path); + bool has_old = full_path && file_stat_secure(full_path, &st); + bool match = has_old && (unsigned long long)st.st_size == check_size && (long long)st.st_mtime == check_mtime; bool sent = send_status(fd, match ? STATUS_OK : STATUS_NEXT); free(full_path); @@ -196,6 +203,12 @@ void handler(int file_descriptor) { close(file_descriptor); return; } + if (ssl && !tls_client_identity_allowed(ssl)) { + log_message(LOG_LEVEL_ERROR, "Rejected TLS client with unauthorized identity"); + config_delete(config); + close(file_descriptor); + return; + } char resolved_destination[PATH_MAX]; char* canonical_destination = realpath(config->receive_root_directory, NULL); const char* destination = @@ -298,6 +311,7 @@ static void print_server_usage(void) { printf(" --cert TLS certificate file (PEM)\n"); printf(" --key TLS private key file (PEM)\n"); printf(" --ca TLS CA certificate file (PEM)\n"); + printf(" --client-cn Required TLS client certificate CN\n"); printf(" --destination-root Authorized destination root (default: .)\n"); printf(" --allow-delete Permit manifest deletion\n"); printf(" --allow-unauthenticated Allow plaintext/anonymous network clients\n"); @@ -331,6 +345,8 @@ int main(int argc, char* argv[]) { tls_key = argv[++i]; } else if (strcmp(argv[i], "--ca") == 0 && i + 1 < argc) { tls_ca = argv[++i]; + } else if (strcmp(argv[i], "--client-cn") == 0 && i + 1 < argc) { + required_client_cn = argv[++i]; } else if (strcmp(argv[i], "--destination-root") == 0 && i + 1 < argc) { destination_root = argv[++i]; } else if (strcmp(argv[i], "--allow-delete") == 0) { @@ -379,8 +395,8 @@ int main(int argc, char* argv[]) { return 1; } if (use_tls) { - if (!tls_cert || !tls_key || !tls_ca) { - fprintf(stderr, "Error: --tls requires --cert, --key, and --ca\n"); + if (!tls_cert || !tls_key || !tls_ca || !required_client_cn) { + fprintf(stderr, "Error: --tls requires --cert, --key, --ca, and --client-cn\n"); server_delete(&g_server); return 1; } diff --git a/src/shared/chunk.c b/src/shared/chunk.c index 2ff008c..b879dc8 100644 --- a/src/shared/chunk.c +++ b/src/shared/chunk.c @@ -215,13 +215,14 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { return NULL; } memcpy(file_data, data_pointer, file_data_size); - data_destroy(file->data); - file->data = data_create(file_data, file_data_size); - if (file->data == NULL) { + Data* replacement = data_create(file_data, file_data_size); + if (replacement == NULL) { file_destroy(file); array_list_delete(files); return NULL; } + data_destroy(file->data); + file->data = replacement; data_pointer += file_data_size; remaining_size -= file_data_size; diff --git a/src/shared/config.c b/src/shared/config.c index 833a8b4..b99303e 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -121,7 +121,8 @@ static bool validate_received_config(const Config* config) { valid_wire_bool(config->delete_after) && valid_wire_bool(config->relative) && valid_wire_bool(config->prune_empty_dirs) && valid_wire_bool(config->partial) && valid_wire_bool(config->delete_before) && valid_wire_bool(config->checksum) && - config->compression_level >= 1 && config->compression_level <= 22 && + (!config->use_compression || + (config->compression_level >= 1 && config->compression_level <= 22)) && config->chunk_size > 0 && config->chunk_size <= MAX_CHUNK_SIZE && config->delta_block_size >= DELTA_BLOCK_SIZE_MIN && config->delta_block_size <= DELTA_BLOCK_SIZE_MAX && diff --git a/src/shared/file.c b/src/shared/file.c index 3df95bb..ac1c36c 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -23,6 +23,8 @@ #include "protocol.h" #include "utils.h" +#define MAX_SERVER_DELETE_COUNT 100000U + bool file_checksum(File* file, uint64_t* checksum) { if (!file || !checksum || !file->data) return false; @@ -158,15 +160,19 @@ static bool to_disk_secure(const char* path, const void* data, unsigned long lon static int open_secure_parent(const char* path, char** leaf_out, bool create_dirs); bool file_path_exists_secure(const char* path) { - if (!path) + struct stat st; + return file_stat_secure(path, &st); +} + +bool file_stat_secure(const char* path, struct stat* st) { + if (!path || !st) return false; char* leaf = NULL; int parent_fd = open_secure_parent(path, &leaf, false); if (parent_fd < 0) return false; - struct stat st; int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); - bool exists = fd >= 0 && fstat(fd, &st) == 0; + bool exists = fd >= 0 && fstat(fd, st) == 0; if (fd >= 0) close(fd); close(parent_fd); @@ -457,8 +463,16 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ } } + Data* replacement = data_create(new_data, (size_t)new_size); + if (replacement == NULL) { + file_destroy(file); + free(old_data); + delta_signature_destroy(sig); + send_status(fd, STATUS_ERROR); + return NULL; + } data_destroy(file->data); - file->data = data_create(new_data, (size_t)new_size); + file->data = replacement; free(old_data); delta_signature_destroy(sig); @@ -510,6 +524,7 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ delta_signature_destroy(sig); free(old_data); + send_status(fd, STATUS_ERROR); return NULL; } @@ -1085,7 +1100,8 @@ int receive_manifest(int fd, const Config* config, int* next_status) { return *status_out == STATUS_FINISHED ? 0 : -1; } fprintf(stderr, "Deleting files not in manifest...\n"); - bool deletion_ok = delete_extras(config->receive_root_directory, manifest); + bool deletion_ok = + delete_extras_limited(config->receive_root_directory, manifest, MAX_SERVER_DELETE_COUNT); array_list_delete(manifest); return deletion_ok ? 0 : -1; } diff --git a/src/shared/file.h b/src/shared/file.h index 33fbd52..b103bfe 100644 --- a/src/shared/file.h +++ b/src/shared/file.h @@ -41,6 +41,7 @@ bool file_save_to_disk(const char* root_directory, const File* file, const Confi void file_set_authorized_root(int fd, const char* canonical_path); File* receive_incremental_check(int fd, const Config* config, bool* skipped); bool file_path_exists_secure(const char* path); +bool file_stat_secure(const char* path, struct stat* st); int receive_manifest(int fd, const Config* config, int* next_status); #endif diff --git a/src/shared/metadata.c b/src/shared/metadata.c index 215327b..2b0a7ab 100644 --- a/src/shared/metadata.c +++ b/src/shared/metadata.c @@ -76,6 +76,11 @@ FileMetadata* metadata_from_buf(char** buf) { memcpy(&mtime_nsec, *buf, sizeof(mtime_nsec)); *buf += sizeof(mtime_nsec); m->mtime_nsec = (long)mtime_nsec; + if (present != 1 || mtime_nsec < 0 || mtime_nsec >= 1000000000LL || mode < 0 || uid < 0 || + gid < 0) { + free(m); + return NULL; + } return m; } @@ -175,7 +180,7 @@ FileMetadata* metadata_receive(int file_descriptor, int* ok) { void file_restore_metadata(const char* path, const FileMetadata* metadata) { if (metadata == NULL) return; - mode_t safe_mode = metadata->mode & 0777 & ~(S_IWGRP | S_IWOTH); + mode_t safe_mode = metadata->mode & 07777 & ~(S_ISUID | S_ISGID); if (chmod(path, safe_mode) != 0) log_message(LOG_LEVEL_WARNING, "Failed to chmod %s: %s", path, strerror(errno)); /* Never apply client-supplied ownership. The descriptor API below is the @@ -193,7 +198,7 @@ bool file_restore_metadata_fd(int fd, const FileMetadata* metadata) { if (fd < 0 || metadata == NULL) return metadata == NULL; bool ok = true; - mode_t safe_mode = metadata->mode & 0777 & ~(S_IWGRP | S_IWOTH); + mode_t safe_mode = metadata->mode & 07777 & ~(S_ISUID | S_ISGID); if (fchmod(fd, safe_mode) != 0) ok = false; /* Client uid/gid values are deliberately not authoritative. */ diff --git a/src/shared/protocol.c b/src/shared/protocol.c index 56fb852..b38680a 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -13,7 +13,6 @@ #define RECEIVE_TIMEOUT_SEC 60 /* 60 second per-message timeout */ #define SEND_TIMEOUT_SEC 60 -#define MAX_CONNECTION_MEMORY (1024ULL * 1024 * 1024) /* 1 GB total per connection */ static __thread int io_read_fd = -1; static __thread int io_write_fd = -1; @@ -25,15 +24,12 @@ static struct timespec bw_last_refill = {0, 0}; static mtx_t bw_mutex; static once_flag bw_mutex_once = ONCE_FLAG_INIT; -static __thread unsigned long long total_allocated_bytes = 0; - void io_set_fds(int read_fd, int write_fd) { io_read_fd = read_fd; io_write_fd = write_fd; /* A descriptor switch starts a new transport; never reuse a TLS object belonging to a previous connection or test pipe. */ io_ssl = NULL; - total_allocated_bytes = 0; } static void bw_mutex_init(void) { @@ -247,8 +243,7 @@ char* receive_str(int file_descriptor) { size_t size; if (!receive_n_data(file_descriptor, &size, sizeof(size_t))) return NULL; - if (size > MAX_STRING_SIZE || size > SIZE_MAX - 1 || - size + 1 > MAX_CONNECTION_MEMORY - total_allocated_bytes) { + if (size > MAX_STRING_SIZE || size > SIZE_MAX - 1) { log_message(LOG_LEVEL_ERROR, "String size %zu exceeds maximum %llu", size, (unsigned long long)MAX_STRING_SIZE); return NULL; @@ -266,7 +261,6 @@ char* receive_str(int file_descriptor) { return NULL; } data[size] = '\0'; - total_allocated_bytes += size + 1; log_message(LOG_LEVEL_DEBUG, "Received String: %s", data); return data; } @@ -291,12 +285,6 @@ Data* receive_data(int file_descriptor) { return NULL; } size_t allocation_size = size == 0 ? 1 : (size_t)size; - if (allocation_size > MAX_CONNECTION_MEMORY - total_allocated_bytes) { - log_message(LOG_LEVEL_ERROR, "Per-connection memory limit exceeded (%llu + %llu > %llu)", - (unsigned long long)total_allocated_bytes, size, - (unsigned long long)MAX_CONNECTION_MEMORY); - return NULL; - } void* data = malloc(allocation_size); if (data == NULL) return NULL; @@ -304,12 +292,10 @@ Data* receive_data(int file_descriptor) { free(data); return NULL; } - total_allocated_bytes += allocation_size; log_message(LOG_LEVEL_DEBUG, "Received %lld data", size); Data* result = data_create(data, (size_t)size); if (!result) { free(data); - total_allocated_bytes -= allocation_size; } return result; } diff --git a/src/shared/utils.c b/src/shared/utils.c index d0359e0..295b2fb 100644 --- a/src/shared/utils.c +++ b/src/shared/utils.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -19,6 +20,10 @@ void utils_set_authorized_root(int fd, const char* canonical_path) { authorized_root_path = canonical_path ? str_dup(canonical_path) : NULL; } +void utils_set_authorized_root_fd(int fd) { + utils_set_authorized_root(fd, NULL); +} + static 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] == '/'); @@ -192,7 +197,8 @@ static bool is_dir_in_manifest(const char* rel_path, ArrayList* manifest) { return false; } -static bool delete_extras_fd(int dirfd, const char* rel_path, ArrayList* manifest) { +static bool delete_extras_fd(int dirfd, const char* rel_path, ArrayList* manifest, + size_t max_delete, size_t* deleted_count) { int scanfd = dup(dirfd); if (scanfd < 0) return false; @@ -222,7 +228,7 @@ static bool delete_extras_fd(int dirfd, const char* rel_path, ArrayList* manifes int childfd = openat(dirfd, entry->d_name, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); bool child_removed = false; if (childfd >= 0) { - child_removed = delete_extras_fd(childfd, child_rel, manifest); + child_removed = delete_extras_fd(childfd, child_rel, manifest, max_delete, deleted_count); close(childfd); } if (child_removed && !is_dir_in_manifest(child_rel, manifest) && @@ -241,8 +247,15 @@ static bool delete_extras_fd(int dirfd, const char* rel_path, ArrayList* manifes } } if (!found) { + if (*deleted_count >= max_delete) { + operation_ok = false; + free(child_rel); + continue; + } if (unlinkat(dirfd, entry->d_name, 0) != 0 && errno != ENOENT) operation_ok = false; + else + (*deleted_count)++; fprintf(stderr, " Deleted: %s\n", child_rel); } else { all_removed = false; @@ -255,18 +268,23 @@ static bool delete_extras_fd(int dirfd, const char* rel_path, ArrayList* manifes return operation_ok; } -bool delete_extras(const char* dest_root, ArrayList* manifest) { +bool delete_extras_limited(const char* dest_root, ArrayList* manifest, size_t max_delete) { int rootfd = authorized_root_fd >= 0 ? open_authorized_destination(dest_root) : open(dest_root, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); if (rootfd < 0) return false; - bool ok = delete_extras_fd(rootfd, "", manifest); + size_t deleted_count = 0; + bool ok = delete_extras_fd(rootfd, "", manifest, max_delete, &deleted_count); if (close(rootfd) != 0) ok = false; return ok; } +bool delete_extras(const char* dest_root, ArrayList* manifest) { + return delete_extras_limited(dest_root, manifest, SIZE_MAX); +} + bool has_path_traversal(const char* path) { if (!path) return false; diff --git a/src/shared/utils.h b/src/shared/utils.h index 9eeb17f..6d3e8d2 100644 --- a/src/shared/utils.h +++ b/src/shared/utils.h @@ -2,6 +2,7 @@ #define UTILS_H #include "array_list.h" +#include #include bool mkdir_r(const char* path); @@ -9,7 +10,9 @@ char* str_dup(const char* string); char* path_cat(const char* path1, const char* path2); bool glob_match(const char* pattern, const char* str); bool delete_extras(const char* dest_root, ArrayList* manifest); +bool delete_extras_limited(const char* dest_root, ArrayList* manifest, size_t max_delete); void utils_set_authorized_root(int fd, const char* canonical_path); +void utils_set_authorized_root_fd(int fd); bool has_path_traversal(const char* path); #endif diff --git a/tests/integration/test_tls.py b/tests/integration/test_tls.py index d74ef15..29074b2 100644 --- a/tests/integration/test_tls.py +++ b/tests/integration/test_tls.py @@ -102,7 +102,7 @@ class TestTLSBasic: with ServerManager() as server: server.start(extra_args=[ "--tls", "--cert", certs["server_cert"], "--key", certs["server_key"], - "--ca", certs["ca"], + "--ca", certs["ca"], "--client-cn", "fastsync-client", ]) result, dur = run_client( SOURCE_DIR, DEST_DIR, @@ -126,7 +126,7 @@ class TestTLSBasic: with ServerManager() as server: server.start(extra_args=[ "--tls", "--cert", certs["server_cert"], "--key", certs["server_key"], - "--ca", certs["ca"], + "--ca", certs["ca"], "--client-cn", "fastsync-client", ]) result, dur = run_client( SOURCE_DIR, DEST_DIR, @@ -151,7 +151,7 @@ class TestTLSBasic: with ServerManager() as server: server.start(extra_args=[ "--tls", "--cert", certs["server_cert"], "--key", certs["server_key"], - "--ca", certs["ca"], + "--ca", certs["ca"], "--client-cn", "fastsync-client", ]) result, dur = run_client( SOURCE_DIR, DEST_DIR, -- 2.52.0 From 78876b110eefee6e044b75bd060b2fe231beb0f4 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 15 Aug 2026 13:44:31 +0200 Subject: [PATCH 5/5] fix: harden remaining review findings --- src/server/server.c | 2 +- src/shared/chunk.c | 33 ++++++++++++++++++++++++++++----- src/shared/delta.c | 7 +++++++ src/shared/file.c | 22 ++++++++++++++++++++-- src/shared/metadata.c | 4 ++-- src/shared/multiprocessing.c | 4 ++-- src/shared/protocol.c | 14 +++++++++++++- src/shared/utils.c | 21 +++++++++++++++------ 8 files changed, 88 insertions(+), 19 deletions(-) diff --git a/src/server/server.c b/src/server/server.c index 26f02b9..44e389e 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -203,7 +203,7 @@ void handler(int file_descriptor) { close(file_descriptor); return; } - if (ssl && !tls_client_identity_allowed(ssl)) { + if (ssl && required_client_cn && !tls_client_identity_allowed(ssl)) { log_message(LOG_LEVEL_ERROR, "Rejected TLS client with unauthorized identity"); config_delete(config); close(file_descriptor); diff --git a/src/shared/chunk.c b/src/shared/chunk.c index b879dc8..bea1a6a 100644 --- a/src/shared/chunk.c +++ b/src/shared/chunk.c @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -62,15 +63,31 @@ void chunk_destroy(void* item) { } static unsigned long long per_file_serialize_size(File* file, bool use_metadata) { - return sizeof(size_t) + strlen(file->path) + - (use_metadata ? sizeof(int) + (file->metadata ? FILE_METADATA_WIRE_SIZE : 0) : 0) + - sizeof(size_t) + file->data->size; + unsigned long long size = sizeof(size_t); + size_t path_len = strlen(file->path); + unsigned long long metadata_size = + use_metadata ? sizeof(int) + (file->metadata ? FILE_METADATA_WIRE_SIZE : 0) : 0; + if ((unsigned long long)path_len > ULLONG_MAX - size) + return 0; + size += path_len; + if (metadata_size > ULLONG_MAX - size) + return 0; + size += metadata_size; + if (sizeof(size_t) > ULLONG_MAX - size) + return 0; + size += sizeof(size_t); + if ((unsigned long long)file->data->size > ULLONG_MAX - size) + return 0; + return size + file->data->size; } Data* chunk_serialize(Chunk* chunk, bool use_metadata) { unsigned long long data_size = 0; for (int i = 0; i < chunk->element_count; i++) { - data_size += per_file_serialize_size(chunk->items[i], use_metadata); + unsigned long long file_size = per_file_serialize_size(chunk->items[i], use_metadata); + if (file_size == 0 || file_size > ULLONG_MAX - data_size || data_size + file_size > SIZE_MAX) + return NULL; + data_size += file_size; } Data* data = data_create_empty(data_size); if (data == NULL) { @@ -174,8 +191,14 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { } file->metadata = metadata_from_buf(&data_pointer); remaining_size -= sizeof(int); - if (file->metadata) + if (present_flag == 1) { + if (file->metadata == NULL) { + file_destroy(file); + array_list_delete(files); + return NULL; + } remaining_size -= FILE_METADATA_WIRE_SIZE; + } } if (remaining_size < sizeof(size_t)) { diff --git a/src/shared/delta.c b/src/shared/delta.c index 343c0be..172b526 100644 --- a/src/shared/delta.c +++ b/src/shared/delta.c @@ -121,6 +121,13 @@ DeltaSignature* delta_signature_deserialize(const Data* data) { return NULL; } + if (sig->block_size == 0 || sig->block_size > DELTA_BLOCK_SIZE_MAX || + sig->file_size > DELTA_MAX_FILE_SIZE || sig->file_size == 0 || + (sig->file_size + sig->block_size - 1) / sig->block_size != sig->block_count) { + free(sig); + return NULL; + } + uint64_t expected = sizeof(uint64_t) + sizeof(uint32_t) + sizeof(uint32_t) + (uint64_t)sig->block_count * (sizeof(uint32_t) + sizeof(uint32_t)); if (data->size < expected) { diff --git a/src/shared/file.c b/src/shared/file.c index ac1c36c..0575655 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -24,6 +24,7 @@ #include "utils.h" #define MAX_SERVER_DELETE_COUNT 100000U +#define MAX_FILE_DATA_SIZE (64ULL * 1024 * 1024) bool file_checksum(File* file, uint64_t* checksum) { if (!file || !checksum || !file->data) @@ -171,8 +172,8 @@ bool file_stat_secure(const char* path, struct stat* st) { int parent_fd = open_secure_parent(path, &leaf, false); if (parent_fd < 0) return false; - int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); - bool exists = fd >= 0 && fstat(fd, st) == 0; + int fd = openat(parent_fd, leaf, O_RDONLY | O_NONBLOCK | O_CLOEXEC | O_NOFOLLOW); + bool exists = fd >= 0 && fstat(fd, st) == 0 && S_ISREG(st->st_mode); if (fd >= 0) close(fd); close(parent_fd); @@ -514,6 +515,12 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ send_status(fd, STATUS_ERROR); return NULL; } + if (uncompressed->size > MAX_FILE_DATA_SIZE) { + data_destroy(uncompressed); + file_destroy(file); + send_status(fd, STATUS_ERROR); + return NULL; + } file_data = uncompressed; } @@ -675,6 +682,12 @@ File* receive_incremental_check(int fd, const Config* config, bool* skipped) { send_status(fd, STATUS_ERROR); return NULL; } + if (uncompressed->size > MAX_FILE_DATA_SIZE) { + data_destroy(uncompressed); + file_destroy(file); + send_status(fd, STATUS_ERROR); + return NULL; + } file_data = uncompressed; } @@ -1043,6 +1056,11 @@ File* file_receive(const Config* config, int file_descriptor) { file_destroy(file); return NULL; } + if (file_data_uncompressed->size > MAX_FILE_DATA_SIZE) { + data_destroy(file_data_uncompressed); + file_destroy(file); + return NULL; + } file_data = file_data_uncompressed; } data_destroy(file->data); diff --git a/src/shared/metadata.c b/src/shared/metadata.c index 2b0a7ab..2dbb7af 100644 --- a/src/shared/metadata.c +++ b/src/shared/metadata.c @@ -180,7 +180,7 @@ FileMetadata* metadata_receive(int file_descriptor, int* ok) { void file_restore_metadata(const char* path, const FileMetadata* metadata) { if (metadata == NULL) return; - mode_t safe_mode = metadata->mode & 07777 & ~(S_ISUID | S_ISGID); + mode_t safe_mode = metadata->mode & 0777 & ~(S_IWGRP | S_IWOTH); if (chmod(path, safe_mode) != 0) log_message(LOG_LEVEL_WARNING, "Failed to chmod %s: %s", path, strerror(errno)); /* Never apply client-supplied ownership. The descriptor API below is the @@ -198,7 +198,7 @@ bool file_restore_metadata_fd(int fd, const FileMetadata* metadata) { if (fd < 0 || metadata == NULL) return metadata == NULL; bool ok = true; - mode_t safe_mode = metadata->mode & 07777 & ~(S_ISUID | S_ISGID); + mode_t safe_mode = metadata->mode & 0777 & ~(S_IWGRP | S_IWOTH); if (fchmod(fd, safe_mode) != 0) ok = false; /* Client uid/gid values are deliberately not authoritative. */ diff --git a/src/shared/multiprocessing.c b/src/shared/multiprocessing.c index 5edbec2..09f38e5 100644 --- a/src/shared/multiprocessing.c +++ b/src/shared/multiprocessing.c @@ -234,9 +234,9 @@ int receive_thread(void* pipeline_context) { } char* full_path = path_cat(config->receive_root_directory, check_path); struct stat st; - bool has_old = full_path && lstat(full_path, &st) == 0; + bool has_old = full_path && file_stat_secure(full_path, &st); bool match = has_old && (unsigned long long)st.st_size == check_size && - (long long)st.st_mtime == check_mtime; + (long long)st.st_mtime == check_mtime && S_ISREG(st.st_mode); if (!send_status(file_descriptor, match ? STATUS_OK : STATUS_NEXT)) RECEIVE_THREAD_FAIL(); free(full_path); diff --git a/src/shared/protocol.c b/src/shared/protocol.c index b38680a..8e90001 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -13,6 +13,7 @@ #define RECEIVE_TIMEOUT_SEC 60 /* 60 second per-message timeout */ #define SEND_TIMEOUT_SEC 60 +#define MAX_CONNECTION_MEMORY (256ULL * 1024 * 1024) /* bounded cumulative receive budget */ static __thread int io_read_fd = -1; static __thread int io_write_fd = -1; @@ -24,12 +25,15 @@ static struct timespec bw_last_refill = {0, 0}; static mtx_t bw_mutex; static once_flag bw_mutex_once = ONCE_FLAG_INIT; +static __thread unsigned long long total_allocated_bytes; + void io_set_fds(int read_fd, int write_fd) { io_read_fd = read_fd; io_write_fd = write_fd; /* A descriptor switch starts a new transport; never reuse a TLS object belonging to a previous connection or test pipe. */ io_ssl = NULL; + total_allocated_bytes = 0; } static void bw_mutex_init(void) { @@ -243,7 +247,8 @@ char* receive_str(int file_descriptor) { size_t size; if (!receive_n_data(file_descriptor, &size, sizeof(size_t))) return NULL; - if (size > MAX_STRING_SIZE || size > SIZE_MAX - 1) { + if (size > MAX_STRING_SIZE || size > SIZE_MAX - 1 || + size + 1 > MAX_CONNECTION_MEMORY - total_allocated_bytes) { log_message(LOG_LEVEL_ERROR, "String size %zu exceeds maximum %llu", size, (unsigned long long)MAX_STRING_SIZE); return NULL; @@ -261,6 +266,7 @@ char* receive_str(int file_descriptor) { return NULL; } data[size] = '\0'; + total_allocated_bytes += size + 1; log_message(LOG_LEVEL_DEBUG, "Received String: %s", data); return data; } @@ -285,6 +291,10 @@ Data* receive_data(int file_descriptor) { return NULL; } size_t allocation_size = size == 0 ? 1 : (size_t)size; + if (allocation_size > MAX_CONNECTION_MEMORY - total_allocated_bytes) { + log_message(LOG_LEVEL_ERROR, "Per-connection memory limit exceeded"); + return NULL; + } void* data = malloc(allocation_size); if (data == NULL) return NULL; @@ -292,10 +302,12 @@ Data* receive_data(int file_descriptor) { free(data); return NULL; } + total_allocated_bytes += allocation_size; log_message(LOG_LEVEL_DEBUG, "Received %lld data", size); Data* result = data_create(data, (size_t)size); if (!result) { free(data); + total_allocated_bytes -= allocation_size; } return result; } diff --git a/src/shared/utils.c b/src/shared/utils.c index 295b2fb..89c4761 100644 --- a/src/shared/utils.c +++ b/src/shared/utils.c @@ -231,9 +231,14 @@ static bool delete_extras_fd(int dirfd, const char* rel_path, ArrayList* manifes child_removed = delete_extras_fd(childfd, child_rel, manifest, max_delete, deleted_count); close(childfd); } - if (child_removed && !is_dir_in_manifest(child_rel, manifest) && - unlinkat(dirfd, entry->d_name, AT_REMOVEDIR) != 0 && errno != ENOENT) { - operation_ok = false; + if (child_removed && !is_dir_in_manifest(child_rel, manifest)) { + if (*deleted_count >= max_delete) { + operation_ok = false; + } else if (unlinkat(dirfd, entry->d_name, AT_REMOVEDIR) != 0 && errno != ENOENT) { + operation_ok = false; + } else { + (*deleted_count)++; + } } else if (!child_removed) { all_removed = false; } @@ -269,9 +274,13 @@ static bool delete_extras_fd(int dirfd, const char* rel_path, ArrayList* manifes } bool delete_extras_limited(const char* dest_root, ArrayList* manifest, size_t max_delete) { - int rootfd = authorized_root_fd >= 0 - ? open_authorized_destination(dest_root) - : open(dest_root, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + int rootfd; + if (authorized_root_fd >= 0) { + rootfd = + authorized_root_path ? open_authorized_destination(dest_root) : dup(authorized_root_fd); + } else { + rootfd = open(dest_root, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + } if (rootfd < 0) return false; size_t deleted_count = 0; -- 2.52.0