diff --git a/RSYNC_COMPAT.md b/RSYNC_COMPAT.md index f60d539..d71a103 100644 --- a/RSYNC_COMPAT.md +++ b/RSYNC_COMPAT.md @@ -212,7 +212,7 @@ This document maps rsync's full feature set to FastSync's current implementation | Max data/string/chunk sizes | Prevent OOM attacks | ✅ Implemented | Per-message limits | | Per-connection memory limit | 1GB per connection | ✅ Implemented | `MAX_CONNECTION_MEMORY` | | `--trust-sender` | Trust remote sender's file list | ❌ Not Implemented | | -| `--old-args` | Disable modern arg protection | ❌ Not Implemented | | +| `--old-args` | Disable modern arg protection | ✅ Implemented | SSH-only legacy mode; restores raw remote command construction and permits shell interpretation of the configured server path | | `--ignore-missing-args` | Ignore missing source args | ❌ Not Implemented | | | `--delete-missing-args` | Delete missing source args | ❌ Not Implemented | | diff --git a/src/client/client_cli.c b/src/client/client_cli.c index d32488d..770e815 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -371,6 +371,7 @@ static const OptionEntry OPTION_TABLE[] = { {"--partial", NULL, OPT_FLAG, offsetof(Config, partial)}, {"--secluded-args", NULL, OPT_NOOP, 0}, {"--update", "-u", OPT_FLAG, offsetof(Config, update)}, + {"--old-args", NULL, OPT_FLAG, offsetof(Config, old_args)}, {"--links", "-l", OPT_FLAG, offsetof(Config, follow_symlinks)}, {"--copy-links", NULL, OPT_FLAG, offsetof(Config, copy_links)}, {"--safe-links", NULL, OPT_FLAG, offsetof(Config, safe_links)}, diff --git a/src/client/client_send.c b/src/client/client_send.c index 0d47a77..5adfa11 100644 --- a/src/client/client_send.c +++ b/src/client/client_send.c @@ -57,7 +57,7 @@ static Client* connect_transfer_client(const Config* config) { return NULL; } return client_connect_ssh(config->ssh_destination, config->ssh_port, - config->fastsync_server_path); + config->fastsync_server_path, config->old_args); } Client* client = client_create(); diff --git a/src/client/usage.c b/src/client/usage.c index 3dc3182..f754440 100644 --- a/src/client/usage.c +++ b/src/client/usage.c @@ -83,6 +83,8 @@ void print_usage(void) { printf(" --partial-dir Directory for partial files\n"); printf(" --fastsync-server-path \n"); printf(" Path to fastsync-server on remote (default: fastsync-server)\n"); + printf( + " --old-args Disable safe SSH command argument quoting (legacy compatibility)\n"); printf(" -l, --links Copy symlinks as symlinks\n"); printf(" --copy-links Transform symlinks into referent files\n"); printf(" --safe-links Skip symlinks that point outside transfer tree\n"); diff --git a/src/shared/config.c b/src/shared/config.c index 1eec716..b480918 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -96,6 +96,7 @@ static void config_set_defaults(Config* config) { config->relative = false; config->rsh_command = NULL; config->rsync_path = NULL; + config->old_args = false; config->temp_dir = NULL; config->compare_dest = NULL; config->copy_dest = NULL; diff --git a/src/shared/config.h b/src/shared/config.h index 8a84ecd..1bd6a46 100644 --- a/src/shared/config.h +++ b/src/shared/config.h @@ -109,6 +109,7 @@ typedef struct Config { // Issue #130: Remote shell/connection options char* rsh_command; char* rsync_path; + bool old_args; char* temp_dir; char* compare_dest; char* copy_dest; diff --git a/src/shared/transport_ssh.c b/src/shared/transport_ssh.c index 9e663c7..d63d920 100644 --- a/src/shared/transport_ssh.c +++ b/src/shared/transport_ssh.c @@ -3,6 +3,7 @@ #include "utils.h" #include #include +#include #include #include #include @@ -15,6 +16,12 @@ typedef struct { char* remote_path; } RemoteDest; +static void ssh_child_setup_failed(int status_fd) { + ssize_t wret = write(status_fd, "x", 1); + (void)wret; + _exit(1); +} + static void remote_dest_destroy(RemoteDest* r) { free(r->user); free(r->host); @@ -68,7 +75,52 @@ static int parse_remote_dest(const char* dest, RemoteDest* r) { return 0; } -Client* client_connect_ssh(const char* destination, int port, const char* server_path) { +char* ssh_build_remote_command(const char* server_path, bool old_args) { + const char* path = server_path ? server_path : "fastsync-server"; + const char* suffix = " --stdio"; + size_t path_len = strlen(path); + size_t suffix_len = strlen(suffix); + + if (old_args) { + if (path_len > SIZE_MAX - suffix_len - 1) + return NULL; + char* command = malloc(path_len + suffix_len + 1); + if (!command) + return NULL; + memcpy(command, path, path_len); + memcpy(command + path_len, suffix, suffix_len + 1); + return command; + } + + /* Quote the executable as one remote-shell word. This is the default safety boundary. */ + size_t quote_count = 0; + for (const char* p = path; *p; p++) + if (*p == '\'') + quote_count++; + if (path_len > SIZE_MAX - suffix_len - 4 || + quote_count > (SIZE_MAX - path_len - suffix_len - 4) / 4) + return NULL; + size_t command_len = path_len + quote_count * 4 + suffix_len + 4; + char* command = malloc(command_len + 1); + if (!command) + return NULL; + char* out = command; + *out++ = '\''; + for (const char* p = path; *p; p++) { + if (*p == '\'') { + memcpy(out, "'\\''", 4); + out += 4; + } else { + *out++ = *p; + } + } + *out++ = '\''; + memcpy(out, suffix, suffix_len + 1); + return command; +} + +Client* client_connect_ssh(const char* destination, int port, const char* server_path, + bool old_args) { RemoteDest r; if (parse_remote_dest(destination, &r) != 0) { char* escaped = output_escape(destination, false); @@ -113,11 +165,12 @@ Client* client_connect_ssh(const char* destination, int port, const char* server if (pid == 0) { close(sv[0]); close(exec_pipe[0]); - fcntl(exec_pipe[1], F_SETFD, FD_CLOEXEC); - if (sv[1] != STDIN_FILENO) - dup2(sv[1], STDIN_FILENO); - if (sv[1] != STDOUT_FILENO) - dup2(sv[1], STDOUT_FILENO); + if (fcntl(exec_pipe[1], F_SETFD, FD_CLOEXEC) < 0) + ssh_child_setup_failed(exec_pipe[1]); + if (sv[1] != STDIN_FILENO && dup2(sv[1], STDIN_FILENO) < 0) + ssh_child_setup_failed(exec_pipe[1]); + if (sv[1] != STDOUT_FILENO && dup2(sv[1], STDOUT_FILENO) < 0) + ssh_child_setup_failed(exec_pipe[1]); if (sv[1] > 1) close(sv[1]); @@ -128,7 +181,7 @@ Client* client_connect_ssh(const char* destination, int port, const char* server ssh_user_len = strlen(r.host) + 1; char* ssh_user = malloc(ssh_user_len); if (!ssh_user) - _exit(1); + ssh_child_setup_failed(exec_pipe[1]); if (r.user && r.user[0] != '\0') snprintf(ssh_user, ssh_user_len, "%s@%s", r.user, r.host); else @@ -137,6 +190,9 @@ Client* client_connect_ssh(const char* destination, int port, const char* server char* ssh_argv[16]; int ac = 0; char port_str[16]; + char* remote_command = ssh_build_remote_command(server_path, old_args); + if (!remote_command) + ssh_child_setup_failed(exec_pipe[1]); ssh_argv[ac++] = "ssh"; ssh_argv[ac++] = "-o"; ssh_argv[ac++] = "Compression=no"; @@ -150,14 +206,11 @@ Client* client_connect_ssh(const char* destination, int port, const char* server ssh_argv[ac++] = port_str; } ssh_argv[ac++] = ssh_user; - ssh_argv[ac++] = (char*)(server_path ? server_path : "fastsync-server"); - ssh_argv[ac++] = "--stdio"; + ssh_argv[ac++] = remote_command; ssh_argv[ac] = NULL; execvp("ssh", ssh_argv); log_perror("exec of ssh failed"); - ssize_t wret = write(exec_pipe[1], "x", 1); - (void)wret; - _exit(1); + ssh_child_setup_failed(exec_pipe[1]); } close(sv[1]); @@ -167,7 +220,7 @@ Client* client_connect_ssh(const char* destination, int port, const char* server ssize_t n = read(exec_pipe[0], &exec_status, 1); close(exec_pipe[0]); - if (n > 0) { + if (n != 0) { close(sv[0]); waitpid(pid, NULL, 0); remote_dest_destroy(&r); diff --git a/src/shared/transport_ssh.h b/src/shared/transport_ssh.h index 315f37d..e46c687 100644 --- a/src/shared/transport_ssh.h +++ b/src/shared/transport_ssh.h @@ -3,6 +3,8 @@ #include "transport_tcp.h" -Client* client_connect_ssh(const char* destination, int port, const char* server_path); +Client* client_connect_ssh(const char* destination, int port, const char* server_path, + bool old_args); +char* ssh_build_remote_command(const char* server_path, bool old_args); #endif diff --git a/tests/test_client_cli.c b/tests/test_client_cli.c index 97d1455..8dda975 100644 --- a/tests/test_client_cli.c +++ b/tests/test_client_cli.c @@ -766,6 +766,17 @@ static void test_parse_args_rejects_unsafe_negation() { } } +static void test_parse_args_old_args() { + Config* cfg = config_create(); + char* argv[] = {"fastsync", "--old-args", "/src", "/dst"}; + int positional_args[2]; + int positional_count = 0; + + EXPECT_EQ_INT(parse_args(cfg, 4, argv, positional_args, &positional_count), 0); + EXPECT_TRUE(cfg->old_args); + config_delete(cfg); +} + static void test_parse_args_fsync() { Config* cfg = config_create(); char* argv[] = {"fastsync", "--fsync", "/src", "/dst"}; @@ -1036,6 +1047,7 @@ void test_client_cli() { test_parse_args_negation_order(); test_parse_args_no_preserve_blocks_implicit_metadata(); test_parse_args_rejects_unsafe_negation(); + test_parse_args_old_args(); test_parse_args_fsync(); test_parse_args_existing(); test_parse_args_ignore_times(); diff --git a/tests/test_transport_ssh.c b/tests/test_transport_ssh.c index e085cbb..7a2f3d6 100644 --- a/tests/test_transport_ssh.c +++ b/tests/test_transport_ssh.c @@ -4,34 +4,39 @@ static void test_ssh_connect_invalid_dest_no_colon() { /* cppcheck-suppress constVariablePointer */ - Client* client = client_connect_ssh("invalid-destination-no-colon", 22, NULL); + Client* client = client_connect_ssh("invalid-destination-no-colon", 22, NULL, false); EXPECT_NULL(client); } static void test_ssh_connect_invalid_dest_empty() { /* cppcheck-suppress constVariablePointer */ - Client* client = client_connect_ssh("", 22, NULL); + Client* client = client_connect_ssh("", 22, NULL, false); EXPECT_NULL(client); } -/* Test client_connect_ssh with malformed destination (just a colon). - * parse_remote_dest succeeds, ssh is exec'd and fails, but the function - * creates a Client that must be cleaned up. */ +/* A child that cannot exec ssh must not be returned as a successful client. */ static void test_ssh_connect_malformed() { - Client* client = client_connect_ssh(":", 22, NULL); - /* ssh binary exists, so exec succeeds; the function returns a Client. - * We just verify it doesn't crash and clean up properly. */ - if (client != NULL) { - client_disconnect(client); - client_delete(client); + const char* old_path = getenv("PATH"); + char* saved_path = old_path ? strdup(old_path) : NULL; + setenv("PATH", "", 1); + + /* cppcheck-suppress constVariablePointer */ + Client* client = client_connect_ssh(":", 22, NULL, false); + + if (saved_path) { + setenv("PATH", saved_path, 1); + free(saved_path); + } else { + unsetenv("PATH"); } - EXPECT_TRUE(true); + + EXPECT_NULL(client); } /* Test client_connect_ssh with valid format but unreachable host. * The function launches ssh which will fail to connect, returns a Client. */ static void test_ssh_connect_unreachable() { - Client* client = client_connect_ssh("nonexistent.invalid:/remote/path", 22, NULL); + Client* client = client_connect_ssh("nonexistent.invalid:/remote/path", 22, NULL, false); if (client != NULL) { client_disconnect(client); client_delete(client); @@ -39,9 +44,24 @@ static void test_ssh_connect_unreachable() { EXPECT_TRUE(true); } +static void test_ssh_remote_command_argument_modes() { + char* command = ssh_build_remote_command("fast sync; touch /tmp/pwned", false); + EXPECT_EQ_STR(command, "'fast sync; touch /tmp/pwned' --stdio"); + free(command); + + command = ssh_build_remote_command("fast'sync", false); + EXPECT_EQ_STR(command, "'fast'\\''sync' --stdio"); + free(command); + + command = ssh_build_remote_command("fast sync; touch /tmp/pwned", true); + EXPECT_EQ_STR(command, "fast sync; touch /tmp/pwned --stdio"); + free(command); +} + void test_transport_ssh() { test_ssh_connect_invalid_dest_no_colon(); test_ssh_connect_invalid_dest_empty(); test_ssh_connect_malformed(); test_ssh_connect_unreachable(); + test_ssh_remote_command_argument_modes(); }