TapTap/FastSync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

fix(client): harden CLI args, log escaping, and local artifact opens

Browse Source 
- 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.
This commit is contained in:
TapTap
2026-09-14 16:39:19 +02:00
parent dfa2a42028
commit 10c4ffebdf
6 changed files with 169 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/client/client_cli.c
+56 -9
View File
@@ -19,6 +19,7 @@
#include "usage.h"
#include "utils.h"
#include <errno.h>
#include <fcntl.h>
#include <limits.h>
#include <time.h>
#include <signal.h>
@@ -27,6 +28,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
/* 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 : "<allocation failed>", 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 : "<allocation failed>", strerror(errno));
escaped ? escaped : "<allocation failed>", 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 : "<allocation failed>", (int)UTILS_MAX_LINE_LEN);
} else {
log_message(LOG_LEVEL_ERROR, "could not read pattern file '%s': %s",
escaped ? escaped : "<allocation failed>", strerror(errno));
}
free(escaped);
free(line);
fclose(fp);
return -1;
}
if (n == 0)
break;
char* p = line;
while (*p == ' ' || *p == '\t')
p++;
src/client/client_send.c
+25 -8
View File
@@ -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 : "<allocation failed>");
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 : "<allocation failed>");
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 : "<allocation failed>",
escaped_src ? escaped_src : "<allocation failed>");
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 : "<allocation failed>");
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 : "<no path>");
escaped_path ? escaped_path : "<allocation failed>");
free(escaped_path);
ok = false;
break;
}
src/client/client_validation.c
+6 -4
View File
@@ -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) {
src/client/scanner.c
+14 -3
View File
@@ -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 : "<allocation failed>", 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 : "<allocation failed>");
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 : "<allocation failed>");
free(escaped_entry);
}
free(abs_path);
scanner->failed = true;
return NULL;