daemon: sanitize incoming virtual hostnamejk/daemon-interpolate

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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

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
@@ -505,6 +505,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.
  */
 static void parse_host_arg(char *extra_args, int buflen)
@@ -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