From 78876b110eefee6e044b75bd060b2fe231beb0f4 Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 15 Aug 2026 13:44:31 +0200 Subject: [PATCH] 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;