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