From 820d7650cc670d3e4195aad3a5343158c316e8fa Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 26 Jul 2017 10:24:20 -0700 Subject: [PATCH 01/10] 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(+) diff --git a/connect.c b/connect.c index fd7ffe1840..0e8e05d83a 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; From 2d90add5ad216807ec1433e5367fae730e74a4cb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 28 Jul 2017 15:23:32 -0400 Subject: [PATCH 02/10] t5813: add test for hostname starting with dash Per the explanation in the previous patch, this should be (and is) rejected. Signed-off-by: Jeff King Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- t/t5813-proto-disable-ssh.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/t/t5813-proto-disable-ssh.sh b/t/t5813-proto-disable-ssh.sh index a954ead8af..0ecdceecd6 100755 --- a/t/t5813-proto-disable-ssh.sh +++ b/t/t5813-proto-disable-ssh.sh @@ -17,4 +17,13 @@ test_proto "host:path" ssh "remote:repo.git" test_proto "ssh://" ssh "ssh://remote$PWD/remote/repo.git" test_proto "git+ssh://" ssh "git+ssh://remote$PWD/remote/repo.git" +# Don't even bother setting up a "-remote" directory, as ssh would generally +# complain about the bogus option rather than completing our request. Our +# fake wrapper actually _can_ handle this case, but it's more robust to +# simply confirm from its output that it did not run at all. +test_expect_success 'hostnames starting with dash are rejected' ' + test_must_fail git clone ssh://-remote/repo.git dash-host 2>stderr && + ! grep ^ssh: stderr +' + test_done From 2491f77b90c2e5d47acbe7472c17e7de0af74f63 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 28 Jul 2017 15:25:45 -0400 Subject: [PATCH 03/10] 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 --- cache.h | 8 ++++++++ connect.c | 2 +- path.c | 5 +++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/cache.h b/cache.h index 1a2cec0b88..b9fc3a8e33 100644 --- a/cache.h +++ b/cache.h @@ -991,6 +991,14 @@ char *strip_path_suffix(const char *path, const char *suffix); int daemon_avoid_alias(const char *path); extern int is_ntfs_dotgit(const char *name); +/* + * Returns true iff "str" could be confused as a command-line option when + * passed to a sub-program like "ssh". Note that this has nothing to do with + * shell-quoting, which should be handled separately; we're assuming here that + * the string makes it verbatim to the sub-program. + */ +int looks_like_command_line_option(const char *str); + /** * Return a newly allocated string with the evaluation of * "$XDG_CONFIG_HOME/git/$filename" if $XDG_CONFIG_HOME is non-empty, otherwise diff --git a/connect.c b/connect.c index 0e8e05d83a..a0091acb1f 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"); diff --git a/path.c b/path.c index 8b7e168129..b214ac3fe6 100644 --- a/path.c +++ b/path.c @@ -1178,6 +1178,11 @@ int is_ntfs_dotgit(const char *name) } } +int looks_like_command_line_option(const char *str) +{ + return str && str[0] == '-'; +} + char *xdg_config_home(const char *filename) { const char *home, *config_home; From 3be4cf09cd3d0747af3ecdb8dc3962a0969b731e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 28 Jul 2017 15:26:50 -0400 Subject: [PATCH 04/10] 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 +++++ t/t5532-fetch-proxy.sh | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/connect.c b/connect.c index a0091acb1f..bdf2ca08ac 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); diff --git a/t/t5532-fetch-proxy.sh b/t/t5532-fetch-proxy.sh index 5531bd1af4..d3b2651b61 100755 --- a/t/t5532-fetch-proxy.sh +++ b/t/t5532-fetch-proxy.sh @@ -40,4 +40,9 @@ test_expect_success 'fetch through proxy works' ' test_cmp expect actual ' +test_expect_success 'funny hostnames are rejected before running proxy' ' + test_must_fail git fetch git://-remote/repo.git 2>stderr && + ! grep "proxying for" stderr +' + test_done From aeeb2d496859419ac1ba1da1162d6f3610f7f1f3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 28 Jul 2017 15:28:55 -0400 Subject: [PATCH 05/10] 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 +++ t/t5810-proto-disable-local.sh | 23 +++++++++++++++++++++++ t/t5813-proto-disable-ssh.sh | 14 ++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/connect.c b/connect.c index bdf2ca08ac..d77d39771b 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); diff --git a/t/t5810-proto-disable-local.sh b/t/t5810-proto-disable-local.sh index 563592d8a8..c1ef99b85c 100755 --- a/t/t5810-proto-disable-local.sh +++ b/t/t5810-proto-disable-local.sh @@ -11,4 +11,27 @@ test_expect_success 'setup repository to clone' ' test_proto "file://" file "file://$PWD" test_proto "path" file . +test_expect_success 'setup repo with dash' ' + git init --bare repo.git && + git push repo.git HEAD && + mv repo.git "$PWD/-repo.git" +' + +# This will fail even without our rejection because upload-pack will +# complain about the bogus option. So let's make sure that GIT_TRACE +# doesn't show us even running upload-pack. +# +# We must also be sure to use "fetch" and not "clone" here, as the latter +# actually canonicalizes our input into an absolute path (which is fine +# to allow). +test_expect_success 'repo names starting with dash are rejected' ' + rm -f trace.out && + test_must_fail env GIT_TRACE="$PWD/trace.out" git fetch -- -repo.git && + ! grep upload-pack trace.out +' + +test_expect_success 'full paths still work' ' + git fetch "$PWD/-repo.git" +' + test_done diff --git a/t/t5813-proto-disable-ssh.sh b/t/t5813-proto-disable-ssh.sh index 0ecdceecd6..3f084ee306 100755 --- a/t/t5813-proto-disable-ssh.sh +++ b/t/t5813-proto-disable-ssh.sh @@ -26,4 +26,18 @@ test_expect_success 'hostnames starting with dash are rejected' ' ! grep ^ssh: stderr ' +test_expect_success 'setup repo with dash' ' + git init --bare remote/-repo.git && + git push remote/-repo.git HEAD +' + +test_expect_success 'repo names starting with dash are rejected' ' + test_must_fail git clone remote:-repo.git dash-path 2>stderr && + ! grep ^ssh: stderr +' + +test_expect_success 'full paths still work' ' + git clone "remote:$PWD/remote/-repo.git" dash-path +' + test_done From 5e0649dc65fe33e8cf38823350e9d7951f6a6346 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 30 Jul 2017 14:45:13 -0700 Subject: [PATCH 06/10] Git 2.7.6 Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.7.6.txt | 25 +++++++++++++++++++++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.7.6.txt diff --git a/Documentation/RelNotes/2.7.6.txt b/Documentation/RelNotes/2.7.6.txt new file mode 100644 index 0000000000..4c6d1dcd4a --- /dev/null +++ b/Documentation/RelNotes/2.7.6.txt @@ -0,0 +1,25 @@ +Git v2.7.6 Release Notes +======================== + +Fixes since v2.7.5 +------------------ + + * A "ssh://..." URL can result in a "ssh" command line with a + hostname that begins with a dash "-", which would cause the "ssh" + command to instead (mis)treat it as an option. This is now + prevented by forbidding such a hostname (which will not be + necessary in the real world). + + * Similarly, when GIT_PROXY_COMMAND is configured, the command is + run with host and port that are parsed out from "ssh://..." URL; + a poorly written GIT_PROXY_COMMAND could be tricked into treating + a string that begins with a dash "-". This is now prevented by + forbidding such a hostname and port number (again, which will not + be necessary in the real world). + + * In the same spirit, a repository name that begins with a dash "-" + is also forbidden now. + +Credits go to Brian Neel at GitLab, Joern Schneeweisz of Recurity +Labs and Jeff King at GitHub. + diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index c80681ef00..2c156bce82 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.7.5 +DEF_VER=v2.7.6 LF=' ' diff --git a/RelNotes b/RelNotes index 1609b5af0d..a256349b9d 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.7.5.txt \ No newline at end of file +Documentation/RelNotes/2.7.6.txt \ No newline at end of file From 8d7f72f176ea133c16e55f386a0b79a1cd46ff69 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 30 Jul 2017 14:49:08 -0700 Subject: [PATCH 07/10] Git 2.8.6 Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.8.6.txt | 4 ++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.8.6.txt diff --git a/Documentation/RelNotes/2.8.6.txt b/Documentation/RelNotes/2.8.6.txt new file mode 100644 index 0000000000..d8db55d920 --- /dev/null +++ b/Documentation/RelNotes/2.8.6.txt @@ -0,0 +1,4 @@ +Git v2.8.6 Release Notes +======================== + +This release forward-ports the fix for "ssh://..." URL from Git v2.7.6 diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 253c63277b..3fd99767da 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.8.5 +DEF_VER=v2.8.6 LF=' ' diff --git a/RelNotes b/RelNotes index c395dd85d1..611ee8791d 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.8.5.txt \ No newline at end of file +Documentation/RelNotes/2.8.6.txt \ No newline at end of file From 4d4165b80d6b91a255e2847583bd4df98b5d54e1 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 30 Jul 2017 14:53:25 -0700 Subject: [PATCH 08/10] Git 2.9.5 Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.9.5.txt | 4 ++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.9.5.txt diff --git a/Documentation/RelNotes/2.9.5.txt b/Documentation/RelNotes/2.9.5.txt new file mode 100644 index 0000000000..668313ae55 --- /dev/null +++ b/Documentation/RelNotes/2.9.5.txt @@ -0,0 +1,4 @@ +Git v2.9.5 Release Notes +======================== + +This release forward-ports the fix for "ssh://..." URL from Git v2.7.6 diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index f0c6bb0cb1..1576764388 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.9.4 +DEF_VER=v2.9.5 LF=' ' diff --git a/RelNotes b/RelNotes index a5f3cbf539..4b3626ed26 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.9.4.txt \ No newline at end of file +Documentation/RelNotes/2.9.5.txt \ No newline at end of file From 0bfff8146f8c055fd95af4567286929ba8216fa7 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 30 Jul 2017 15:00:04 -0700 Subject: [PATCH 09/10] Git 2.10.4 Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.10.4.txt | 4 ++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.10.4.txt diff --git a/Documentation/RelNotes/2.10.4.txt b/Documentation/RelNotes/2.10.4.txt new file mode 100644 index 0000000000..ee8142ad24 --- /dev/null +++ b/Documentation/RelNotes/2.10.4.txt @@ -0,0 +1,4 @@ +Git v2.10.4 Release Notes +========================= + +This release forward-ports the fix for "ssh://..." URL from Git v2.7.6 diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 40700d547b..f0b293d4ff 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.10.3 +DEF_VER=v2.10.4 LF=' ' diff --git a/RelNotes b/RelNotes index 75030cb5e0..6b165ea13f 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.10.3.txt \ No newline at end of file +Documentation/RelNotes/2.10.4.txt \ No newline at end of file From 3b827444811d7eddeddd44850f5dbbb4d59747f5 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 30 Jul 2017 15:02:37 -0700 Subject: [PATCH 10/10] Git 2.11.3 Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.11.3.txt | 4 ++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.11.3.txt diff --git a/Documentation/RelNotes/2.11.3.txt b/Documentation/RelNotes/2.11.3.txt new file mode 100644 index 0000000000..4e3b78d0e8 --- /dev/null +++ b/Documentation/RelNotes/2.11.3.txt @@ -0,0 +1,4 @@ +Git v2.11.3 Release Notes +========================= + +This release forward-ports the fix for "ssh://..." URL from Git v2.7.6 diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index d207f15fb0..01d3c71340 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.11.2 +DEF_VER=v2.11.3 LF=' ' diff --git a/RelNotes b/RelNotes index 9f01cfb579..ff91cb22f9 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.11.2.txt \ No newline at end of file +Documentation/RelNotes/2.11.3.txt \ No newline at end of file