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.
This commit is contained in:
2026-09-14 16:08:56 +02:00
parent df887c73b1
commit f75a69f96a
5 changed files with 104 additions and 20 deletions
+23 -6
View File
@@ -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) {
+4 -1
View File
@@ -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
+19 -3
View File
@@ -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;
+29
View File
@@ -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();
+29 -10
View File
@@ -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();