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

diff --git a/daemon.c b/daemon.c
index 4b0218457610fe994bec256d51b9c01f368383ea..b0b2b5382050edfc7fd7fc2b48e135e02b7fe582 100644 (file)
--- 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 3eb7d3186e161d2aa3a2fa872c014ad31dd91de0..b7e283252d7d73e937ef87096801f2d252e95961 100755 (executable)
--- 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