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(); }