From 820d7650cc670d3e4195aad3a5343158c316e8fa Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 26 Jul 2017 10:24:20 -0700 Subject: connect: reject ssh hostname that begins with a dash When commands like "git fetch" talk with ssh://$rest_of_URL/, the code splits $rest_of_URL into components like host, port, etc., and then spawns the underlying "ssh" program by formulating argv[] array that has: - the path to ssh command taken from GIT_SSH_COMMAND, etc. - dashed options like '-batch' (for Tortoise), '-p ' as needed. - ssh_host, which is supposed to be the hostname parsed out of $rest_of_URL. - then the command to be run on the other side, e.g. git upload-pack. If the ssh_host ends up getting '-', the argv[] that is used to spawn the command becomes something like: { "ssh", "-p", "22", "-", "command", "to", "run", NULL } which obviously is bogus, but depending on the actual value of "", will make "ssh" parse and use it as an option. Prevent this by forbidding ssh_host that begins with a "-". Noticed-by: Joern Schneeweisz of Recurity Labs Reported-by: Brian at GitLab Signed-off-by: Junio C Hamano Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- connect.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'connect.c') diff --git a/connect.c b/connect.c index fd7ffe184..0e8e05d83 100644 --- a/connect.c +++ b/connect.c @@ -754,6 +754,9 @@ struct child_process *git_connect(int fd[2], const char *url, return NULL; } + if (ssh_host[0] == '-') + die("strange hostname '%s' blocked", ssh_host); + ssh = getenv("GIT_SSH_COMMAND"); if (!ssh) { const char *base; -- cgit v1.2.1 From 2491f77b90c2e5d47acbe7472c17e7de0af74f63 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 28 Jul 2017 15:25:45 -0400 Subject: connect: factor out "looks like command line option" check We reject hostnames that start with a dash because they may be confused for command-line options. Let's factor out that notion into a helper function, as we'll use it in more places. And while it's simple now, it's not clear if some systems might need more complex logic to handle all cases. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- connect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'connect.c') diff --git a/connect.c b/connect.c index 0e8e05d83..a0091acb1 100644 --- a/connect.c +++ b/connect.c @@ -754,7 +754,7 @@ struct child_process *git_connect(int fd[2], const char *url, return NULL; } - if (ssh_host[0] == '-') + if (looks_like_command_line_option(ssh_host)) die("strange hostname '%s' blocked", ssh_host); ssh = getenv("GIT_SSH_COMMAND"); -- cgit v1.2.1 From 3be4cf09cd3d0747af3ecdb8dc3962a0969b731e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 28 Jul 2017 15:26:50 -0400 Subject: connect: reject dashed arguments for proxy commands If you have a GIT_PROXY_COMMAND configured, we will run it with the host/port on the command-line. If a URL contains a mischievous host like "--foo", we don't know how the proxy command may handle it. It's likely to break, but it may also do something dangerous and unwanted (technically it could even do something useful, but that seems unlikely). We should err on the side of caution and reject this before we even run the command. The hostname check matches the one we do in a similar circumstance for ssh. The port check is not present for ssh, but there it's not necessary because the syntax is "-p ", and there's no ambiguity on the parsing side. It's not clear whether you can actually get a negative port to the proxy here or not. Doing: git fetch git://remote:-1234/repo.git keeps the "-1234" as part of the hostname, with the default port of 9418. But it's a good idea to keep this check close to the point of running the command to make it clear that there's no way to circumvent it (and at worst it serves as a belt-and-suspenders check). Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- connect.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'connect.c') diff --git a/connect.c b/connect.c index a0091acb1..bdf2ca08a 100644 --- a/connect.c +++ b/connect.c @@ -553,6 +553,11 @@ static struct child_process *git_proxy_connect(int fd[2], char *host) get_host_and_port(&host, &port); + if (looks_like_command_line_option(host)) + die("strange hostname '%s' blocked", host); + if (looks_like_command_line_option(port)) + die("strange port '%s' blocked", port); + proxy = xmalloc(sizeof(*proxy)); child_process_init(proxy); argv_array_push(&proxy->args, git_proxy_command); -- cgit v1.2.1 From aeeb2d496859419ac1ba1da1162d6f3610f7f1f3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 28 Jul 2017 15:28:55 -0400 Subject: connect: reject paths that look like command line options If we get a repo path like "-repo.git", we may try to invoke "git-upload-pack -repo.git". This is going to fail, since upload-pack will interpret it as a set of bogus options. But let's reject this before we even run the sub-program, since we would not want to allow any mischief with repo names that actually are real command-line options. You can still ask for such a path via git-daemon, but there's no security problem there, because git-daemon enters the repo itself and then passes "." on the command line. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- connect.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'connect.c') diff --git a/connect.c b/connect.c index bdf2ca08a..d77d39771 100644 --- a/connect.c +++ b/connect.c @@ -727,6 +727,9 @@ struct child_process *git_connect(int fd[2], const char *url, conn = xmalloc(sizeof(*conn)); child_process_init(conn); + if (looks_like_command_line_option(path)) + die("strange pathname '%s' blocked", path); + strbuf_addstr(&cmd, prog); strbuf_addch(&cmd, ' '); sq_quote_buf(&cmd, path); -- cgit v1.2.1