Execute multihop commands directly, no shell

This avoids problems with shell escaping if arguments contain special
characters.

Author: mkj

src/cli-main.c

@@ -77,9 +77,8 @@ int main(int argc, char ** argv) {
 	}
 
 #if DROPBEAR_CLI_PROXYCMD
-	if (cli_opts.proxycmd) {
+	if (cli_opts.proxycmd || cli_opts.proxyexec) {
 		cli_proxy_cmd(&sock_in, &sock_out, &proxy_cmd_pid);
-		m_free(cli_opts.proxycmd);
 		if (signal(SIGINT, kill_proxy_sighandler) == SIG_ERR ||
 			signal(SIGTERM, kill_proxy_sighandler) == SIG_ERR ||
 			signal(SIGHUP, kill_proxy_sighandler) == SIG_ERR) {
@@ -101,7 +100,8 @@ int main(int argc, char ** argv) {
 }
 #endif /* DBMULTI stuff */
 
-static void exec_proxy_cmd(const void *user_data_cmd) {
+#if DROPBEAR_CLI_PROXYCMD
+static void shell_proxy_cmd(const void *user_data_cmd) {
 	const char *cmd = user_data_cmd;
 	char *usershell;
 
@@ -110,41 +110,62 @@ static void exec_proxy_cmd(const void *user_data_cmd) {
 	dropbear_exit("Failed to run '%s'\n", cmd);
 }
 
-#if DROPBEAR_CLI_PROXYCMD
+static void exec_proxy_cmd(const void *unused) {
+	(void)unused;
+	run_command(cli_opts.proxyexec[0], cli_opts.proxyexec, ses.maxfd);
+	dropbear_exit("Failed to run '%s'\n", cli_opts.proxyexec[0]);
+}
+
 static void cli_proxy_cmd(int *sock_in, int *sock_out, pid_t *pid_out) {
-	char * ex_cmd = NULL;
-	size_t ex_cmdlen;
+	char * cmd_arg = NULL;
+	void (*exec_fn)(const void *user_data) = NULL;
 	int ret;
 
+	/* exactly one of cli_opts.proxycmd or cli_opts.proxyexec should be set */
+
 	/* File descriptor "-j &3" */
-	if (*cli_opts.proxycmd == '&') {
+	if (cli_opts.proxycmd && *cli_opts.proxycmd == '&') {
 		char *p = cli_opts.proxycmd + 1;
 		int sock = strtoul(p, &p, 10);
 		/* must be a single number, and not stdin/stdout/stderr */
 		if (sock > 2 && sock < 1024 && *p == '\0') {
 			*sock_in = sock;
 			*sock_out = sock;
-			return;
+			goto cleanup;
 		}
 	}
 
-	/* Normal proxycommand */
-
-	/* So that spawn_command knows which shell to run */
-	fill_passwd(cli_opts.own_user);
-
-	ex_cmdlen = strlen(cli_opts.proxycmd) + 6; /* "exec " + command + '\0' */
-	ex_cmd = m_malloc(ex_cmdlen);
-	snprintf(ex_cmd, ex_cmdlen, "exec %s", cli_opts.proxycmd);
+	if (cli_opts.proxycmd) {
+		/* Normal proxycommand */
+		size_t shell_cmdlen;
+		/* So that spawn_command knows which shell to run */
+		fill_passwd(cli_opts.own_user);
+
+		shell_cmdlen = strlen(cli_opts.proxycmd) + 6; /* "exec " + command + '\0' */
+		cmd_arg = m_malloc(shell_cmdlen);
+		snprintf(cmd_arg, shell_cmdlen, "exec %s", cli_opts.proxycmd);
+		exec_fn = shell_proxy_cmd;
+	} else {
+		/* No shell */
+		exec_fn = exec_proxy_cmd;
+	}
 
-	ret = spawn_command(exec_proxy_cmd, ex_cmd,
-			sock_out, sock_in, NULL, pid_out);
-	DEBUG1(("cmd: %s  pid=%d", ex_cmd,*pid_out))
-	m_free(ex_cmd);
+	ret = spawn_command(exec_fn, cmd_arg, sock_out, sock_in, NULL, pid_out);
 	if (ret == DROPBEAR_FAILURE) {
 		dropbear_exit("Failed running proxy command");
 		*sock_in = *sock_out = -1;
 	}
+
+cleanup:
+	m_free(cli_opts.proxycmd);
+	m_free(cmd_arg);
+	if (cli_opts.proxyexec) {
+		char **a = NULL;
+		for (a = cli_opts.proxyexec; *a; a++) {
+			m_free_direct(*a);
+		}
+		m_free(cli_opts.proxyexec);
+	}
 }
 
 static void kill_proxy_sighandler(int UNUSED(signo)) {

src/cli-runopts.c

@@ -556,62 +556,88 @@ void loadidentityfile(const char* filename, int warnfail) {
 
 /* Fill out -i, -y, -W options that make sense for all
  * the intermediate processes */
-static char* multihop_passthrough_args(void) {
-	char *args = NULL;
-	unsigned int len, total;
+static char** multihop_args(const char* argv0, const char* prior_hops) {
+	/* null terminated array */
+	char **args = NULL;
+	size_t max_args = 14, pos = 0, len;
 #if DROPBEAR_CLI_PUBKEY_AUTH
 	m_list_elem *iter;
 #endif
-	/* Sufficient space for non-string args */
-	len = 100;
 
-	/* String arguments have arbitrary length, so determine space required */
-	if (cli_opts.proxycmd) {
-		len += strlen(cli_opts.proxycmd);
-	}
 #if DROPBEAR_CLI_PUBKEY_AUTH
 	for (iter = cli_opts.privkeys->first; iter; iter = iter->next)
 	{
-		sign_key * key = (sign_key*)iter->item;
-		len += 4 + strlen(key->filename);
+		/* "-i file" for each */
+		max_args += 2;
 	}
 #endif
 
-	args = m_malloc(len);
-	total = 0;
+	args = m_malloc(sizeof(char*) * max_args);
+	pos = 0;
 
-	/* Create new argument string */
+	args[pos] = m_strdup(argv0);
+	pos++;
 
 	if (cli_opts.quiet) {
-		total += m_snprintf(args+total, len-total, "-q ");
+		args[pos] = m_strdup("-q");
+		pos++;
 	}
 
 	if (cli_opts.no_hostkey_check) {
-		total += m_snprintf(args+total, len-total, "-y -y ");
+		args[pos] = m_strdup("-y");
+		pos++;
+		args[pos] = m_strdup("-y");
+		pos++;
 	} else if (cli_opts.always_accept_key) {
-		total += m_snprintf(args+total, len-total, "-y ");
+		args[pos] = m_strdup("-y");
+		pos++;
 	}
 
 	if (cli_opts.batch_mode) {
-		total += m_snprintf(args+total, len-total, "-o BatchMode=yes ");
+		args[pos] = m_strdup("-o");
+		pos++;
+		args[pos] = m_strdup("BatchMode=yes");
+		pos++;
 	}
 
 	if (cli_opts.proxycmd) {
-		total += m_snprintf(args+total, len-total, "-J '%s' ", cli_opts.proxycmd);
+		args[pos] = m_strdup("-J");
+		pos++;
+		args[pos] = m_strdup(cli_opts.proxycmd);
+		pos++;
 	}
 
 	if (opts.recv_window != DEFAULT_RECV_WINDOW) {
-		total += m_snprintf(args+total, len-total, "-W %u ", opts.recv_window);
+		args[pos] = m_strdup("-W");
+		pos++;
+		args[pos] = m_malloc(11);
+		m_snprintf(args[pos], 11, "%u", opts.recv_window);
+		pos++;
 	}
 
 #if DROPBEAR_CLI_PUBKEY_AUTH
 	for (iter = cli_opts.privkeys->first; iter; iter = iter->next)
 	{
 		sign_key * key = (sign_key*)iter->item;
-		total += m_snprintf(args+total, len-total, "-i %s ", key->filename);
+		args[pos] = m_strdup("-i");
+		pos++;
+		args[pos] = m_strdup(key->filename);
+		pos++;
 	}
 #endif /* DROPBEAR_CLI_PUBKEY_AUTH */
 
+	/* last hop */
+	args[pos] = m_strdup("-B");
+	pos++;
+	len = strlen(cli_opts.remotehost) + strlen(cli_opts.remoteport) + 2;
+	args[pos] = m_malloc(len);
+	snprintf(args[pos], len, "%s:%s", cli_opts.remotehost, cli_opts.remoteport);
+	pos++;
+
+	/* hostnames of prior hops */
+	args[pos] = m_strdup(prior_hops);
+	pos++;
+
 	return args;
 }
 
@@ -626,15 +652,15 @@ static char* multihop_passthrough_args(void) {
  * etc for as many hosts as we want.
  *
  * Note that "-J" arguments aren't actually used, instead
- * below sets cli_opts.proxycmd directly.
+ * below sets cli_opts.proxyexec directly.
  *
  * Ports for hosts can be specified as host/port.
  */
 static void parse_multihop_hostname(const char* orighostarg, const char* argv0) {
 	char *userhostarg = NULL;
 	char *hostbuf = NULL;
 	char *last_hop = NULL;
-	char *remainder = NULL;
+	char *prior_hops = NULL;
 
 	/* both scp and rsync parse a user@host argument
 	 * and turn it into "-l user host". This breaks
@@ -652,39 +678,37 @@ static void parse_multihop_hostname(const char* orighostarg, const char* argv0)
 	}
 	userhostarg = hostbuf;
 
+	/* Split off any last hostname and use that as remotehost/remoteport.
+	 * That is used for authorized_keys checking etc */
 	last_hop = strrchr(userhostarg, ',');
 	if (last_hop) {
 		if (last_hop == userhostarg) {
 			dropbear_exit("Bad multi-hop hostnames");
 		}
 		*last_hop = '\0';
 		last_hop++;
-		remainder = userhostarg;
+		prior_hops = userhostarg;
 		userhostarg = last_hop;
 	}
 
+	/* Update cli_opts.remotehost and cli_opts.remoteport */
 	parse_hostname(userhostarg);
 
-	if (last_hop) {
-		/* Set up the proxycmd */
-		unsigned int cmd_len = 0;
-		char *passthrough_args = multihop_passthrough_args();
-		cmd_len = strlen(argv0) + strlen(remainder)
-			+ strlen(cli_opts.remotehost) + strlen(cli_opts.remoteport)
-			+ strlen(passthrough_args)
-			+ 30;
-		/* replace proxycmd. old -J arguments have been copied
-		   to passthrough_args */
-		cli_opts.proxycmd = m_realloc(cli_opts.proxycmd, cmd_len);
-		m_snprintf(cli_opts.proxycmd, cmd_len, "%s -B %s:%s %s %s",
-				argv0, cli_opts.remotehost, cli_opts.remoteport,
-				passthrough_args, remainder);
+	/* Construct any multihop proxy command. Use proxyexec to
+	 * avoid worrying about shell escaping. */
+	if (prior_hops) {
+		cli_opts.proxyexec = multihop_args(argv0, prior_hops);
+		/* Any -J argument has been copied to proxyexec */
+		if (cli_opts.proxycmd) {
+			m_free(cli_opts.proxycmd);
+		}
+
 #ifndef DISABLE_ZLIB
-		/* The stream will be incompressible since it's encrypted. */
+		/* This outer stream will be incompressible since it's encrypted. */
 		opts.allow_compress = 0;
 #endif
-		m_free(passthrough_args);
 	}
+
 	m_free(hostbuf);
 }
 #endif /* DROPBEAR_CLI_MULTIHOP */

src/dbutil.c

@@ -371,7 +371,6 @@ int spawn_command(void(*exec_fn)(const void *user_data), const void *exec_data,
 void run_shell_command(const char* cmd, unsigned int maxfd, char* usershell) {
 	char * argv[4];
 	char * baseshell = NULL;
-	unsigned int i;
 
 	baseshell = basename(usershell);
 
@@ -393,6 +392,12 @@ void run_shell_command(const char* cmd, unsigned int maxfd, char* usershell) {
 		argv[1] = NULL;
 	}
 
+	run_command(usershell, argv, maxfd);
+}
+
+void run_command(const char* argv0, char** args, unsigned int maxfd) {
+	unsigned int i;
+
 	/* Re-enable SIGPIPE for the executed process */
 	if (signal(SIGPIPE, SIG_DFL) == SIG_ERR) {
 		dropbear_exit("signal() error");
@@ -404,7 +409,7 @@ void run_shell_command(const char* cmd, unsigned int maxfd, char* usershell) {
 		m_close(i);
 	}
 
-	execv(usershell, argv);
+	execv(argv0, args);
 }
 
 #if DEBUG_TRACE

src/dbutil.h

@@ -63,6 +63,7 @@ char * stripcontrol(const char * text);
 int spawn_command(void(*exec_fn)(const void *user_data), const void *exec_data,
 		int *writefd, int *readfd, int *errfd, pid_t *pid);
 void run_shell_command(const char* cmd, unsigned int maxfd, char* usershell);
+void run_command(const char* argv0, char** args, unsigned int maxfd);
 #if ENABLE_CONNECT_UNIX
 int connect_unix(const char* addr);
 #endif

src/runopts.h

@@ -194,7 +194,12 @@ typedef struct cli_runopts {
 	unsigned int netcat_port;
 #endif
 #if DROPBEAR_CLI_PROXYCMD
+	/* A proxy command to run via the user's shell */
 	char *proxycmd;
+#endif
+#if DROPBEAR_CLI_MULTIHOP
+	/* Similar to proxycmd, but is arguments for execve(), not shell */
+	char **proxyexec;
 #endif
 	const char *bind_arg;
 	char *bind_address;