fix: harden network security boundaries #213
@@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
+23
-3
@@ -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 <path> TLS CA certificate file (PEM)\n");
|
||||
printf(" --destination-root <path> 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;
|
||||
}
|
||||
|
||||
+22
-1
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
+5
-1
@@ -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);
|
||||
|
||||
+25
-8
@@ -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;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -11,6 +11,7 @@
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include <sys/socket.h>
|
||||
#include <sys/stat.h>
|
||||
#include <sys/wait.h>
|
||||
#include <unistd.h>
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user