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