From 94bc83c5930c8c73fb0106b629123e2413b371af Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 17 Feb 2015 03:37:35 -0500 Subject: [PATCH 1/3] git_connect: let user override virtual-host we send to daemon When we connect to a git-daemon at a given host and port, we actually send the string "localhost:9418" to the other side, which allows it to do virtual-hosting lookups. For testing and debugging, we'd like to be able to send arbitrary strings, rather than the hostname we actually connected to. Using "insteadOf" config does not work for this purpose, as the hostname determination happens at a very low level, right before we feed the hostname to our lookup routines. You could use /etc/hosts or similar to get around this, but we cannot do that portably from our test suite. Instead, this patch provides an environment variable that can be used to send an arbitrary string. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- connect.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/connect.c b/connect.c index 5047402a1a..661af292db 100644 --- a/connect.c +++ b/connect.c @@ -673,10 +673,20 @@ struct child_process *git_connect(int fd[2], const char *url, printf("Diag: path=%s\n", path ? path : "NULL"); conn = NULL; } else if (protocol == PROTO_GIT) { + /* + * Set up virtual host information based on where we will + * connect, unless the user has overridden us in + * the environment. + */ + char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST"); + if (target_host) + target_host = xstrdup(target_host); + else + target_host = xstrdup(hostandport); + /* These underlying connection commands die() if they * cannot connect. */ - char *target_host = xstrdup(hostandport); if (git_use_proxy(hostandport)) conn = git_proxy_connect(fd, hostandport); else From 5248f2dd4fe763ef9d1267f50481deee36ee57c1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 17 Feb 2015 03:40:57 -0500 Subject: [PATCH 2/3] t5570: test git-daemon's --interpolated-path option We did not test this at all; let's just give a basic sanity check that we can find a path based on virtual hosting, and that the downcase canonicalization works. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5570-git-daemon.sh | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh index 6b16379951..3eb7d3186e 100755 --- a/t/t5570-git-daemon.sh +++ b/t/t5570-git-daemon.sh @@ -141,5 +141,21 @@ test_expect_success 'push disabled' "test_remote_error 'service not enab test_expect_success 'read access denied' "test_remote_error -x 'no such repository' fetch repo.git " test_expect_success 'not exported' "test_remote_error -n 'repository not exported' fetch repo.git " +stop_git_daemon +start_git_daemon --interpolated-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH/%H%D" + +test_expect_success 'access repo via interpolated hostname' ' + repo="$GIT_DAEMON_DOCUMENT_ROOT_PATH/localhost/interp.git" && + git init --bare "$repo" && + git push "$repo" HEAD && + >"$repo"/git-daemon-export-ok && + rm -rf tmp.git && + GIT_OVERRIDE_VIRTUAL_HOST=localhost \ + git clone --bare "$GIT_DAEMON_URL/interp.git" tmp.git && + rm -rf tmp.git && + GIT_OVERRIDE_VIRTUAL_HOST=LOCALHOST \ + git clone --bare "$GIT_DAEMON_URL/interp.git" tmp.git +' + stop_git_daemon test_done From b48537305229d1a4f25633f71941ee52d2582017 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 17 Feb 2015 14:09:24 -0500 Subject: [PATCH 3/3] daemon: sanitize incoming virtual hostname We use the daemon_avoid_alias function to make sure that the pathname the user gives us is sane. However, after applying that check, we might then interpolate the path using a string given by the server admin, but which may contain more untrusted data from the client. We should be sure to sanitize this data, as well. We cannot use daemon_avoid_alias here, as it is more strict than we need in requiring a leading '/'. At the same time, we can be much more strict here. We are interpreting a hostname, which should not contain slashes or excessive runs of dots, as those things are not allowed in DNS names. Note that in addition to cleansing the hostname field, we must check the "canonical hostname" (%CH) as well as the port (%P), which we take as a raw string. For the canonical hostname, this comes from an actual DNS lookup on the accessed IP, which makes it a much less likely vector for problems. But it does not hurt to sanitize it in the same way. Unfortunately we cannot test this case easily, as it would involve a custom hostname lookup. We do not need to check %IP, as it comes straight from inet_ntop, so must have a sane form. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- daemon.c | 50 ++++++++++++++++++++++++++++++++++++++----- t/t5570-git-daemon.sh | 11 ++++++++++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/daemon.c b/daemon.c index 4b02184576..b0b2b53820 100644 --- a/daemon.c +++ b/daemon.c @@ -504,6 +504,45 @@ static void parse_host_and_port(char *hostport, char **host, } } +/* + * Sanitize a string from the client so that it's OK to be inserted into a + * filesystem path. Specifically, we disallow slashes, runs of "..", and + * trailing and leading dots, which means that the client cannot escape + * our base path via ".." traversal. + */ +static void sanitize_client_strbuf(struct strbuf *out, const char *in) +{ + for (; *in; in++) { + if (*in == '/') + continue; + if (*in == '.' && (!out->len || out->buf[out->len - 1] == '.')) + continue; + strbuf_addch(out, *in); + } + + while (out->len && out->buf[out->len - 1] == '.') + strbuf_setlen(out, out->len - 1); +} + +static char *sanitize_client(const char *in) +{ + struct strbuf out = STRBUF_INIT; + sanitize_client_strbuf(&out, in); + return strbuf_detach(&out, NULL); +} + +/* + * Like sanitize_client, but we also perform any canonicalization + * to make life easier on the admin. + */ +static char *canonicalize_client(const char *in) +{ + struct strbuf out = STRBUF_INIT; + sanitize_client_strbuf(&out, in); + strbuf_tolower(&out); + return strbuf_detach(&out, NULL); +} + /* * Read the host as supplied by the client connection. */ @@ -525,10 +564,10 @@ static void parse_host_arg(char *extra_args, int buflen) parse_host_and_port(val, &host, &port); if (port) { free(tcp_port); - tcp_port = xstrdup(port); + tcp_port = sanitize_client(port); } free(hostname); - hostname = xstrdup_tolower(host); + hostname = canonicalize_client(host); } /* On to the next one */ @@ -561,8 +600,9 @@ static void parse_host_arg(char *extra_args, int buflen) ip_address = xstrdup(addrbuf); free(canon_hostname); - canon_hostname = xstrdup(ai->ai_canonname ? - ai->ai_canonname : ip_address); + canon_hostname = ai->ai_canonname ? + sanitize_client(ai->ai_canonname) : + xstrdup(ip_address); freeaddrinfo(ai); } @@ -584,7 +624,7 @@ static void parse_host_arg(char *extra_args, int buflen) addrbuf, sizeof(addrbuf)); free(canon_hostname); - canon_hostname = xstrdup(hent->h_name); + canon_hostname = sanitize_client(hent->h_name); free(ip_address); ip_address = xstrdup(addrbuf); } diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh index 3eb7d3186e..b7e283252d 100755 --- a/t/t5570-git-daemon.sh +++ b/t/t5570-git-daemon.sh @@ -157,5 +157,16 @@ test_expect_success 'access repo via interpolated hostname' ' git clone --bare "$GIT_DAEMON_URL/interp.git" tmp.git ' +test_expect_success 'hostname cannot break out of directory' ' + rm -rf tmp.git && + repo="$GIT_DAEMON_DOCUMENT_ROOT_PATH/../escape.git" && + git init --bare "$repo" && + git push "$repo" HEAD && + >"$repo"/git-daemon-export-ok && + test_must_fail \ + env GIT_OVERRIDE_VIRTUAL_HOST=.. \ + git clone --bare "$GIT_DAEMON_URL/escape.git" tmp.git +' + stop_git_daemon test_done