From f75a69f96aeece3dcf06c1c94094b87e2b469112 Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 14 Sep 2026 16:08:56 +0200 Subject: [PATCH] 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();