From f75a69f96aeece3dcf06c1c94094b87e2b469112 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:08:56 +0200 Subject: [PATCH 01/16] fix(ssh): reject option-injection destinations (C1) A remote destination's user@host token is passed to ssh in option position, so a host beginning with '-' (e.g. -oProxyCommand=...) was parsed by ssh as an option, allowing arbitrary command execution. - config_parse_ssh_dest now validates the user@host prefix and returns -1 (with a clear logged error) for an empty host or a user/host that starts with '-'; config_parse_transport_dest propagates the failure. - transport_ssh.c's parse_remote_dest applies the same validation as defense-in-depth, and ssh_build_client_argv inserts a '--' end-of-options marker before the destination token. - Unit tests cover -oProxyCommand=... / -prefixed hosts / empty host rejection and the argv shape. --- src/shared/config.c | 29 ++++++++++++++++++++++------ src/shared/config.h | 5 ++++- src/shared/transport_ssh.c | 22 ++++++++++++++++++--- tests/test_config.c | 29 ++++++++++++++++++++++++++++ tests/test_transport_ssh.c | 39 ++++++++++++++++++++++++++++---------- 5 files changed, 104 insertions(+), 20 deletions(-) diff --git a/src/shared/config.c b/src/shared/config.c index c5a3779..b7ad0d0 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -613,19 +613,36 @@ int config_parse_transport_dest(Config* config) { int daemon_ret = config_parse_daemon_dest(config); if (daemon_ret != 0) return daemon_ret; - config_parse_ssh_dest(config); - return 0; + /* 0 for a local destination (nothing parsed) or a valid SSH destination; + * -1 (already logged) for an injection-shaped user@host. */ + return config_parse_ssh_dest(config); } -void config_parse_ssh_dest(Config* config) { +int config_parse_ssh_dest(Config* config) { + if (!config || !config->receive_root_directory) + return 0; if (!config_is_remote_dest(config->receive_root_directory)) - return; + return 0; + const char* dest = config->receive_root_directory; + const char* colon = strchr(dest, ':'); + /* The user@host token is passed to ssh in option position, so a user or host + * beginning with '-' would be consumed by ssh as an option (argument + * injection: e.g. "-oProxyCommand=..."). An empty host is likewise not a + * valid destination. Validate before any wire/argv construction. */ + const char* at = memchr(dest, '@', (size_t)(colon - dest)); + const char* host = at ? at + 1 : dest; + size_t host_len = (size_t)(colon - host); + size_t user_len = at ? (size_t)(at - dest) : 0; + if (host_len == 0 || host[0] == '-' || (user_len > 0 && dest[0] == '-')) + return daemon_dest_parse_error("invalid remote destination user@host (must not be empty or " + "start with '-')", + dest); config->transport = TRANSPORT_SSH; - config->ssh_destination = str_dup(config->receive_root_directory); - const char* colon = strchr(config->receive_root_directory, ':'); + config->ssh_destination = str_dup(dest); char* path = str_dup(colon + 1); free(config->receive_root_directory); config->receive_root_directory = path; + return 0; } void config_burn_auth(Config* config) { diff --git a/src/shared/config.h b/src/shared/config.h index 05b01f9..509e3ee 100644 --- a/src/shared/config.h +++ b/src/shared/config.h @@ -851,7 +851,10 @@ bool config_send(int file_descriptor, const Config* config); bool config_send_wire_block(int file_descriptor, const Config* config); Config* config_receive(int file_descriptor); bool config_is_remote_dest(const char* s); -void config_parse_ssh_dest(Config* config); +/* Parse a single-colon host:path SSH destination (0 = not an SSH destination or + * parsed successfully, -1 = rejected, e.g. a user@host beginning with '-'; the + * reason is logged). */ +int config_parse_ssh_dest(Config* config); /* A ConfigValidateFunc may return this sentinel to tell * config_receive_with_validate that the callback ALREADY sent a terminal status diff --git a/src/shared/transport_ssh.c b/src/shared/transport_ssh.c index 97eb1ea..b9d1f93 100644 --- a/src/shared/transport_ssh.c +++ b/src/shared/transport_ssh.c @@ -75,6 +75,15 @@ static int parse_remote_dest(const char* dest, RemoteDest* r) { memcpy(r->host, dest, host_len); r->host[host_len] = '\0'; } + /* The user@host token is handed to ssh in option position. Reject anything + * that ssh would consume as an option (a leading '-') or an empty host, so a + * crafted destination can never inject an ssh option such as + * -oProxyCommand=... . This mirrors config_parse_ssh_dest's validation and + * is defense-in-depth for callers that bypass it. */ + if (r->host[0] == '\0' || r->host[0] == '-' || (r->user[0] != '\0' && r->user[0] == '-')) { + remote_dest_destroy(r); + return -1; + } return 0; } @@ -216,10 +225,10 @@ char** ssh_build_client_argv(const char* rsh_command, int port, const char* user nwords = 1; } - /* Fixed tail: three -o pairs (6) + optional -p/value (2) + user@host + - * remote command + terminating NULL. */ + /* Fixed tail: three -o pairs (6) + optional -p/value (2) + the "--" end of + * options marker + user@host + remote command + terminating NULL. */ int port_extra = (port > 0 && port != 22) ? 2 : 0; - size_t total = (size_t)nwords + 6 + (size_t)port_extra + 3; + size_t total = (size_t)nwords + 6 + (size_t)port_extra + 4; char** argv = calloc(total, sizeof(char*)); if (!argv) { for (int i = 0; i < nwords; i++) @@ -253,6 +262,13 @@ char** ssh_build_client_argv(const char* rsh_command, int port, const char* user goto fail_argv; ac++; } + /* End of options: guarantees the user@host token that follows is treated as + * the destination and never re-interpreted as an ssh option, even if every + * caller-side validation were bypassed. */ + argv[ac] = str_dup("--"); + if (!argv[ac]) + goto fail_argv; + ac++; argv[ac] = str_dup(userhost); if (!argv[ac]) goto fail_argv; diff --git a/tests/test_config.c b/tests/test_config.c index c4c83e9..f45baa2 100644 --- a/tests/test_config.c +++ b/tests/test_config.c @@ -87,6 +87,34 @@ static void test_config_ssh_dest_no_user() { config_delete(cfg); } +/* C1: the user@host token is passed to ssh in option position, so a host or user + * beginning with '-' (e.g. "-oProxyCommand=...") must be rejected before any + * argv is built, and an empty host must be rejected too. */ +static void test_config_ssh_dest_rejects_option_injection() { + Config* cfg = make_config("1.0", "/src", "-oProxyCommand=id:/dst", true, false, false, false, + false, 1, false, 0); + EXPECT_EQ_INT(config_parse_ssh_dest(cfg), -1); + EXPECT_EQ_INT(cfg->transport, TRANSPORT_TCP); + EXPECT_NULL(cfg->ssh_destination); + config_delete(cfg); + + cfg = + make_config("1.0", "/src", "-user@host:/dst", true, false, false, false, false, 1, false, 0); + EXPECT_EQ_INT(config_parse_ssh_dest(cfg), -1); + config_delete(cfg); + + cfg = make_config("1.0", "/src", "user@:/dst", true, false, false, false, false, 1, false, 0); + EXPECT_EQ_INT(config_parse_ssh_dest(cfg), -1); + config_delete(cfg); + + /* config_parse_transport_dest propagates the rejection (and still returns 1 + * for daemon syntax first). */ + cfg = make_config("1.0", "/src", "-oProxyCommand=id:/dst", true, false, false, false, false, 1, + false, 0); + EXPECT_EQ_INT(config_parse_transport_dest(cfg), -1); + config_delete(cfg); +} + static void test_config_daemon_dest_parse() { Config* cfg = make_config("1.0", "/src", "dahost::files/sub/dir", true, false, false, false, false, 1, false, 0); @@ -2789,6 +2817,7 @@ void test_config() { test_config_ssh_dest(); test_config_ssh_dest_local_path(); test_config_ssh_dest_no_user(); + test_config_ssh_dest_rejects_option_injection(); test_config_daemon_dest_parse(); test_config_daemon_dest_no_path(); test_config_daemon_dest_double_slash_normalized(); diff --git a/tests/test_transport_ssh.c b/tests/test_transport_ssh.c index 94a896a..2889444 100644 --- a/tests/test_transport_ssh.c +++ b/tests/test_transport_ssh.c @@ -66,16 +66,18 @@ static void test_ssh_remote_command_argument_modes() { free(command); } -/* The build for a single-word argv is [prog, six -o args, user, command]. */ +/* The build for a single-word argv is [prog, six -o args, "--", user, command]. */ static void test_ssh_build_client_argv_default_is_ssh() { char** argv = ssh_build_client_argv(NULL, 0, "u@h", "'srv' --stdio"); EXPECT_NOT_NULL(argv); EXPECT_EQ_STR(argv[0], "ssh"); EXPECT_EQ_STR(argv[1], "-o"); - EXPECT_EQ_STR(argv[7], "u@h"); - EXPECT_EQ_STR(argv[8], "'srv' --stdio"); - EXPECT_NULL(argv[9]); + /* The "--" end-of-options marker precedes the destination token. */ + EXPECT_EQ_STR(argv[7], "--"); + EXPECT_EQ_STR(argv[8], "u@h"); + EXPECT_EQ_STR(argv[9], "'srv' --stdio"); + EXPECT_NULL(argv[10]); ssh_free_client_argv(argv); } @@ -84,7 +86,7 @@ static void test_ssh_build_client_argv_uses_custom_rsh() { char** argv = ssh_build_client_argv("myrsh", 0, "u@h", "rc"); EXPECT_NOT_NULL(argv); EXPECT_EQ_STR(argv[0], "myrsh"); - EXPECT_NULL(argv[9]); + EXPECT_NULL(argv[10]); ssh_free_client_argv(argv); } @@ -96,21 +98,37 @@ static void test_ssh_build_client_argv_whitespace_command_and_port() { EXPECT_EQ_STR(argv[0], "ssh"); EXPECT_EQ_STR(argv[1], "-p"); EXPECT_EQ_STR(argv[2], "2222"); - EXPECT_NULL(argv[11]); + EXPECT_NULL(argv[12]); ssh_free_client_argv(argv); argv = ssh_build_client_argv("ssh", 2222, "u@h", "rc"); EXPECT_NOT_NULL(argv); EXPECT_EQ_STR(argv[0], "ssh"); - /* Flat [prog, -o x6, -p, port, user, command]. */ + /* Flat [prog, -o x6, -p, port, "--", user, command]. */ EXPECT_EQ_STR(argv[7], "-p"); EXPECT_EQ_STR(argv[8], "2222"); - EXPECT_EQ_STR(argv[9], "u@h"); - EXPECT_EQ_STR(argv[10], "rc"); - EXPECT_NULL(argv[11]); + EXPECT_EQ_STR(argv[9], "--"); + EXPECT_EQ_STR(argv[10], "u@h"); + EXPECT_EQ_STR(argv[11], "rc"); + EXPECT_NULL(argv[12]); ssh_free_client_argv(argv); } +/* C1: a destination host/user beginning with '-' would be parsed by ssh as an + * option (argument injection: -oProxyCommand=...), and an empty host is never + * valid. These are refused before any child is forked, so no Client is + * returned and no command can run. */ +static void test_ssh_connect_rejects_option_host() { + /* cppcheck-suppress constVariablePointer */ + Client* client = client_connect_ssh("-oProxyCommand=touch /tmp/pwned:/remote", 22, NULL, false, + NULL, false, NULL, 0); + EXPECT_NULL(client); + client = client_connect_ssh("-evil:/remote", 22, NULL, false, NULL, false, NULL, 0); + EXPECT_NULL(client); + client = client_connect_ssh("user@:/remote", 22, NULL, false, NULL, false, NULL, 0); + EXPECT_NULL(client); +} + /* --remote-option=OPT appends OPT to the remote command line after " --stdio", * each escaped as its own single-quoted shell word. Metacharacters that could * break out of the quoting are neutralized (never injected), matching the @@ -162,6 +180,7 @@ void test_transport_ssh() { test_ssh_connect_invalid_dest_empty(); test_ssh_connect_malformed(); test_ssh_connect_unreachable(); + test_ssh_connect_rejects_option_host(); test_ssh_remote_command_argument_modes(); test_ssh_build_client_argv_default_is_ssh(); test_ssh_build_client_argv_uses_custom_rsh(); From 0d6c1f784fc9470eb79dc8ce1abad0516e7757a8 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:09:01 +0200 Subject: [PATCH 02/16] fix(daemon-conf): reject empty hosts/auth allow-lists (C4) A present hosts allow/hosts deny/auth users key with an empty or separator-only value produced a zero-length list, silently meaning no ACL / no auth and contradicting the strict-parse contract. store_host_list and the auth users parser now track how many entries a present key actually added and fail the load with a clear error when it is zero, so a restrictive directive can never silently become open. Unit tests cover empty, whitespace-only and comma-only values. --- src/shared/daemon_conf.c | 22 ++++++++++++++++++ tests/test_daemon_conf.c | 50 ++++++++++++++++++++++++++++++++++------ 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/src/shared/daemon_conf.c b/src/shared/daemon_conf.c index 300e196..0e30cb3 100644 --- a/src/shared/daemon_conf.c +++ b/src/shared/daemon_conf.c @@ -133,6 +133,7 @@ static bool store_host_list(char*** list, int* count, const char* value, const c return false; } char* save = NULL; + int added = 0; for (char* token = strtok_r(copy, ", \t", &save); token; token = strtok_r(NULL, ", \t", &save)) { if (!host_pattern_valid(token)) { if (module_name) @@ -163,8 +164,20 @@ static bool store_host_list(char*** list, int* count, const char* value, const c return false; } (*list)[(*count)++] = dup; + added++; } free(copy); + /* A present key with an empty (or separator-only) value would otherwise + * install a zero-length list, i.e. no ACL at all: a strict-parse config must + * never silently turn a restrictive directive into "allow everyone". */ + if (added == 0) { + if (module_name) + set_error(err, err_size, "module '%s': '%s' must list at least one host pattern", module_name, + key); + else + set_error(err, err_size, "'%s' must list at least one host pattern", key); + return false; + } return true; } @@ -403,6 +416,7 @@ static bool apply_module_key(DaemonModule* module, char* key, char* value, char* return false; } char* save = NULL; + int added = 0; for (char* token = strtok_r(list, ",", &save); token; token = strtok_r(NULL, ",", &save)) { const char* user = trim_ws(token); if (*user == '\0') @@ -430,8 +444,16 @@ static bool apply_module_key(DaemonModule* module, char* key, char* value, char* return false; } module->auth_users[module->auth_user_count++] = dup; + added++; } free(list); + /* An empty/separator-only value must not silently disable authentication: + * the key's presence is an explicit request for an allow-list. */ + if (added == 0) { + set_error(err, err_size, "module '%s': 'auth users' must list at least one user", + module->name); + return false; + } return true; } if (key_equals(key, "max connections")) diff --git a/tests/test_daemon_conf.c b/tests/test_daemon_conf.c index 3cddf0f..9b8f412 100644 --- a/tests/test_daemon_conf.c +++ b/tests/test_daemon_conf.c @@ -391,6 +391,22 @@ static void test_daemon_conf_auth_users_validated() { EXPECT_EQ_STR(ok_conf->modules[0].auth_users[0], "alice"); EXPECT_EQ_STR(ok_conf->modules[0].auth_users[1], "bob"); daemon_conf_free(ok_conf); + + /* C4: an empty or separator-only `auth users` value is a parse error. It + * would otherwise leave the module with a zero-length allow-list, silently + * disabling the authentication the operator asked for. */ + const char* empty_auth[] = { + "[m]\npath = /x\nauth users = \n", + "[m]\npath = /x\nauth users = , ,\n", + "[m]\npath = /x\nauth users = \t\n", + }; + for (size_t i = 0; i < sizeof(empty_auth) / sizeof(empty_auth[0]); i++) { + EXPECT_EQ_INT(write_conf(empty_auth[i], &path), 0); + const DaemonConf* rejected = daemon_conf_load(path, err, sizeof(err)); + free(path); + EXPECT_NULL(rejected); + EXPECT_TRUE(strstr(err, "'auth users' must list at least one user") != NULL); + } } /* Wave 3 daemon hardening: configurable global/per-module connection caps, @@ -475,13 +491,33 @@ static void test_daemon_conf_limits_and_hosts_parse() { EXPECT_EQ_INT(conf->modules[0].max_connections, 0); daemon_conf_free(conf); - /* An empty hosts list is not an error (no patterns are added). */ - EXPECT_EQ_INT(write_conf("hosts allow = \n[m]\npath = /x\n", &path), 0); - conf = daemon_conf_load(path, err, sizeof(err)); - free(path); - EXPECT_NOT_NULL(conf); - EXPECT_EQ_INT(conf->global.hosts_allow_count, 0); - daemon_conf_free(conf); + /* C4: a present hosts key with an empty/separator-only value must not silently + * install a zero-length (allow-everyone) list. */ + const char* empty_hosts[] = { + "hosts allow = \n[m]\npath = /x\n", + "hosts deny = \n[m]\npath = /x\n", + "hosts allow = , ,\n[m]\npath = /x\n", + "hosts deny = \t\n[m]\npath = /x\n", + }; + for (size_t i = 0; i < sizeof(empty_hosts) / sizeof(empty_hosts[0]); i++) { + EXPECT_EQ_INT(write_conf(empty_hosts[i], &path), 0); + const DaemonConf* rejected = daemon_conf_load(path, err, sizeof(err)); + free(path); + EXPECT_NULL(rejected); + EXPECT_TRUE(strstr(err, "must list at least one host pattern") != NULL); + } + + const char* empty_module_hosts[] = { + "[m]\npath = /x\nhosts allow = \n", + "[m]\npath = /x\nhosts deny = ,\n", + }; + for (size_t i = 0; i < sizeof(empty_module_hosts) / sizeof(empty_module_hosts[0]); i++) { + EXPECT_EQ_INT(write_conf(empty_module_hosts[i], &path), 0); + const DaemonConf* rejected = daemon_conf_load(path, err, sizeof(err)); + free(path); + EXPECT_NULL(rejected); + EXPECT_TRUE(strstr(err, "must list at least one host pattern") != NULL); + } } static void test_daemon_hosts_allowed() { From 551c1870059825be64e4890059b8a67b104d15c1 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:09:21 +0200 Subject: [PATCH 03/16] fix(tls): AEAD-only 1.2 suites, server preference, TOCTOU key load, IP SAN C5: restrict the TLS 1.2 and below cipher list to ECDHE AEAD suites (ECDHE+AESGCM:ECDHE+CHACHA20, minus NULL/eNULL/MD5/RC4/3DES) instead of HIGH (which includes CBC), and set SSL_OP_CIPHER_SERVER_PREFERENCE so the server's order decides the negotiated cipher. Client and server share create_ssl_ctx, so both are updated. C7: load the private key through an O_RDONLY|O_NOFOLLOW|O_CLOEXEC fd, fstat that fd and validate owner/mode (now also rejecting group/other execute bits), then load from the fd via BIO_new_fd. This removes the stat-to-load TOCTOU race while keeping the exact-owner/0600 policy. C8: verify an IP-literal client hostname against the certificate IP SAN with X509_VERIFY_PARAM_set1_ip_asc instead of SSL_set1_host (a DNS check), falling back to SSL_set1_host for real names. Unit tests assert the server-preference option, the absence of CBC/RC4/ 3DES suites, and that context creation still succeeds. --- src/shared/transport_tls.c | 91 +++++++++++++++++++++++++++++++------- tests/test_transport_tls.c | 11 +++++ 2 files changed, 87 insertions(+), 15 deletions(-) diff --git a/src/shared/transport_tls.c b/src/shared/transport_tls.c index f95a7bc..839f2bc 100644 --- a/src/shared/transport_tls.c +++ b/src/shared/transport_tls.c @@ -4,7 +4,9 @@ #include "transport_tcp.h" #include "utils.h" #include +#include #include +#include #include #include #include @@ -34,6 +36,59 @@ static void log_ssl_errors(void) { } } +/* Load the TLS private key through an already-opened, no-follow descriptor so + * the owner/mode policy is checked on the SAME file object that is loaded: an + * attacker cannot swap the path between a stat() and a later open() (TOCTOU). + * The exact-owner / 0600 policy is preserved and group/other execute bits are + * rejected as well. Ownership of the descriptor passes to the BIO and is + * released exactly once by BIO_free() (BIO_CLOSE). */ +static bool load_private_key_secure(SSL_CTX* ctx, const char* key) { + int fd = open(key, O_RDONLY | O_NOFOLLOW | O_CLOEXEC); + if (fd < 0) { + char* escaped = output_escape(key, false); + log_message(LOG_LEVEL_ERROR, "Failed to open private key: %s", + escaped ? escaped : ""); + free(escaped); + return false; + } + struct stat key_stat; + if (fstat(fd, &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 | S_IXGRP | S_IXOTH))) { + log_message(LOG_LEVEL_ERROR, + "TLS private key must be a regular file owned by the current user and private " + "(mode 0600)"); + close(fd); + return false; + } + BIO* bio = BIO_new_fd(fd, BIO_CLOSE); + if (!bio) { + close(fd); + log_message(LOG_LEVEL_ERROR, "Failed to read private key"); + return false; + } + EVP_PKEY* pkey = PEM_read_bio_PrivateKey(bio, NULL, NULL, NULL); + BIO_free(bio); /* releases fd via BIO_CLOSE */ + if (!pkey) { + char* escaped = output_escape(key, false); + log_message(LOG_LEVEL_ERROR, "Failed to load private key: %s", + escaped ? escaped : ""); + free(escaped); + log_ssl_errors(); + return false; + } + int use_ok = SSL_CTX_use_PrivateKey(ctx, pkey); + EVP_PKEY_free(pkey); + if (use_ok != 1) { + char* escaped = output_escape(key, false); + log_message(LOG_LEVEL_ERROR, "Failed to use private key: %s", + escaped ? escaped : ""); + free(escaped); + log_ssl_errors(); + return false; + } + return true; +} + static SSL_CTX* create_ssl_ctx(bool is_server, const char* cert, const char* key, const char* ca_path) { if (!is_server && !ca_path) { @@ -56,12 +111,21 @@ static SSL_CTX* create_ssl_ctx(bool is_server, const char* cert, const char* key #ifdef SSL_OP_NO_RENEGOTIATION SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION); #endif + /* Let the server's own preference order decide the negotiated cipher rather + * than the client's, so a client cannot steer both peers into a weaker (but + * still offered) suite. */ + SSL_CTX_set_options(ctx, SSL_OP_CIPHER_SERVER_PREFERENCE); 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) { + /* TLS 1.2 and below: an AEAD-only suite list. "HIGH" still includes CBC + * suites (Lucky13/POODLE-adjacent MAC-then-encrypt constructions), so restrict + * the list to ECDHE key agreement with an AEAD record cipher (AES-GCM or + * ChaCha20-Poly1305). A NULL/weak/3DES cipher is never selectable. */ + if (SSL_CTX_set_cipher_list(ctx, "ECDHE+AESGCM:ECDHE+CHACHA20:!aNULL:!eNULL:!MD5:!RC4:!3DES") != + 1) { SSL_CTX_free(ctx); return NULL; } @@ -79,13 +143,6 @@ static SSL_CTX* create_ssl_ctx(bool is_server, const char* cert, const char* key #endif 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) { char* escaped = output_escape(cert, false); log_message(LOG_LEVEL_ERROR, "Failed to load certificate: %s", @@ -95,12 +152,7 @@ static SSL_CTX* create_ssl_ctx(bool is_server, const char* cert, const char* key SSL_CTX_free(ctx); return NULL; } - if (SSL_CTX_use_PrivateKey_file(ctx, key, SSL_FILETYPE_PEM) <= 0) { - char* escaped = output_escape(key, false); - log_message(LOG_LEVEL_ERROR, "Failed to load private key: %s", - escaped ? escaped : ""); - free(escaped); - log_ssl_errors(); + if (!load_private_key_secure(ctx, key)) { SSL_CTX_free(ctx); return NULL; } @@ -144,7 +196,16 @@ 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) { - if (SSL_set1_host(ssl, hostname) != 1) { + /* An IP-literal host must be verified against the certificate's IP SAN + * (X509_check_ip_asc), not as a DNS name: SSL_set1_host would look for a + * DNS SAN that a legitimate IP-SAN certificate never carries. */ + struct in_addr ipv4; + struct in6_addr ipv6; + bool is_ip_literal = + inet_pton(AF_INET, hostname, &ipv4) == 1 || inet_pton(AF_INET6, hostname, &ipv6) == 1; + int set_ok = is_ip_literal ? X509_VERIFY_PARAM_set1_ip_asc(SSL_get0_param(ssl), hostname) + : SSL_set1_host(ssl, hostname); + if (set_ok != 1) { SSL_free(ssl); return NULL; } diff --git a/tests/test_transport_tls.c b/tests/test_transport_tls.c index 28bbba5..cc13aea 100644 --- a/tests/test_transport_tls.c +++ b/tests/test_transport_tls.c @@ -24,6 +24,17 @@ static void test_server_create_tls_without_certs() { #ifdef SSL_OP_NO_RENEGOTIATION EXPECT_TRUE((SSL_CTX_get_options(ctx) & SSL_OP_NO_RENEGOTIATION) != 0); #endif + /* C5: the server's preference order decides the cipher and the TLS 1.2 list is + * AEAD-only (no CBC/RC4/3DES legacy suites). */ + EXPECT_TRUE((SSL_CTX_get_options(ctx) & SSL_OP_CIPHER_SERVER_PREFERENCE) != 0); + STACK_OF(SSL_CIPHER)* ciphers = SSL_CTX_get_ciphers(ctx); + EXPECT_NOT_NULL(ciphers); + for (int i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) { + const char* name = SSL_CIPHER_get_name(sk_SSL_CIPHER_value(ciphers, i)); + EXPECT_TRUE(name != NULL && strstr(name, "CBC") == NULL); + EXPECT_TRUE(name != NULL && strstr(name, "RC4") == NULL); + EXPECT_TRUE(name != NULL && strstr(name, "3DES") == NULL); + } server_delete(&s); EXPECT_NULL(s); } From 80c1ff321c8be45cccb3d4364689be12bd12a0c7 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:09:25 +0200 Subject: [PATCH 04/16] fix(credentials): length-check before legacy-hex scan (C9) secret_is_legacy_hex indexed s[0..63] without first checking the string length, reading out of bounds for a shorter secret. Require strlen(s) == 64 before scanning, and add a unit test that short and 63-hex-digit secrets are rejected as ordinary malformed verifiers (never misreported as legacy). --- src/shared/credentials.c | 4 ++-- tests/test_credentials.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/shared/credentials.c b/src/shared/credentials.c index f1a0c98..7f79c52 100644 --- a/src/shared/credentials.c +++ b/src/shared/credentials.c @@ -158,13 +158,13 @@ static int hex_value(char c) { * digits. Such a line is refused loudly (and never accepted) so an operator * cannot keep a replayable bearer digest in place after the protocol bump. */ static bool secret_is_legacy_hex(const char* s) { - if (!s) + if (!s || strlen(s) != 64) return false; for (int i = 0; i < 64; i++) { if (hex_value(s[i]) < 0) return false; } - return s[64] == '\0'; + return true; } bool credentials_b64_encode(const uint8_t* in, size_t n, char* out, size_t out_sz) { diff --git a/tests/test_credentials.c b/tests/test_credentials.c index 86d4dd6..80280e7 100644 --- a/tests/test_credentials.c +++ b/tests/test_credentials.c @@ -473,6 +473,34 @@ static void test_credentials_store_rejects_legacy_hex() { free(path); } +/* C9: the legacy-hex detector must check the length before indexing 64 bytes, so + * a short secret is never read out of bounds. Such a line is rejected for the + * ordinary "expected verifier" reason, never as legacy. */ +static void test_credentials_store_rejects_short_secret() { + char contents[CREDENTIAL_MAX_LINE]; + snprintf(contents, sizeof(contents), "alice:%s\n", "abc"); + char* path = make_tmp_file(contents); + EXPECT_NOT_NULL(path); + char err[512]; + const CredentialStore* store = credentials_load(path, NULL, err, sizeof(err)); + EXPECT_NULL(store); + EXPECT_TRUE(strstr(err, "legacy unsalted") == NULL); + rm_temp(path); + free(path); + + char short_hex[64]; + memset(short_hex, 'a', 63); + short_hex[63] = '\0'; + snprintf(contents, sizeof(contents), "alice:%s\n", short_hex); + path = make_tmp_file(contents); + EXPECT_NOT_NULL(path); + store = credentials_load(path, NULL, err, sizeof(err)); + EXPECT_NULL(store); + EXPECT_TRUE(strstr(err, "legacy unsalted") == NULL); + rm_temp(path); + free(path); +} + static void test_credentials_store_duplicate_rejected() { char line[CREDENTIAL_MAX_LINE]; EXPECT_TRUE(make_store_line("alice", KAT_PASSWORD, CREDENTIAL_MIN_ITERS, line, sizeof(line))); @@ -1066,6 +1094,7 @@ void test_credentials(void) { test_credentials_store_parse_valid(); test_credentials_store_parse_rejects_malformed(); test_credentials_store_rejects_legacy_hex(); + test_credentials_store_rejects_short_secret(); test_credentials_store_duplicate_rejected(); test_credentials_store_rejects_nonuniform_iters(); test_credentials_store_parse_missing_file(); From 9da5a0a9ed9c1b685238bcf64c09c31b2d8c2869 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:09:37 +0200 Subject: [PATCH 05/16] fix(server): gate --force by --allow-delete and secure root super default C2: --force is deletion authority (an incoming regular file may remove a non-empty destination directory tree, and --delete-missing-args may remove a non-empty directory mirror), but it was not masked by the operator --allow-delete policy. The handler now clears config->force_delete unless --allow-delete was given, exactly like --delete and --delete-missing-args. C3: a standalone TCP / --stdio server running as root defaulted to SUPER_MODE_AUTO, so an untrusted client --devices/--write-devices/ --super could make it create device nodes, write raw devices, or apply client-chosen ownership. A privileged standalone receiver now forces SUPER_MODE_OFF unless the operator opts in with the new server-only --allow-super flag. Non-root receivers are unchanged, and the daemon path keeps its per-module `client owner = yes` gate. --allow-super is rejected with --no-super or --daemon. C6: tls_client_identity_allowed now rejects a CN whose reported length reached the buffer bound, so a truncated over-long CN cannot be matched by a required --client-cn prefix. Tests: an integration regression proving --force cannot replace a destination directory without --allow-delete; standalone-default tests for --copy-as refusal and (root-only) skipped device creation; a CLI unit test for the new flag. The integration shared_server fixture opts in with --allow-super so the existing root-only ownership/device/copy-as tests continue to exercise the opted-in configuration. README and RSYNC_COMPAT document the flag and the force/delete gating. --- README.md | 4 +- RSYNC_COMPAT.md | 12 ++--- src/server/server.c | 41 ++++++++++++++- src/server/server_cli.c | 12 +++++ src/server/server_cli.h | 8 +++ tests/conftest.py | 8 ++- tests/integration/test_features.py | 84 +++++++++++++++++++++++++++++- tests/test_server_cli.c | 23 ++++++++ 8 files changed, 180 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index b7d8a0e..9ab5b00 100644 --- a/README.md +++ b/README.md @@ -178,6 +178,7 @@ transfer is never aborted. | `--ca ` | TLS CA certificate file for verification (PEM) | | `--destination-root ` | Authorized destination root (default: `.`) | | `--allow-delete` | Permit manifest deletion | +| `--allow-super` | Standalone/`--stdio` only: keep super-user activities enabled for a **root** receiver. Without it a root standalone server forces `SUPER_MODE_OFF`, so client `--devices`/`--write-devices`/`--super` and client-chosen ownership requests are skipped/refused. No effect when not root. | | `--allow-unauthenticated` | Permit plaintext TCP clients. For an `auth users` module this opts in **loopback plaintext only**; remote auth still requires verified TLS, so the flag never permits remote plaintext auth. | | `-v, --verbose` | Enable debug logging | | `--help` | Show help | @@ -498,7 +499,8 @@ link-target transfer remains incomplete. | | `--ca ` | CA file for peer verification. | | `--destination-root ` | Confine received files to this server-side root; defaults to the current directory. | -| `--allow-delete` | Permit client delete manifests. Deletion is refused by default. | +| `--allow-delete` | Permit client delete manifests. Deletion is refused by default. This also gates `--force` (which can recursively replace/remove a destination directory tree). | +| `--allow-super` | Standalone/`--stdio` only: keep super-user activities enabled for a **root** receiver. Without it a root standalone server forces `SUPER_MODE_OFF`, so client `--devices`/`--write-devices`/`--super` and client-chosen ownership requests are skipped/refused. No effect when not root. Daemon modules opt in per module with `client owner = yes`. | | `-v`, `--verbose` | Enable debug logging. | | `--help` | Print server usage. | diff --git a/RSYNC_COMPAT.md b/RSYNC_COMPAT.md index a51bbff..616aefc 100644 --- a/RSYNC_COMPAT.md +++ b/RSYNC_COMPAT.md @@ -257,14 +257,14 @@ why plain `--append` works on the normal atomic path, not only with `--inplace`. | `-N`, `--crtimes` | Preserve create times | ⛔ Impossible/Divergence | Birth-times cannot be set by any portable filesystem call (`utimensat`/`futimens` only set atime/mtime), so this row is an explicit **Impossible/Divergence** (Phase 7 Wave B). Capture + transmit stays: `statx(STATX_BTIME)` on Linux records the source birth time as a wire field; the receiver logs a debug note that it cannot be applied and continues — never failing the transfer and never pretending it worked. On platforms without `statx` it parses as a documented no-op (flag accepted; nothing is captured). Implies metadata transmission. Wire: new `crtime` fields + a `preserve_crtimes` config boolean; `PROTOCOL_VERSION` bumped **2.11.0 → 2.12.0** (see the Phase-4 metadata-time notes) | | `-O`, `--omit-dir-times` | Omit dirs from --times | ✅ Implemented | Real modifier now that FastSync preserves directory times. With metadata on, the scanner captures every traversed source directory's mtime (and atime under `-U`) and the sender transmits them in trailing `STATUS_DIR_TIMES` frame(s) **after all file data and the optional delete manifest** (chunked at the receiver's `MAX_MANIFEST_ENTRIES` per-frame cap); a dir-time entry only RECORDS metadata and never creates the directory, so empty source directories stay untransferred. The receiver defers applying them until its delete / `--delay-updates` publication phases have committed, so writing or removing a child never clobbers a parent directory's mtime (rsync applies directory times at the end for exactly this reason). When `-O` is set (the boolean crosses the wire) the receiver does not apply any of them; without `-O` an `-a`/`--preserve` transfer now restores directory times (reversing the old "never preserves dir times" divergence). Wire change: the terminal `STATUS_DIR_TIMES` frame; `PROTOCOL_VERSION` bumped **2.16.0 → 2.17.0** | | `-J`, `--omit-link-times` | Omit symlinks from --times | ✅ Implemented | Real modifier now that FastSync preserves symlink times. Symlink entries already carried their metadata on `STATUS_SYMLINK`; the receiver now applies it with **no-follow primitives only** (`utimensat(..., AT_SYMLINK_NOFOLLOW)`, plus best-effort `fchmodat(..., AT_SYMLINK_NOFOLLOW)` and policy-gated `fchownat(..., AT_SYMLINK_NOFOLLOW)`), so the link itself is stamped without ever dereferencing it, confined fd-relative below the authorized receive root. A symlink has no children, so the times are applied immediately at creation. When `-J` is set (the boolean crosses the wire) the receiver skips the timestamps (mode/ownership are unaffected); without `-J` an `-a`/`-l` transfer restores symlink mtimes. Wire change alongside `-O`: the shared `STATUS_DIR_TIMES` frame; `PROTOCOL_VERSION` bumped **2.16.0 → 2.17.0** | -| `--super` | Receiver attempts super-user activities | ✅ Implemented | Phase 7 Wave E: receiver-side **safe-subset + clear-refusal** privilege model, tri-state `super_mode` (auto/on/off). `--super` **permits** the receiver to attempt super-user activities — ownership application and char/block device-node creation — that are already confined fd-relative below the authorized receive root; `--no-super` **forbids** them even when the receiver is root; the default (`auto`) preserves the pre-existing **best-effort** behavior of *attempting* them (not only when already root: an unprivileged attempt is refused by the kernel and skipped per entry, matching FastSync's history). The server additionally accepts an operator-level `--no-super` veto that forces `OFF` for every connection it accepts (so it also refuses any client `--copy-as`/`--super`); the `--fake-super` owner replay and the `--write-devices` write path are gated by the same policy. **FastSync never elevates**: no `setuid`/`seteuid`/`setgid` is ever called, and `--super` never bypasses the confinement floor (`file_open_secure_parent`, `O_NOFOLLOW`, root checks) — it only permits an attempt that is already confined. `--super` does **not** imply `--numeric-ids` and never enables client-chosen ownership on its own: ownership is applied only when an explicit identity policy (`--usermap`/`--groupmap`/`--chown`/`--numeric-ids`/`--copy-as`) is also given. A non-root receiver given `--super` logs exactly one warning at activation and each confined attempt is then refused by the kernel and skipped per entry (never aborts); `--no-super` suppresses ownership, char/block `mknod`, `--write-devices` and the fake-super owner replay, while unprivileged FIFO creation is unaffected. Wire: one trailing `super_mode` int on the config frame (validated 0..2), sent **before** the `--copy-as` block (fixed order: super int, then copy-as presence int + ids); `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0**. **Documented divergence from rsync:** rsync's `--super` runs the receiver with elevated privilege; FastSync only permits a confined attempt and never elevates | +| `--super` | Receiver attempts super-user activities | ✅ Implemented | Phase 7 Wave E: receiver-side **safe-subset + clear-refusal** privilege model, tri-state `super_mode` (auto/on/off). `--super` **permits** the receiver to attempt super-user activities — ownership application and char/block device-node creation — that are already confined fd-relative below the authorized receive root; `--no-super` **forbids** them even when the receiver is root; the default (`auto`) preserves the pre-existing **best-effort** behavior of *attempting* them (not only when already root: an unprivileged attempt is refused by the kernel and skipped per entry, matching FastSync's history). The server additionally accepts an operator-level `--no-super` veto that forces `OFF` for every connection it accepts (so it also refuses any client `--copy-as`/`--super`); a **privileged (root) standalone/`--stdio` receiver now also defaults to `OFF`** unless the operator opts in with the new server-only `--allow-super` flag (an unprivileged receiver is unchanged, since the kernel refuses the confined attempts anyway; the `--daemon` path keeps its per-module `client owner = yes` opt-in); the `--fake-super` owner replay and the `--write-devices` write path are gated by the same policy. **FastSync never elevates**: no `setuid`/`seteuid`/`setgid` is ever called, and `--super` never bypasses the confinement floor (`file_open_secure_parent`, `O_NOFOLLOW`, root checks) — it only permits an attempt that is already confined. `--super` does **not** imply `--numeric-ids` and never enables client-chosen ownership on its own: ownership is applied only when an explicit identity policy (`--usermap`/`--groupmap`/`--chown`/`--numeric-ids`/`--copy-as`) is also given. A non-root receiver given `--super` logs exactly one warning at activation and each confined attempt is then refused by the kernel and skipped per entry (never aborts); `--no-super` suppresses ownership, char/block `mknod`, `--write-devices` and the fake-super owner replay, while unprivileged FIFO creation is unaffected. Wire: one trailing `super_mode` int on the config frame (validated 0..2), sent **before** the `--copy-as` block (fixed order: super int, then copy-as presence int + ids); `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0**. **Documented divergence from rsync:** rsync's `--super` runs the receiver with elevated privilege; FastSync only permits a confined attempt and never elevates | | `--fake-super` | Store/recover privileged attrs via xattrs | ✅ Implemented | Phase 7 Wave B: full record **and replay**. The receiver writes the source `uid:gid:mode:mtime_sec:mtime_nsec` into a reserved `user.fastsync.stat` xattr on each written file (best-effort, fd-relative, format unchanged), then immediately re-applies it via `fake_super_restore_fd`: `fchown` (only where privileged — a non-root EPERM/EACCES is skipped silently, matching FastSync's identity philosophy), `fchmod`, and `futimens`. The OWNER leg is additionally skipped unless an explicit ownership identity policy (`--numeric-ids`/`--usermap`/`--groupmap`/`--chown`/`--copy-as`) is active — `--fake-super` on its own only *records* the source owner and must not act as an un-gated chown primitive — when `--no-super` forbids super-user activities (even for root), or when an active `--copy-as` is authoritative, so the recorded source owner can never override a forced `--copy-as` owner; the xattr record is still stored/replayed for a later privileged restore and mode/mtime still apply, so unprivileged `--fake-super` keeps working. The restored mode goes through the same sanitization as the normal metadata path (group/other write bits are never granted, so a recorded 0666 restores as 0644), so fake-super replay can never grant group/other-write that plain `--preserve` would refuse. Absence or a malformed record is a silent no-op, never fatal. The recording format diverges from rsync's `user.rsync.%stat%`; no cross-tool conversion is attempted. Implies metadata transmission so the source uid/gid/mode/mtime are available. Both it and `-X`/`-A` are incompatible with `-s` (chunk serialization), rejected up front | | `--open-noatime` | Avoid changing access time when opening files | ✅ Implemented | Sender-side policy: the sender opens source files with `O_NOATIME` (Linux) when reading them for transfer, so the open/read does NOT bump the source's on-disk access time. Degrades safely when `O_NOATIME` is unavailable (not defined) or refused (`EPERM`, since it needs `CAP_FOWNER` or file ownership): the code falls back to a normal open, so the data always transfers — only the atime-bump is skipped. It does not itself capture/preserve atime; it only avoids modifying it. **Client-only, never crosses the wire.** Exposed as `file_open_for_read()` and applied to both the buffered data path and the sendfile path | | `--numeric-ids` | Do not map uid/gid by name | ✅ Implemented | Ownership is applied through FastSync's opt-in identity path (see the Phase-4 identity notes below). `--numeric-ids` is a mapping-policy modifier: when applying ownership it uses the transmitted numeric uid/gid directly, skipping the name lookup. Without an ownership-affecting option it is inert (FastSync only applies ownership when the user opts in). It does not need `-M` to be parsed, but ownership is only applied when metadata (hence the source uid/gid) is actually transmitted (see the notes) | | `--usermap=STRING` | Map usernames | ✅ Implemented | Opt-in ownership application. rsync subset implemented: comma-separated `FROM:TO` rules evaluated in order, first match wins; `FROM`/`TO` are group/user names (resolved on the SOURCE machine at parse time), `*` (FROM matches any id / TO = the receiving process's current euid), and an `@N` or bare `N` numeric id. Rules are carried over the wire as resolved numeric id pairs; the receiver applies a matching rule (else falls back to `--chown`, `--numeric-ids`, then a best-effort name lookup) via an fd-relative `fchown`. Malformed/unresolvable specs are rejected with a clear error, never a silent no-op. Implies metadata preservation so the source uid/gid travel. Only effective when the receiver can actually change ownership (root or membership); otherwise it warns and continues | | `--groupmap=STRING` | Map group names | ✅ Implemented | Same rsync subset and semantics as `--usermap` but for the group (gid) side and the group databases. See the Phase-4 identity notes | | `--chown=USER:GROUP` | Map owner and group | ✅ Implemented | Opt-in ownership override applied receiver-side. Forms: `USER:GROUP`, `USER` (owner only), `:GROUP` (group only); a `*` for USER/GROUP means the current/root user or group as appropriate; an `@N`/bare `N` numeric id is accepted. A `:` inside a name may be escaped as `\:`. Equivalent to a trailing `*:*` usermap+groupmap rule (so an explicit `--usermap`/`--groupmap` match wins). Malformed or unresolvable specs are clear parse errors. Implies metadata preservation. Only effective when the receiver has permission to chown; otherwise it warns and continues (rsync parity) | -| `--copy-as=USER[:GROUP]` | Perform the copy as another user/group | ✅ Implemented | Safe-subset implementation, an explicit divergence from rsync's **real identity switching**. rsync makes the receiving process actually assume USER/GROUP (setuid/setgid); FastSync's receiver is multithreaded, so a real credential drop would be unsafe and is never attempted — FastSync never calls `setuid`/`seteuid`/`setgid`. Instead the receiver FORCES the ownership of every entry it writes to `copy_as_uid`/`copy_as_gid` through the existing confined, fd-relative identity path (the same `fchown`/`fchownat` mechanism as `--chown`/`--usermap`/`--groupmap`; symlinks use `fchownat(..., AT_SYMLINK_NOFOLLOW)`, and directories — including intermediate parents created implicitly while writing a nested file — and char/block/FIFO nodes are owned no-follow too, so a directory never keeps the receiver's owner while its children get the target owner), with `--copy-as` at the **highest priority** — it beats usermap/groupmap/`--chown`/`--numeric-ids` and the best-effort name lookup. This REQUIRES a privileged (root) receiver: an unprivileged receiver REFUSES the whole transfer up front at the config handshake (`server_module_gate`, running inside `config_receive_with_validate` before the `STATUS_OK` ack) with a clear error and no file data exchanged — never a silent wrong-ownership result. A server running with an operator `--no-super` veto also refuses it, and a **daemon** refuses `--copy-as`, like every other client-chosen-ownership request (`--numeric-ids`/`--chown`/`--usermap`/`--groupmap`/`--fake-super`/explicit `--super`), unless the selected module opts in with `client owner = yes`; without that per-module opt-in a daemon must not honor an arbitrary client-selected owner (the standalone listener and SSH `--stdio` server keep honoring these for their single operator-authorized root). `--fake-super` interaction: `--copy-as` is authoritative, so the recorded source owner is never replayed over the forced target owner. If the ownership apply still fails with EPERM/EACCES (capability-restricted root, root-squash, read-only mount) the failure is logged at ERROR and the **entry is reported as failed** rather than written with the wrong owner, which fails the transfer (fail-fast) so overall success is never reported with the wrong owner. USER is resolved on the client against the user database (a name, an `@N`/bare `N` numeric id, or `*` meaning the client's current euid); when `:GROUP` is present it is resolved against the group database (`*` meaning the client's egid). **Group-default rule:** when the group is omitted FastSync uses the user's primary gid (`getpwuid(uid)->pw_gid`); a numeric id with no local passwd entry has no primary gid to look up, so `gid` falls back to `uid` (documented divergence). Malformed/empty/unresolvable specs are clear parse errors, never a silent no-op. Never elevates privileges and never bypasses the confined receive root. Implies metadata preservation (the source uid/gid must be transmitted). Wire: a new trailing config-frame block **sent after** the `--super` int (presence int, then the two int32 ids, both validated `>= 0` on receive; the ids are also rejected if they do not fit int32 at CLI parse time); `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0** | +| `--copy-as=USER[:GROUP]` | Perform the copy as another user/group | ✅ Implemented | Safe-subset implementation, an explicit divergence from rsync's **real identity switching**. rsync makes the receiving process actually assume USER/GROUP (setuid/setgid); FastSync's receiver is multithreaded, so a real credential drop would be unsafe and is never attempted — FastSync never calls `setuid`/`seteuid`/`setgid`. Instead the receiver FORCES the ownership of every entry it writes to `copy_as_uid`/`copy_as_gid` through the existing confined, fd-relative identity path (the same `fchown`/`fchownat` mechanism as `--chown`/`--usermap`/`--groupmap`; symlinks use `fchownat(..., AT_SYMLINK_NOFOLLOW)`, and directories — including intermediate parents created implicitly while writing a nested file — and char/block/FIFO nodes are owned no-follow too, so a directory never keeps the receiver's owner while its children get the target owner), with `--copy-as` at the **highest priority** — it beats usermap/groupmap/`--chown`/`--numeric-ids` and the best-effort name lookup. This REQUIRES a privileged (root) receiver: an unprivileged receiver REFUSES the whole transfer up front at the config handshake (`server_module_gate`, running inside `config_receive_with_validate` before the `STATUS_OK` ack) with a clear error and no file data exchanged — never a silent wrong-ownership result. A server running with an operator `--no-super` veto also refuses it; a privileged (root) standalone/`--stdio` server refuses it by default too and only honors it after the operator passes `--allow-super`, and a **daemon** refuses `--copy-as`, like every other client-chosen-ownership request (`--numeric-ids`/`--chown`/`--usermap`/`--groupmap`/`--fake-super`/explicit `--super`), unless the selected module opts in with `client owner = yes`; without that per-module opt-in a daemon must not honor an arbitrary client-selected owner (a root standalone listener and SSH `--stdio` server honor these for their single operator-authorized root only when started with `--allow-super`). `--fake-super` interaction: `--copy-as` is authoritative, so the recorded source owner is never replayed over the forced target owner. If the ownership apply still fails with EPERM/EACCES (capability-restricted root, root-squash, read-only mount) the failure is logged at ERROR and the **entry is reported as failed** rather than written with the wrong owner, which fails the transfer (fail-fast) so overall success is never reported with the wrong owner. USER is resolved on the client against the user database (a name, an `@N`/bare `N` numeric id, or `*` meaning the client's current euid); when `:GROUP` is present it is resolved against the group database (`*` meaning the client's egid). **Group-default rule:** when the group is omitted FastSync uses the user's primary gid (`getpwuid(uid)->pw_gid`); a numeric id with no local passwd entry has no primary gid to look up, so `gid` falls back to `uid` (documented divergence). Malformed/empty/unresolvable specs are clear parse errors, never a silent no-op. Never elevates privileges and never bypasses the confined receive root. Implies metadata preservation (the source uid/gid must be transmitted). Wire: a new trailing config-frame block **sent after** the `--super` int (presence int, then the two int32 ids, both validated `>= 0` on receive; the ids are also rejected if they do not fit int32 at CLI parse time); `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0** | **Phase-4 metadata-time notes:** `-U/--atimes`, `-N/--crtimes`, `-O/--omit-dir-times`, `-J/--omit-link-times`, and `--open-noatime` are new. @@ -639,7 +639,7 @@ now transmits targets (the prior behavior was broken/partial); its status moved - **Host access control (`hosts allow`/`hosts deny`):** both keys accept a comma- and/or whitespace-separated list of patterns and may appear globally and/or per module (multiple config-file lines append; a `--dparam` override replaces). Supported patterns are `*` (match all), an IPv4 or IPv6 literal (`10.0.0.1`, `2001:db8::1`), and an IPv4/IPv6 CIDR (`10.0.0.0/8`, `2001:db8::/32`). Hostname patterns are **not** supported: because the peer is always a numeric address and no reverse DNS is performed, a hostname/glob pattern would silently never match, so it is rejected at load time (fail-closed) instead of being accepted as a dead rule. An IPv4 peer on a dual-stack IPv6 listener is normalized from its `::ffff:a.b.c.d` form so IPv4 patterns match it. rsync-like semantics: a matching `hosts deny` rejects; if any `hosts allow` entries exist, a peer matching none of them is rejected; deny takes precedence over allow. The daemon enforces the global list first, then the selected module's list, **before authentication** in `server_module_gate`, with an audit log line naming the peer, the module and the outcome. The numeric peer address is obtained with `getpeername`+`inet_ntop` (`utils_fd_peer_ip`, handling both address families); when it cannot be obtained a module with any ACL fails closed (refused), while an ACL-free module continues and logs at debug. A malformed pattern (e.g. an out-of-range CIDR prefix) is a parse error at load time. - **Connection caps, shared registry and auth lockout:** the global `max connections` key (default 100) is plumbed into the listener (`transport_tcp.c`), which rejects a connection once the accept-loop parent's active-child count reaches it; the IPv4/IPv6 peer is logged for every accepted connection. Because the listener forks one child per connection, the per-module `max connections` cap, the global `max connections per host` cap, and the auth-failure counter live in a fixed-size registry carved from an anonymous shared mapping (`daemon_limits.c`, `mmap(MAP_SHARED|MAP_ANONYMOUS)`) created by the parent before the accept loop, so every forked child shares the same counters (C11 atomics only — never a pthread lock, which can deadlock in a forked child). The parent reserves a registry slot per accepted connection and the child records the selected module and source IP once known; the parent's `SIGCHLD` handler reclaims the slot when the child dies (including `SIGKILL`) and re-derives the per-module and per-source occupancy counts from the surviving REGISTERED slots, so a child killed mid-registration cannot leak a count. The per-source table has a bounded lifetime: an entry with no live connection is reclaimed after its lockout expires or it has been idle (300 s); if the table is genuinely full the per-source cap/lockout fails open for new sources (per-module cap and ACLs still apply) with a rate-limited warning. The per-module cap (0 = unlimited) is enforced after the module lookup and before auth; per-source identity reuses the normalized numeric peer address (`utils_fd_peer_ip`, IPv4-mapped IPv6 collapsed to IPv4), and a trusted loopback peer (127.0.0.0/8 / `::1`, `utils_fd_peer_is_local`) is exempt from the per-source cap and the auth lockout because all local clients share one address (the per-module/global caps still apply). Clients behind a shared NAT/proxy address likewise share one per-source budget and lockout counter. A failed authentication increments the shared per-source failure count and, once `auth lockout threshold` (default 10; 0 disables) is reached, the source is refused for `auth lockout duration` seconds (default 300) before any challenge is sent, even when the next attempt is handled by a different forked child; a successful authentication clears the counter. On a failed authentication the per-connection child still sleeps the global `auth failure delay` (default 500 ms, 0 disables, capped at 5000) via `nanosleep`, rate-limiting online guessing without delaying a success. A missing registry (allocation failure) degrades to the global cap and host ACLs rather than refusing to start. - **Module selection & confinement:** the client requests a module with an rsync-style `host::module[/path]` destination. The module name crosses the wire as a trailing string on the config frame (bumping `PROTOCOL_VERSION` 2.14.0 → 2.15.0; the bump is required because the config-frame layout changed and the strict same-version handshake is what prevents a peer from desynchronizing on the new trailing field). The daemon looks the module up in ITS OWN config and uses the module's `path` as the authorized root through the exact same `configure_authorization` confinement the standalone server applies to `--destination-root` (`file_open_secure_parent`, `has_path_traversal`, `path_is_within`); the client never supplies the root, every client-chosen-ownership/super-user request is refused unless the module declares `client owner = yes` (the daemon's per-module opt-in, see below), and the operator `--no-super` veto forces super-user activities off for every daemon connection. The client's `/path` part is relative inside the module and is rejected if absolute or if it contains `..`. Unknown modules are refused before any data moves (the run fails cleanly at the config handshake). An absolute destination and a module request against a non-daemon server are also refused. -- **`client owner` (client-chosen-ownership opt-in):** by default a daemon module refuses every request that would let the client pick an owner or ask for super-user activities — `--numeric-ids`, `--chown`, `--usermap`/`--groupmap`, `--fake-super`, `--copy-as`, and an explicit `--super` — at the config handshake (before `STATUS_OK`), because a daemon has no per-module opt-in for client-chosen ownership and any anonymous client could otherwise force arbitrary owner ids inside the module root. `client owner = yes` opts a single module in, allowing those requests within that module's root (the standalone listener and the SSH `--stdio` server always honor them for their single operator-authorized root). Without the opt-in the daemon also forces super-user **device** activity off for that connection — char/block device-node creation (`--devices`) and `--write-devices` — even under the default `AUTO` mode, so a non-opted module can never be made to `mknod` or write a raw device; those entries are skipped (not refused) so an ordinary `-a` push still succeeds without device nodes. The opt-in does **not** lift the privilege requirement: `--copy-as` still needs a root receiver, and the operator `--no-super` veto still forces super-user activities off for every connection. The daemon logs a prominent startup warning for each `client owner = yes` module so the operator's deliberate choice is visible. +- **`client owner` (client-chosen-ownership opt-in):** by default a daemon module refuses every request that would let the client pick an owner or ask for super-user activities — `--numeric-ids`, `--chown`, `--usermap`/`--groupmap`, `--fake-super`, `--copy-as`, and an explicit `--super` — at the config handshake (before `STATUS_OK`), because a daemon has no per-module opt-in for client-chosen ownership and any anonymous client could otherwise force arbitrary owner ids inside the module root. `client owner = yes` opts a single module in, allowing those requests within that module's root (a root standalone listener and SSH `--stdio` server honor them for their single operator-authorized root only when started with `--allow-super`). Without the opt-in the daemon also forces super-user **device** activity off for that connection — char/block device-node creation (`--devices`) and `--write-devices` — even under the default `AUTO` mode, so a non-opted module can never be made to `mknod` or write a raw device; those entries are skipped (not refused) so an ordinary `-a` push still succeeds without device nodes. The opt-in does **not** lift the privilege requirement: `--copy-as` still needs a root receiver, and the operator `--no-super` veto still forces super-user activities off for every connection. The daemon logs a prominent startup warning for each `client owner = yes` module so the operator's deliberate choice is visible. - **`read only` safe default:** every network transfer FastSync currently supports is a push that writes under the module root, so a `read only` module refuses the connection (clear server log "module is read only"; the client exits non-zero, nothing is transferred). A future pull/list operation can be opened up when it exists; the knob is already stored. - **`auth users` (A7 SCRAM-SHA-256 authentication):** a module that declares `auth users` requires the client to present credentials. The config frame carries ONLY the username; the daemon answers an auth-required module with `STATUS_AUTH_CHALLENGE` (PBKDF2 iteration count, 16-byte salt, 32-byte server nonce), the client answers with `STATUS_AUTH_RESPONSE` (fresh 32-byte client nonce + a 32-byte ClientProof), and the daemon accepts only when the proof verifies **and** the username is **on the module's `auth users` list** and has a store entry, replying `STATUS_AUTH_OK` with a 32-byte ServerSignature the client verifies before proceeding. Verification is constant-time over fixed 32-byte keys (the compare runs even for a miss), username membership uses a constant-time full-length scan, and an unknown/off-list user still receives a challenge and runs the same math against a dummy verifier: a deterministic per-username salt (`HMAC-SHA256(store dummy key, username)`), the store-wide uniform iteration count and dummy keys. Re-probing the same unknown username therefore yields an identical salt and iteration count while a different username yields a different salt, so there is no user-enumeration or timing oracle. The daemon logs the username but **never the password, proof or keys**. A module WITHOUT `auth users` stays open (legitimate rsync configuration); credentials sent to such a module are ignored. Read-only is orthogonal: even a correctly authenticated push to a `read only` module is still refused (all FastSync network transfers write). Fail-closed policy: a daemon whose config declares `auth users` on any module refuses to start unless a credential store was given (`--password-file` and/or `--early-input`); a missing or empty store is never silently treated as "open". A failed handshake (missing credentials, unknown/off-list user, wrong proof or malformed data) yields a single generic `STATUS_AUTH_FAILED` and the daemon closes before any data moves. The dummy key is persisted in an owner-only `.dummykey` sidecar (auto-created on first load, mode 0600) so the dummy salt stays stable across daemon restarts, closing the restart-gated enumeration channel. The sidecar is secret material and must be protected like the credential store (owner-only 0600, included with the store in backups and rotation). It must be preserved across restarts for that guarantee; if it cannot be created (a process-substitution/FIFO store path such as `/dev/fd/N`, a read-only filesystem, a missing directory, or a create/write/fsync/link/fchmod failure), the daemon logs a warning and uses a transient per-run key, so unknown-user challenges change across restarts and the cross-restart guarantee does not hold for that deployment. One residual is accepted: the store iteration count is observable pre-auth by design, since the miss path must match a hit. **Transport policy (hardening A7-3/S1):** an auth-required module accepts credentials only when either (a) the connection is an encrypted, verified TLS connection whose client certificate matches `--client-cn`, or (b) the connection is plaintext from a loopback TCP peer **and** the operator explicitly passed `--allow-unauthenticated`. A remote plaintext peer, and a loopback plaintext peer without that flag, are refused at the config gate before any challenge is sent; `--allow-unauthenticated` never permits remote plaintext auth (remote peers still require verified TLS). Daemon modules are a `--daemon`-only feature — the SSH `--stdio` path never loads a daemon config and is not an auth transport for them. Because the loopback allowance trusts whichever peer the kernel reports as `127.0.0.1`, it assumes nothing relays remote connections to the daemon: a local TCP forwarder or TLS-terminating proxy in front of an auth-module listener makes remote clients appear as loopback and bypasses the mutual-TLS identity check, so do not front an auth-module listener with such a relay. - **Credential store format:** server `--password-file`/`--early-input` files are line-based `user:$fastsync$1$pbkdf2-sha256$$$$`, one per line (standard base64; 16-byte salt, 32-byte keys; `iters` in `[100000, 10000000]`, default 600000). Every entry in the resulting store must agree on `iters` (a store whose entries disagree, or where a layered `--early-input` disagrees with `--password-file`, is rejected). Generate lines with `fastsync-server --hash-credentials FILE [--iterations N]`; the emitted lines are secret material, so redirect them to an owner-only (mode 0600) file (the tool warns on stderr if stdout is a group/other-accessible regular file). Blank lines and lines starting with `#`/`;` are comments; the parser is strict (a malformed line fails the whole load, so a typo can never let a different set of users in). **The legacy `user:SHA256HEX` form is hard-rejected** with an actionable "legacy" error; there is no auto-upgrade, so a replayable bearer digest can never be loaded by a 2.19.0 daemon. The client `--password-file` holds `user:password` on its first meaningful line (the literal password, used only for the handshake then burned); keep both files readable only by their owner (mode 0600). Per-username wire length is bounded (256 chars) and every decoded salt/key length is validated. Loading the store also maintains an owner-only `.dummykey` sidecar (auto-created, mode 0600, exactly 32 bytes) holding the store-wide dummy key that shapes unknown-user challenges; persist it across daemon restarts so those challenges stay stable, and treat a sidecar with the wrong owner, a mode other than exactly 0600, the wrong size or the wrong type as a fatal load error (fail closed). If the sidecar cannot be created (e.g. a process-substitution store path such as `/dev/fd/N`, a read-only filesystem, a missing directory, or a create/write/fsync/link/fchmod failure), the daemon logs a warning and uses a transient per-run key, so the cross-restart stability guarantee does not hold there. @@ -662,7 +662,7 @@ now transmits targets (the prior behavior was broken/partial); its status moved | `--trust-sender` | Trust remote sender's file list | ✅ Implemented | Long-form-only, receiver-local policy that never crosses the wire. The receiver skips its redundant up-front re-validation of the incoming file list (empty/`..` path rejection and the escaping-symlink-target containment), trusting the sender instead of double-checking (fewer checks, faster, potentially unsafe, matching rsync). Off by default. The low-level fd-relative confinement primitives (`file_open_secure_parent`, the O_NOFOLLOW parent walk, leaf/destination confinement) are deliberately KEPT even under `--trust-sender`, so a hostile sender still cannot write or link outside the authorized root (see Phase-5 notes below) | | `--old-args` | Disable modern arg protection | ✅ Implemented | SSH-only; accepted for CLI compatibility but is now a **documented no-op**: FastSync always single-quote-escapes the remote server path and each `--remote-option` value (`ssh_build_remote_command`), so a metacharacter-bearing `--rsync-path` can never be interpreted by the remote shell. The flag no longer disables that quoting (the old raw-construction behavior was an injection foot-gun and is removed); the safety-relevant behavior is identical either way | | `--ignore-missing-args` | Ignore missing source args | ✅ Implemented | FastSync has a single source-root argument (which always exists), so the "explicitly requested source arguments" are the `--files-from` entries and the flags only ever apply there (inert without `--files-from`, like `-R`). Without the flag a listed-but-missing entry stays a hard pre-transfer error (nothing is transferred). With it each missing entry is skipped: nothing is sent for it, it never enters the keep-set, and the run succeeds for the rest — an all-missing non-empty list succeeds transferring nothing, matching rsync. `--dirs` + `--files-from` missing entries are skipped the same way. Every skipped entry is logged and a per-run warning names the count, so the handling is never a silent no-op. Divergences: an EMPTY `--files-from` file stays a hard error in every mode (no argument was requested at all; rsync likewise reports "no source files specified"); missing-arg skipping only applies to the pre-transfer list validation, so an entry that is present at preflight and vanishes mid-transfer still fails (matching rsync, whose flag "does not affect subsequent vanished-file errors"); `--no-ignore-missing-args` is not a supported negation | -| `--delete-missing-args` | Delete missing source args | ✅ Implemented | Implies `--ignore-missing-args` (order-independent) and additionally removes each missing entry's destination mirror receiver-side. The mirror is computed exactly like a present sibling's wire path: the bare relative entry under `-R`, otherwise the full source-mirror path below the destination root. rsync parity, verified against the man page: it does **not** imply `--delete` generally and is "independent of any other type of delete processing" — unrelated destination extras are untouched unless `--delete` is also present. Composition with `--delete` + timing: the exact-path deletions commit with the manifest, early for `--delete-before`/`--delete-during`, else only after a fully-successful transfer (delete-after/commit). A non-empty directory mirror is removed only when `--force` or `--delete` is in effect (otherwise it is left with a warning and the run continues, like rsync); an absent mirror is a no-op. An explicitly listed missing arg is a user request, not an excluded file: its deletion is never blocked by the filter-exclusion protection of excluded destination mirrors (a mirror sitting inside a filter-excluded directory is still removed). Safety/policy: gated by the server `--allow-delete` policy like `--delete`; the request paths cross the wire only in the delete-manifest frame and are confined by the same receiver validation as the keep-set (non-empty, relative, traversal-free, bounded by the per-section/per-frame manifest caps); the `--delay-updates` staging directory and basis snapshots are protected exactly as in the extras walker. Divergence: the missing-args deletions are not counted toward `--max-delete` (they are explicit per-path requests, not discovered extras). See the Phase-3 wire note below for the `PROTOCOL_VERSION` bump | +| `--delete-missing-args` | Delete missing source args | ✅ Implemented | Implies `--ignore-missing-args` (order-independent) and additionally removes each missing entry's destination mirror receiver-side. The mirror is computed exactly like a present sibling's wire path: the bare relative entry under `-R`, otherwise the full source-mirror path below the destination root. rsync parity, verified against the man page: it does **not** imply `--delete` generally and is "independent of any other type of delete processing" — unrelated destination extras are untouched unless `--delete` is also present. Composition with `--delete` + timing: the exact-path deletions commit with the manifest, early for `--delete-before`/`--delete-during`, else only after a fully-successful transfer (delete-after/commit). A non-empty directory mirror is removed only when `--force` or `--delete` is in effect (otherwise it is left with a warning and the run continues, like rsync); an absent mirror is a no-op. `--force` is deletion authority and is therefore gated by the server `--allow-delete` policy exactly like `--delete`/`--delete-missing-args`: without it the receiver clears the flag, so a client cannot use `--force` to recursively replace or remove a destination directory tree. An explicitly listed missing arg is a user request, not an excluded file: its deletion is never blocked by the filter-exclusion protection of excluded destination mirrors (a mirror sitting inside a filter-excluded directory is still removed). Safety/policy: gated by the server `--allow-delete` policy like `--delete`; the request paths cross the wire only in the delete-manifest frame and are confined by the same receiver validation as the keep-set (non-empty, relative, traversal-free, bounded by the per-section/per-frame manifest caps); the `--delay-updates` staging directory and basis snapshots are protected exactly as in the extras walker. Divergence: the missing-args deletions are not counted toward `--max-delete` (they are explicit per-path requests, not discovered extras). See the Phase-3 wire note below for the `PROTOCOL_VERSION` bump | ## 16. Batch Operations @@ -832,9 +832,9 @@ These are the last compatibility items and the closing phase toward rsync flag p **Wave E (LAST) — Privilege: `--super`/`--no-super` and `--copy-as=USER[:GROUP]` (✅ implemented).** FastSync adopts a **safe-subset + clear-refusal** privilege model: it never blind-elevates and never calls `setuid`/`seteuid`/`setgid`. All privileged operations remain fd-relative and confined below the authorized receive root. -`--super`/`--no-super` set a receiver-side tri-state `Config->super_mode` (`SUPER_MODE_AUTO`/`ON`/`OFF`). `privilege_super_permitted()` / `privilege_super_mode_permitted()` (src/shared/identity.c) return true for `ON` and `AUTO` (AUTO preserves FastSync's historical best-effort attempt, where the kernel refuses an unprivileged call and the caller skips it) and false only for `OFF`. The gate covers every super-user activity FastSync performs: ownership application (`identity_apply_ownership`/`_link`), char/block device-node creation (`file_save_special_to_disk`), writes into an existing device (`--write-devices`), and the `--fake-super` owner replay. Unprivileged FIFO creation is deliberately unaffected. `--super` does **not** imply `--numeric-ids`: ownership is applied only when an explicit identity policy (`--usermap`/`--groupmap`/`--chown`/`--numeric-ids`/`--copy-as`) is also given. `--no-super` suppresses those activities even for a root receiver. A non-root receiver given `--super` logs one warning at activation (`identity_set_active`); each confined attempt is then refused by the kernel and skipped, never aborting. The confinement floor is unchanged (`file_open_secure_parent`, `O_NOFOLLOW`, root/path checks). Operator control: the server CLI accepts `--no-super`, a veto that forces `OFF` for every connection, refuses any client `--copy-as`, and neutralizes an explicit `--super` (the connection is accepted but no super-user activity is attempted). On a daemon, a module that has not opted in with `client owner = yes` additionally has super-user device activity forced off (see the Daemon Mode notes). +`--super`/`--no-super` set a receiver-side tri-state `Config->super_mode` (`SUPER_MODE_AUTO`/`ON`/`OFF`). `privilege_super_permitted()` / `privilege_super_mode_permitted()` (src/shared/identity.c) return true for `ON` and `AUTO` (AUTO preserves FastSync's historical best-effort attempt, where the kernel refuses an unprivileged call and the caller skips it) and false only for `OFF`. The gate covers every super-user activity FastSync performs: ownership application (`identity_apply_ownership`/`_link`), char/block device-node creation (`file_save_special_to_disk`), writes into an existing device (`--write-devices`), and the `--fake-super` owner replay. Unprivileged FIFO creation is deliberately unaffected. `--super` does **not** imply `--numeric-ids`: ownership is applied only when an explicit identity policy (`--usermap`/`--groupmap`/`--chown`/`--numeric-ids`/`--copy-as`) is also given. `--no-super` suppresses those activities even for a root receiver. A non-root receiver given `--super` logs one warning at activation (`identity_set_active`); each confined attempt is then refused by the kernel and skipped, never aborting. The confinement floor is unchanged (`file_open_secure_parent`, `O_NOFOLLOW`, root/path checks). Operator control: the server CLI accepts `--no-super`, a veto that forces `OFF` for every connection, refuses any client `--copy-as`, and neutralizes an explicit `--super` (the connection is accepted but no super-user activity is attempted). A privileged (root) standalone/`--stdio` server instead defaults to `OFF` and requires the server-only `--allow-super` opt-in to attempt any super-user activity; a non-root server is unchanged. On a daemon, a module that has not opted in with `client owner = yes` additionally has super-user device activity forced off (see the Daemon Mode notes). -`--copy-as=USER[:GROUP]` is the safe subset. FastSync's receiver is multithreaded, so a real credential switch is unsafe; instead the receiver forces the ownership of **every entry it writes** — regular files, symlinks, directories (including implicitly-created parents), and special nodes — to the resolved target ids through the confined fd-relative identity path. USER is resolved on the client (name, `@N`/bare N, or `*` = client euid); when `:GROUP` is omitted the user's primary gid is used (falling back to `gid == uid` for a numeric id with no local passwd entry). It requires a privileged (root) receiver: an unprivileged receiver refuses the whole transfer at the config handshake, before `STATUS_OK`, so no data is ever written with the wrong ownership. A `--copy-as` chown failure on a capability-restricted root is logged at ERROR (never silently downgraded). `--copy-as` implies metadata (`--no-preserve` is rejected) and `--fake-super` cannot override it. Daemon policy: a `--daemon` receiver refuses **every** client-chosen-ownership / super-user request — `--numeric-ids`, `--chown`, `--usermap`/`--groupmap`, `--fake-super`, `--copy-as`, and explicit `--super` — unless the selected module opts in with `client owner = yes`; without that per-module opt-in any client could force arbitrary ownership inside the module root (the standalone listener and the SSH-launched `--stdio` server, which each serve one operator-authorized root, honor these requests). A `--copy-as` chown failure on a capability-restricted root marks the entry as failed rather than reporting success with the wrong owner. +`--copy-as=USER[:GROUP]` is the safe subset. FastSync's receiver is multithreaded, so a real credential switch is unsafe; instead the receiver forces the ownership of **every entry it writes** — regular files, symlinks, directories (including implicitly-created parents), and special nodes — to the resolved target ids through the confined fd-relative identity path. USER is resolved on the client (name, `@N`/bare N, or `*` = client euid); when `:GROUP` is omitted the user's primary gid is used (falling back to `gid == uid` for a numeric id with no local passwd entry). It requires a privileged (root) receiver: an unprivileged receiver refuses the whole transfer at the config handshake, before `STATUS_OK`, so no data is ever written with the wrong ownership. A `--copy-as` chown failure on a capability-restricted root is logged at ERROR (never silently downgraded). `--copy-as` implies metadata (`--no-preserve` is rejected) and `--fake-super` cannot override it. Daemon policy: a `--daemon` receiver refuses **every** client-chosen-ownership / super-user request — `--numeric-ids`, `--chown`, `--usermap`/`--groupmap`, `--fake-super`, `--copy-as`, and explicit `--super` — unless the selected module opts in with `client owner = yes`; without that per-module opt-in any client could force arbitrary ownership inside the module root (a root standalone listener or SSH-launched `--stdio` server, which each serve one operator-authorized root, honors these requests only when started with `--allow-super`). A `--copy-as` chown failure on a capability-restricted root marks the entry as failed rather than reporting success with the wrong owner. **Wire:** two trailing config-frame blocks after the `--iconv` spec, in fixed order — `send_privilege_options`/`receive_privilege_options` (one `super_mode` int, validated `0..2`), then `send_copy_as_options`/`receive_copy_as_options` (presence int + two int32 ids, validated `>= 0`, with `copy_as_set ⇒ use_metadata`). `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0**. **Divergences from rsync:** rsync's `--super` elevates the receiver and `--copy-as` actually switches its credentials; FastSync never elevates and only permits/forwards confined attempts, and `--copy-as` forces ownership rather than switching identity. diff --git a/src/server/server.c b/src/server/server.c index 708fe7f..b977023 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -37,6 +37,12 @@ static bool allow_unauthenticated; * root), so no super-user activity is attempted and any client --copy-as is * refused. Set once in main before the accept loop / stdio handler. */ static bool server_no_super; +/* --allow-super: standalone/--stdio opt-in that preserves the historical + * permissive super mode for a root receiver. When false, a privileged + * standalone receiver forces SUPER_MODE_OFF for every connection (C3), so a + * client cannot make it create device nodes / write raw devices / apply + * client-chosen ownership. */ +static bool server_allow_super; static const char* required_client_cn; /* --iconv CONVERT_SPEC the server was itself started with (borrowed argv * pointer). Its LOCAL half may override the local charset the client assumed; @@ -191,8 +197,12 @@ static bool tls_client_identity_allowed(SSL* ssl) { int length = X509_NAME_get_text_by_NID(X509_get_subject_name(certificate), NID_commonName, common_name, sizeof(common_name)); size_t required_length = strlen(required_client_cn); - bool allowed = length >= 0 && (size_t)length == required_length && - required_length < sizeof(common_name) && + /* X509_NAME_get_text_by_NID truncates an over-long CN to the buffer size; a + * returned length at the buffer bound means the CN was silently shortened, so + * a required-name prefix could be matched by a longer CN with extra suffix. + * Reject any result that reached the bound. */ + bool allowed = length >= 0 && (size_t)length < sizeof(common_name) - 1 && + (size_t)length == required_length && required_length < sizeof(common_name) && credentials_secure_equal(common_name, required_client_cn, required_length); X509_free(certificate); return allowed; @@ -592,6 +602,20 @@ static const char* server_module_gate(const Config* config, void* context) { if (gate_ctx) gate_ctx->super_mode_override = SUPER_MODE_OFF; } + /* C3: a privileged (root) STANDALONE/--stdio receiver defaults to + * SUPER_MODE_OFF. Without this a client --devices/--write-devices/--super + * would let a root server create arbitrary device nodes and write raw devices, + * and client-chosen ownership (--numeric-ids/--chown/--usermap/--groupmap) + * would be applied, with no operator opt-in. The operator must pass + * --allow-super to restore the historical permissive behavior; an + * unprivileged receiver is unaffected (the kernel refuses the confined + * attempts) and the daemon path keeps its per-module `client owner = yes` + * gate. */ + if (g_daemon_conf == NULL && geteuid() == 0 && !server_allow_super) { + effective.super_mode = SUPER_MODE_OFF; + if (gate_ctx) + gate_ctx->super_mode_override = SUPER_MODE_OFF; + } /* --copy-as (P7 Wave E, protocol 2.18.0): FastSync's safe subset forces the ownership of every written entry to the requested ids, which needs a privileged (root) receiver. An unprivileged receiver REFUSES the whole @@ -751,6 +775,13 @@ void handler(int file_descriptor) { goto done; } config->use_delete = config->use_delete && allow_delete; + /* --force (receiver-side) is deletion authority too: it lets an incoming + * regular file recursively remove a non-empty destination directory tree, and + * lets --delete-missing-args remove a non-empty directory mirror. Without + * the operator's --allow-delete it must be inert, exactly like --delete and + * --delete-missing-args, so a client cannot use --force to bypass the delete + * policy. */ + config->force_delete = config->force_delete && allow_delete; /* --iconv (protocol 2.16.0): install the receiver-side wire->local conversion now that the client's full CONVERT_SPEC has been received and validated, before any received file name is decoded. The server's own --iconv (if @@ -1010,6 +1041,11 @@ static void print_server_usage(void) { printf(" --no-super Operator veto: never attempt super-user activities\n"); printf(" (ownership, device nodes) even as root, and refuse\n"); printf(" any client --copy-as/--super request\n"); + printf(" --allow-super Standalone/--stdio only: keep super-user activities\n"); + printf(" enabled for a root receiver. Without it a root\n"); + printf(" standalone server forces SUPER_MODE_OFF, so client\n"); + printf(" --devices/--write-devices/--super and ownership\n"); + printf(" requests are refused/skipped. No effect when not root\n"); printf(" --iconv=LOCAL[,REMOTE] Declare this server's LOCAL charset for file-name\n"); printf(" conversion: received names are translated to this\n"); printf(" charset (the wire charset still comes from the\n"); @@ -1134,6 +1170,7 @@ int main(int argc, char* argv[]) { trust_sender = opts.trust_sender; allow_unauthenticated = opts.allow_unauthenticated; server_no_super = opts.no_super; + server_allow_super = opts.allow_super; server_iconv_spec = opts.iconv_spec; signal(SIGINT, cleanup); signal(SIGTERM, cleanup); diff --git a/src/server/server_cli.c b/src/server/server_cli.c index 794a97a..3cdaa75 100644 --- a/src/server/server_cli.c +++ b/src/server/server_cli.c @@ -179,6 +179,8 @@ int server_cli_parse(int argc, char* argv[], ServerCliOptions* opts, char* err, opts->trust_sender = true; } else if (arg_is(argv[i], "--no-super")) { opts->no_super = true; + } else if (arg_is(argv[i], "--allow-super")) { + opts->allow_super = true; } else if (arg_is(argv[i], "--allow-unauthenticated")) { opts->allow_unauthenticated = true; } else if (arg_has_value(argv[i], "--iconv", &inline_value)) { @@ -259,6 +261,16 @@ int server_cli_parse(int argc, char* argv[], ServerCliOptions* opts, char* err, set_error(err, err_size, "--hash-credentials cannot be combined with --daemon or --stdio"); return -1; } + if (opts->allow_super && opts->no_super) { + set_error(err, err_size, "--allow-super and --no-super are mutually exclusive"); + return -1; + } + if (opts->allow_super && opts->daemon_mode) { + set_error(err, err_size, + "--allow-super is for a standalone/--stdio server; daemon modules opt in per " + "module with 'client owner = yes'"); + return -1; + } if (opts->hash_iterations_set && opts->hash_credentials_file == NULL) { set_error(err, err_size, "--iterations requires --hash-credentials"); return -1; diff --git a/src/server/server_cli.h b/src/server/server_cli.h index ab19a7f..8965def 100644 --- a/src/server/server_cli.h +++ b/src/server/server_cli.h @@ -45,6 +45,14 @@ typedef struct ServerCliOptions { * device-node creation) even when running as root. Applies to --stdio and * --daemon alike; also makes the server refuse any client --copy-as. */ bool no_super; /* --no-super */ + /* --allow-super: standalone/--stdio only opt-in that keeps the historical + * permissive behavior for a PRIVILEGED (root) receiver. Without it a root + * standalone server forces SUPER_MODE_OFF, so a client --devices / + * --write-devices / --super / ownership request cannot make it create device + * nodes, write raw devices, or apply client-chosen ownership. Non-root + * receivers are unaffected (the kernel refuses the confined attempts). The + * daemon path instead uses the per-module `client owner = yes` opt-in. */ + bool allow_super; /* --allow-super */ /* --iconv=CONVERT_SPEC: the server's own LOCAL charset declaration. The * client's full spec rides the wire config frame anyway; when the server is * started with its own --iconv, its LOCAL half overrides the local charset diff --git a/tests/conftest.py b/tests/conftest.py index d4dd89d..e5619ba 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -16,7 +16,13 @@ def shared_server(): Under pytest-xdist this session fixture is instantiated once per worker process, so each worker gets its own server on an ephemeral port.""" server = ServerManager() - server.start() + # --allow-super keeps the historical permissive super mode for a root + # receiver: the integration suite's root-only ownership/device/copy-as tests + # exercise that opted-in configuration. The secure default (a root + # standalone server without --allow-super forces SUPER_MODE_OFF) is covered + # explicitly by TestStandaloneSuperDefault in test_features.py. Non-root + # runs are unaffected by the flag. + server.start(extra_args=["--allow-super"]) yield server server.stop() diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 96372cb..381fd06 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -1561,8 +1561,33 @@ class TestDelete: assert not missing, f"Missing: {missing}" assert not mismatches, f"Mismatch: {mismatches}" - -class TestProgress: + @pytest.mark.ci + def test_force_cannot_replace_directory_without_allow_delete(self): + """C2: --force is deletion authority (an incoming file may recursively + remove a non-empty destination directory tree). A server started without + --allow-delete must clear it, so the operator's delete policy cannot be + bypassed with --force.""" + source = os.path.join(TEST_DATA_DIR, "force_src") + dest = os.path.join(TEST_DATA_DIR, "force_dst") + clean_dir(source) + clean_dir(dest) + with open(os.path.join(source, "blocker"), "wb") as f: + f.write(b"incoming file\n") + received = get_dest_received_dir(dest, source) + blocker = os.path.join(received, "blocker") + os.makedirs(blocker) + nested = os.path.join(blocker, "nested.txt") + with open(nested, "w") as f: + f.write("survivor") + # Deliberately NO --allow-delete. + server = ServerManager() + server.start() + try: + run_client(source, dest, flags=["--force"], port=server.port) + finally: + server.stop() + assert os.path.isdir(blocker), "unauthorized --force removed a destination directory" + assert os.path.exists(nested), "unauthorized --force removed a nested file" def test_progress_output(self, shared_server): clean_dir(DEST_DIR) result, dur = run_client( @@ -4578,6 +4603,61 @@ class TestSuperPrivilege: f"--no-super must suppress fake-super's owner replay: uid={st.st_uid} gid={st.st_gid}" +class TestStandaloneSuperDefault: + """C3: a privileged (root) STANDALONE server without --allow-super forces + SUPER_MODE_OFF, so a client cannot make it create device nodes, write raw + devices, apply ownership, or use --copy-as. The shared_server fixture opts in + with --allow-super to keep the historical behavior available to the existing + root-only tests; these tests start their own un-opted server.""" + + @pytest.mark.ci + def test_copy_as_refused_without_allow_super(self): + """--copy-as is a client-chosen-ownership request and must be refused by + a standalone server that did not opt in with --allow-super (on a non-root + receiver it is refused for lack of privilege either way).""" + source = os.path.join(TEST_DATA_DIR, "super_default_src") + dest = os.path.join(TEST_DATA_DIR, "super_default_dst") + clean_dir(source) + clean_dir(dest) + with open(os.path.join(source, "f.txt"), "wb") as f: + f.write(b"no copy-as\n") + server = ServerManager() + server.start() # deliberately no --allow-super + try: + result, _ = run_client(source, dest, + flags=["--preserve", "--copy-as=@65534:@65534"], + port=server.port) + finally: + server.stop() + assert result.returncode != 0, ( + "standalone server accepted --copy-as without --allow-super" + ) + + @pytest.mark.skipif(os.geteuid() != 0, reason="root can create the source device node") + def test_devices_skipped_without_allow_super(self): + """Root standalone server without --allow-super must skip device-node + creation even for a client --devices request (the run still succeeds and + the regular file transfers).""" + source = os.path.join(TEST_DATA_DIR, "super_default_dev_src") + dest = os.path.join(TEST_DATA_DIR, "super_default_dev_dst") + clean_dir(source) + clean_dir(dest) + with open(os.path.join(source, "plain.txt"), "wb") as f: + f.write(b"regular\n") + os.mknod(os.path.join(source, "null"), stat.S_IFCHR | 0o666, os.makedev(1, 3)) + server = ServerManager() + server.start() # deliberately no --allow-super + try: + result, _ = run_client(source, dest, flags=["--devices"], port=server.port) + finally: + server.stop() + assert result.returncode == 0, f"exit {result.returncode}: {(result.stderr or '')[:200]}" + received = get_dest_received_dir(dest, source) + assert not os.path.lexists(os.path.join(received, "null")), ( + "root standalone server created a device node without --allow-super" + ) + + class TestHardLinks: """-H/--hard-links: source files sharing an inode are re-created as hard links to one another on the destination (dedup preserved, first copy diff --git a/tests/test_server_cli.c b/tests/test_server_cli.c index c057ebd..04f031b 100644 --- a/tests/test_server_cli.c +++ b/tests/test_server_cli.c @@ -32,6 +32,28 @@ static void test_server_cli_defaults() { EXPECT_FALSE(opts.allow_delete); EXPECT_FALSE(opts.allow_unauthenticated); EXPECT_FALSE(opts.no_super); + EXPECT_FALSE(opts.allow_super); + server_cli_options_free(&opts); +} + +/* C3: --allow-super is the standalone/--stdio opt-in for a privileged receiver; + * it never combines with --no-super, and daemon modules use their own per-module + * `client owner = yes` opt-in instead. */ +static void test_server_cli_allow_super() { + const char* args[] = {"fastsync-server", "--allow-super", "--destination-root", "/srv"}; + ServerCliOptions opts; + EXPECT_EQ_INT(parse_ok(args, 4, &opts), 0); + EXPECT_TRUE(opts.allow_super); + server_cli_options_free(&opts); + + char err[256]; + const char* a1[] = {"s", "--allow-super", "--no-super"}; + EXPECT_EQ_INT(server_cli_parse(3, (char**)a1, &opts, err, sizeof(err)), -1); + EXPECT_TRUE(strstr(err, "mutually exclusive") != NULL); + + const char* a2[] = {"s", "--daemon", "--config=/tmp/x.conf", "--allow-super"}; + EXPECT_EQ_INT(server_cli_parse(4, (char**)a2, &opts, err, sizeof(err)), -1); + EXPECT_TRUE(strstr(err, "client owner") != NULL); server_cli_options_free(&opts); } @@ -224,5 +246,6 @@ void test_server_cli() { test_server_cli_password_and_early_input(); test_server_cli_password_requires_daemon(); test_server_cli_no_super(); + test_server_cli_allow_super(); test_server_cli_help(); } From a2370433b2b13ae0bd557de369ab95d0a5ea89cd Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:19:26 +0200 Subject: [PATCH 06/16] fix(receiver): non-blocking receiver opens, inplace type gate, dry-run/B4/B5/B6 Address confirmed receiver security findings B1-B6: B1 (HIGH): add O_NONBLOCK to the three receiver read-opens that opened an existing destination/basis entry before the S_ISREG gate (incremental_check_open_destination, basis_open_regular, hardlink_read_source) so a client-planted FIFO can no longer block the receive thread forever while the post-open type gate still rejects it. B2 (HIGH/MED): --inplace now fstatat(AT_SYMLINK_NOFOLLOW)-probes the target and refuses any existing non-regular entry, opens with O_NONBLOCK, and re-checks S_ISREG on the opened fd. This stops a FIFO from hanging the open and stops a char/block device from being written directly (bypassing --write-devices). B3 (MED): under --dry-run the incremental quick-skip no longer reads/hashes the destination file for --checksum/--delta; it decides from metadata only and reports would-transfer when the comparison is inconclusive, closing the read-only-module content-hash oracle. B4 (LOW): xattr_name_appliable() now gates the two system.posix_acl_* names on preserve_acls (--acls), not the derived use_xattrs (--xattrs OR --acls). The receiver drops (never applies) ACL entries when -A was not negotiated while keeping user.* working for -X. B5 (INFO): receive_manifest_section() charges a per-entry overhead against MAX_MANIFEST_BYTES and the aggregate entry count across all three sections is capped at MAX_MANIFEST_ENTRIES. B6 (MED): data_charge_session() reserves decompressed/chunk-copy bytes against the owning ProtocolSession (MAX_CONNECTION_MEMORY) and records them on the Data so data_destroy() releases them via the Data.owner path. Applied to the whole-file/append/delta decompression sites and chunk_deserialize() per-file copies; a missing session owner degrades to the previous uncharged behavior. Tests: FIFO destination/basis non-hang (with alarm), --inplace FIFO/device refusal, dry-run no-read oracle test plus updated metadata-only dry-run tests, ACL-without--acls drop, manifest total-entry cap, and chunk session charging. --- src/shared/chunk.c | 46 ++++++++ src/shared/chunk.h | 11 ++ src/shared/file.c | 30 ++++- src/shared/file_receive.c | 92 +++++++++++++--- src/shared/xattr.c | 61 ++++++++--- src/shared/xattr.h | 15 ++- tests/fuzz/fuzz_xattr_block.c | 22 ++-- tests/integration/test_features.py | 43 +++++++- tests/test_chunk.c | 48 ++++++++ tests/test_file.c | 91 ++++++++++++++++ tests/test_server.c | 169 +++++++++++++++++++++++++++++ tests/test_xattr.c | 70 ++++++++++-- 12 files changed, 635 insertions(+), 63 deletions(-) diff --git a/src/shared/chunk.c b/src/shared/chunk.c index 1f3be3a..ab0fb4c 100644 --- a/src/shared/chunk.c +++ b/src/shared/chunk.c @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -20,6 +21,33 @@ #define MAX_FILE_DATA_SIZE (64ULL * 1024 * 1024) #define MAX_FILES_PER_CHUNK 65536U +/* Reserve `charge` against `session`'s connection budget. This mirrors the + static protocol_reserve_memory() in protocol.c: the receive-side call sites + only have the Data.owner pointer (a ProtocolSession*), and protocol.c is out + of scope for this fix, so the same atomic CAS accounting is reproduced here. + The matching release always goes through data_destroy()'s Data.owner path. */ +static bool chunk_session_reserve(ProtocolSession* session, size_t charge) { + unsigned long long allocated = atomic_load(&session->total_allocated_bytes); + while (true) { + if (allocated > MAX_CONNECTION_MEMORY || + (unsigned long long)charge > MAX_CONNECTION_MEMORY - allocated) + return false; + if (atomic_compare_exchange_weak(&session->total_allocated_bytes, &allocated, + allocated + (unsigned long long)charge)) + return true; + } +} + +bool data_charge_session(Data* data, ProtocolSession* session, size_t charge) { + if (!data || charge == 0 || session == NULL) + return true; + if (!chunk_session_reserve(session, charge)) + return false; + data->owner = session; + data->protocol_charge = charge; + return true; +} + Chunk* chunk_create(File** items, int element_count) { if (element_count < 0 || (element_count > 0 && items == NULL)) return NULL; @@ -370,6 +398,15 @@ Chunk* chunk_deserialize(Data* data, bool use_metadata) { Data* replacement = data_create(file_data, file_data_size); if (replacement == NULL) goto error; + /* Charge the retained per-file copy to the connection budget (when the + inbound chunk carries an owning session) so the queued copies are not + held outside MAX_CONNECTION_MEMORY (B6). A NULL owner (e.g. a local + batch apply) leaves the copy uncharged. */ + if (!data_charge_session(replacement, data->owner, allocation_size)) { + log_message(LOG_LEVEL_ERROR, "Per-connection memory limit exceeded for chunk file data"); + data_destroy(replacement); + goto error; + } data_destroy(file->data); file->data = replacement; data_pointer += file_data_size; @@ -466,12 +503,21 @@ Chunk* receive_chunk_data(int fd, const Config* config) { } Data* data_to_process = chunk_data; if (config->use_compression) { + /* Preserve the inbound session across decompression so the (larger) + decompressed chunk is charged to the same connection budget; the + compressed buffer's own charge is released by data_destroy below. */ + ProtocolSession* owner = chunk_data->owner; data_to_process = data_decompress_limited(chunk_data, MAX_CHUNK_SIZE); data_destroy(chunk_data); if (data_to_process == NULL) { log_message(LOG_LEVEL_ERROR, "Failed to decompress chunk"); return NULL; } + if (!data_charge_session(data_to_process, owner, data_to_process->size)) { + log_message(LOG_LEVEL_ERROR, "Per-connection memory limit exceeded for decompressed chunk"); + data_destroy(data_to_process); + return NULL; + } } // Reject chunks larger than the maximum allowed size to prevent OOM. diff --git a/src/shared/chunk.h b/src/shared/chunk.h index 2c04e85..65202ad 100644 --- a/src/shared/chunk.h +++ b/src/shared/chunk.h @@ -23,4 +23,15 @@ Data* chunk_compress_with_threads(Chunk* chunk, int compression_level, bool use_ int compression_threads); Chunk* receive_chunk_data(int fd, const Config* config); +/* Charge `charge` retained bytes of `data` against `session`'s per-connection + * budget (MAX_CONNECTION_MEMORY), mirroring the protocol layer's accounting, and + * record them on `data` so data_destroy() returns the charge through the + * Data.owner path. Returns false (leaving `data` uncharged) when the ceiling + * would be exceeded. A NULL/zero-size charge or a NULL session is a no-op + * success. The receive-side decompression and chunk-copy paths know the owning + * session only through the Data.owner of the buffer they are processing, so + * this is the entry point that lets them participate in the connection budget + * without a session handle (B6). */ +bool data_charge_session(Data* data, ProtocolSession* session, size_t charge); + #endif diff --git a/src/shared/file.c b/src/shared/file.c index 9d5aa23..47b13ab 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -890,13 +890,35 @@ static bool file_to_disk_secure_impl(const char* path, const void* data, if (inplace) { /* --inplace writes directly into the destination; a scratch --temp-dir does not apply and must never redirect these writes. */ - fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW, 0644); + /* Type gate BEFORE opening: an existing destination entry that is not a + regular file (FIFO, socket, char/block device, directory) must never be + opened for writing. Opening a FIFO would block the receive thread + forever and writing into a device would bypass the --write-devices / + super-mode gate (a client-controlled device write). fstatat with + AT_SYMLINK_NOFOLLOW does not follow a symlink and does not block. */ + struct stat pre_stat; + if (fstatat(dirfd, leaf, &pre_stat, AT_SYMLINK_NOFOLLOW) == 0 && !S_ISREG(pre_stat.st_mode)) { + close(dirfd); + free(leaf); + return false; + } + /* O_NONBLOCK: a no-op for a regular file, but a raced-in FIFO cannot block + the open before the post-open S_ISREG re-check rejects it. */ + fd = openat(dirfd, leaf, O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK, 0644); if (fd >= 0) { struct stat destination_stat; + /* Re-check the opened descriptor: a concurrent replacement between the + fstatat probe and the open (or a device/FIFO raced in) must never be + written through. */ + if (fstat(fd, &destination_stat) != 0 || !S_ISREG(destination_stat.st_mode)) { + close(fd); + close(dirfd); + free(leaf); + return false; + } bool newer = false; - if (update && metadata && fstat(fd, &destination_stat) == 0 && - S_ISREG(destination_stat.st_mode)) { - newer = stat_is_newer(&destination_stat, metadata); + if (update && metadata && stat_is_newer(&destination_stat, metadata)) { + newer = true; } if (newer) { ok = true; diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index fc2bd44..8e3c2c7 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -12,6 +12,7 @@ #include "array_list.h" #include "charset.h" #include "chmod.h" +#include "chunk.h" #include "compression.h" #include "config.h" #include "data.h" @@ -27,6 +28,11 @@ #define MAX_SERVER_DELETE_COUNT 100000U #define MAX_FILE_DATA_SIZE MAX_RECEIVE_WHOLE_FILE_SIZE +/* Retained cost of one delete-manifest entry beyond its path bytes: the + ArrayList pointer slot plus an approximate malloc header/rounding for the + heap copy. Charged against MAX_MANIFEST_BYTES so a frame full of tiny paths + cannot retain far more than the byte budget (B5). */ +#define MANIFEST_ENTRY_OVERHEAD (sizeof(char*) + 16) bool file_save_to_disk(const char* root_directory, const File* file, const Config* config) { return file_save_to_disk_full(root_directory, file, config) != FILE_SAVE_ERROR; @@ -127,7 +133,10 @@ static bool hardlink_read_source(const char* path, void** out_buf, unsigned long *source_absent = errno == ENOENT || errno == ENOTDIR; return false; } - int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + /* O_NONBLOCK is a no-op for a regular file but makes openat() fail/succeed + immediately for a client-planted FIFO instead of blocking the receive + thread forever; the post-open S_ISREG gate below is the actual type check. */ + int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); int saved_errno = errno; free(leaf); close(parent_fd); @@ -944,7 +953,7 @@ static bool receive_file_xattrs(File* file, int fd, const Config* config) { if (!config->use_xattrs) return true; int xok = 0; - FileXattrList* list = xattr_receive(fd, &xok); + FileXattrList* list = xattr_receive(fd, &xok, config->preserve_acls); if (!xok) { xattr_list_free(list); return false; @@ -1009,6 +1018,7 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ !compression_should_skip_with_suffixes( check_path, config->skip_compress_suffixes, config->skip_compress_set ? config->skip_compress_count : -1)) { + ProtocolSession* owner = delta_data->owner; raw_delta = data_decompress_limited(delta_data, MAX_RECEIVE_WHOLE_FILE_SIZE); data_destroy(delta_data); if (!raw_delta) { @@ -1017,6 +1027,15 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ *failed = true; return NULL; } + /* Charge the decompressed delta to the connection budget (the paired + wire buffer's charge was just released). */ + if (!data_charge_session(raw_delta, owner, raw_delta->size)) { + data_destroy(raw_delta); + free(old_data); + delta_signature_destroy(sig); + *failed = true; + return NULL; + } } Delta* delta = delta_deserialize(raw_delta); @@ -1131,12 +1150,20 @@ static File* receive_delta_file(int fd, const Config* config, const char* check_ file->path, config->skip_compress_suffixes, config->skip_compress_set ? config->skip_compress_count : -1)) { Data* uncompressed = data_decompress_limited(file_data, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = file_data->owner; data_destroy(file_data); if (uncompressed == NULL) { file_destroy(file); *failed = true; return NULL; } + if (!data_charge_session(uncompressed, owner, uncompressed->size)) { + data_destroy(uncompressed); + file_destroy(file); + send_status(fd, STATUS_ERROR); + *failed = true; + return NULL; + } if (uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(uncompressed); file_destroy(file); @@ -1193,7 +1220,9 @@ static bool basis_open_regular(const char* path, unsigned long long expected_siz int parent_fd = file_open_secure_parent(path, &leaf, false); if (parent_fd < 0) return false; - int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + /* O_NONBLOCK: a client-planted FIFO must not block the receiver's openat() + forever; the fstat()/S_ISREG gate below rejects it immediately. */ + int fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); free(leaf); close(parent_fd); if (fd < 0) @@ -1637,11 +1666,17 @@ static File* receive_full_file(int fd, const Config* config, const char* path) { config->skip_compress_set ? config->skip_compress_count : -1)) { Data* uncompressed = data_decompress_limited(file_data, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = file_data->owner; data_destroy(file_data); if (uncompressed == NULL) { file_destroy(file); return NULL; } + if (!data_charge_session(uncompressed, owner, uncompressed->size)) { + data_destroy(uncompressed); + file_destroy(file); + return NULL; + } if (uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(uncompressed); file_destroy(file); @@ -1776,7 +1811,9 @@ static IncrementalCheckOutcome incremental_check_open_destination(IncrementalChe char* leaf = NULL; int parent_fd = file_open_secure_parent(full_path, &leaf, false); if (parent_fd >= 0) { - state->old_fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); + /* O_NONBLOCK: an existing FIFO at the destination must not block this + openat(); the S_ISREG gate below rejects the non-regular entry. */ + state->old_fd = openat(parent_fd, leaf, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); free(leaf); close(parent_fd); state->has_old_file = state->old_fd >= 0 && fstat(state->old_fd, &state->old_st) == 0 && @@ -1815,7 +1852,15 @@ static IncrementalCheckOutcome incremental_check_quick_skip(IncrementalCheckStat bool try_delta = config->use_delta && !config->whole_file && has_old_file && delta_should_attempt(old_size, state->check_size, config->delta_max_file_size); bool checksum_needs_read = size_equal && !config->ignore_times && config->checksum; - bool need_old_data = checksum_needs_read || try_delta; + /* --dry-run must never read the destination file's CONTENTS: a client could + otherwise use `--dry-run --checksum` against a read-only module as a + 1-bit content oracle (hash match / mismatch) and force arbitrary reads. + Decide from metadata alone; when metadata is inconclusive (checksum or + delta would have required the body) report would-transfer. The real + (non-dry-run) behavior below is unchanged. */ + bool need_old_data = !config->dry_run && (checksum_needs_read || try_delta); + if (config->dry_run) + try_delta = false; *out_try_delta = try_delta; if (need_old_data && has_old_file && old_size > 0 && old_size <= MAX_RECEIVE_WHOLE_FILE_SIZE && @@ -1836,7 +1881,12 @@ static IncrementalCheckOutcome incremental_check_quick_skip(IncrementalCheckStat } bool match = false; - if (checksum_needs_read) { + if (config->dry_run) { + /* Metadata-only decision: a size match plus a matching mtime is treated as + up to date; --checksum/--delta cannot be verified without reading, so an + otherwise inconclusive comparison is a would-transfer. */ + match = size_equal && !config->ignore_times && (config->size_only || match_by_metadata); + } else if (checksum_needs_read) { uint8_t old_digest[CHECKSUM_MAX_DIGEST_LEN]; size_t old_len = 0; bool hashed = checksum_digest((ChecksumAlgo)config->checksum_algo, config->checksum_seed, @@ -2050,7 +2100,7 @@ static IncrementalCheckOutcome incremental_check_try_append_resume(IncrementalCh } if (config->use_xattrs) { int xok = 0; - append_xattrs = xattr_receive(fd, &xok); + append_xattrs = xattr_receive(fd, &xok, config->preserve_acls); if (!xok) { xattr_list_free(append_xattrs); return INCREMENTAL_ERROR; @@ -2066,11 +2116,17 @@ static IncrementalCheckOutcome incremental_check_try_append_resume(IncrementalCh config->skip_compress_set ? config->skip_compress_count : -1)) { Data* uncompressed = data_decompress_limited(tail, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = tail->owner; data_destroy(tail); if (uncompressed == NULL) { xattr_list_free(append_xattrs); return INCREMENTAL_ERROR; } + if (!data_charge_session(uncompressed, owner, uncompressed->size)) { + data_destroy(uncompressed); + xattr_list_free(append_xattrs); + return INCREMENTAL_ERROR; + } if (uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(uncompressed); xattr_list_free(append_xattrs); @@ -2301,11 +2357,17 @@ File* file_receive(const Config* config, int file_descriptor) { config->skip_compress_set ? config->skip_compress_count : -1)) { Data* file_data_uncompressed = data_decompress_limited(file_data, MAX_RECEIVE_WHOLE_FILE_SIZE); + ProtocolSession* owner = file_data->owner; data_destroy(file_data); if (file_data_uncompressed == NULL) { file_destroy(file); return NULL; } + if (!data_charge_session(file_data_uncompressed, owner, file_data_uncompressed->size)) { + data_destroy(file_data_uncompressed); + file_destroy(file); + return NULL; + } if (file_data_uncompressed->size > MAX_FILE_DATA_SIZE) { data_destroy(file_data_uncompressed); file_destroy(file); @@ -2694,19 +2756,21 @@ File* file_receive_special(int file_descriptor) { manifest). Returns an owned DeleteManifest, or NULL after sending STATUS_ERROR when the frame is malformed (bad count, empty/absolute path, path traversal, or an aggregate size beyond MAX_MANIFEST_BYTES). */ -static bool receive_manifest_section(int fd, ArrayList* list, size_t* manifest_bytes) { +static bool receive_manifest_section(int fd, ArrayList* list, size_t* manifest_bytes, + size_t* manifest_entries) { int count; if (!receive_int(fd, &count)) { send_status(fd, STATUS_ERROR); return false; } - if (count < 0 || count > MAX_MANIFEST_ENTRIES) { + if (count < 0 || count > MAX_MANIFEST_ENTRIES || + (size_t)count > MAX_MANIFEST_ENTRIES - *manifest_entries) { send_status(fd, STATUS_ERROR); return false; } for (int i = 0; i < count; i++) { char* s = receive_wire_str(fd); - size_t entry_size = s ? strlen(s) : 0; + size_t entry_size = s ? strlen(s) + MANIFEST_ENTRY_OVERHEAD : 0; if (!s || s[0] == '\0' || s[0] == '/' || has_path_traversal(s) || entry_size > MAX_MANIFEST_BYTES - *manifest_bytes || (*manifest_bytes += entry_size) > MAX_MANIFEST_BYTES || !array_list_add(list, s)) { @@ -2715,6 +2779,7 @@ static bool receive_manifest_section(int fd, ArrayList* list, size_t* manifest_b return false; } } + *manifest_entries += (size_t)count; return true; } @@ -2733,9 +2798,10 @@ DeleteManifest* receive_manifest_entries(int fd) { return NULL; } size_t manifest_bytes = 0; - if (!receive_manifest_section(fd, manifest->keeps, &manifest_bytes) || - !receive_manifest_section(fd, manifest->protected, &manifest_bytes) || - !receive_manifest_section(fd, manifest->missing, &manifest_bytes)) { + size_t manifest_entries = 0; + if (!receive_manifest_section(fd, manifest->keeps, &manifest_bytes, &manifest_entries) || + !receive_manifest_section(fd, manifest->protected, &manifest_bytes, &manifest_entries) || + !receive_manifest_section(fd, manifest->missing, &manifest_bytes, &manifest_entries)) { delete_manifest_free(manifest); return NULL; } diff --git a/src/shared/xattr.c b/src/shared/xattr.c index d01a38b..269a867 100644 --- a/src/shared/xattr.c +++ b/src/shared/xattr.c @@ -73,13 +73,17 @@ bool xattr_list_append(FileXattrList* list, const char* name, const void* value, /* A Linux xattr name is "namespace.name" with an optional leading "trusted.", * "system.", "security.", "user.", or "trusted." prefix. We only ever touch - * the unprivileged "user.*" namespace and the two POSIX ACL xattrs carried in - * the "system." namespace. Everything else -- especially "security.*" (ACLs, - * capabilities, SELinux labels) and "trusted.*" -- is refused so a client can - * never compel the receiver to apply a privileged attribute it would not - * otherwise be able to set (and which would be a local privilege escalation if - * it could). */ -bool xattr_name_appliable(const char* name) { + * the unprivileged "user.*" namespace and, only when --acls/-A was negotiated, + * the two POSIX ACL xattrs carried in the "system." namespace. Everything else + * -- especially "security.*" (ACLs, capabilities, SELinux labels) and + * "trusted.*" -- is refused so a client can never compel the receiver to apply a + * privileged attribute it would not otherwise be able to set (and which would be + * a local privilege escalation if it could). + * + * The ACL gate is deliberate: --xattrs/-X alone derives use_xattrs but must NOT + * authorize the ACL names, otherwise a -X client could plant an ACL the + * receiver never opted into (B4). */ +bool xattr_name_appliable(const char* name, bool preserve_acls) { if (!name || name[0] == '\0') return false; size_t len = strlen(name); @@ -95,12 +99,21 @@ bool xattr_name_appliable(const char* name) { if (strncmp(name, "user.", 5) == 0) return name[5] != '\0'; if (strcmp(name, "system.posix_acl_access") == 0) - return true; + return preserve_acls; if (strcmp(name, "system.posix_acl_default") == 0) - return true; + return preserve_acls; return false; } +/* The two POSIX ACL xattr names: the only names whose applicablity is + * conditional (they require --acls). Used by the receiver to distinguish "not + * negotiated" (drop the entry, keep user.* working for -X) from a genuinely + * disallowed namespace (hard reject). */ +static bool xattr_name_is_posix_acl(const char* name) { + return name != NULL && (strcmp(name, "system.posix_acl_access") == 0 || + strcmp(name, "system.posix_acl_default") == 0); +} + /* ---- SENDER: capture ---- */ FileXattrList* xattr_capture_path(const char* path) { @@ -130,7 +143,9 @@ FileXattrList* xattr_capture_path(const char* path) { if (name_len == 0) break; /* trailing double NUL not expected; stop */ offset += (ssize_t)name_len + 1; - if (!xattr_name_appliable(name)) + /* Capture is sender-side: the scanner has already gated on -X/-A, so the + per-name whitelist here allows the ACL names (true). */ + if (!xattr_name_appliable(name, true)) continue; ssize_t value_size = getxattr(path, name, NULL, 0); if (value_size < 0) @@ -190,7 +205,7 @@ bool xattr_send(int fd, const FileXattrList* list) { return true; } -FileXattrList* xattr_receive(int fd, int* ok) { +FileXattrList* xattr_receive(int fd, int* ok, bool preserve_acls) { if (ok) *ok = 0; int count; @@ -232,11 +247,19 @@ FileXattrList* xattr_receive(int fd, int* ok) { xattr_list_free(list); return NULL; } - if (!xattr_name_appliable(name)) { - log_message(LOG_LEVEL_ERROR, "rejected xattr block: disallowed namespace for '%s'", name); - free(name); - xattr_list_free(list); - return NULL; + bool skip = false; + if (!xattr_name_appliable(name, preserve_acls)) { + if (!preserve_acls && xattr_name_is_posix_acl(name)) { + /* -X without -A: the sender may still carry ACLs, but the receiver must + never apply an ACL it was not asked to preserve. Consume and drop the + entry (keeping -X compatibility) rather than failing the transfer. */ + skip = true; + } else { + log_message(LOG_LEVEL_ERROR, "rejected xattr block: disallowed namespace for '%s'", name); + free(name); + xattr_list_free(list); + return NULL; + } } int32_t value_len32; if (!receive_n_data(fd, &value_len32, sizeof(value_len32))) { @@ -273,6 +296,12 @@ FileXattrList* xattr_receive(int fd, int* ok) { return NULL; } } + if (skip) { + free(value); + free(name); + budget += (size_t)name_len32 + (size_t)value_len32; + continue; + } if (!xattr_list_append(list, name, value, (size_t)value_len32)) { free(value); free(name); diff --git a/src/shared/xattr.h b/src/shared/xattr.h index 55f22dd..5c16c53 100644 --- a/src/shared/xattr.h +++ b/src/shared/xattr.h @@ -59,9 +59,11 @@ void xattr_list_free(FileXattrList* list); bool xattr_list_append(FileXattrList* list, const char* name, const void* value, size_t value_len); /* True when `name` is a well-formed xattr name AND belongs to a namespace this - * build is authorized to apply (user.* or the two POSIX ACL xattrs). Used for - * both capture and receiver-side validation. */ -bool xattr_name_appliable(const char* name); + * build is authorized to apply. `user.*` is always accepted for -X; the two + * POSIX ACL xattrs are accepted only when `preserve_acls` (--acls/-A) is set, so + * a plain -X run can never carry or apply an ACL the receiver did not ask for. + * Used for both capture and receiver-side validation. */ +bool xattr_name_appliable(const char* name, bool preserve_acls); /* Sender: read the whitelisted xattrs of `path` into a new list. Returns NULL * when the path has no appliable xattrs (or the filesystem has no xattr @@ -70,9 +72,12 @@ FileXattrList* xattr_capture_path(const char* path); /* Wire: bounded serialization. xattr_send returns false on write failure; an * empty/NULL list transmits a zero-count block. xattr_receive returns NULL and - * sets *ok = 0 on any malformed / oversized / non-whitelisted entry. */ + * sets *ok = 0 on any malformed / oversized / non-whitelisted entry. When + * `preserve_acls` is false, any POSIX ACL entries are consumed and DROPPED (so + * a -X transfer still succeeds and never applies an ACL it did not negotiate); + * a genuinely disallowed namespace is still rejected. */ bool xattr_send(int fd, const FileXattrList* list); -FileXattrList* xattr_receive(int fd, int* ok); +FileXattrList* xattr_receive(int fd, int* ok, bool preserve_acls); /* Receiver: apply every entry fd-relative (fsetxattr) to the just-written file * descriptor. A per-attribute failure (e.g. ACL set refused for non-root on a diff --git a/tests/fuzz/fuzz_xattr_block.c b/tests/fuzz/fuzz_xattr_block.c index 26e3775..f1ba210 100644 --- a/tests/fuzz/fuzz_xattr_block.c +++ b/tests/fuzz/fuzz_xattr_block.c @@ -75,7 +75,7 @@ static void write_best_effort(int fd, const void* data, size_t size) { } static void receive_stream(const unsigned char* prefix, size_t prefix_len, const uint8_t* data, - size_t size) { + size_t size, bool preserve_acls) { int sv[2]; if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) != 0) return; @@ -91,7 +91,7 @@ static void receive_stream(const unsigned char* prefix, size_t prefix_len, const shutdown(sv[0], SHUT_WR); int ok = 0; - FileXattrList* list = xattr_receive(sv[1], &ok); + FileXattrList* list = xattr_receive(sv[1], &ok, preserve_acls); xattr_list_free(list); close(sv[0]); @@ -102,14 +102,18 @@ int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { if (!g_block_ready) build_canonical_block(); - /* Raw bytes as the whole block. */ - receive_stream(NULL, 0, data, size); + /* Raw bytes as the whole block. Exercise both the -X-only (no ACLs) and the + * -A (ACL names accepted) receiver gates. */ + for (int acls = 0; acls < 2; acls++) { + bool preserve_acls = acls != 0; + receive_stream(NULL, 0, data, size, preserve_acls); - /* Valid framing so the fuzzer mutates the entry list, the first value and - * the second entry respectively instead of stopping at the count. */ - receive_stream(g_block, g_off_after_entry0, data, size); - receive_stream(g_block, g_off_value0, data, size); - receive_stream(g_block, g_off_after_count, data, size); + /* Valid framing so the fuzzer mutates the entry list, the first value and + * the second entry respectively instead of stopping at the count. */ + receive_stream(g_block, g_off_after_entry0, data, size, preserve_acls); + receive_stream(g_block, g_off_value0, data, size, preserve_acls); + receive_stream(g_block, g_off_after_count, data, size, preserve_acls); + } return 0; } diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 96372cb..1c34c00 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -444,9 +444,10 @@ class TestRemoteDryRun: self._seed(source) clean_dir(dest) - # Populate the destination with a real transfer, then make exactly one - # file differ (content+size) and add a brand-new file. - result, _ = run_client(source, dest, port=shared_server.port) + # Populate the destination with a real transfer that preserves mtimes + # (--preserve), then make exactly one file differ (content+size) and add + # a brand-new file. + result, _ = run_client(source, dest, flags=["--preserve"], port=shared_server.port) assert result.returncode == 0, f"seed transfer failed: {result.stderr[:200]}" received = get_dest_received_dir(dest, source) @@ -456,9 +457,11 @@ class TestRemoteDryRun: f.write(b"newly added\n") before = _snapshot_tree(received) - # --checksum makes the up-to-date decision content-based (the seed - # transfer did not preserve mtimes), so keep.txt/deep.txt report skip. - result, _ = run_client(source, dest, flags=["--dry-run", "--checksum"], + # --checksum must NOT read destination contents in a dry-run (B3), so + # the up-to-date decision is metadata-only. The --preserve seed made + # keep.txt and deep.txt size+mtime-identical; the dry-run must also + # transmit metadata (--preserve) for that metadata to be comparable. + result, _ = run_client(source, dest, flags=["--dry-run", "--checksum", "--preserve"], port=shared_server.port) assert result.returncode == 0, f"remote dry-run failed: {result.stderr[:300]}" assert "Dry run:" in result.stdout, result.stdout[:200] @@ -470,6 +473,34 @@ class TestRemoteDryRun: assert "deep.txt" not in result.stdout, result.stdout assert _snapshot_tree(received) == before, "remote dry-run mutated the destination" + @pytest.mark.ci + def test_remote_dry_run_checksum_does_not_read_destination(self, shared_server): + """B3: --dry-run --checksum against a read-only module must not read the + destination file's content (a 1-bit hash oracle). A same-size/same-content + file whose mtime differs is therefore reported as would-transfer because + the metadata-only decision is inconclusive, instead of being hashed and + silently skipped.""" + source = os.path.join(TEST_DATA_DIR, "remote_dry_oracle_src") + dest = os.path.join(TEST_DATA_DIR, "remote_dry_oracle_dst") + self._seed(source) + clean_dir(dest) + result, _ = run_client(source, dest, flags=["--preserve"], port=shared_server.port) + assert result.returncode == 0, result.stderr[:200] + received = get_dest_received_dir(dest, source) + + target = os.path.join(received, "keep.txt") + # Identical size and content, but a deliberately different mtime. + os.utime(target, (1000000000, 1000000000)) + before = _snapshot_tree(received) + + result, _ = run_client(source, dest, flags=["--dry-run", "--checksum", "--preserve"], + port=shared_server.port) + assert result.returncode == 0, result.stderr[:300] + assert "keep.txt" in result.stdout, ( + f"dry-run --checksum must not read the destination to prove equality: {result.stdout}" + ) + assert _snapshot_tree(received) == before, "dry-run mutated the destination" + @pytest.mark.ci def test_remote_dry_run_into_empty_dest_creates_nothing(self, shared_server): source = os.path.join(TEST_DATA_DIR, "remote_dry_empty_src") diff --git a/tests/test_chunk.c b/tests/test_chunk.c index b1d6cf6..6266582 100644 --- a/tests/test_chunk.c +++ b/tests/test_chunk.c @@ -1,8 +1,10 @@ #include "chunk.h" +#include "protocol.h" #include "test_utils.h" #include "utils.h" #include +#include #include #include @@ -279,6 +281,51 @@ static void test_chunk_special_rdev_out_of_range_rejected() { chunk_destroy(chunk); } +/* B6: chunk_deserialize() charges each retained per-file copy to the owning + * session's connection budget (MAX_CONNECTION_MEMORY) so queued chunk payloads + * are not held outside the per-connection ceiling; destroying the chunk returns + * the charge through the Data.owner path. */ +static void test_chunk_deserialize_charges_session_budget() { + const char* path = "temp_chunk_charge.txt"; + const char* content = "charge me to the connection budget"; + unlink(path); + file_write_to_disk(path, content, strlen(content), false, false); + struct stat st; + EXPECT_EQ_INT(stat(path, &st), 0); + + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + ProtocolSession session; + protocol_session_init(&session, p[0], p[1]); + protocol_session_set_max_alloc(&session, 4ULL * 1024 * 1024); + + File* f = file_create(path); + EXPECT_NOT_NULL(f); + f->data->size = (unsigned long long)st.st_size; + EXPECT_TRUE(file_load_data(f)); + File* files[1] = {f}; + Chunk* chunk = chunk_create(files, 1); + EXPECT_NOT_NULL(chunk); + Data* serialized = chunk_serialize(chunk, false); + EXPECT_NOT_NULL(serialized); + /* Simulate a received buffer carrying its owning session. */ + serialized->owner = &session; + + Chunk* deserialized = chunk_deserialize(serialized, false); + EXPECT_NOT_NULL(deserialized); + unsigned long long charged = atomic_load(&session.total_allocated_bytes); + EXPECT_EQ_INT((int)charged, (int)strlen(content)); + chunk_destroy(deserialized); + /* The copy's charge is released with the File/Data on destroy. */ + EXPECT_EQ_INT((int)atomic_load(&session.total_allocated_bytes), 0); + + data_destroy(serialized); + chunk_destroy(chunk); + close(p[0]); + close(p[1]); + unlink(path); +} + void test_chunk() { test_file_operations(); test_chunk_operations(); @@ -286,4 +333,5 @@ void test_chunk() { test_chunk_symlink_roundtrip(); test_chunk_special_rdev_roundtrip(); test_chunk_special_rdev_out_of_range_rejected(); + test_chunk_deserialize_charges_session_budget(); } diff --git a/tests/test_file.c b/tests/test_file.c index 73c3431..37fe83e 100644 --- a/tests/test_file.c +++ b/tests/test_file.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -994,6 +995,94 @@ static void test_inplace_overwrite_truncates_shorter_payload() { rmdir(root); } +/* B2: --inplace must refuse an existing non-regular destination entry. A FIFO + would block open(O_WRONLY) forever and a device node would be written + directly, bypassing the --write-devices/super gate. Forked with an alarm so + a regression is a prompt failure instead of a hung suite. */ +static void test_inplace_refuses_fifo_destination() { + const char* root = "test_inplace_fifo_tmp"; + const char* path = "test_inplace_fifo_tmp/fifo"; + unlink(path); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + EXPECT_EQ_INT(mkfifo(path, 0600), 0); + + pid_t pid = fork(); + if (pid == 0) { + alarm(10); + File* f = file_create("fifo"); + if (!f) + _exit(1); + const char* content = "payload"; + f->data->data = malloc(strlen(content)); + if (!f->data->data) + _exit(1); + memcpy(f->data->data, content, strlen(content)); + f->data->size = strlen(content); + Config* cfg = config_create(); + if (!cfg) + _exit(1); + cfg->inplace = true; + bool written = file_save_to_disk(root, f, cfg); + file_destroy(f); + config_delete(cfg); + _exit(written ? 1 : 0); /* must be refused */ + } + int status; + waitpid(pid, &status, 0); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + struct stat st; + EXPECT_EQ_INT(lstat(path, &st), 0); + EXPECT_TRUE(S_ISFIFO(st.st_mode)); /* left untouched */ + unlink(path); + rmdir(root); +} + +/* B2: an existing char device must not be written by --inplace. mknod needs + privilege, so a non-root run skips gracefully. /dev/null's (1:3) rdev makes + the negative case harmless if it ever regresses. */ +static void test_inplace_refuses_device_destination() { + const char* root = "test_inplace_dev_tmp"; + const char* path = "test_inplace_dev_tmp/dev"; + unlink(path); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + if (mknod(path, S_IFCHR | 0600, makedev(1, 3)) != 0) { + rmdir(root); + return; /* no privilege to create a device node: skip */ + } + + pid_t pid = fork(); + if (pid == 0) { + alarm(10); + File* f = file_create("dev"); + if (!f) + _exit(1); + const char* content = "payload"; + f->data->data = malloc(strlen(content)); + if (!f->data->data) + _exit(1); + memcpy(f->data->data, content, strlen(content)); + f->data->size = strlen(content); + Config* cfg = config_create(); + if (!cfg) + _exit(1); + cfg->inplace = true; + bool written = file_save_to_disk(root, f, cfg); + file_destroy(f); + config_delete(cfg); + _exit(written ? 1 : 0); /* must be refused */ + } + int status; + waitpid(pid, &status, 0); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + struct stat st; + EXPECT_EQ_INT(lstat(path, &st), 0); + EXPECT_TRUE(S_ISCHR(st.st_mode)); /* still a device, not replaced */ + unlink(path); + rmdir(root); +} + /* Explicit directory entries (--dirs) create the directory under the receive root through the same save funnel, creating parents as needed, and reject traversal the same way a file path does. */ @@ -1606,4 +1695,6 @@ void test_file() { test_inplace_overwrite_clears_special_mode_bits(); test_inplace_overwrite_metadata_strips_special_bits(); test_inplace_overwrite_truncates_shorter_payload(); + test_inplace_refuses_fifo_destination(); + test_inplace_refuses_device_destination(); } diff --git a/tests/test_server.c b/tests/test_server.c index b2246e8..2dff95e 100644 --- a/tests/test_server.c +++ b/tests/test_server.c @@ -844,6 +844,172 @@ static void test_special_socket_path_log_escaped() { EXPECT_NOT_NULL(strstr(output, "socket not recreated: evil\\#012path")); } +/* B1: a client-planted FIFO at the destination must not block the receiver's + * incremental-check open. With the O_NONBLOCK open plus the post-open S_ISREG + * gate the FIFO is simply "no existing regular file", so the receiver proceeds + * to a full transfer; without O_NONBLOCK the child blocks in openat() and the + * alarm(30) kills it. */ +static void test_incremental_check_fifo_destination_does_not_hang() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + char* root = make_check_root("qffo"); + EXPECT_NOT_NULL(root); + cfg->receive_root_directory = str_dup(root); + char path[1024]; + snprintf(path, sizeof(path), "%s/file.txt", root); + EXPECT_EQ_INT(mkfifo(path, 0600), 0); + + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + + pid_t pid = fork(); + if (pid == 0) { + alarm(30); + close(p[1]); + io_set_fds(p[0], p[0]); + bool skipped = false; + File* file = receive_incremental_check(p[0], cfg, &skipped); + bool ok = file != NULL && !skipped; + file_destroy(file); + config_delete(cfg); + close(p[0]); + _exit(ok ? 0 : 1); + } else { + close(p[0]); + io_set_fds(p[1], p[1]); + EXPECT_TRUE(send_str(p[1], "file.txt")); + unsigned long long size = 4; + long long mtime = 42; + long long mtime_nsec = 0; + EXPECT_TRUE(send_n_data(p[1], &size, sizeof(size))); + EXPECT_TRUE(send_n_data(p[1], &mtime, sizeof(mtime))); + EXPECT_TRUE(send_n_data(p[1], &mtime_nsec, sizeof(mtime_nsec))); + Status s; + EXPECT_TRUE(receive_status(p[1], &s)); + EXPECT_EQ_INT(s, STATUS_NEXT); + + Data* body = data_create_reserve(4); + EXPECT_NOT_NULL(body); + body->data = malloc(4); + EXPECT_NOT_NULL(body->data); + memcpy(body->data, "data", 4); + body->size = 4; + EXPECT_TRUE(send_data(p[1], body)); + data_destroy(body); + + int status; + waitpid(pid, &status, 0); + close(p[1]); + config_delete(cfg); + unlink(path); + rmdir(root); + free(root); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + } +} + +/* B1: a FIFO planted in a --link-dest basis directory must not block + * basis_open_regular() either; the basis match is simply declined. */ +static void test_incremental_check_basis_fifo_does_not_hang() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + char* root = make_check_root("qbfi"); + EXPECT_NOT_NULL(root); + cfg->receive_root_directory = str_dup(root); + char basis_dir[1024]; + char basis_path[2048]; + snprintf(basis_dir, sizeof(basis_dir), "%s/basis", root); + EXPECT_EQ_INT(mkdir(basis_dir, 0700), 0); + snprintf(basis_path, sizeof(basis_path), "%s/file.txt", basis_dir); + EXPECT_EQ_INT(mkfifo(basis_path, 0600), 0); + EXPECT_EQ_INT(config_basis_append(cfg, BASIS_DEST_LINK, "basis"), 0); + + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + + pid_t pid = fork(); + if (pid == 0) { + alarm(30); + close(p[1]); + io_set_fds(p[0], p[0]); + bool skipped = false; + File* file = receive_incremental_check(p[0], cfg, &skipped); + bool ok = file != NULL && !skipped; + file_destroy(file); + config_delete(cfg); + close(p[0]); + _exit(ok ? 0 : 1); + } else { + close(p[0]); + io_set_fds(p[1], p[1]); + EXPECT_TRUE(send_str(p[1], "file.txt")); + unsigned long long size = 4; + long long mtime = 42; + long long mtime_nsec = 0; + EXPECT_TRUE(send_n_data(p[1], &size, sizeof(size))); + EXPECT_TRUE(send_n_data(p[1], &mtime, sizeof(mtime))); + EXPECT_TRUE(send_n_data(p[1], &mtime_nsec, sizeof(mtime_nsec))); + /* config_has_basis() makes the request carry the source digest. */ + uint8_t wire_len = 8; + uint8_t digest[8] = {0}; + EXPECT_TRUE(send_n_data(p[1], &wire_len, sizeof(wire_len))); + EXPECT_TRUE(send_n_data(p[1], digest, sizeof(digest))); + Status s; + EXPECT_TRUE(receive_status(p[1], &s)); + EXPECT_EQ_INT(s, STATUS_NEXT); + + Data* body = data_create_reserve(4); + EXPECT_NOT_NULL(body); + body->data = malloc(4); + EXPECT_NOT_NULL(body->data); + memcpy(body->data, "data", 4); + body->size = 4; + EXPECT_TRUE(send_data(p[1], body)); + data_destroy(body); + + int status; + waitpid(pid, &status, 0); + close(p[1]); + config_delete(cfg); + unlink(basis_path); + rmdir(basis_dir); + rmdir(root); + free(root); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + } +} + +/* B5: the aggregate entry count across the three manifest sections is capped at + * MAX_MANIFEST_ENTRIES, and a section that would push the total over the cap is + * rejected before its entries are read (so a tiny first section followed by a + * huge claimed second section fails fast). */ +static void test_receive_manifest_total_entry_cap() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + cfg->receive_root_directory = str_dup("/tmp/dst"); + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + io_set_fds(p[0], p[1]); + + EXPECT_TRUE(send_int(p[1], 1)); + EXPECT_TRUE(send_str(p[1], "keep.txt")); + /* The second section alone is within its per-section cap, but 1 + it exceeds + the cross-section cap; the receiver must reject at the count. */ + EXPECT_TRUE(send_int(p[1], MAX_MANIFEST_ENTRIES)); + EXPECT_NULL(receive_manifest_entries(p[0])); + Status status; + EXPECT_TRUE(receive_status(p[1], &status)); + EXPECT_EQ_INT(status, STATUS_ERROR); + + close(p[0]); + close(p[1]); + config_delete(cfg); +} + void test_server() { test_special_socket_path_log_escaped(); if (!is_running_under_valgrind()) { @@ -856,6 +1022,9 @@ void test_server() { test_incremental_check_size_mismatch_full_transfer(); test_incremental_check_dry_run_reports_transfer_without_writing(); test_incremental_check_delta_oversize_reports_failure(); + test_incremental_check_fifo_destination_does_not_hang(); + test_incremental_check_basis_fifo_does_not_hang(); + test_receive_manifest_total_entry_cap(); test_late_manifest_abort_frees_keepset(); test_late_manifest_eof_frees_keepset(); test_late_second_manifest_frees_both(); diff --git a/tests/test_xattr.c b/tests/test_xattr.c index 4997ae2..f82cb3f 100644 --- a/tests/test_xattr.c +++ b/tests/test_xattr.c @@ -17,7 +17,7 @@ static void run_recv_helper(int fd) { int ok = 0; - FileXattrList* list = xattr_receive(fd, &ok); + FileXattrList* list = xattr_receive(fd, &ok, false); if (!ok) _exit(1); if (!list) { @@ -64,7 +64,7 @@ static void test_xattr_wire_roundtrip() { static void run_recv_must_fail(int fd) { int ok = 0; - FileXattrList* list = xattr_receive(fd, &ok); + FileXattrList* list = xattr_receive(fd, &ok, false); /* A NULL list with ok==0 is the expected rejection. */ if (ok == 0 && list == NULL) _exit(0); @@ -148,15 +148,64 @@ static void test_xattr_count_bound() { /* The captured list on a plain file reflects only whitelisted namespaces * (Linux only; skipped when the filesystem has no xattr support). */ static void test_xattr_capture_and_appliable() { - EXPECT_FALSE(xattr_name_appliable(NULL)); - EXPECT_FALSE(xattr_name_appliable("")); - EXPECT_FALSE(xattr_name_appliable("security.selinux")); - EXPECT_FALSE(xattr_name_appliable("trusted.blob")); - EXPECT_TRUE(xattr_name_appliable("user.foo")); + EXPECT_FALSE(xattr_name_appliable(NULL, false)); + EXPECT_FALSE(xattr_name_appliable("", false)); + EXPECT_FALSE(xattr_name_appliable("security.selinux", false)); + EXPECT_FALSE(xattr_name_appliable("trusted.blob", false)); + EXPECT_TRUE(xattr_name_appliable("user.foo", false)); + EXPECT_TRUE(xattr_name_appliable("user.foo", true)); /* The reserved fake-super key is receiver-only and never forwarded/applied. */ - EXPECT_FALSE(xattr_name_appliable("user.fastsync.stat")); - EXPECT_TRUE(xattr_name_appliable("system.posix_acl_access")); - EXPECT_TRUE(xattr_name_appliable("system.posix_acl_default")); + EXPECT_FALSE(xattr_name_appliable("user.fastsync.stat", false)); + EXPECT_FALSE(xattr_name_appliable("user.fastsync.stat", true)); + /* B4: the ACL names require --acls; -X alone must not authorize them. */ + EXPECT_FALSE(xattr_name_appliable("system.posix_acl_access", false)); + EXPECT_FALSE(xattr_name_appliable("system.posix_acl_default", false)); + EXPECT_TRUE(xattr_name_appliable("system.posix_acl_access", true)); + EXPECT_TRUE(xattr_name_appliable("system.posix_acl_default", true)); +} + +/* B4: a `-X`-only receiver (preserve_acls false) must NOT apply an incoming + * ACL xattr, while a user.* attribute in the same block still survives. The + * ACL entry is dropped, not applied (and the -X transfer is not failed). */ +static void run_recv_drops_acl_keeps_user(int fd) { + int ok = 0; + FileXattrList* list = xattr_receive(fd, &ok, false); + if (!ok || list == NULL) + _exit(1); + bool saw_user = false; + for (int i = 0; i < list->count; i++) { + if (strcmp(list->items[i].name, "system.posix_acl_access") == 0) + _exit(1); /* ACL must have been dropped */ + if (strcmp(list->items[i].name, "user.keep") == 0) + saw_user = true; + } + xattr_list_free(list); + _exit(saw_user ? 0 : 1); +} + +static void test_xattr_receive_drops_acl_without_preserve_acls() { + int p[2]; + EXPECT_EQ_INT(pipe(p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + pid_t pid = fork(); + if (pid == 0) { + close(p[1]); + io_set_fds(p[0], p[0]); + run_recv_drops_acl_keeps_user(p[0]); + } + close(p[0]); + io_set_fds(p[1], p[1]); + FileXattrList* list = xattr_list_new(); + EXPECT_NOT_NULL(list); + EXPECT_TRUE(xattr_list_append(list, "system.posix_acl_access", "\x02\x00\x00\x00", 4)); + EXPECT_TRUE(xattr_list_append(list, "user.keep", "yes", 3)); + xattr_send(p[1], list); + xattr_list_free(list); + int status; + waitpid(pid, &status, 0); + close(p[1]); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); } /* MINOR-2: a --link-dest / -H copy fallback (linkat refused) must still apply @@ -360,6 +409,7 @@ void test_xattr() { test_xattr_reject_oversized_value(); test_xattr_count_bound(); test_xattr_capture_and_appliable(); + test_xattr_receive_drops_acl_without_preserve_acls(); test_link_copy_fallback_preserves_xattrs(); test_fake_super_restore(); test_fake_super_owner_gate(); From e48f19ee2bb8d0472b6aa6ac8776b320e612de3a Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:39:05 +0200 Subject: [PATCH 07/16] fix(compression): fail truncated zstd frames instead of spinning data_decompress_limited() looped while ZSTD_decompressStream() returned a positive hint. A truncated frame keeps returning that hint with all input consumed, so a malformed/truncated payload spun forever (CPU DoS). Detect input exhaustion with an incomplete frame and fail via the existing cleanup, skipping the check when the output buffer merely needs to grow first. Add a fork+alarm regression test that truncates a valid frame and asserts decompression returns NULL promptly. --- src/shared/compression.c | 14 ++++++++++++++ tests/test_compression.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/shared/compression.c b/src/shared/compression.c index 7609921..c40c534 100644 --- a/src/shared/compression.c +++ b/src/shared/compression.c @@ -324,6 +324,20 @@ Data* data_decompress_limited(Data* compressed_data, size_t maximum_size) { uncompressed_data->data = new_data; output.dst = new_data; output.size = buf_size; + /* Re-attempt with the larger output buffer; the truncated-frame check + * below must not reject a complete frame that merely filled the previous + * buffer exactly. */ + continue; + } + /* A positive hint with all input consumed means the frame is incomplete: a + * truncated stream would otherwise spin here forever (ZSTD_decompressStream + * keeps returning the same hint). Fail instead of burning CPU. */ + if (ret != 0 && input.pos == input.size) { + log_message(LOG_LEVEL_ERROR, + "Truncated zstd frame: input exhausted with %zu bytes still expected", ret); + data_destroy(uncompressed_data); + uncompressed_data = NULL; + goto cleanup; } } while (ret > 0); diff --git a/tests/test_compression.c b/tests/test_compression.c index da51531..2ebb8da 100644 --- a/tests/test_compression.c +++ b/tests/test_compression.c @@ -6,6 +6,7 @@ #include "utils.h" #include #include +#include #include #include @@ -211,9 +212,47 @@ static void test_data_compress_reused_contexts_multithreaded() { compression_free_thread_contexts(); } +/* A truncated zstd frame used to make the decompressor spin forever: the + * stream call keeps returning a positive hint with all input consumed. Run the + * decompression in a child with an alarm so a regression (infinite loop) is + * caught as a timeout failure instead of hanging the whole unit suite. */ +static void test_data_decompress_truncated_frame_fails() { + const char* original = + "The quick brown fox jumps over the lazy dog. The quick brown fox jumps over the lazy dog."; + size_t len = strlen(original); + char* buf = malloc(len); + EXPECT_NOT_NULL(buf); + memcpy(buf, original, len); + Data* input = data_create(buf, len); + EXPECT_NOT_NULL(input); + + pid_t pid = fork(); + EXPECT_TRUE(pid >= 0); + if (pid == 0) { + alarm(10); /* kills the child if the decompressor hangs */ + Data* compressed = data_compress(input, 3); + if (compressed && compressed->size > 1) { + compressed->size -= 1; /* drop the final byte: frame is now incomplete */ + Data* out = data_decompress(compressed); + bool failed_cleanly = (out == NULL); + data_destroy(out); + data_destroy(compressed); + _exit(failed_cleanly ? 0 : 1); + } + data_destroy(compressed); + _exit(2); + } + int status; + waitpid(pid, &status, 0); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + + data_destroy(input); +} + void test_compression() { test_data_compress_decompress_roundtrip(); test_data_compress_decompress_large(); + test_data_decompress_truncated_frame_fails(); test_skip_compress_suffix_matching(); test_data_compress_with_threads_roundtrip(); test_data_compress_reused_contexts_multithreaded(); From 58a28334b86fa73b17d64c8c0b3dc027b82653e3 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:39:10 +0200 Subject: [PATCH 08/16] fix(utils): bound glob matching and line reads Replace the recursive glob matcher with an iterative O(pattern*string) dynamic program. The old recursion explored exponentially many paths for overlapping '*'/'**' wildcards (e.g. '*a*a*...*b' against a long run of 'a'), a CPU DoS reachable from --exclude/--include patterns and .rsync-filter. A differential fuzz against the original matcher confirms identical results. Doc: has_path_traversal() is a lexical '..' check only. Add utils_getdelim_bounded(): a getdelim-style reader that never allocates beyond UTILS_MAX_LINE_LEN, used to cap untrusted list/filter line reads. Tests: pathological glob completes quickly; bounded reader returns EFBIG on an over-long record. --- src/shared/utils.c | 170 +++++++++++++++++++++++++++++--------- src/shared/utils.h | 19 +++++ tests/test_glob.c | 31 +++++++ tests/test_shared_utils.c | 29 +++++++ 4 files changed, 212 insertions(+), 37 deletions(-) diff --git a/src/shared/utils.c b/src/shared/utils.c index 64a5581..07b3d45 100644 --- a/src/shared/utils.c +++ b/src/shared/utils.c @@ -378,59 +378,155 @@ char* output_escape(const char* string, bool eight_bit_output) { return escaped; } +ssize_t utils_getdelim_bounded(FILE* stream, char** line, size_t* cap, int delim, size_t max_len) { + if (!stream || !line || !cap || max_len == 0) { + errno = EINVAL; + return -1; + } + size_t limit = max_len + 1; /* content bytes plus the terminating NUL */ + if (*line == NULL || *cap < 2) { + size_t initial = limit < 256 ? limit : 256; + char* buf = malloc(initial); + if (!buf) + return -1; + free(*line); + *line = buf; + *cap = initial; + } + size_t len = 0; + int c; + while ((c = getc_unlocked(stream)) != EOF) { + if (len >= max_len) { + errno = EFBIG; + return -1; + } + if (len + 2 > *cap) { + size_t new_cap = *cap * 2; + if (new_cap < len + 2) + new_cap = len + 2; + if (new_cap > limit) + new_cap = limit; + char* grown = realloc(*line, new_cap); + if (!grown) + return -1; + *line = grown; + *cap = new_cap; + } + (*line)[len++] = (char)c; + if (c == delim) + break; + } + if (c == EOF && len == 0) + return 0; + (*line)[len] = '\0'; + return (ssize_t)len; +} + /* Match a glob pattern against a string. Supported wildcards: * ? matches any single character except '/'. * * matches any sequence of characters within one path component (no '/'). * ** matches any sequence of characters, including '/' (cross-directory). * slash-star-star-slash is treated as a cross-directory wildcard when it appears between * literals. - */ + * + * The matcher is an iterative O(pattern * string) dynamic program rather than the + * original backtracking recursion: overlapping `*`/`**` wildcards made a pattern + * like `*a*a*a*...*b` run in exponential time against a long run of `a`, a CPU + * denial-of-service vector reachable from a hostile --exclude/--include pattern + * or `.rsync-filter`. The DP reasons over (pattern position, string position) + * so every state is visited once; the transitions below mirror the original + * recursion exactly. */ bool glob_match(const char* pattern, const char* str) { - while (*pattern) { - if (*pattern == '*') { - if (*(pattern + 1) == '*') { - /* globstar: match across directories */ - pattern += 2; - if (*pattern == '\0') - return true; - if (*pattern == '/') - pattern++; - while (*str) { - if (glob_match(pattern, str)) - return true; - str++; + if (!pattern || !str) + return false; + size_t pattern_len = strlen(pattern); + size_t str_len = strlen(str); + if (pattern_len == 0) + return str_len == 0; + /* Defensive work cap: the DP is bounded by pattern*string states, but a + * 64 KiB pattern against a 64 KiB path would still cost billions of steps. + * Treat the pattern as non-matching above the cap instead of burning CPU. */ + if (str_len > (SIZE_MAX / (pattern_len + 1)) - 1) + return false; + if ((pattern_len + 1) * (str_len + 1) > 64u * 1024u * 1024u) + return false; + + size_t row_bytes = str_len + 1; + /* Rows for pattern positions i, i+1, i+2 and i+3 are live at once (the + * globstar transition can skip up to three pattern bytes). Four rotating + * rows keep memory at O(string length); a stack buffer avoids an allocation + * for the common short-leaf case. */ + enum { STACK_ROW = 257 }; + uint8_t stack_rows[4 * STACK_ROW]; + uint8_t* rows = stack_rows; + if (row_bytes > STACK_ROW) { + rows = malloc(4 * row_bytes); + if (!rows) + return false; + } + +#define GLOB_ROW(i) (rows + ((pattern_len - (i)) & 3) * row_bytes) + + /* Base row: pattern position `pattern_len` matches only the string's end. */ + for (size_t j = 0; j <= str_len; j++) + GLOB_ROW(pattern_len)[j] = (j == str_len) ? 1 : 0; + + for (size_t i = pattern_len; i-- > 0;) { + const char pc = pattern[i]; + uint8_t* cur = GLOB_ROW(i); + const uint8_t* next = GLOB_ROW(i + 1); + if (pc == '*') { + if (i + 1 < pattern_len && pattern[i + 1] == '*') { + /* Globstar: skip `**` and an optional following '/', then consume any + * (possibly empty) run of characters -- including '/'. */ + size_t rest = i + 2; + if (rest < pattern_len && pattern[rest] == '/') + rest++; + const uint8_t* rest_row = GLOB_ROW(rest); + for (size_t j = str_len + 1; j-- > 0;) { + bool v = rest_row[j] != 0; + if (!v && j < str_len) + v = cur[j + 1] != 0; + cur[j] = v ? 1 : 0; + } + } else { + /* Single `*`: zero characters, or one non-'/' character. */ + for (size_t j = str_len + 1; j-- > 0;) { + bool v = next[j] != 0; + if (!v && j < str_len && str[j] != '/') + v = cur[j + 1] != 0; + cur[j] = v ? 1 : 0; } - return glob_match(pattern, str); } - /* single *: match within one path component */ - pattern++; - while (*str && *str != '/') { - if (glob_match(pattern, str)) - return true; - str++; + } else if (pc == '?') { + for (size_t j = str_len + 1; j-- > 0;) { + bool v = j < str_len && str[j] != '/' && next[j + 1] != 0; + cur[j] = v ? 1 : 0; } - return glob_match(pattern, str); - } else if (*pattern == '?') { - if (!*str || *str == '/') - return false; - pattern++; - str++; } else { - if (*pattern != *str) { - /* allow literal / ** / rest to match any number of directories */ - if (*pattern == '/' && *(pattern + 1) == '*' && *(pattern + 2) == '*') { - const char* rest = pattern + 3; - if (*rest == '/') + /* Literal: consume an equal character, or -- for a '/' immediately before + * a globstar -- let the '/' match zero directories and continue at `**`. */ + for (size_t j = str_len + 1; j-- > 0;) { + bool v = false; + if (j < str_len && str[j] == pc) { + v = next[j + 1] != 0; + } else if (pc == '/' && i + 2 < pattern_len && pattern[i + 1] == '*' && + pattern[i + 2] == '*') { + size_t rest = i + 3; + if (rest < pattern_len && pattern[rest] == '/') rest++; - return glob_match(rest, str); + v = GLOB_ROW(rest)[j] != 0; } - return false; + cur[j] = v ? 1 : 0; } - pattern++; - str++; } } - return *str == '\0'; + + bool matched = GLOB_ROW(0)[0] != 0; +#undef GLOB_ROW + if (rows != stack_rows) + free(rows); + return matched; } bool format_human_bytes(unsigned long long bytes, char* buffer, size_t buffer_size) { diff --git a/src/shared/utils.h b/src/shared/utils.h index ff83ac0..cda0cd8 100644 --- a/src/shared/utils.h +++ b/src/shared/utils.h @@ -4,7 +4,9 @@ #include "array_list.h" #include #include +#include #include +#include /* Small open-addressing string hash set used to turn quadratic membership * scans into O(path length) exact-match lookups (the --delete keep-set and the @@ -79,6 +81,17 @@ bool path_index_has_descendant(const PathIndex* index, const char* path); char* str_dup(const char* string); char* output_escape(const char* string, bool eight_bit_output); +/* Upper bound on one line/token read from a local list file (--files-from, + * --exclude-from/--include-from, .rsync-filter). Mirrors MAX_STRING_SIZE and + * stops a hostile multi-gigabyte line from forcing unbounded allocation. */ +#define UTILS_MAX_LINE_LEN (64 * 1024) +/* Read one `delim`-terminated record from `stream` into *line (grown as needed + * and NUL-terminated), refusing to consume/allocate more than `max_len` bytes + * of content. Returns the number of bytes stored (delimiter included, matching + * getdelim), 0 at end of file, or -1 on error (errno is EFBIG when the record + * exceeds `max_len`, ENOMEM on allocation failure). *line and *cap are updated + * as the buffer grows and the caller owns *line. */ +ssize_t utils_getdelim_bounded(FILE* stream, char** line, size_t* cap, int delim, size_t max_len); char* path_cat(const char* path1, const char* path2); bool glob_match(const char* pattern, const char* str); /* Result of a bounded extra-file deletion run. */ @@ -148,6 +161,12 @@ const char* utils_get_authorized_root_path(void); * callers guarantee this); this is containment by string, not by resolved * symlinks. Shared by the utils and file secure-walk root confinement. */ bool path_is_within_root(const char* root, const char* path); +/* True when `path` contains a ".." component. This is a purely lexical + * dot-dot check: an absolute path is NOT rejected here, because default + * (non-relative) transfers legitimately put the sender's absolute source path + * on the wire and the receiver re-roots it under the destination with + * path_cat(). Callers that accept a strictly relative path (e.g. batch paths) + * must reject a leading '/' themselves (see utils_valid_batch_path). */ bool has_path_traversal(const char* path); bool utils_valid_batch_path(const char* path); bool format_human_bytes(unsigned long long bytes, char* buffer, size_t buffer_size); diff --git a/tests/test_glob.c b/tests/test_glob.c index ded4bc6..f59016f 100644 --- a/tests/test_glob.c +++ b/tests/test_glob.c @@ -1,7 +1,9 @@ #include "test_glob.h" #include "utils.h" #include "test_utils.h" +#include #include +#include static void test_glob_exact_match() { EXPECT_TRUE(glob_match("foo", "foo")); @@ -76,6 +78,34 @@ static void test_glob_doublestar_mid() { EXPECT_FALSE(glob_match("a/**/b", "a/x/bad")); } +/* The old backtracking matcher explored an exponential number of paths for a + * pattern with many `*` wildcards against a long run that never matches the + * trailing literal. The iterative matcher must stay bounded: 30 `*a` groups + * followed by `b` against ten thousand `a`s is a few hundred thousand states, + * not 2^30 recursion nodes. */ +static void test_glob_pathological_is_bounded() { + char pattern[128]; + size_t pos = 0; + for (int i = 0; i < 30; i++) { + pattern[pos++] = '*'; + pattern[pos++] = 'a'; + } + pattern[pos++] = 'b'; + pattern[pos] = '\0'; + + char* text = malloc(10001); + EXPECT_NOT_NULL(text); + memset(text, 'a', 10000); + text[10000] = '\0'; + + clock_t start = clock(); + EXPECT_FALSE(glob_match(pattern, text)); + double elapsed = (double)(clock() - start) / CLOCKS_PER_SEC; + EXPECT_TRUE(elapsed < 5.0); + + free(text); +} + void test_glob() { test_glob_exact_match(); test_glob_question_mark(); @@ -91,4 +121,5 @@ void test_glob() { test_glob_doublestar_prefix(); test_glob_doublestar_suffix(); test_glob_doublestar_mid(); + test_glob_pathological_is_bounded(); } diff --git a/tests/test_shared_utils.c b/tests/test_shared_utils.c index c89c006..448dd86 100644 --- a/tests/test_shared_utils.c +++ b/tests/test_shared_utils.c @@ -519,9 +519,38 @@ static void test_path_index_semantics() { path_index_free(&empty); } +/* utils_getdelim_bounded must return normal short lines unchanged and refuse an + * over-long record with EFBIG rather than allocating without bound. */ +static void test_getdelim_bounded() { + FILE* fp = tmpfile(); + EXPECT_NOT_NULL(fp); + const char* short_line = "short\n"; + EXPECT_EQ_INT((int)fwrite(short_line, 1, strlen(short_line), fp), (int)strlen(short_line)); + char big[32]; + memset(big, 'x', 20); + big[20] = '\n'; + EXPECT_EQ_INT((int)fwrite(big, 1, 21, fp), 21); + rewind(fp); + + char* line = NULL; + size_t cap = 0; + ssize_t n = utils_getdelim_bounded(fp, &line, &cap, '\n', 64); + EXPECT_EQ_INT((int)n, 6); + EXPECT_EQ_STR(line, "short\n"); + + errno = 0; + n = utils_getdelim_bounded(fp, &line, &cap, '\n', 10); + EXPECT_EQ_INT((int)n, -1); + EXPECT_EQ_INT(errno, EFBIG); + + free(line); + fclose(fp); +} + void test_shared_utils() { test_path_index_bounded(); test_path_index_semantics(); + test_getdelim_bounded(); test_walker_removes_extras_keeps_manifest_and_protected(); test_walker_keeps_nested_manifest_dirs(); test_walker_max_delete_exceeded_deletes_nothing(); From 1a26bde2d4090b2b79a3c609b9e60b6b0db1c2db Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:39:14 +0200 Subject: [PATCH 09/16] fix(file_list): bound entry length and reject embedded NUL bytes Read list files through utils_getdelim_bounded() so a single multi-gigabyte line can no longer force unbounded allocation; over-long entries fail with a clear error. Also add the documented memchr() NUL-byte check (excluding the NUL delimiter in NUL-separated mode). Tests: an over-long entry is rejected with an 'exceeds' diagnostic. --- src/shared/file_list.c | 24 ++++++++++++++++++++---- tests/test_file_list.c | 30 +++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/shared/file_list.c b/src/shared/file_list.c index 3297a87..fc04e0b 100644 --- a/src/shared/file_list.c +++ b/src/shared/file_list.c @@ -56,8 +56,14 @@ static int normalize_entry(const char* raw, size_t len, bool strip_line_endings, snprintf(err, err_size, "absolute path entries are not allowed: '%.*s'", print_len, raw); return -1; } - /* Reject NUL bytes inside a token defensively (NUL-delimited mode splits on - * them, so this only guards against embedded garbage). */ + /* Reject NUL bytes inside a token defensively. In NUL-delimited mode the + * delimiter itself is the final byte and is expected; in line mode any NUL is + * embedded garbage (strlen-based parsing would otherwise silently truncate). */ + size_t scan_len = strip_line_endings ? len : len - 1; + if (memchr(raw, '\0', scan_len)) { + snprintf(err, err_size, "entry contains an embedded NUL byte"); + return -1; + } char* dup = malloc(len + 1); if (!dup) { snprintf(err, err_size, "memory allocation failed"); @@ -158,10 +164,20 @@ FileListSet* file_list_load(const char* path, bool null_separated, char* err, si StringList raw = {0}; char* line = NULL; size_t line_cap = 0; - ssize_t n; bool ok = true; char delim = null_separated ? '\0' : '\n'; - while (ok && (n = getdelim(&line, &line_cap, delim, fp)) != -1) { + while (ok) { + ssize_t n = utils_getdelim_bounded(fp, &line, &line_cap, delim, UTILS_MAX_LINE_LEN); + if (n < 0) { + if (errno == EFBIG) + snprintf(err, err_size, "entry in file list exceeds %d bytes", (int)UTILS_MAX_LINE_LEN); + else + snprintf(err, err_size, "error reading file list: %s", strerror(errno)); + ok = false; + break; + } + if (n == 0) + break; int r = normalize_entry(line, (size_t)n, !null_separated, &raw, err, err_size); if (r < 0) { ok = false; diff --git a/tests/test_file_list.c b/tests/test_file_list.c index 80a0ca8..777b43a 100644 --- a/tests/test_file_list.c +++ b/tests/test_file_list.c @@ -1,5 +1,6 @@ #include "test_file_list.h" #include "file_list.h" +#include "utils.h" #include "test_utils.h" #include #include @@ -189,8 +190,35 @@ static void test_deep_paths_are_bounded() { free(entry); } +/* An over-long list entry must be rejected cleanly instead of being read + without a bound (the reader never allocates beyond UTILS_MAX_LINE_LEN). */ +static void test_oversized_entry_rejected() { + const char* path = "test_file_list_oversized.txt"; + FILE* fp = fopen(path, "wb"); + EXPECT_NOT_NULL(fp); + char chunk[4096]; + memset(chunk, 'a', sizeof(chunk)); + size_t total = 0; + while (total <= UTILS_MAX_LINE_LEN) { + EXPECT_EQ_INT((int)fwrite(chunk, 1, sizeof(chunk), fp), (int)sizeof(chunk)); + total += sizeof(chunk); + } + EXPECT_EQ_INT(fputc('\n', fp), '\n'); + fclose(fp); + + char err[160]; + FileListSet* set = file_list_load(path, false, err, sizeof(err)); + if (set) { + file_list_destroy(set); + EXPECT_FAIL("over-long entry was accepted"); + } + EXPECT_TRUE(strstr(err, "exceeds") != NULL); + remove(path); +} + void test_file_list() { test_membership_matches_reference(); test_ancestor_and_descendant_queries(); test_deep_paths_are_bounded(); -} + test_oversized_entry_rejected(); +} \ No newline at end of file From 5b0ec5880d752d8a67f354ae006bf9c323a8ea40 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:39:14 +0200 Subject: [PATCH 10/16] fix(filter): bound .rsync-filter lines and guard capacity growth Read per-directory filter files through the bounded reader, guard the rule list's capacity doubling against INT_MAX/2 overflow, and escape the local directory path before logging a read failure. --- src/shared/filter.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/shared/filter.c b/src/shared/filter.c index 8e7ae77..d93d7d6 100644 --- a/src/shared/filter.c +++ b/src/shared/filter.c @@ -2,6 +2,7 @@ #include "log.h" #include "utils.h" #include +#include #include #include #include @@ -176,6 +177,8 @@ bool filter_rule_list_add(FilterRuleList* list, FilterRule* rule) { if (!list || !rule) return false; if (list->count == list->capacity) { + if (list->capacity > INT_MAX / 2) + return false; int new_cap = list->capacity > 0 ? list->capacity * 2 : 8; FilterRule** grown = realloc(list->items, (size_t)new_cap * sizeof(FilterRule*)); if (!grown) @@ -322,8 +325,10 @@ FilterRuleList* filter_file_read(const char* dir_path, const char* owner_rel, bo if (!fp) { if (errno == ENOENT || errno == ENOTDIR) return filter_rule_list_create(); - log_message(LOG_LEVEL_WARNING, "Could not read .rsync-filter in %s: %s", dir_path, - strerror(errno)); + char* escaped_dir = output_escape(dir_path, log_get_8_bit_output()); + log_message(LOG_LEVEL_WARNING, "Could not read .rsync-filter in %s: %s", + escaped_dir ? escaped_dir : "", strerror(errno)); + free(escaped_dir); return filter_rule_list_create(); } if (exists) @@ -336,9 +341,20 @@ FilterRuleList* filter_file_read(const char* dir_path, const char* owner_rel, bo } char* line = NULL; size_t line_cap = 0; - ssize_t n; bool ok = true; - while ((n = getline(&line, &line_cap, fp)) != -1) { + while (true) { + ssize_t n = utils_getdelim_bounded(fp, &line, &line_cap, '\n', UTILS_MAX_LINE_LEN); + if (n < 0) { + if (errno == EFBIG) { + snprintf(err, err_size, "line in .rsync-filter exceeds %d bytes", (int)UTILS_MAX_LINE_LEN); + } else { + snprintf(err, err_size, "error reading .rsync-filter: %s", strerror(errno)); + } + ok = false; + break; + } + if (n == 0) + break; const char* p = line; while (*p == ' ' || *p == '\t') p++; From dfa2a4202809c0c6f9a6c3302729e0f63179d33a Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:39:19 +0200 Subject: [PATCH 11/16] fix(protocol): retry EINTR on receive and clamp SSL_write length protocol_receive_n_data_until() aborted on a signal-interrupted plaintext read (and on SSL_ERROR_SYSCALL with errno==EINTR); retry both, matching the send path and protocol_read_status_until(). Also clamp each SSL_write() to INT_MAX so a >INT_MAX size_t request can never truncate into a partial write. --- src/shared/protocol.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/shared/protocol.c b/src/shared/protocol.c index 014fa11..c5b7991 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -302,10 +302,14 @@ bool protocol_send_n_data(ProtocolSession* session, const void* data, size_t dat if (pfd.revents & (POLLERR | POLLNVAL)) return false; ssize_t bytes_send; - if (session->ssl) - bytes_send = SSL_write(session->ssl, (const char*)data + total_bytes_send, chunk); - else + if (session->ssl) { + /* SSL_write takes an int length; clamp a >INT_MAX request into chunks so + * the size_t downcast can never truncate into a negative/partial write. */ + size_t ssl_chunk = chunk > (size_t)INT_MAX ? (size_t)INT_MAX : chunk; + bytes_send = SSL_write(session->ssl, (const char*)data + total_bytes_send, (int)ssl_chunk); + } else { bytes_send = write(fd, (const char*)data + total_bytes_send, chunk); + } if (bytes_send <= 0) { if (session->ssl) { int ssl_err = SSL_get_error(session->ssl, (int)bytes_send); @@ -387,6 +391,13 @@ static bool protocol_receive_n_data_until(ProtocolSession* session, void* data, wait_events = ssl_err == SSL_ERROR_WANT_WRITE ? POLLOUT : POLLIN; continue; } + /* A signal interrupts the blocking TLS read: retry (mirrors the send + path and protocol_read_status_until) so the loop reaches its next + abort/deadline checkpoint instead of failing spuriously. */ + if (ssl_err == SSL_ERROR_SYSCALL && errno == EINTR) + continue; + } else if (errno == EINTR) { + continue; } if (bytes_received == 0) log_message(LOG_LEVEL_ERROR, "Connection closed while receiving data"); From 10c4ffebdf21613a25bd63e5ae256fa078d1f67a Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:39:19 +0200 Subject: [PATCH 12/16] fix(client): harden CLI args, log escaping, and local artifact opens - parse_ull_arg() rejects a leading '-'/'+' (strtoull would silently wrap -1 to ULLONG_MAX) and --chunk-size/--delta-max enforce their upper bounds. - Escape local untrusted paths before logging (client_send, scanner, --filter rule, pattern-file reads) with output_escape(..., 8-bit mode). - Read --exclude-from/--include-from through the bounded line reader. - Open --log-file with O_NOFOLLOW|O_CLOEXEC, mode 0600, via open+fdopen; create --write-batch with O_NOFOLLOW|O_CLOEXEC, mode 0600. - Reject --dry-run together with --write-batch (dry-run must not write the batch file), alongside the existing --read-batch/--only-write-batch rules. Tests: signed/oversized numeric rejection, over-long pattern file, dry-run + write-batch unit and integration coverage. --- src/client/client_cli.c | 65 ++++++++++++++++++++++++++++----- src/client/client_send.c | 33 +++++++++++++---- src/client/client_validation.c | 10 +++-- src/client/scanner.c | 17 +++++++-- tests/integration/test_batch.py | 14 ++++++- tests/test_client_cli.c | 55 ++++++++++++++++++++++++++++ 6 files changed, 169 insertions(+), 25 deletions(-) diff --git a/src/client/client_cli.c b/src/client/client_cli.c index 4014e8b..6a2abba 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -19,6 +19,7 @@ #include "usage.h" #include "utils.h" #include +#include #include #include #include @@ -27,6 +28,7 @@ #include #include #include +#include /* Async-signal-safe abort flag set by the SIGINT/SIGTERM handler. Exposed via * client_send.h so the send loops can poll it. Defined here (not in @@ -429,8 +431,14 @@ static int parse_info_flags(const char* value, Config* config) { return 0; } -/* Parse a string as an unsigned long long. Returns 0 on success, -1 on error. */ +/* Parse a string as an unsigned long long. Returns 0 on success, -1 on error. + * A leading '-'/'+' (or whitespace) is rejected outright: strtoull would + * otherwise silently wrap a negative value to a huge unsigned one. */ static int parse_ull_arg(const char* val, unsigned long long* out, const char* optname) { + if (!val || val[0] < '0' || val[0] > '9') { + log_message(LOG_LEVEL_ERROR, "%s must be a non-negative integer", optname); + return -1; + } char* end; errno = 0; unsigned long long v = strtoull(val, &end, 10); @@ -537,7 +545,10 @@ static int config_add_filter(Config* config, const char* rule) { char err[160]; FilterRule* parsed = filter_rule_parse(rule, err, sizeof(err)); if (!parsed) { - log_message(LOG_LEVEL_ERROR, "invalid --filter rule '%s': %s", rule, err); + char* escaped = output_escape(rule, log_get_8_bit_output()); + log_message(LOG_LEVEL_ERROR, "invalid --filter rule '%s': %s", + escaped ? escaped : "", err); + free(escaped); return -1; } filter_rule_free(parsed); @@ -1303,10 +1314,15 @@ static bool cli_handle_ssh_and_pattern_options(CliParseCtx* ctx) { ctx->exit_code = -1; return true; } - if (val >= DELTA_MIN_FILE_SIZE) + if (val >= DELTA_MIN_FILE_SIZE && val <= DELTA_MAX_FILE_SIZE) { config->delta_max_file_size = val; - else + } else if (val < DELTA_MIN_FILE_SIZE) { log_message(LOG_LEVEL_WARNING, "--delta-max value %llu too small, using default", val); + } else { + log_message(LOG_LEVEL_ERROR, "--delta-max must not exceed %llu bytes", + (unsigned long long)DELTA_MAX_FILE_SIZE); + ctx->exit_code = -1; + } return true; } return false; @@ -1463,6 +1479,12 @@ static bool cli_handle_io_options(CliParseCtx* ctx) { ctx->exit_code = -1; return true; } + if (val > MAX_CHUNK_SIZE) { + log_message(LOG_LEVEL_ERROR, "--chunk-size must be between 1 and %llu", + (unsigned long long)MAX_CHUNK_SIZE); + ctx->exit_code = -1; + return true; + } config->chunk_size = val; return true; } @@ -1479,11 +1501,20 @@ static bool cli_handle_io_options(CliParseCtx* ctx) { fclose(config->log_file); config->log_file = NULL; } - FILE* lf = fopen(ctx->argv[++ctx->i], "a"); + const char* log_path = ctx->argv[++ctx->i]; + /* Refuse a symlinked target and never leak the descriptor across exec: an + * attacker who can plant a symlink in the working directory must not be + * able to redirect (or truncate) an arbitrary file via --log-file. The log + * is created with owner-only permissions. */ + int log_fd = open(log_path, O_WRONLY | O_CREAT | O_APPEND | O_NOFOLLOW | O_CLOEXEC, 0600); + FILE* lf = log_fd >= 0 ? fdopen(log_fd, "a") : NULL; if (!lf) { - char* escaped = output_escape(ctx->argv[ctx->i], false); + int open_errno = errno; + if (log_fd >= 0) + close(log_fd); + char* escaped = output_escape(log_path, false); log_message(LOG_LEVEL_ERROR, "could not open log file '%s': %s", - escaped ? escaped : "", strerror(errno)); + escaped ? escaped : "", strerror(open_errno)); free(escaped); ctx->exit_code = -1; return true; @@ -2011,8 +2042,24 @@ static int read_patterns_from_file(const char* filepath, char*** patterns, int* } char* line = NULL; size_t line_size = 0; - ssize_t n; - while ((n = getline(&line, &line_size, fp)) != -1) { + while (true) { + ssize_t n = utils_getdelim_bounded(fp, &line, &line_size, '\n', UTILS_MAX_LINE_LEN); + if (n < 0) { + char* escaped = output_escape(filepath, false); + if (errno == EFBIG) { + log_message(LOG_LEVEL_ERROR, "pattern file '%s' has a line exceeding %d bytes", + escaped ? escaped : "", (int)UTILS_MAX_LINE_LEN); + } else { + log_message(LOG_LEVEL_ERROR, "could not read pattern file '%s': %s", + escaped ? escaped : "", strerror(errno)); + } + free(escaped); + free(line); + fclose(fp); + return -1; + } + if (n == 0) + break; char* p = line; while (*p == ' ' || *p == '\t') p++; diff --git a/src/client/client_send.c b/src/client/client_send.c index 3f3cfa5..9db0221 100644 --- a/src/client/client_send.c +++ b/src/client/client_send.c @@ -310,8 +310,11 @@ static bool files_from_list_check(const Config* config, ArrayList* missing_dest, return false; } if (set->count == 0) { + char* escaped_list = + output_escape(config->files_from ? config->files_from : "", log_get_8_bit_output()); log_message(LOG_LEVEL_ERROR, "--files-from file '%s' contains no entries; nothing to transfer", - config->files_from ? config->files_from : ""); + escaped_list ? escaped_list : ""); + free(escaped_list); return false; } bool ignore = config->ignore_missing_args || config->delete_missing_args; @@ -329,7 +332,10 @@ static bool files_from_list_check(const Config* config, ArrayList* missing_dest, free(full); if (ignore) { (*skipped_out)++; - log_info_message(LOG_INFO_MISC, "skipping missing --files-from entry '%s'", entry); + char* escaped_entry = output_escape(entry, log_get_8_bit_output()); + log_info_message(LOG_INFO_MISC, "skipping missing --files-from entry '%s'", + escaped_entry ? escaped_entry : ""); + free(escaped_entry); if (config->delete_missing_args && missing_dest) { char* mirror = files_from_missing_dest_path(config, entry); if (!mirror || !array_list_add(missing_dest, mirror)) { @@ -340,8 +346,13 @@ static bool files_from_list_check(const Config* config, ArrayList* missing_dest, } continue; } - log_message(LOG_LEVEL_ERROR, "--files-from entry '%s' not found in source '%s'", entry, - config->send_directory); + char* escaped_entry = output_escape(entry, log_get_8_bit_output()); + char* escaped_src = output_escape(config->send_directory, log_get_8_bit_output()); + log_message(LOG_LEVEL_ERROR, "--files-from entry '%s' not found in source '%s'", + escaped_entry ? escaped_entry : "", + escaped_src ? escaped_src : ""); + free(escaped_entry); + free(escaped_src); return false; } free(full); @@ -588,8 +599,12 @@ static void remove_transferred_sources(const Config* config, ArrayList* paths) { close(dirfd); continue; } - if (unlinkat(dirfd, leaf, 0) != 0) - log_message(LOG_LEVEL_WARNING, "Could not remove source file %s", source->path); + if (unlinkat(dirfd, leaf, 0) != 0) { + char* escaped_path = output_escape(source->path, log_get_8_bit_output()); + log_message(LOG_LEVEL_WARNING, "Could not remove source file %s", + escaped_path ? escaped_path : ""); + free(escaped_path); + } close(dirfd); } } @@ -2120,7 +2135,7 @@ int write_batch_from_source(const Config* config, const char* batch_path) { prepared_scanner_destroy(&prepared); return 1; } - int fd = open(batch_path, O_WRONLY | O_CREAT | O_TRUNC, 0644); + int fd = open(batch_path, O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW | O_CLOEXEC, 0600); if (fd < 0) { log_perror("could not create batch file"); directory_scanner_destroy(scanner); @@ -2135,8 +2150,10 @@ int write_batch_from_source(const Config* config, const char* batch_path) { if (f == NULL || f->data == NULL) continue; if (f->data->size > 0 && f->data->data == NULL && !file_load_data(f)) { + char* escaped_path = output_escape(f->path ? f->path : "", log_get_8_bit_output()); log_message(LOG_LEVEL_ERROR, "batch: failed to load data for %s", - f->path ? f->path : ""); + escaped_path ? escaped_path : ""); + free(escaped_path); ok = false; break; } diff --git a/src/client/client_validation.c b/src/client/client_validation.c index ef2a6e2..2d47559 100644 --- a/src/client/client_validation.c +++ b/src/client/client_validation.c @@ -25,12 +25,14 @@ bool validate_config(const Config* config) { /* A dry-run of a local batch apply is not meaningful: --read-batch bypasses the client-side scan/server decision entirely, so dry-run would have no wire state to report (and must not be used as a mutation escape hatch). - --only-write-batch likewise never contacts a receiver. Reject both up + --only-write-batch likewise never contacts a receiver. --write-batch DOES + run a live transfer but additionally mutates the filesystem by emitting the + batch file, so a dry-run must not write it either. Reject all three up front instead of silently ignoring --dry-run. */ - if (config->dry_run && (read_batch || only_write_batch)) { + if (config->dry_run && (read_batch || only_write_batch || write_batch)) { log_message(LOG_LEVEL_ERROR, - "--dry-run cannot be combined with --read-batch or --only-write-batch; " - "a dry-run of a local batch apply is not meaningful"); + "--dry-run cannot be combined with --read-batch, --only-write-batch, or " + "--write-batch; a dry-run must not mutate anything, including batch files"); return false; } if (read_batch) { diff --git a/src/client/scanner.c b/src/client/scanner.c index 3a8722d..0afd31d 100644 --- a/src/client/scanner.c +++ b/src/client/scanner.c @@ -284,7 +284,10 @@ static int open_directory_filter_context(DirectoryScanner* scanner, const Filter filter_file_read(scanner->current_path, scanner->current_rel ? scanner->current_rel : "", &exists, err, sizeof(err)); if (!own) { - log_message(LOG_LEVEL_ERROR, "invalid .rsync-filter in %s: %s", scanner->current_path, err); + char* escaped_path = output_escape(scanner->current_path, log_get_8_bit_output()); + log_message(LOG_LEVEL_ERROR, "invalid .rsync-filter in %s: %s", + escaped_path ? escaped_path : "", err); + free(escaped_path); scanner->failed = true; return -1; } @@ -777,11 +780,19 @@ static File* dirs_file_for_entry(DirectoryScanner* scanner, const char* entry) { nothing (missing entries never appear there). Without the flags it stays a hard pre-transfer error. */ if (scanner->options.ignore_missing_args) { - log_info_message(LOG_INFO_MISC, "skipping missing --files-from entry '%s'", entry); + char* escaped_entry = output_escape(entry, log_get_8_bit_output()); + log_info_message(LOG_INFO_MISC, "skipping missing --files-from entry '%s'", + escaped_entry ? escaped_entry : ""); + free(escaped_entry); free(abs_path); return NULL; } - log_message(LOG_LEVEL_ERROR, "--dirs listed entry is not present under the source: %s", entry); + { + char* escaped_entry = output_escape(entry, log_get_8_bit_output()); + log_message(LOG_LEVEL_ERROR, "--dirs listed entry is not present under the source: %s", + escaped_entry ? escaped_entry : ""); + free(escaped_entry); + } free(abs_path); scanner->failed = true; return NULL; diff --git a/tests/integration/test_batch.py b/tests/integration/test_batch.py index 15565bf..4388081 100644 --- a/tests/integration/test_batch.py +++ b/tests/integration/test_batch.py @@ -117,4 +117,16 @@ def test_batch_modes_conflict(): cmd = _run(["--source-dir", SOURCE_DIR, "--dest-dir", DEST1] + flags) result = subprocess.run(cmd, capture_output=True, text=True, timeout=180) assert result.returncode != 0, \ - f"expected conflict failure for {flags}: {result.stderr}" \ No newline at end of file + f"expected conflict failure for {flags}: {result.stderr}" + + +@pytest.mark.ci +def test_dry_run_rejects_write_batch(): + """--dry-run must not emit a batch file (it must not mutate anything).""" + if os.path.exists(BATCH_FILE): + os.unlink(BATCH_FILE) + cmd = _run(["--source-dir", SOURCE_DIR, "--dest-dir", DEST1, + "--dry-run", "--write-batch", BATCH_FILE]) + result = subprocess.run(cmd, capture_output=True, text=True, timeout=180) + assert result.returncode != 0, result.stderr + assert not os.path.exists(BATCH_FILE), "dry-run must not create a batch file" \ No newline at end of file diff --git a/tests/test_client_cli.c b/tests/test_client_cli.c index 538e2ff..a0d6207 100644 --- a/tests/test_client_cli.c +++ b/tests/test_client_cli.c @@ -3275,6 +3275,58 @@ static void test_parse_args_block_size() { config_delete(cfg); } +/* An over-long --exclude-from/--include-from line is rejected at parse time + * rather than being read without a bound. */ +static void test_parse_args_pattern_file_oversized_rejected() { + const char* list_path = "cli_pattern_oversized.txt"; + size_t len = UTILS_MAX_LINE_LEN + 4096; + char* big = malloc(len); + EXPECT_NOT_NULL(big); + memset(big, 'a', len); + write_file_bytes(list_path, big, len); + free(big); + + Config* cfg = config_create(); + int positional_args[2]; + int positional_count = 0; + char* argv[] = {"fastsync", "--exclude-from", (char*)list_path, "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 5, argv, positional_args, &positional_count), -1); + config_delete(cfg); + remove(list_path); +} + +/* A leading '-'/'+' must be rejected for every unsigned numeric option so + * strtoull can never silently wrap (e.g. -1 -> ULLONG_MAX). */ +static void test_parse_args_unsigned_options_reject_sign() { + static const char* const opts[] = {"--chunk-size", "--bwlimit", "--delta-max"}; + for (size_t i = 0; i < sizeof(opts) / sizeof(opts[0]); i++) { + Config* cfg = config_create(); + int positional_args[2]; + int positional_count = 0; + char* argv[] = {"fastsync", (char*)opts[i], "-1", "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 5, argv, positional_args, &positional_count), -1); + config_delete(cfg); + } + + /* An over-cap --chunk-size is rejected at parse time (max 64 MiB). */ + Config* cfg = config_create(); + int positional_args[2]; + int positional_count = 0; + char* big_argv[] = {"fastsync", "--chunk-size", "67108865", "/src", "/dst"}; + EXPECT_EQ_INT(parse_args(cfg, 5, big_argv, positional_args, &positional_count), -1); + config_delete(cfg); +} + +/* --dry-run must not emit a batch file, so it is rejected alongside + * --read-batch/--only-write-batch. */ +static void test_validate_config_dry_run_rejects_write_batch() { + Config* cfg = valid_client_config(); + cfg->dry_run = true; + cfg->write_batch = str_dup("batch.dat"); + EXPECT_FALSE(validate_config(cfg)); + config_delete(cfg); +} + void test_client_cli() { test_validate_config_required_paths(); test_parse_args_numeric_ids(); @@ -3432,4 +3484,7 @@ void test_client_cli() { test_parse_args_remote_option_short_M(); test_parse_args_no_motd(); test_parse_args_password_file(); + test_parse_args_pattern_file_oversized_rejected(); + test_parse_args_unsigned_options_reject_sign(); + test_validate_config_dry_run_rejects_write_batch(); } From 34abaadb9a705d6aa092ff8c9722720fbedd064d Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 17:11:02 +0200 Subject: [PATCH 13/16] fix: address low/informational sec-parser follow-ups - client_cli: capture errno before output_escape() in read_patterns_from_file() so an over-long line is still reported as EFBIG instead of the (possibly malloc-clobbered) errno. - file_list: guard string_list_add() capacity doubling against overflow (capacity > INT_MAX / 2), matching filter_rule_list_add(); callers already surface the false as a memory-allocation error. - compression: ZSTD_isError() is true for ZSTD_CONTENTSIZE_UNKNOWN, which made the 3x unknown-size fallback dead code. Test the CONTENTSIZE_ERROR/UNKNOWN sentinels explicitly so unknown-size frames reach the estimate path (still bounded by the existing hard limit) while invalid frames are rejected. Known-size frames and the 100 MB ceiling/overflow checks are unchanged. - tests: add an unknown-content-size-frame decompression test. Tests: ./build/tests and ./build-asan/tests all pass (42/42); clang-format + cppcheck clean. --- src/client/client_cli.c | 7 +++-- src/shared/compression.c | 10 +++++-- src/shared/file_list.c | 2 ++ tests/test_compression.c | 59 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 5 deletions(-) diff --git a/src/client/client_cli.c b/src/client/client_cli.c index 6a2abba..b8637c3 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -2045,13 +2045,16 @@ static int read_patterns_from_file(const char* filepath, char*** patterns, int* while (true) { ssize_t n = utils_getdelim_bounded(fp, &line, &line_size, '\n', UTILS_MAX_LINE_LEN); if (n < 0) { + /* output_escape() may allocate (and clobber errno): capture the reader's + * errno first so an over-long line is still reported as EFBIG. */ + int saved_errno = errno; char* escaped = output_escape(filepath, false); - if (errno == EFBIG) { + if (saved_errno == EFBIG) { log_message(LOG_LEVEL_ERROR, "pattern file '%s' has a line exceeding %d bytes", escaped ? escaped : "", (int)UTILS_MAX_LINE_LEN); } else { log_message(LOG_LEVEL_ERROR, "could not read pattern file '%s': %s", - escaped ? escaped : "", strerror(errno)); + escaped ? escaped : "", strerror(saved_errno)); } free(escaped); free(line); diff --git a/src/shared/compression.c b/src/shared/compression.c index c40c534..dad630c 100644 --- a/src/shared/compression.c +++ b/src/shared/compression.c @@ -243,9 +243,13 @@ Data* data_decompress_limited(Data* compressed_data, size_t maximum_size) { log_debug_message(LOG_DEBUG_UTIL, "Start to decompress data"); unsigned long long dst_size = ZSTD_getFrameContentSize(compressed_data->data, compressed_data->size); - if (ZSTD_isError(dst_size)) { - log_message(LOG_LEVEL_ERROR, "Failed to get decompressed size: %s", - ZSTD_getErrorName(dst_size)); + /* ZSTD_isError() is also true for ZSTD_CONTENTSIZE_ERROR and + * ZSTD_CONTENTSIZE_UNKNOWN (both are encoded near (size_t)-1), so test the + * sentinels explicitly instead of blanket-rejecting every error-ish value: + * only CONTENTSIZE_ERROR means an unreadable header, while CONTENTSIZE_UNKNOWN + * must reach the estimate fallback below. */ + if (dst_size == ZSTD_CONTENTSIZE_ERROR) { + log_message(LOG_LEVEL_ERROR, "Failed to get decompressed size: invalid zstd frame"); return NULL; } diff --git a/src/shared/file_list.c b/src/shared/file_list.c index fc04e0b..5b2a138 100644 --- a/src/shared/file_list.c +++ b/src/shared/file_list.c @@ -23,6 +23,8 @@ static void string_list_destroy(StringList* list) { static bool string_list_add(StringList* list, const char* text) { if (list->count == list->capacity) { + if (list->capacity > INT_MAX / 2) + return false; int new_cap = list->capacity > 0 ? list->capacity * 2 : 16; char** grown = realloc(list->items, (size_t)new_cap * sizeof(char*)); if (!grown) diff --git a/tests/test_compression.c b/tests/test_compression.c index 2ebb8da..063f060 100644 --- a/tests/test_compression.c +++ b/tests/test_compression.c @@ -9,6 +9,7 @@ #include #include #include +#include static void test_data_compress_decompress_roundtrip() { const char original[] = "Hello, World! This is test data for compression round-trip!"; @@ -139,6 +140,63 @@ static void test_chunk_compress_decompress_roundtrip() { unlink(path2); } +/* Build a zstd frame whose header omits the content size (the content size + * flag is cleared), which ZSTD_getFrameContentSize reports as + * ZSTD_CONTENTSIZE_UNKNOWN. */ +static Data* make_unknown_size_frame(const void* src, size_t len) { + ZSTD_CCtx* cctx = ZSTD_createCCtx(); + if (!cctx) + return NULL; + ZSTD_CCtx_setParameter(cctx, ZSTD_c_contentSizeFlag, 0); + size_t cap = ZSTD_compressBound(len); + Data* out = data_create_empty(cap); + if (!out) { + ZSTD_freeCCtx(cctx); + return NULL; + } + ZSTD_inBuffer in = {src, len, 0}; + ZSTD_outBuffer ob = {out->data, cap, 0}; + size_t ret; + do { + ret = ZSTD_compressStream2(cctx, &ob, &in, ZSTD_e_end); + if (ZSTD_isError(ret)) { + data_destroy(out); + ZSTD_freeCCtx(cctx); + return NULL; + } + } while (ret > 0); + out->size = ob.pos; + ZSTD_freeCCtx(cctx); + return out; +} + +/* ZSTD_CONTENTSIZE_UNKNOWN is flagged by ZSTD_isError(), so a naive + * ZSTD_isError() check rejects every unknown-size frame. Such a frame must + * instead reach the 3x estimate fallback and decompress correctly. */ +static void test_data_decompress_unknown_size_frame() { + const char original[] = "unknown-content-size frame: the decompressor must use the 3x estimate, " + "not reject the frame as an error."; + size_t len = strlen(original); + char* buf = malloc(len); + EXPECT_NOT_NULL(buf); + memcpy(buf, original, len); + + Data* frame = make_unknown_size_frame(buf, len); + free(buf); + EXPECT_NOT_NULL(frame); + /* Guard the premise of the test: the frame really has no stored size. */ + EXPECT_EQ_INT((int)ZSTD_getFrameContentSize(frame->data, frame->size), + (int)ZSTD_CONTENTSIZE_UNKNOWN); + + Data* decompressed = data_decompress(frame); + EXPECT_NOT_NULL(decompressed); + EXPECT_EQ_INT((int)decompressed->size, (int)len); + EXPECT_EQ_INT(memcmp(decompressed->data, original, len), 0); + + data_destroy(decompressed); + data_destroy(frame); +} + typedef struct { int id; int iterations; @@ -252,6 +310,7 @@ static void test_data_decompress_truncated_frame_fails() { void test_compression() { test_data_compress_decompress_roundtrip(); test_data_compress_decompress_large(); + test_data_decompress_unknown_size_frame(); test_data_decompress_truncated_frame_fails(); test_skip_compress_suffix_matching(); test_data_compress_with_threads_roundtrip(); From 825ba6975349952b0270a166c044224b4e0b6163 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 17:16:01 +0200 Subject: [PATCH 14/16] fix(server): reject --allow-super with --stdio, fix module host-list append Re-review findings on the C3/C4 hardening branch: - --stdio is the SSH transport whose remote argv is composed by the client (including via --remote-option), so accepting --allow-super there let a client defeat the C3 secure default for a root receiver. Reject it at CLI parse time (standalone TCP only) and force the process-global flag off for --stdio as defense in depth. Correct the help text and README/RSYNC_COMPAT: the --stdio argv is client-composed, super stays off, and a forced command is needed if the default must hold. - daemon_conf: the per-module 'hosts allow'/'hosts deny' call sites passed module_name and replace in the wrong order, so multiple lines replaced instead of appended and the empty-value error omitted the module name. Pass (module->name, false) like the global keys; add a unit test for two per-module allow/deny lines appending. - tls: read the client CN via ASN1_STRING_to_UTF8 so an exactly-required-length name is accepted and only actual over-length CNs are rejected. --- README.md | 10 ++++-- RSYNC_COMPAT.md | 10 +++--- src/server/server.c | 75 ++++++++++++++++++++++++---------------- src/server/server_cli.c | 16 +++++++-- src/server/server_cli.h | 17 +++++---- src/shared/daemon_conf.c | 4 +-- tests/test_daemon_conf.c | 23 ++++++++++++ tests/test_server_cli.c | 13 +++++-- 8 files changed, 118 insertions(+), 50 deletions(-) diff --git a/README.md b/README.md index 9ab5b00..26d1725 100644 --- a/README.md +++ b/README.md @@ -178,7 +178,7 @@ transfer is never aborted. | `--ca ` | TLS CA certificate file for verification (PEM) | | `--destination-root ` | Authorized destination root (default: `.`) | | `--allow-delete` | Permit manifest deletion | -| `--allow-super` | Standalone/`--stdio` only: keep super-user activities enabled for a **root** receiver. Without it a root standalone server forces `SUPER_MODE_OFF`, so client `--devices`/`--write-devices`/`--super` and client-chosen ownership requests are skipped/refused. No effect when not root. | +| `--allow-super` | Standalone TCP listener only: keep super-user activities enabled for a **root** receiver. Without it a root standalone server forces `SUPER_MODE_OFF`, so client `--devices`/`--write-devices`/`--super` and client-chosen ownership requests are skipped/refused. **Rejected with `--stdio`** (the SSH remote argv is client-composed, so a client could otherwise pass it and defeat the secure default; operators exposing `fastsync-server --stdio` over SSH must use a forced command if the default must hold). No effect when not root. | | `--allow-unauthenticated` | Permit plaintext TCP clients. For an `auth users` module this opts in **loopback plaintext only**; remote auth still requires verified TLS, so the flag never permits remote plaintext auth. | | `-v, --verbose` | Enable debug logging | | `--help` | Show help | @@ -305,6 +305,12 @@ The remote host must have `fastsync-server` available in `PATH`, or use working directory, so use a destination below that directory unless the remote server is otherwise configured with a matching authorized root. +The remote `--stdio` server argv is composed by the client, so it must never +be trusted to opt a root receiver into super-user activities: `--allow-super` +is rejected with `--stdio` and super stays off on that path. Operators +exposing `fastsync-server --stdio` over SSH must use a forced command (e.g. an +`authorized_keys` `command=` entry) if the default must hold. + ```bash ssh user@host 'mkdir -p destination' ./build/client /path/to/source user@host:destination @@ -500,7 +506,7 @@ link-target transfer remains incomplete. | | `--destination-root ` | Confine received files to this server-side root; defaults to the current directory. | | `--allow-delete` | Permit client delete manifests. Deletion is refused by default. This also gates `--force` (which can recursively replace/remove a destination directory tree). | -| `--allow-super` | Standalone/`--stdio` only: keep super-user activities enabled for a **root** receiver. Without it a root standalone server forces `SUPER_MODE_OFF`, so client `--devices`/`--write-devices`/`--super` and client-chosen ownership requests are skipped/refused. No effect when not root. Daemon modules opt in per module with `client owner = yes`. | +| `--allow-super` | Standalone TCP listener only: keep super-user activities enabled for a **root** receiver. Without it a root standalone server forces `SUPER_MODE_OFF`, so client `--devices`/`--write-devices`/`--super` and client-chosen ownership requests are skipped/refused. Rejected with `--stdio` (the SSH remote argv is client-composed; use a forced command if the default must hold). No effect when not root. Daemon modules opt in per module with `client owner = yes`. | | `-v`, `--verbose` | Enable debug logging. | | `--help` | Print server usage. | diff --git a/RSYNC_COMPAT.md b/RSYNC_COMPAT.md index 616aefc..f25945a 100644 --- a/RSYNC_COMPAT.md +++ b/RSYNC_COMPAT.md @@ -257,14 +257,14 @@ why plain `--append` works on the normal atomic path, not only with `--inplace`. | `-N`, `--crtimes` | Preserve create times | ⛔ Impossible/Divergence | Birth-times cannot be set by any portable filesystem call (`utimensat`/`futimens` only set atime/mtime), so this row is an explicit **Impossible/Divergence** (Phase 7 Wave B). Capture + transmit stays: `statx(STATX_BTIME)` on Linux records the source birth time as a wire field; the receiver logs a debug note that it cannot be applied and continues — never failing the transfer and never pretending it worked. On platforms without `statx` it parses as a documented no-op (flag accepted; nothing is captured). Implies metadata transmission. Wire: new `crtime` fields + a `preserve_crtimes` config boolean; `PROTOCOL_VERSION` bumped **2.11.0 → 2.12.0** (see the Phase-4 metadata-time notes) | | `-O`, `--omit-dir-times` | Omit dirs from --times | ✅ Implemented | Real modifier now that FastSync preserves directory times. With metadata on, the scanner captures every traversed source directory's mtime (and atime under `-U`) and the sender transmits them in trailing `STATUS_DIR_TIMES` frame(s) **after all file data and the optional delete manifest** (chunked at the receiver's `MAX_MANIFEST_ENTRIES` per-frame cap); a dir-time entry only RECORDS metadata and never creates the directory, so empty source directories stay untransferred. The receiver defers applying them until its delete / `--delay-updates` publication phases have committed, so writing or removing a child never clobbers a parent directory's mtime (rsync applies directory times at the end for exactly this reason). When `-O` is set (the boolean crosses the wire) the receiver does not apply any of them; without `-O` an `-a`/`--preserve` transfer now restores directory times (reversing the old "never preserves dir times" divergence). Wire change: the terminal `STATUS_DIR_TIMES` frame; `PROTOCOL_VERSION` bumped **2.16.0 → 2.17.0** | | `-J`, `--omit-link-times` | Omit symlinks from --times | ✅ Implemented | Real modifier now that FastSync preserves symlink times. Symlink entries already carried their metadata on `STATUS_SYMLINK`; the receiver now applies it with **no-follow primitives only** (`utimensat(..., AT_SYMLINK_NOFOLLOW)`, plus best-effort `fchmodat(..., AT_SYMLINK_NOFOLLOW)` and policy-gated `fchownat(..., AT_SYMLINK_NOFOLLOW)`), so the link itself is stamped without ever dereferencing it, confined fd-relative below the authorized receive root. A symlink has no children, so the times are applied immediately at creation. When `-J` is set (the boolean crosses the wire) the receiver skips the timestamps (mode/ownership are unaffected); without `-J` an `-a`/`-l` transfer restores symlink mtimes. Wire change alongside `-O`: the shared `STATUS_DIR_TIMES` frame; `PROTOCOL_VERSION` bumped **2.16.0 → 2.17.0** | -| `--super` | Receiver attempts super-user activities | ✅ Implemented | Phase 7 Wave E: receiver-side **safe-subset + clear-refusal** privilege model, tri-state `super_mode` (auto/on/off). `--super` **permits** the receiver to attempt super-user activities — ownership application and char/block device-node creation — that are already confined fd-relative below the authorized receive root; `--no-super` **forbids** them even when the receiver is root; the default (`auto`) preserves the pre-existing **best-effort** behavior of *attempting* them (not only when already root: an unprivileged attempt is refused by the kernel and skipped per entry, matching FastSync's history). The server additionally accepts an operator-level `--no-super` veto that forces `OFF` for every connection it accepts (so it also refuses any client `--copy-as`/`--super`); a **privileged (root) standalone/`--stdio` receiver now also defaults to `OFF`** unless the operator opts in with the new server-only `--allow-super` flag (an unprivileged receiver is unchanged, since the kernel refuses the confined attempts anyway; the `--daemon` path keeps its per-module `client owner = yes` opt-in); the `--fake-super` owner replay and the `--write-devices` write path are gated by the same policy. **FastSync never elevates**: no `setuid`/`seteuid`/`setgid` is ever called, and `--super` never bypasses the confinement floor (`file_open_secure_parent`, `O_NOFOLLOW`, root checks) — it only permits an attempt that is already confined. `--super` does **not** imply `--numeric-ids` and never enables client-chosen ownership on its own: ownership is applied only when an explicit identity policy (`--usermap`/`--groupmap`/`--chown`/`--numeric-ids`/`--copy-as`) is also given. A non-root receiver given `--super` logs exactly one warning at activation and each confined attempt is then refused by the kernel and skipped per entry (never aborts); `--no-super` suppresses ownership, char/block `mknod`, `--write-devices` and the fake-super owner replay, while unprivileged FIFO creation is unaffected. Wire: one trailing `super_mode` int on the config frame (validated 0..2), sent **before** the `--copy-as` block (fixed order: super int, then copy-as presence int + ids); `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0**. **Documented divergence from rsync:** rsync's `--super` runs the receiver with elevated privilege; FastSync only permits a confined attempt and never elevates | +| `--super` | Receiver attempts super-user activities | ✅ Implemented | Phase 7 Wave E: receiver-side **safe-subset + clear-refusal** privilege model, tri-state `super_mode` (auto/on/off). `--super` **permits** the receiver to attempt super-user activities — ownership application and char/block device-node creation — that are already confined fd-relative below the authorized receive root; `--no-super` **forbids** them even when the receiver is root; the default (`auto`) preserves the pre-existing **best-effort** behavior of *attempting* them (not only when already root: an unprivileged attempt is refused by the kernel and skipped per entry, matching FastSync's history). The server additionally accepts an operator-level `--no-super` veto that forces `OFF` for every connection it accepts (so it also refuses any client `--copy-as`/`--super`); a **privileged (root) standalone TCP listener now also defaults to `OFF`** unless the operator opts in with the new server-only `--allow-super` flag (the flag is **rejected with `--stdio`**, whose remote argv is composed by the client and must never defeat the secure default; operators exposing `fastsync-server --stdio` over SSH need a forced command if the default must hold. An unprivileged receiver is unchanged, since the kernel refuses the confined attempts anyway; the `--daemon` path keeps its per-module `client owner = yes` opt-in); the `--fake-super` owner replay and the `--write-devices` write path are gated by the same policy. **FastSync never elevates**: no `setuid`/`seteuid`/`setgid` is ever called, and `--super` never bypasses the confinement floor (`file_open_secure_parent`, `O_NOFOLLOW`, root checks) — it only permits an attempt that is already confined. `--super` does **not** imply `--numeric-ids` and never enables client-chosen ownership on its own: ownership is applied only when an explicit identity policy (`--usermap`/`--groupmap`/`--chown`/`--numeric-ids`/`--copy-as`) is also given. A non-root receiver given `--super` logs exactly one warning at activation and each confined attempt is then refused by the kernel and skipped per entry (never aborts); `--no-super` suppresses ownership, char/block `mknod`, `--write-devices` and the fake-super owner replay, while unprivileged FIFO creation is unaffected. Wire: one trailing `super_mode` int on the config frame (validated 0..2), sent **before** the `--copy-as` block (fixed order: super int, then copy-as presence int + ids); `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0**. **Documented divergence from rsync:** rsync's `--super` runs the receiver with elevated privilege; FastSync only permits a confined attempt and never elevates | | `--fake-super` | Store/recover privileged attrs via xattrs | ✅ Implemented | Phase 7 Wave B: full record **and replay**. The receiver writes the source `uid:gid:mode:mtime_sec:mtime_nsec` into a reserved `user.fastsync.stat` xattr on each written file (best-effort, fd-relative, format unchanged), then immediately re-applies it via `fake_super_restore_fd`: `fchown` (only where privileged — a non-root EPERM/EACCES is skipped silently, matching FastSync's identity philosophy), `fchmod`, and `futimens`. The OWNER leg is additionally skipped unless an explicit ownership identity policy (`--numeric-ids`/`--usermap`/`--groupmap`/`--chown`/`--copy-as`) is active — `--fake-super` on its own only *records* the source owner and must not act as an un-gated chown primitive — when `--no-super` forbids super-user activities (even for root), or when an active `--copy-as` is authoritative, so the recorded source owner can never override a forced `--copy-as` owner; the xattr record is still stored/replayed for a later privileged restore and mode/mtime still apply, so unprivileged `--fake-super` keeps working. The restored mode goes through the same sanitization as the normal metadata path (group/other write bits are never granted, so a recorded 0666 restores as 0644), so fake-super replay can never grant group/other-write that plain `--preserve` would refuse. Absence or a malformed record is a silent no-op, never fatal. The recording format diverges from rsync's `user.rsync.%stat%`; no cross-tool conversion is attempted. Implies metadata transmission so the source uid/gid/mode/mtime are available. Both it and `-X`/`-A` are incompatible with `-s` (chunk serialization), rejected up front | | `--open-noatime` | Avoid changing access time when opening files | ✅ Implemented | Sender-side policy: the sender opens source files with `O_NOATIME` (Linux) when reading them for transfer, so the open/read does NOT bump the source's on-disk access time. Degrades safely when `O_NOATIME` is unavailable (not defined) or refused (`EPERM`, since it needs `CAP_FOWNER` or file ownership): the code falls back to a normal open, so the data always transfers — only the atime-bump is skipped. It does not itself capture/preserve atime; it only avoids modifying it. **Client-only, never crosses the wire.** Exposed as `file_open_for_read()` and applied to both the buffered data path and the sendfile path | | `--numeric-ids` | Do not map uid/gid by name | ✅ Implemented | Ownership is applied through FastSync's opt-in identity path (see the Phase-4 identity notes below). `--numeric-ids` is a mapping-policy modifier: when applying ownership it uses the transmitted numeric uid/gid directly, skipping the name lookup. Without an ownership-affecting option it is inert (FastSync only applies ownership when the user opts in). It does not need `-M` to be parsed, but ownership is only applied when metadata (hence the source uid/gid) is actually transmitted (see the notes) | | `--usermap=STRING` | Map usernames | ✅ Implemented | Opt-in ownership application. rsync subset implemented: comma-separated `FROM:TO` rules evaluated in order, first match wins; `FROM`/`TO` are group/user names (resolved on the SOURCE machine at parse time), `*` (FROM matches any id / TO = the receiving process's current euid), and an `@N` or bare `N` numeric id. Rules are carried over the wire as resolved numeric id pairs; the receiver applies a matching rule (else falls back to `--chown`, `--numeric-ids`, then a best-effort name lookup) via an fd-relative `fchown`. Malformed/unresolvable specs are rejected with a clear error, never a silent no-op. Implies metadata preservation so the source uid/gid travel. Only effective when the receiver can actually change ownership (root or membership); otherwise it warns and continues | | `--groupmap=STRING` | Map group names | ✅ Implemented | Same rsync subset and semantics as `--usermap` but for the group (gid) side and the group databases. See the Phase-4 identity notes | | `--chown=USER:GROUP` | Map owner and group | ✅ Implemented | Opt-in ownership override applied receiver-side. Forms: `USER:GROUP`, `USER` (owner only), `:GROUP` (group only); a `*` for USER/GROUP means the current/root user or group as appropriate; an `@N`/bare `N` numeric id is accepted. A `:` inside a name may be escaped as `\:`. Equivalent to a trailing `*:*` usermap+groupmap rule (so an explicit `--usermap`/`--groupmap` match wins). Malformed or unresolvable specs are clear parse errors. Implies metadata preservation. Only effective when the receiver has permission to chown; otherwise it warns and continues (rsync parity) | -| `--copy-as=USER[:GROUP]` | Perform the copy as another user/group | ✅ Implemented | Safe-subset implementation, an explicit divergence from rsync's **real identity switching**. rsync makes the receiving process actually assume USER/GROUP (setuid/setgid); FastSync's receiver is multithreaded, so a real credential drop would be unsafe and is never attempted — FastSync never calls `setuid`/`seteuid`/`setgid`. Instead the receiver FORCES the ownership of every entry it writes to `copy_as_uid`/`copy_as_gid` through the existing confined, fd-relative identity path (the same `fchown`/`fchownat` mechanism as `--chown`/`--usermap`/`--groupmap`; symlinks use `fchownat(..., AT_SYMLINK_NOFOLLOW)`, and directories — including intermediate parents created implicitly while writing a nested file — and char/block/FIFO nodes are owned no-follow too, so a directory never keeps the receiver's owner while its children get the target owner), with `--copy-as` at the **highest priority** — it beats usermap/groupmap/`--chown`/`--numeric-ids` and the best-effort name lookup. This REQUIRES a privileged (root) receiver: an unprivileged receiver REFUSES the whole transfer up front at the config handshake (`server_module_gate`, running inside `config_receive_with_validate` before the `STATUS_OK` ack) with a clear error and no file data exchanged — never a silent wrong-ownership result. A server running with an operator `--no-super` veto also refuses it; a privileged (root) standalone/`--stdio` server refuses it by default too and only honors it after the operator passes `--allow-super`, and a **daemon** refuses `--copy-as`, like every other client-chosen-ownership request (`--numeric-ids`/`--chown`/`--usermap`/`--groupmap`/`--fake-super`/explicit `--super`), unless the selected module opts in with `client owner = yes`; without that per-module opt-in a daemon must not honor an arbitrary client-selected owner (a root standalone listener and SSH `--stdio` server honor these for their single operator-authorized root only when started with `--allow-super`). `--fake-super` interaction: `--copy-as` is authoritative, so the recorded source owner is never replayed over the forced target owner. If the ownership apply still fails with EPERM/EACCES (capability-restricted root, root-squash, read-only mount) the failure is logged at ERROR and the **entry is reported as failed** rather than written with the wrong owner, which fails the transfer (fail-fast) so overall success is never reported with the wrong owner. USER is resolved on the client against the user database (a name, an `@N`/bare `N` numeric id, or `*` meaning the client's current euid); when `:GROUP` is present it is resolved against the group database (`*` meaning the client's egid). **Group-default rule:** when the group is omitted FastSync uses the user's primary gid (`getpwuid(uid)->pw_gid`); a numeric id with no local passwd entry has no primary gid to look up, so `gid` falls back to `uid` (documented divergence). Malformed/empty/unresolvable specs are clear parse errors, never a silent no-op. Never elevates privileges and never bypasses the confined receive root. Implies metadata preservation (the source uid/gid must be transmitted). Wire: a new trailing config-frame block **sent after** the `--super` int (presence int, then the two int32 ids, both validated `>= 0` on receive; the ids are also rejected if they do not fit int32 at CLI parse time); `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0** | +| `--copy-as=USER[:GROUP]` | Perform the copy as another user/group | ✅ Implemented | Safe-subset implementation, an explicit divergence from rsync's **real identity switching**. rsync makes the receiving process actually assume USER/GROUP (setuid/setgid); FastSync's receiver is multithreaded, so a real credential drop would be unsafe and is never attempted — FastSync never calls `setuid`/`seteuid`/`setgid`. Instead the receiver FORCES the ownership of every entry it writes to `copy_as_uid`/`copy_as_gid` through the existing confined, fd-relative identity path (the same `fchown`/`fchownat` mechanism as `--chown`/`--usermap`/`--groupmap`; symlinks use `fchownat(..., AT_SYMLINK_NOFOLLOW)`, and directories — including intermediate parents created implicitly while writing a nested file — and char/block/FIFO nodes are owned no-follow too, so a directory never keeps the receiver's owner while its children get the target owner), with `--copy-as` at the **highest priority** — it beats usermap/groupmap/`--chown`/`--numeric-ids` and the best-effort name lookup. This REQUIRES a privileged (root) receiver: an unprivileged receiver REFUSES the whole transfer up front at the config handshake (`server_module_gate`, running inside `config_receive_with_validate` before the `STATUS_OK` ack) with a clear error and no file data exchanged — never a silent wrong-ownership result. A server running with an operator `--no-super` veto also refuses it; a privileged (root) standalone TCP listener refuses it by default too and only honors it after the operator passes `--allow-super` (the flag is rejected with `--stdio`, where the client-composed remote argv could otherwise defeat the default; a forced command is required if the default must hold), and a **daemon** refuses `--copy-as`, like every other client-chosen-ownership request (`--numeric-ids`/`--chown`/`--usermap`/`--groupmap`/`--fake-super`/explicit `--super`), unless the selected module opts in with `client owner = yes`; without that per-module opt-in a daemon must not honor an arbitrary client-selected owner (a root standalone listener honors these for its single operator-authorized root only when started with `--allow-super`). `--fake-super` interaction: `--copy-as` is authoritative, so the recorded source owner is never replayed over the forced target owner. If the ownership apply still fails with EPERM/EACCES (capability-restricted root, root-squash, read-only mount) the failure is logged at ERROR and the **entry is reported as failed** rather than written with the wrong owner, which fails the transfer (fail-fast) so overall success is never reported with the wrong owner. USER is resolved on the client against the user database (a name, an `@N`/bare `N` numeric id, or `*` meaning the client's current euid); when `:GROUP` is present it is resolved against the group database (`*` meaning the client's egid). **Group-default rule:** when the group is omitted FastSync uses the user's primary gid (`getpwuid(uid)->pw_gid`); a numeric id with no local passwd entry has no primary gid to look up, so `gid` falls back to `uid` (documented divergence). Malformed/empty/unresolvable specs are clear parse errors, never a silent no-op. Never elevates privileges and never bypasses the confined receive root. Implies metadata preservation (the source uid/gid must be transmitted). Wire: a new trailing config-frame block **sent after** the `--super` int (presence int, then the two int32 ids, both validated `>= 0` on receive; the ids are also rejected if they do not fit int32 at CLI parse time); `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0** | **Phase-4 metadata-time notes:** `-U/--atimes`, `-N/--crtimes`, `-O/--omit-dir-times`, `-J/--omit-link-times`, and `--open-noatime` are new. @@ -639,7 +639,7 @@ now transmits targets (the prior behavior was broken/partial); its status moved - **Host access control (`hosts allow`/`hosts deny`):** both keys accept a comma- and/or whitespace-separated list of patterns and may appear globally and/or per module (multiple config-file lines append; a `--dparam` override replaces). Supported patterns are `*` (match all), an IPv4 or IPv6 literal (`10.0.0.1`, `2001:db8::1`), and an IPv4/IPv6 CIDR (`10.0.0.0/8`, `2001:db8::/32`). Hostname patterns are **not** supported: because the peer is always a numeric address and no reverse DNS is performed, a hostname/glob pattern would silently never match, so it is rejected at load time (fail-closed) instead of being accepted as a dead rule. An IPv4 peer on a dual-stack IPv6 listener is normalized from its `::ffff:a.b.c.d` form so IPv4 patterns match it. rsync-like semantics: a matching `hosts deny` rejects; if any `hosts allow` entries exist, a peer matching none of them is rejected; deny takes precedence over allow. The daemon enforces the global list first, then the selected module's list, **before authentication** in `server_module_gate`, with an audit log line naming the peer, the module and the outcome. The numeric peer address is obtained with `getpeername`+`inet_ntop` (`utils_fd_peer_ip`, handling both address families); when it cannot be obtained a module with any ACL fails closed (refused), while an ACL-free module continues and logs at debug. A malformed pattern (e.g. an out-of-range CIDR prefix) is a parse error at load time. - **Connection caps, shared registry and auth lockout:** the global `max connections` key (default 100) is plumbed into the listener (`transport_tcp.c`), which rejects a connection once the accept-loop parent's active-child count reaches it; the IPv4/IPv6 peer is logged for every accepted connection. Because the listener forks one child per connection, the per-module `max connections` cap, the global `max connections per host` cap, and the auth-failure counter live in a fixed-size registry carved from an anonymous shared mapping (`daemon_limits.c`, `mmap(MAP_SHARED|MAP_ANONYMOUS)`) created by the parent before the accept loop, so every forked child shares the same counters (C11 atomics only — never a pthread lock, which can deadlock in a forked child). The parent reserves a registry slot per accepted connection and the child records the selected module and source IP once known; the parent's `SIGCHLD` handler reclaims the slot when the child dies (including `SIGKILL`) and re-derives the per-module and per-source occupancy counts from the surviving REGISTERED slots, so a child killed mid-registration cannot leak a count. The per-source table has a bounded lifetime: an entry with no live connection is reclaimed after its lockout expires or it has been idle (300 s); if the table is genuinely full the per-source cap/lockout fails open for new sources (per-module cap and ACLs still apply) with a rate-limited warning. The per-module cap (0 = unlimited) is enforced after the module lookup and before auth; per-source identity reuses the normalized numeric peer address (`utils_fd_peer_ip`, IPv4-mapped IPv6 collapsed to IPv4), and a trusted loopback peer (127.0.0.0/8 / `::1`, `utils_fd_peer_is_local`) is exempt from the per-source cap and the auth lockout because all local clients share one address (the per-module/global caps still apply). Clients behind a shared NAT/proxy address likewise share one per-source budget and lockout counter. A failed authentication increments the shared per-source failure count and, once `auth lockout threshold` (default 10; 0 disables) is reached, the source is refused for `auth lockout duration` seconds (default 300) before any challenge is sent, even when the next attempt is handled by a different forked child; a successful authentication clears the counter. On a failed authentication the per-connection child still sleeps the global `auth failure delay` (default 500 ms, 0 disables, capped at 5000) via `nanosleep`, rate-limiting online guessing without delaying a success. A missing registry (allocation failure) degrades to the global cap and host ACLs rather than refusing to start. - **Module selection & confinement:** the client requests a module with an rsync-style `host::module[/path]` destination. The module name crosses the wire as a trailing string on the config frame (bumping `PROTOCOL_VERSION` 2.14.0 → 2.15.0; the bump is required because the config-frame layout changed and the strict same-version handshake is what prevents a peer from desynchronizing on the new trailing field). The daemon looks the module up in ITS OWN config and uses the module's `path` as the authorized root through the exact same `configure_authorization` confinement the standalone server applies to `--destination-root` (`file_open_secure_parent`, `has_path_traversal`, `path_is_within`); the client never supplies the root, every client-chosen-ownership/super-user request is refused unless the module declares `client owner = yes` (the daemon's per-module opt-in, see below), and the operator `--no-super` veto forces super-user activities off for every daemon connection. The client's `/path` part is relative inside the module and is rejected if absolute or if it contains `..`. Unknown modules are refused before any data moves (the run fails cleanly at the config handshake). An absolute destination and a module request against a non-daemon server are also refused. -- **`client owner` (client-chosen-ownership opt-in):** by default a daemon module refuses every request that would let the client pick an owner or ask for super-user activities — `--numeric-ids`, `--chown`, `--usermap`/`--groupmap`, `--fake-super`, `--copy-as`, and an explicit `--super` — at the config handshake (before `STATUS_OK`), because a daemon has no per-module opt-in for client-chosen ownership and any anonymous client could otherwise force arbitrary owner ids inside the module root. `client owner = yes` opts a single module in, allowing those requests within that module's root (a root standalone listener and SSH `--stdio` server honor them for their single operator-authorized root only when started with `--allow-super`). Without the opt-in the daemon also forces super-user **device** activity off for that connection — char/block device-node creation (`--devices`) and `--write-devices` — even under the default `AUTO` mode, so a non-opted module can never be made to `mknod` or write a raw device; those entries are skipped (not refused) so an ordinary `-a` push still succeeds without device nodes. The opt-in does **not** lift the privilege requirement: `--copy-as` still needs a root receiver, and the operator `--no-super` veto still forces super-user activities off for every connection. The daemon logs a prominent startup warning for each `client owner = yes` module so the operator's deliberate choice is visible. +- **`client owner` (client-chosen-ownership opt-in):** by default a daemon module refuses every request that would let the client pick an owner or ask for super-user activities — `--numeric-ids`, `--chown`, `--usermap`/`--groupmap`, `--fake-super`, `--copy-as`, and an explicit `--super` — at the config handshake (before `STATUS_OK`), because a daemon has no per-module opt-in for client-chosen ownership and any anonymous client could otherwise force arbitrary owner ids inside the module root. `client owner = yes` opts a single module in, allowing those requests within that module's root (a root standalone TCP listener honors them for its single operator-authorized root only when started with `--allow-super`; the flag is rejected with `--stdio`, whose client-composed remote argv must never opt back into super mode). Without the opt-in the daemon also forces super-user **device** activity off for that connection — char/block device-node creation (`--devices`) and `--write-devices` — even under the default `AUTO` mode, so a non-opted module can never be made to `mknod` or write a raw device; those entries are skipped (not refused) so an ordinary `-a` push still succeeds without device nodes. The opt-in does **not** lift the privilege requirement: `--copy-as` still needs a root receiver, and the operator `--no-super` veto still forces super-user activities off for every connection. The daemon logs a prominent startup warning for each `client owner = yes` module so the operator's deliberate choice is visible. - **`read only` safe default:** every network transfer FastSync currently supports is a push that writes under the module root, so a `read only` module refuses the connection (clear server log "module is read only"; the client exits non-zero, nothing is transferred). A future pull/list operation can be opened up when it exists; the knob is already stored. - **`auth users` (A7 SCRAM-SHA-256 authentication):** a module that declares `auth users` requires the client to present credentials. The config frame carries ONLY the username; the daemon answers an auth-required module with `STATUS_AUTH_CHALLENGE` (PBKDF2 iteration count, 16-byte salt, 32-byte server nonce), the client answers with `STATUS_AUTH_RESPONSE` (fresh 32-byte client nonce + a 32-byte ClientProof), and the daemon accepts only when the proof verifies **and** the username is **on the module's `auth users` list** and has a store entry, replying `STATUS_AUTH_OK` with a 32-byte ServerSignature the client verifies before proceeding. Verification is constant-time over fixed 32-byte keys (the compare runs even for a miss), username membership uses a constant-time full-length scan, and an unknown/off-list user still receives a challenge and runs the same math against a dummy verifier: a deterministic per-username salt (`HMAC-SHA256(store dummy key, username)`), the store-wide uniform iteration count and dummy keys. Re-probing the same unknown username therefore yields an identical salt and iteration count while a different username yields a different salt, so there is no user-enumeration or timing oracle. The daemon logs the username but **never the password, proof or keys**. A module WITHOUT `auth users` stays open (legitimate rsync configuration); credentials sent to such a module are ignored. Read-only is orthogonal: even a correctly authenticated push to a `read only` module is still refused (all FastSync network transfers write). Fail-closed policy: a daemon whose config declares `auth users` on any module refuses to start unless a credential store was given (`--password-file` and/or `--early-input`); a missing or empty store is never silently treated as "open". A failed handshake (missing credentials, unknown/off-list user, wrong proof or malformed data) yields a single generic `STATUS_AUTH_FAILED` and the daemon closes before any data moves. The dummy key is persisted in an owner-only `.dummykey` sidecar (auto-created on first load, mode 0600) so the dummy salt stays stable across daemon restarts, closing the restart-gated enumeration channel. The sidecar is secret material and must be protected like the credential store (owner-only 0600, included with the store in backups and rotation). It must be preserved across restarts for that guarantee; if it cannot be created (a process-substitution/FIFO store path such as `/dev/fd/N`, a read-only filesystem, a missing directory, or a create/write/fsync/link/fchmod failure), the daemon logs a warning and uses a transient per-run key, so unknown-user challenges change across restarts and the cross-restart guarantee does not hold for that deployment. One residual is accepted: the store iteration count is observable pre-auth by design, since the miss path must match a hit. **Transport policy (hardening A7-3/S1):** an auth-required module accepts credentials only when either (a) the connection is an encrypted, verified TLS connection whose client certificate matches `--client-cn`, or (b) the connection is plaintext from a loopback TCP peer **and** the operator explicitly passed `--allow-unauthenticated`. A remote plaintext peer, and a loopback plaintext peer without that flag, are refused at the config gate before any challenge is sent; `--allow-unauthenticated` never permits remote plaintext auth (remote peers still require verified TLS). Daemon modules are a `--daemon`-only feature — the SSH `--stdio` path never loads a daemon config and is not an auth transport for them. Because the loopback allowance trusts whichever peer the kernel reports as `127.0.0.1`, it assumes nothing relays remote connections to the daemon: a local TCP forwarder or TLS-terminating proxy in front of an auth-module listener makes remote clients appear as loopback and bypasses the mutual-TLS identity check, so do not front an auth-module listener with such a relay. - **Credential store format:** server `--password-file`/`--early-input` files are line-based `user:$fastsync$1$pbkdf2-sha256$$$$`, one per line (standard base64; 16-byte salt, 32-byte keys; `iters` in `[100000, 10000000]`, default 600000). Every entry in the resulting store must agree on `iters` (a store whose entries disagree, or where a layered `--early-input` disagrees with `--password-file`, is rejected). Generate lines with `fastsync-server --hash-credentials FILE [--iterations N]`; the emitted lines are secret material, so redirect them to an owner-only (mode 0600) file (the tool warns on stderr if stdout is a group/other-accessible regular file). Blank lines and lines starting with `#`/`;` are comments; the parser is strict (a malformed line fails the whole load, so a typo can never let a different set of users in). **The legacy `user:SHA256HEX` form is hard-rejected** with an actionable "legacy" error; there is no auto-upgrade, so a replayable bearer digest can never be loaded by a 2.19.0 daemon. The client `--password-file` holds `user:password` on its first meaningful line (the literal password, used only for the handshake then burned); keep both files readable only by their owner (mode 0600). Per-username wire length is bounded (256 chars) and every decoded salt/key length is validated. Loading the store also maintains an owner-only `.dummykey` sidecar (auto-created, mode 0600, exactly 32 bytes) holding the store-wide dummy key that shapes unknown-user challenges; persist it across daemon restarts so those challenges stay stable, and treat a sidecar with the wrong owner, a mode other than exactly 0600, the wrong size or the wrong type as a fatal load error (fail closed). If the sidecar cannot be created (e.g. a process-substitution store path such as `/dev/fd/N`, a read-only filesystem, a missing directory, or a create/write/fsync/link/fchmod failure), the daemon logs a warning and uses a transient per-run key, so the cross-restart stability guarantee does not hold there. @@ -832,9 +832,9 @@ These are the last compatibility items and the closing phase toward rsync flag p **Wave E (LAST) — Privilege: `--super`/`--no-super` and `--copy-as=USER[:GROUP]` (✅ implemented).** FastSync adopts a **safe-subset + clear-refusal** privilege model: it never blind-elevates and never calls `setuid`/`seteuid`/`setgid`. All privileged operations remain fd-relative and confined below the authorized receive root. -`--super`/`--no-super` set a receiver-side tri-state `Config->super_mode` (`SUPER_MODE_AUTO`/`ON`/`OFF`). `privilege_super_permitted()` / `privilege_super_mode_permitted()` (src/shared/identity.c) return true for `ON` and `AUTO` (AUTO preserves FastSync's historical best-effort attempt, where the kernel refuses an unprivileged call and the caller skips it) and false only for `OFF`. The gate covers every super-user activity FastSync performs: ownership application (`identity_apply_ownership`/`_link`), char/block device-node creation (`file_save_special_to_disk`), writes into an existing device (`--write-devices`), and the `--fake-super` owner replay. Unprivileged FIFO creation is deliberately unaffected. `--super` does **not** imply `--numeric-ids`: ownership is applied only when an explicit identity policy (`--usermap`/`--groupmap`/`--chown`/`--numeric-ids`/`--copy-as`) is also given. `--no-super` suppresses those activities even for a root receiver. A non-root receiver given `--super` logs one warning at activation (`identity_set_active`); each confined attempt is then refused by the kernel and skipped, never aborting. The confinement floor is unchanged (`file_open_secure_parent`, `O_NOFOLLOW`, root/path checks). Operator control: the server CLI accepts `--no-super`, a veto that forces `OFF` for every connection, refuses any client `--copy-as`, and neutralizes an explicit `--super` (the connection is accepted but no super-user activity is attempted). A privileged (root) standalone/`--stdio` server instead defaults to `OFF` and requires the server-only `--allow-super` opt-in to attempt any super-user activity; a non-root server is unchanged. On a daemon, a module that has not opted in with `client owner = yes` additionally has super-user device activity forced off (see the Daemon Mode notes). +`--super`/`--no-super` set a receiver-side tri-state `Config->super_mode` (`SUPER_MODE_AUTO`/`ON`/`OFF`). `privilege_super_permitted()` / `privilege_super_mode_permitted()` (src/shared/identity.c) return true for `ON` and `AUTO` (AUTO preserves FastSync's historical best-effort attempt, where the kernel refuses an unprivileged call and the caller skips it) and false only for `OFF`. The gate covers every super-user activity FastSync performs: ownership application (`identity_apply_ownership`/`_link`), char/block device-node creation (`file_save_special_to_disk`), writes into an existing device (`--write-devices`), and the `--fake-super` owner replay. Unprivileged FIFO creation is deliberately unaffected. `--super` does **not** imply `--numeric-ids`: ownership is applied only when an explicit identity policy (`--usermap`/`--groupmap`/`--chown`/`--numeric-ids`/`--copy-as`) is also given. `--no-super` suppresses those activities even for a root receiver. A non-root receiver given `--super` logs one warning at activation (`identity_set_active`); each confined attempt is then refused by the kernel and skipped, never aborting. The confinement floor is unchanged (`file_open_secure_parent`, `O_NOFOLLOW`, root/path checks). Operator control: the server CLI accepts `--no-super`, a veto that forces `OFF` for every connection, refuses any client `--copy-as`, and neutralizes an explicit `--super` (the connection is accepted but no super-user activity is attempted). A privileged (root) standalone TCP listener instead defaults to `OFF` and requires the server-only `--allow-super` opt-in to attempt any super-user activity (the flag is rejected with `--stdio`, whose client-composed remote argv must never defeat the default; use a forced command if the default must hold); a non-root server is unchanged. On a daemon, a module that has not opted in with `client owner = yes` additionally has super-user device activity forced off (see the Daemon Mode notes). -`--copy-as=USER[:GROUP]` is the safe subset. FastSync's receiver is multithreaded, so a real credential switch is unsafe; instead the receiver forces the ownership of **every entry it writes** — regular files, symlinks, directories (including implicitly-created parents), and special nodes — to the resolved target ids through the confined fd-relative identity path. USER is resolved on the client (name, `@N`/bare N, or `*` = client euid); when `:GROUP` is omitted the user's primary gid is used (falling back to `gid == uid` for a numeric id with no local passwd entry). It requires a privileged (root) receiver: an unprivileged receiver refuses the whole transfer at the config handshake, before `STATUS_OK`, so no data is ever written with the wrong ownership. A `--copy-as` chown failure on a capability-restricted root is logged at ERROR (never silently downgraded). `--copy-as` implies metadata (`--no-preserve` is rejected) and `--fake-super` cannot override it. Daemon policy: a `--daemon` receiver refuses **every** client-chosen-ownership / super-user request — `--numeric-ids`, `--chown`, `--usermap`/`--groupmap`, `--fake-super`, `--copy-as`, and explicit `--super` — unless the selected module opts in with `client owner = yes`; without that per-module opt-in any client could force arbitrary ownership inside the module root (a root standalone listener or SSH-launched `--stdio` server, which each serve one operator-authorized root, honors these requests only when started with `--allow-super`). A `--copy-as` chown failure on a capability-restricted root marks the entry as failed rather than reporting success with the wrong owner. +`--copy-as=USER[:GROUP]` is the safe subset. FastSync's receiver is multithreaded, so a real credential switch is unsafe; instead the receiver forces the ownership of **every entry it writes** — regular files, symlinks, directories (including implicitly-created parents), and special nodes — to the resolved target ids through the confined fd-relative identity path. USER is resolved on the client (name, `@N`/bare N, or `*` = client euid); when `:GROUP` is omitted the user's primary gid is used (falling back to `gid == uid` for a numeric id with no local passwd entry). It requires a privileged (root) receiver: an unprivileged receiver refuses the whole transfer at the config handshake, before `STATUS_OK`, so no data is ever written with the wrong ownership. A `--copy-as` chown failure on a capability-restricted root is logged at ERROR (never silently downgraded). `--copy-as` implies metadata (`--no-preserve` is rejected) and `--fake-super` cannot override it. Daemon policy: a `--daemon` receiver refuses **every** client-chosen-ownership / super-user request — `--numeric-ids`, `--chown`, `--usermap`/`--groupmap`, `--fake-super`, `--copy-as`, and explicit `--super` — unless the selected module opts in with `client owner = yes`; without that per-module opt-in any client could force arbitrary ownership inside the module root (a root standalone TCP listener, which serves one operator-authorized root, honors these requests only when started with `--allow-super`; the flag is rejected with `--stdio`). A `--copy-as` chown failure on a capability-restricted root marks the entry as failed rather than reporting success with the wrong owner. **Wire:** two trailing config-frame blocks after the `--iconv` spec, in fixed order — `send_privilege_options`/`receive_privilege_options` (one `super_mode` int, validated `0..2`), then `send_copy_as_options`/`receive_copy_as_options` (presence int + two int32 ids, validated `>= 0`, with `copy_as_set ⇒ use_metadata`). `PROTOCOL_VERSION` bumped **2.17.0 → 2.18.0**. **Divergences from rsync:** rsync's `--super` elevates the receiver and `--copy-as` actually switches its credentials; FastSync never elevates and only permits/forwards confined attempts, and `--copy-as` forces ownership rather than switching identity. diff --git a/src/server/server.c b/src/server/server.c index b977023..c9432f5 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -37,11 +37,14 @@ static bool allow_unauthenticated; * root), so no super-user activity is attempted and any client --copy-as is * refused. Set once in main before the accept loop / stdio handler. */ static bool server_no_super; -/* --allow-super: standalone/--stdio opt-in that preserves the historical - * permissive super mode for a root receiver. When false, a privileged - * standalone receiver forces SUPER_MODE_OFF for every connection (C3), so a - * client cannot make it create device nodes / write raw devices / apply - * client-chosen ownership. */ +/* --allow-super: locally-launched standalone TCP opt-in that preserves the + * historical permissive super mode for a root receiver. When false, a + * privileged standalone receiver forces SUPER_MODE_OFF for every connection + * (C3), so a client cannot make it create device nodes / write raw devices / + * apply client-chosen ownership. It is REJECTED for --stdio (the SSH remote + * argv is composed by the client, so it must never be able to opt a root + * receiver back into super mode); the --stdio path always keeps the secure + * default. */ static bool server_allow_super; static const char* required_client_cn; /* --iconv CONVERT_SPEC the server was itself started with (borrowed argv @@ -193,17 +196,25 @@ static bool tls_client_identity_allowed(SSL* ssl) { 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)); size_t required_length = strlen(required_client_cn); - /* X509_NAME_get_text_by_NID truncates an over-long CN to the buffer size; a - * returned length at the buffer bound means the CN was silently shortened, so - * a required-name prefix could be matched by a longer CN with extra suffix. - * Reject any result that reached the bound. */ - bool allowed = length >= 0 && (size_t)length < sizeof(common_name) - 1 && - (size_t)length == required_length && required_length < sizeof(common_name) && - credentials_secure_equal(common_name, required_client_cn, required_length); + bool allowed = false; + X509_NAME* subject = X509_get_subject_name(certificate); + int index = subject ? X509_NAME_get_index_by_NID(subject, NID_commonName, -1) : -1; + if (index >= 0) { + X509_NAME_ENTRY* entry = X509_NAME_get_entry(subject, index); + ASN1_STRING* data = entry ? X509_NAME_ENTRY_get_data(entry) : NULL; + /* Convert the CN to UTF-8 to get its FULL byte length: unlike + * X509_NAME_get_text_by_NID (which truncates an over-long CN to the buffer + * and reports the truncated length), ASN1_STRING_to_UTF8 never truncates, so + * an exactly-required-length CN is accepted while an over-long one cannot be + * prefix-matched by a shorter required name. */ + unsigned char* utf8 = NULL; + int cn_length = data ? ASN1_STRING_to_UTF8(&utf8, data) : -1; + if (cn_length >= 0 && (size_t)cn_length == required_length) + allowed = credentials_secure_equal((const char*)utf8, required_client_cn, required_length); + if (utf8) + OPENSSL_free(utf8); + } X509_free(certificate); return allowed; } @@ -602,15 +613,17 @@ static const char* server_module_gate(const Config* config, void* context) { if (gate_ctx) gate_ctx->super_mode_override = SUPER_MODE_OFF; } - /* C3: a privileged (root) STANDALONE/--stdio receiver defaults to - * SUPER_MODE_OFF. Without this a client --devices/--write-devices/--super - * would let a root server create arbitrary device nodes and write raw devices, - * and client-chosen ownership (--numeric-ids/--chown/--usermap/--groupmap) - * would be applied, with no operator opt-in. The operator must pass - * --allow-super to restore the historical permissive behavior; an - * unprivileged receiver is unaffected (the kernel refuses the confined - * attempts) and the daemon path keeps its per-module `client owner = yes` - * gate. */ + /* C3: a privileged (root) STANDALONE receiver defaults to SUPER_MODE_OFF. + * Without this a client --devices/--write-devices/--super would let a root + * server create arbitrary device nodes and write raw devices, and + * client-chosen ownership (--numeric-ids/--chown/--usermap/--groupmap) would + * be applied, with no operator opt-in. The operator must pass --allow-super + * to restore the historical permissive behavior; the flag is rejected for + * --stdio, whose client-composed argv must never defeat this default (an + * operator exposing `fastsync-server --stdio` over SSH needs a forced command + * to keep the permissive behavior). An unprivileged receiver is unaffected + * (the kernel refuses the confined attempts) and the daemon path keeps its + * per-module `client owner = yes` gate. */ if (g_daemon_conf == NULL && geteuid() == 0 && !server_allow_super) { effective.super_mode = SUPER_MODE_OFF; if (gate_ctx) @@ -1041,11 +1054,13 @@ static void print_server_usage(void) { printf(" --no-super Operator veto: never attempt super-user activities\n"); printf(" (ownership, device nodes) even as root, and refuse\n"); printf(" any client --copy-as/--super request\n"); - printf(" --allow-super Standalone/--stdio only: keep super-user activities\n"); - printf(" enabled for a root receiver. Without it a root\n"); - printf(" standalone server forces SUPER_MODE_OFF, so client\n"); + printf(" --allow-super Standalone TCP listener only: keep super-user\n"); + printf(" activities enabled for a root receiver. Without it a\n"); + printf(" root standalone server forces SUPER_MODE_OFF, so client\n"); printf(" --devices/--write-devices/--super and ownership\n"); - printf(" requests are refused/skipped. No effect when not root\n"); + printf(" requests are refused/skipped. Never honored with\n"); + printf(" --stdio (the SSH remote argv is client-composed, so\n"); + printf(" super stays off there); no effect when not root\n"); printf(" --iconv=LOCAL[,REMOTE] Declare this server's LOCAL charset for file-name\n"); printf(" conversion: received names are translated to this\n"); printf(" charset (the wire charset still comes from the\n"); @@ -1170,7 +1185,9 @@ int main(int argc, char* argv[]) { trust_sender = opts.trust_sender; allow_unauthenticated = opts.allow_unauthenticated; server_no_super = opts.no_super; - server_allow_super = opts.allow_super; + /* --stdio rejects --allow-super at parse time; force it off here as well so + * this process-global policy cannot be re-enabled by a future caller. */ + server_allow_super = opts.allow_super && !opts.stdio_mode; server_iconv_spec = opts.iconv_spec; signal(SIGINT, cleanup); signal(SIGTERM, cleanup); diff --git a/src/server/server_cli.c b/src/server/server_cli.c index 3cdaa75..4400106 100644 --- a/src/server/server_cli.c +++ b/src/server/server_cli.c @@ -267,8 +267,20 @@ int server_cli_parse(int argc, char* argv[], ServerCliOptions* opts, char* err, } if (opts->allow_super && opts->daemon_mode) { set_error(err, err_size, - "--allow-super is for a standalone/--stdio server; daemon modules opt in per " - "module with 'client owner = yes'"); + "--allow-super is for a locally-launched standalone TCP server; daemon modules opt " + "in per module with 'client owner = yes'"); + return -1; + } + /* --stdio is the SSH transport: the remote server argv is composed by the + * CLIENT (directly and via --remote-option), so a client could otherwise pass + * --allow-super to a root --stdio receiver and defeat the C3 secure default. + * Never honor it there; the super mode stays forced OFF. An operator who + * must keep the historical permissive behavior over SSH has to launch the + * receiver through a forced command, not via client-composed argv. */ + if (opts->allow_super && opts->stdio_mode) { + set_error(err, err_size, + "--allow-super is not accepted with --stdio (the remote argv is client-composed; " + "use a forced command if the default must hold)"); return -1; } if (opts->hash_iterations_set && opts->hash_credentials_file == NULL) { diff --git a/src/server/server_cli.h b/src/server/server_cli.h index 8965def..ea7d625 100644 --- a/src/server/server_cli.h +++ b/src/server/server_cli.h @@ -45,13 +45,16 @@ typedef struct ServerCliOptions { * device-node creation) even when running as root. Applies to --stdio and * --daemon alike; also makes the server refuse any client --copy-as. */ bool no_super; /* --no-super */ - /* --allow-super: standalone/--stdio only opt-in that keeps the historical - * permissive behavior for a PRIVILEGED (root) receiver. Without it a root - * standalone server forces SUPER_MODE_OFF, so a client --devices / - * --write-devices / --super / ownership request cannot make it create device - * nodes, write raw devices, or apply client-chosen ownership. Non-root - * receivers are unaffected (the kernel refuses the confined attempts). The - * daemon path instead uses the per-module `client owner = yes` opt-in. */ + /* --allow-super: locally-launched standalone TCP listener opt-in that keeps + * the historical permissive behavior for a PRIVILEGED (root) receiver. + * Without it a root standalone server forces SUPER_MODE_OFF, so a client + * --devices / --write-devices / --super / ownership request cannot make it + * create device nodes, write raw devices, or apply client-chosen ownership. + * It is rejected for --stdio: that path's remote argv is composed by the + * client (directly and via --remote-option), so it must never opt a root + * receiver back into super mode. Non-root receivers are unaffected (the + * kernel refuses the confined attempts). The daemon path instead uses the + * per-module `client owner = yes` opt-in. */ bool allow_super; /* --allow-super */ /* --iconv=CONVERT_SPEC: the server's own LOCAL charset declaration. The * client's full spec rides the wire config frame anyway; when the server is diff --git a/src/shared/daemon_conf.c b/src/shared/daemon_conf.c index 0e30cb3..88b836f 100644 --- a/src/shared/daemon_conf.c +++ b/src/shared/daemon_conf.c @@ -461,10 +461,10 @@ static bool apply_module_key(DaemonModule* module, char* key, char* value, char* "max connections", module->name, err, err_size); if (key_equals(key, "hosts allow")) return store_host_list(&module->hosts_allow, &module->hosts_allow_count, value, "hosts allow", - false, module->name, err, err_size); + module->name, false, err, err_size); if (key_equals(key, "hosts deny")) return store_host_list(&module->hosts_deny, &module->hosts_deny_count, value, "hosts deny", - false, module->name, err, err_size); + module->name, false, err, err_size); set_error(err, err_size, "unknown key '%s' in module '%s'", key, module->name); return false; } diff --git a/tests/test_daemon_conf.c b/tests/test_daemon_conf.c index 9b8f412..5d0a46d 100644 --- a/tests/test_daemon_conf.c +++ b/tests/test_daemon_conf.c @@ -517,7 +517,30 @@ static void test_daemon_conf_limits_and_hosts_parse() { free(path); EXPECT_NULL(rejected); EXPECT_TRUE(strstr(err, "must list at least one host pattern") != NULL); + /* The diagnostic must name the offending module. */ + EXPECT_TRUE(strstr(err, "module 'm'") != NULL); } + + /* Per-module host lists APPEND across lines like the global ones. (A swapped + * store_host_list call passed the module name as `replace`, so each line + * silently replaced the previous one and only the last survived.) */ + EXPECT_EQ_INT(write_conf("[m]\npath = /x\n" + "hosts allow = 127.0.0.1\n" + "hosts allow = 10.0.0.0/8\n" + "hosts deny = 192.168.0.1\n" + "hosts deny = 2001:db8::/32\n", + &path), + 0); + conf = daemon_conf_load(path, err, sizeof(err)); + free(path); + EXPECT_NOT_NULL(conf); + EXPECT_EQ_INT(conf->modules[0].hosts_allow_count, 2); + EXPECT_EQ_STR(conf->modules[0].hosts_allow[0], "127.0.0.1"); + EXPECT_EQ_STR(conf->modules[0].hosts_allow[1], "10.0.0.0/8"); + EXPECT_EQ_INT(conf->modules[0].hosts_deny_count, 2); + EXPECT_EQ_STR(conf->modules[0].hosts_deny[0], "192.168.0.1"); + EXPECT_EQ_STR(conf->modules[0].hosts_deny[1], "2001:db8::/32"); + daemon_conf_free(conf); } static void test_daemon_hosts_allowed() { diff --git a/tests/test_server_cli.c b/tests/test_server_cli.c index 04f031b..5c2cca9 100644 --- a/tests/test_server_cli.c +++ b/tests/test_server_cli.c @@ -36,9 +36,10 @@ static void test_server_cli_defaults() { server_cli_options_free(&opts); } -/* C3: --allow-super is the standalone/--stdio opt-in for a privileged receiver; - * it never combines with --no-super, and daemon modules use their own per-module - * `client owner = yes` opt-in instead. */ +/* C3: --allow-super is the locally-launched standalone TCP opt-in for a + * privileged receiver; it never combines with --no-super, is refused with + * --stdio (whose client-composed remote argv must not defeat the default), and + * daemon modules use their own per-module `client owner = yes` opt-in instead. */ static void test_server_cli_allow_super() { const char* args[] = {"fastsync-server", "--allow-super", "--destination-root", "/srv"}; ServerCliOptions opts; @@ -54,6 +55,12 @@ static void test_server_cli_allow_super() { const char* a2[] = {"s", "--daemon", "--config=/tmp/x.conf", "--allow-super"}; EXPECT_EQ_INT(server_cli_parse(4, (char**)a2, &opts, err, sizeof(err)), -1); EXPECT_TRUE(strstr(err, "client owner") != NULL); + + /* The SSH/--stdio receiver argv is composed by the client, so --allow-super + * must be rejected there and the C3 secure default stays in force. */ + const char* a3[] = {"s", "--stdio", "--allow-super", "--destination-root", "/srv"}; + EXPECT_EQ_INT(server_cli_parse(5, (char**)a3, &opts, err, sizeof(err)), -1); + EXPECT_TRUE(strstr(err, "--stdio") != NULL); server_cli_options_free(&opts); } From 5893de4a34ada1a2b19e159b1d3389ba2375929b Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 17:19:27 +0200 Subject: [PATCH 15/16] =?UTF-8?q?fix(receiver):=20close=20re-review=20find?= =?UTF-8?q?ings=20=E2=80=94=20dry-run=20basis=20oracle,=20ACL=20capture,?= =?UTF-8?q?=20fsync=20reopen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to a237043 addressing three security/correctness re-review findings. (1) MEDIUM: a server-contacting --dry-run with --compare-dest/--copy-dest/ --link-dest still read and hashed the basis file and compared it with the client-supplied digest, a 1-bit content oracle. basis_match_find() gains a hash_content parameter; the dry-run shortcut passes false and returns no match without touching basis bytes, so an otherwise-matching entry is reported as would-transfer. The real (non-dry-run) path is unchanged. (2) LOW: xattr_capture_path() hardcoded preserve_acls=true, so the receiver's hard-link copy fallback re-applied system.posix_acl_* even when -A was not negotiated. The function now takes preserve_acls and members.* is unaffected; scanner and receiver callers thread the negotiated flag. (3) INFO: the --fsync --link-dest temp reopen now uses O_NONBLOCK and treats a raced-in FIFO's ENXIO as a benign fsync-skip instead of blocking. Tests: dry-run + basis unit test (asserts would-transfer, no content read) and integration test; xattr capture ACL-filter test. Verified strict build, ASan, clang-format, cppcheck, and the CI integration subset. --- src/client/scanner.c | 4 +- src/shared/file.c | 16 ++++-- src/shared/file_receive.c | 41 ++++++++++---- src/shared/xattr.c | 7 ++- src/shared/xattr.h | 11 ++-- tests/integration/test_features.py | 21 +++++++ tests/test_server.c | 88 ++++++++++++++++++++++++++++++ tests/test_xattr.c | 75 +++++++++++++++++++++++++ 8 files changed, 240 insertions(+), 23 deletions(-) diff --git a/src/client/scanner.c b/src/client/scanner.c index 3a8722d..3266c0a 100644 --- a/src/client/scanner.c +++ b/src/client/scanner.c @@ -179,7 +179,7 @@ static bool entry_passes_selection(const FileListSet* file_list, const FilterRul static void scanner_capture_xattrs(const DirectoryScanner* scanner, File* file) { if (!scanner || !file || !(scanner->options.preserve_xattrs || scanner->options.preserve_acls)) return; - file->xattrs = xattr_capture_path(file->path); + file->xattrs = xattr_capture_path(file->path, scanner->options.preserve_acls); } /* Apply --hard-links (-H) detection to one regular File. On a sibling (a @@ -1434,7 +1434,7 @@ static void scan_root_entry(const ScannerOptions* options, const FilterNode* roo } if ((options->preserve_xattrs || options->preserve_acls) && !(file->link_group != 0 && !file->link_first)) - file->xattrs = xattr_capture_path(file->path); + file->xattrs = xattr_capture_path(file->path, options->preserve_acls); if (!array_list_add(root_files, file)) { free(rel); file_destroy(file); diff --git a/src/shared/file.c b/src/shared/file.c index 47b13ab..6d4b7b9 100644 --- a/src/shared/file.c +++ b/src/shared/file.c @@ -1248,11 +1248,19 @@ static bool file_to_disk_secure_link_impl(const char* path, const char* basis_pa if (linked) { int target_dirfd = scratch_dirfd >= 0 ? scratch_dirfd : dirfd; if (use_fsync) { - int tfd = openat(target_dirfd, tmp, O_RDONLY | O_NOFOLLOW | O_CLOEXEC); - if (tfd < 0 || fsync(tfd) != 0) { + /* O_NONBLOCK: the freshly linked temp is normally the basis's regular + file, but a raced-in FIFO at the name must not block this reopen + forever. With O_NONBLOCK such an open fails with ENXIO instead of + blocking, which is treated as a benign fsync-skip (the link itself + is still installed); any other open/fsync failure falls back to the + byte-copy path as before. */ + int tfd = openat(target_dirfd, tmp, O_RDONLY | O_NOFOLLOW | O_CLOEXEC | O_NONBLOCK); + if (tfd < 0) { + if (errno != ENXIO) + linked = false; + } else if (fsync(tfd) != 0) { linked = false; - if (tfd >= 0) - close(tfd); + close(tfd); } else { close(tfd); } diff --git a/src/shared/file_receive.c b/src/shared/file_receive.c index 8e3c2c7..4ead296 100644 --- a/src/shared/file_receive.c +++ b/src/shared/file_receive.c @@ -263,7 +263,8 @@ static FileSaveResult file_save_hardlink_sibling(const char* root_directory, con free(destination_path); return absent_result; } - FileXattrList* sibling_xattrs = cfg->use_xattrs ? xattr_capture_path(staged_first) : NULL; + FileXattrList* sibling_xattrs = + cfg->use_xattrs ? xattr_capture_path(staged_first, cfg->preserve_acls) : NULL; bool ok = file_to_disk_secure_link_attrs( staged_sibling, staged_first, content, content_size, preallocate, file->metadata, preserve_executability, use_fsync, sibling_xattrs, cfg ? cfg->fake_super : false, NULL); @@ -295,7 +296,8 @@ static FileSaveResult file_save_hardlink_sibling(const char* root_directory, con return absent_result; } const char* temp_dir = (cfg && cfg->temp_dir) ? cfg->temp_dir : NULL; - FileXattrList* sibling_xattrs = cfg->use_xattrs ? xattr_capture_path(first_disk) : NULL; + FileXattrList* sibling_xattrs = + cfg->use_xattrs ? xattr_capture_path(first_disk, cfg->preserve_acls) : NULL; bool ok = file_to_disk_secure_link_attrs( destination_path, first_disk, content, content_size, preallocate, file->metadata, preserve_executability, use_fsync, sibling_xattrs, cfg ? cfg->fake_super : false, temp_dir); @@ -1276,14 +1278,27 @@ static bool basis_quick_matches(const Config* config, const struct stat* st, tim /* Search the basis-dir list in command-line order and return the first exact match. When load_content is true the matched bytes are kept in out->content - so the caller can materialize the file without re-reading it. */ + so the caller can materialize the file without re-reading it. + + An exact match ALSO requires the basis bytes' digest to equal the source's, + so `hash_content` gates the content read/hash itself. A server-contacting + --dry-run passes hash_content=false: no basis file may be read or hashed + (that would be a 1-bit content oracle against a client-supplied digest), so a + metadata-only pass can never confirm a hit and declines it. The real path + always passes hash_content=true, keeping its behavior byte-for-byte. */ static bool basis_match_find(const Config* config, const char* check_path, unsigned long long check_size, time_t check_mtime, long check_mtime_nsec, const uint8_t* check_digest, - size_t check_digest_len, bool load_content, BasisMatch* out) { + size_t check_digest_len, bool load_content, bool hash_content, + BasisMatch* out) { memset(out, 0, sizeof(*out)); if (!config || !config_has_basis(config) || config->ignore_times) return false; + /* Dry-run: never read/hash basis content. A hit cannot be decided from + metadata alone, so report no match (the caller treats it as would-transfer) + without touching the file's contents. */ + if (!hash_content) + return false; for (int i = 0; i < config->basis_count; i++) { const BasisDest* entry = &config->basis_dirs[i]; char* basis_dir = path_cat(config->receive_root_directory, entry->path); @@ -1911,10 +1926,13 @@ static IncrementalCheckOutcome incremental_check_quick_skip(IncrementalCheckStat When dry_run is set and the file is not already up to date the receiver must materialize nothing (no basis link/copy, no append/delta/full transfer) and the sender must send no data, so answer STATUS_DRY_RUN_TRANSFER and stop. - The one exception is a --compare-dest exact hit with no destination copy: a - real run would suppress the data without changing the destination, so it - reports as a skip (STATUS_OK) exactly as the full basis path below would. - Everything read here (destination file, basis candidates) is read-only. */ + + The basis lookup is deliberately content-blind: a real run would only accept + a --compare-dest exact hit after hashing the basis file and comparing it with + the client-supplied digest, which in a dry-run is a 1-bit content oracle. + Under dry_run no basis bytes may be read, so an otherwise-matching entry is + treated as would-transfer instead of a skip. Everything read here (the + destination file's metadata, basis candidates' metadata) is read-only. */ static IncrementalCheckOutcome incremental_check_dry_run_shortcut(IncrementalCheckState* state, bool* skipped, bool* would_transfer) { @@ -1925,9 +1943,12 @@ static IncrementalCheckOutcome incremental_check_dry_run_shortcut(IncrementalChe bool skip_via_compare = false; if (config_has_basis(config) && !config->ignore_times) { BasisMatch basis; + /* hash_content=false: a dry-run must not read or hash the basis file. No + content comparison is possible, so no compare-dest hit can be confirmed + and an otherwise-matching file is reported as would-transfer. */ basis_match_find(config, state->check_path, state->check_size, (time_t)state->check_mtime, (long)state->check_mtime_nsec, state->check_digest, state->check_digest_len, - false, &basis); + false, false, &basis); if (basis.hit && basis.type == BASIS_DEST_COMPARE && !state->has_old_file) skip_via_compare = true; basis_match_free(&basis); @@ -1955,7 +1976,7 @@ static IncrementalCheckOutcome incremental_check_try_basis(IncrementalCheckState BasisMatch basis; basis_match_find(config, state->check_path, state->check_size, (time_t)state->check_mtime, (long)state->check_mtime_nsec, state->check_digest, state->check_digest_len, - true, &basis); + true, true, &basis); if (basis.hit) { if (basis.type == BASIS_DEST_COMPARE) { basis_match_free(&basis); diff --git a/src/shared/xattr.c b/src/shared/xattr.c index 269a867..4cfb363 100644 --- a/src/shared/xattr.c +++ b/src/shared/xattr.c @@ -116,7 +116,7 @@ static bool xattr_name_is_posix_acl(const char* name) { /* ---- SENDER: capture ---- */ -FileXattrList* xattr_capture_path(const char* path) { +FileXattrList* xattr_capture_path(const char* path, bool preserve_acls) { if (!path) return NULL; ssize_t list_size = listxattr(path, NULL, 0); @@ -144,8 +144,9 @@ FileXattrList* xattr_capture_path(const char* path) { break; /* trailing double NUL not expected; stop */ offset += (ssize_t)name_len + 1; /* Capture is sender-side: the scanner has already gated on -X/-A, so the - per-name whitelist here allows the ACL names (true). */ - if (!xattr_name_appliable(name, true)) + per-name whitelist here allows the ACL names only when --acls was + negotiated. Without it a plain -X capture never carries an ACL. */ + if (!xattr_name_appliable(name, preserve_acls)) continue; ssize_t value_size = getxattr(path, name, NULL, 0); if (value_size < 0) diff --git a/src/shared/xattr.h b/src/shared/xattr.h index 5c16c53..8277db1 100644 --- a/src/shared/xattr.h +++ b/src/shared/xattr.h @@ -65,10 +65,13 @@ bool xattr_list_append(FileXattrList* list, const char* name, const void* value, * Used for both capture and receiver-side validation. */ bool xattr_name_appliable(const char* name, bool preserve_acls); -/* Sender: read the whitelisted xattrs of `path` into a new list. Returns NULL - * when the path has no appliable xattrs (or the filesystem has no xattr - * support); an empty-but-valid list is never returned distinct from NULL. */ -FileXattrList* xattr_capture_path(const char* path); +/* Sender: read the whitelisted xattrs of `path` into a new list. The POSIX ACL + * names are captured only when `preserve_acls` (--acls/-A) is set, so a plain + * -X run never carries an ACL it was not asked to preserve; `user.*` is + * unaffected. Returns NULL when the path has no appliable xattrs (or the + * filesystem has no xattr support); an empty-but-valid list is never returned + * distinct from NULL. */ +FileXattrList* xattr_capture_path(const char* path, bool preserve_acls); /* Wire: bounded serialization. xattr_send returns false on write failure; an * empty/NULL list transmits a zero-count block. xattr_receive returns NULL and diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index 1c34c00..34f71b0 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -3813,6 +3813,27 @@ class TestBasisDestDirs: assert _read_file(os.path.join(received, self.ADDED)) == \ self._source_tree("c")[self.ADDED], "added file not transferred" + @pytest.mark.ci + def test_dry_run_compare_dest_does_not_read_basis(self, shared_server): + # A dry-run --compare-dest must never read/hash the basis file: doing so + # is a 1-bit content oracle against the client-supplied digest. Even a + # byte-identical basis with a matching size+mtime is therefore reported + # as would-transfer, and nothing is created. + source = self._make_source("basis_dry_src", {self.UNCHANGED: b"stable content v1\n"}) + dest = os.path.join(TEST_DATA_DIR, "basis_dry_dst") + clean_dir(dest) + self._seed_basis(dest, source, "drybasis", {self.UNCHANGED: b"stable content v1\n"}) + before = _snapshot_tree(dest) + result, _ = run_client(source, dest, + flags=["--compare-dest=drybasis", "--dry-run"], + port=shared_server.port) + assert result.returncode == 0, \ + f"dry-run compare-dest failed: {result.stderr[:300]}" + assert self.UNCHANGED in result.stdout, ( + "dry-run compare-dest silently skipped: receiver read the basis content" + ) + assert _snapshot_tree(dest) == before, "dry-run compare-dest mutated the destination" + def test_compare_dest_content_mismatch_forces_transfer(self, shared_server): # The basis holds a file with a DIFFERENT body: even though it shares # the mtime pin, the xxHash check fails and the data must be sent. diff --git a/tests/test_server.c b/tests/test_server.c index 2dff95e..a43e605 100644 --- a/tests/test_server.c +++ b/tests/test_server.c @@ -1,4 +1,5 @@ #include "test_server.h" +#include "checksum.h" #include "config.h" #include "delta.h" #include "file.h" @@ -910,6 +911,92 @@ static void test_incremental_check_fifo_destination_does_not_hang() { } } +/* A server-contacting --dry-run with an alternate basis dir must never read or + hash the basis file. An exact (size+mtime+content) basis match would + otherwise let a client probe the basis bytes against its own supplied digest + (a 1-bit content oracle). The dry-run decision is metadata-only, so even a + byte-identical basis is reported as would-transfer, not a compare-dest skip. */ +static void test_incremental_check_dry_run_basis_does_not_read_content() { + Config* cfg = config_create(); + EXPECT_NOT_NULL(cfg); + cfg->dry_run = true; + char* root = make_check_root("dryb"); + EXPECT_NOT_NULL(root); + cfg->receive_root_directory = str_dup(root); + + char basis_dir[1024]; + char basis_path[2048]; + snprintf(basis_dir, sizeof(basis_dir), "%s/basis", root); + EXPECT_EQ_INT(mkdir(basis_dir, 0700), 0); + const char* content = "basis content that matches\n"; + write_check_file(basis_dir, "file.txt", content); + snprintf(basis_path, sizeof(basis_path), "%s/file.txt", basis_dir); + struct stat bst; + EXPECT_EQ_INT(stat(basis_path, &bst), 0); + EXPECT_EQ_INT(config_basis_append(cfg, BASIS_DEST_COMPARE, "basis"), 0); + + /* The (correct) source digest for the basis bytes: an unfixed dry-run would + read+hash the basis and treat this as an exact compare-dest hit. */ + uint8_t digest[CHECKSUM_MAX_DIGEST_LEN]; + size_t digest_len = 0; + EXPECT_TRUE(checksum_digest((ChecksumAlgo)cfg->checksum_algo, cfg->checksum_seed, content, + strlen(content), digest, sizeof(digest), &digest_len)); + + int p[2]; + EXPECT_EQ_INT(socketpair(AF_UNIX, SOCK_STREAM, 0, p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + + pid_t pid = fork(); + if (pid == 0) { + alarm(30); + close(p[1]); + io_set_fds(p[0], p[0]); + bool skipped = false; + bool would_transfer = false; + File* file = receive_incremental_check_ex(p[0], cfg, &skipped, &would_transfer); + bool ok = file == NULL && !skipped && would_transfer; + file_destroy(file); + config_delete(cfg); + close(p[0]); + _exit(ok ? 0 : 1); + } else { + close(p[0]); + io_set_fds(p[1], p[1]); + EXPECT_TRUE(send_str(p[1], "file.txt")); + unsigned long long size = (unsigned long long)bst.st_size; + long long mtime = (long long)bst.st_mtime; + long long mtime_nsec = 0; +#ifdef __linux__ + mtime_nsec = (long long)bst.st_mtim.tv_nsec; +#endif + EXPECT_TRUE(send_n_data(p[1], &size, sizeof(size))); + EXPECT_TRUE(send_n_data(p[1], &mtime, sizeof(mtime))); + EXPECT_TRUE(send_n_data(p[1], &mtime_nsec, sizeof(mtime_nsec))); + uint8_t wire_len = (uint8_t)digest_len; + EXPECT_TRUE(send_n_data(p[1], &wire_len, sizeof(wire_len))); + EXPECT_TRUE(send_n_data(p[1], digest, digest_len)); + Status s; + EXPECT_TRUE(receive_status(p[1], &s)); + /* A skip here would mean the receiver read+hashed the basis file. */ + EXPECT_EQ_INT(s, STATUS_DRY_RUN_TRANSFER); + + int status; + waitpid(pid, &status, 0); + close(p[1]); + config_delete(cfg); + /* The dry-run must not have materialized anything in the receive root. */ + char dest_path[2048]; + snprintf(dest_path, sizeof(dest_path), "%s/file.txt", root); + EXPECT_FALSE(file_path_exists_secure(dest_path)); + unlink(basis_path); + rmdir(basis_dir); + rmdir(root); + free(root); + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + } +} + /* B1: a FIFO planted in a --link-dest basis directory must not block * basis_open_regular() either; the basis match is simply declined. */ static void test_incremental_check_basis_fifo_does_not_hang() { @@ -1024,6 +1111,7 @@ void test_server() { test_incremental_check_delta_oversize_reports_failure(); test_incremental_check_fifo_destination_does_not_hang(); test_incremental_check_basis_fifo_does_not_hang(); + test_incremental_check_dry_run_basis_does_not_read_content(); test_receive_manifest_total_entry_cap(); test_late_manifest_abort_frees_keepset(); test_late_manifest_eof_frees_keepset(); diff --git a/tests/test_xattr.c b/tests/test_xattr.c index f82cb3f..c64ff1b 100644 --- a/tests/test_xattr.c +++ b/tests/test_xattr.c @@ -273,6 +273,80 @@ static void test_link_copy_fallback_preserves_xattrs() { rmdir(basis_dir); } +/* Capture must honor --acls: xattr_capture_path(path, false) (plain -X) must + * never return the POSIX ACL names, while xattr_capture_path(path, true) (-A) + * does; user.* is captured either way. This is the capture-side counterpart of + * the receiver's --acls gate and must not depend on the caller having checked + * the flag. Guarded on filesystem/ACL support. */ +static void test_xattr_capture_filters_acls() { + const char* path = "test_xattr_capture_acls.txt"; + unlink(path); + int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0600); + if (fd < 0) + return; + bool has_xattr = setxattr(path, "user.fastsync.xprobe", "p", 1, 0) == 0; + if (has_xattr) + removexattr(path, "user.fastsync.xprobe"); + if (!has_xattr) { + close(fd); + unlink(path); + return; /* filesystem without xattr support */ + } + if (setxattr(path, "user.keep", "yes", 3, 0) != 0) { + close(fd); + unlink(path); + return; + } + + /* Synthesize a valid non-trivial POSIX access ACL blob (little-endian): + version 2 followed by USER_OBJ/USER/GROUP_OBJ/MASK/OTHER entries. */ + uint32_t acl_uid = geteuid() == 0 ? 65534u : (uint32_t)geteuid(); + unsigned char blob[4 + 5 * 8]; + uint32_t version = 2; + memcpy(blob, &version, 4); + const uint16_t tags[5] = {0x01, 0x02, 0x04, 0x10, 0x20}; /* OBJ/USER/GROUP/MASK/OTHER */ + const uint16_t perms[5] = {0x04, 0x04, 0x04, 0x04, 0x00}; + const uint32_t ids[5] = {0xFFFFFFFFu, acl_uid, 0xFFFFFFFFu, 0xFFFFFFFFu, 0xFFFFFFFFu}; + size_t off = 4; + for (int i = 0; i < 5; i++) { + memcpy(blob + off, &tags[i], sizeof(tags[i])); + off += sizeof(tags[i]); + memcpy(blob + off, &perms[i], sizeof(perms[i])); + off += sizeof(perms[i]); + memcpy(blob + off, &ids[i], sizeof(ids[i])); + off += sizeof(ids[i]); + } + if (setxattr(path, "system.posix_acl_access", blob, off, 0) != 0) { + close(fd); + unlink(path); + return; /* no unprivileged ACL support: skip silently */ + } + close(fd); + + FileXattrList* plain = xattr_capture_path(path, false); + FileXattrList* with_acls = xattr_capture_path(path, true); + bool plain_user = false, plain_acl = false, acl_user = false, acl_acl = false; + for (int i = 0; plain && i < plain->count; i++) { + if (strcmp(plain->items[i].name, "user.keep") == 0) + plain_user = true; + if (strcmp(plain->items[i].name, "system.posix_acl_access") == 0) + plain_acl = true; + } + for (int i = 0; with_acls && i < with_acls->count; i++) { + if (strcmp(with_acls->items[i].name, "user.keep") == 0) + acl_user = true; + if (strcmp(with_acls->items[i].name, "system.posix_acl_access") == 0) + acl_acl = true; + } + EXPECT_TRUE(plain_user); + EXPECT_FALSE(plain_acl); + EXPECT_TRUE(acl_user); + EXPECT_TRUE(acl_acl); + xattr_list_free(plain); + xattr_list_free(with_acls); + unlink(path); +} + /* --fake-super replay: fake_super_store_fd records the source stat into the * reserved xattr, and fake_super_restore_fd re-applies mode/mtime (and owner, * when the process may) fd-relative. Restore must also be a safe no-op with no @@ -409,6 +483,7 @@ void test_xattr() { test_xattr_reject_oversized_value(); test_xattr_count_bound(); test_xattr_capture_and_appliable(); + test_xattr_capture_filters_acls(); test_xattr_receive_drops_acl_without_preserve_acls(); test_link_copy_fallback_preserves_xattrs(); test_fake_super_restore(); From d5fcfa2c5ce42f78e144cdc375078ce918d194ad Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 17:38:23 +0200 Subject: [PATCH 16/16] style(ssh): drop redundant condition flagged by cppcheck --- src/shared/transport_ssh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/transport_ssh.c b/src/shared/transport_ssh.c index b9d1f93..7236133 100644 --- a/src/shared/transport_ssh.c +++ b/src/shared/transport_ssh.c @@ -80,7 +80,7 @@ static int parse_remote_dest(const char* dest, RemoteDest* r) { * crafted destination can never inject an ssh option such as * -oProxyCommand=... . This mirrors config_parse_ssh_dest's validation and * is defense-in-depth for callers that bypass it. */ - if (r->host[0] == '\0' || r->host[0] == '-' || (r->user[0] != '\0' && r->user[0] == '-')) { + if (r->host[0] == '\0' || r->host[0] == '-' || r->user[0] == '-') { remote_dest_destroy(r); return -1; }