Verify Content-Type from smart HTTP servers

Before parsing a suspected smart-HTTP response verify the returned
Content-Type matches the standard. This protects a client from
attempting to process a payload that smells like a smart-HTTP
server response.

JGit has been doing this check on all responses since the dawn of
time. I mistakenly failed to include it in git-core when smart HTTP
was introduced. At the time I didn't know how to get the Content-Type
from libcurl. I punted, meant to circle back and fix this, and just
plain forgot about it.

Signed-off-by: Shawn Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/http-push.c b/http-push.c
index 8701c1215d21cd0413c1d69be91b309984cf6b1f..ba45b7b501b901e39b34cb44b41cb758dc4ad293 100644 (file)
--- a/http-push.c
+++ b/http-push.c
@@ -1560,7 +1560,7 @@ static int remote_exists(const char *path)
 
        sprintf(url, "%s%s", repo->url, path);
 
-       switch (http_get_strbuf(url, NULL, 0)) {
+       switch (http_get_strbuf(url, NULL, NULL, 0)) {
        case HTTP_OK:
                ret = 1;
                break;
@@ -1584,7 +1584,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
        url = xmalloc(strlen(repo->url) + strlen(path) + 1);
        sprintf(url, "%s%s", repo->url, path);
 
-       if (http_get_strbuf(url, &buffer, 0) != HTTP_OK)
+       if (http_get_strbuf(url, NULL, &buffer, 0) != HTTP_OK)
                die("Couldn't get %s for remote symref\n%s", url,
                    curl_errorstr);
        free(url);

diff --git a/http.c b/http.c
index 44f35256e44ffdda2ffb0ce67825008bef89700f..d868d8b3d97cc1a430e1680f2bc5f780b5e66a84 100644 (file)
--- a/http.c
+++ b/http.c
@@ -788,7 +788,8 @@ int handle_curl_result(struct slot_results *results)
 #define HTTP_REQUEST_STRBUF    0
 #define HTTP_REQUEST_FILE      1
 
-static int http_request(const char *url, void *result, int target, int options)
+static int http_request(const char *url, struct strbuf *type,
+                       void *result, int target, int options)
 {
        struct active_request_slot *slot;
        struct slot_results results;
@@ -838,24 +839,36 @@ static int http_request(const char *url, void *result, int target, int options)
                ret = HTTP_START_FAILED;
        }
 
+       if (type) {
+               char *t;
+               curl_easy_getinfo(slot->curl, CURLINFO_CONTENT_TYPE, &t);
+               if (t)
+                       strbuf_addstr(type, t);
+       }
+
        curl_slist_free_all(headers);
        strbuf_release(&buf);
 
        return ret;
 }
 
-static int http_request_reauth(const char *url, void *result, int target,
+static int http_request_reauth(const char *url,
+                              struct strbuf *type,
+                              void *result, int target,
                               int options)
 {
-       int ret = http_request(url, result, target, options);
+       int ret = http_request(url, type, result, target, options);
        if (ret != HTTP_REAUTH)
                return ret;
-       return http_request(url, result, target, options);
+       return http_request(url, type, result, target, options);
 }
 
-int http_get_strbuf(const char *url, struct strbuf *result, int options)
+int http_get_strbuf(const char *url,
+                   struct strbuf *type,
+                   struct strbuf *result, int options)
 {
-       return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
+       return http_request_reauth(url, type, result,
+                                  HTTP_REQUEST_STRBUF, options);
 }
 
 /*
@@ -878,7 +891,7 @@ static int http_get_file(const char *url, const char *filename, int options)
                goto cleanup;
        }
 
-       ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);
+       ret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);
        fclose(result);
 
        if ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename))
@@ -904,7 +917,7 @@ int http_fetch_ref(const char *base, struct ref *ref)
        int ret = -1;
 
        url = quote_ref_url(base, ref->name);
-       if (http_get_strbuf(url, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
+       if (http_get_strbuf(url, NULL, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
                strbuf_rtrim(&buffer);
                if (buffer.len == 40)
                        ret = get_sha1_hex(buffer.buf, ref->old_sha1);
@@ -997,7 +1010,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
        strbuf_addstr(&buf, "objects/info/packs");
        url = strbuf_detach(&buf, NULL);
 
-       ret = http_get_strbuf(url, &buf, HTTP_NO_CACHE);
+       ret = http_get_strbuf(url, NULL, &buf, HTTP_NO_CACHE);
        if (ret != HTTP_OK)
                goto cleanup;
 

diff --git a/http.h b/http.h
index 0a80d303e369b4adf77ca131c732ba39a990610d..25d19313983f0d0a11ab50dffbb623b105c8960a 100644 (file)
--- a/http.h
+++ b/http.h
@@ -132,7 +132,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,
  *
  * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.
  */
-int http_get_strbuf(const char *url, struct strbuf *result, int options);
+int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);
 
 /*
  * Prints an error message using error() containing url and curl_errorstr,

diff --git a/remote-curl.c b/remote-curl.c
index 9a8b12350712422e660f40fdf5ef9f191d37de99..933c69ac2627c1903276b92b457843e12649f39e 100644 (file)
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -92,6 +92,8 @@ static void free_discovery(struct discovery *d)
 
 static struct discovery* discover_refs(const char *service)
 {
+       struct strbuf exp = STRBUF_INIT;
+       struct strbuf type = STRBUF_INIT;
        struct strbuf buffer = STRBUF_INIT;
        struct discovery *last = last_discovery;
        char *refs_url;
@@ -113,7 +115,7 @@ static struct discovery* discover_refs(const char *service)
        }
        refs_url = strbuf_detach(&buffer, NULL);
 
-       http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
+       http_ret = http_get_strbuf(refs_url, &type, &buffer, HTTP_NO_CACHE);
        switch (http_ret) {
        case HTTP_OK:
                break;
@@ -132,17 +134,20 @@ static struct discovery* discover_refs(const char *service)
        last->buf_alloc = strbuf_detach(&buffer, &last->len);
        last->buf = last->buf_alloc;
 
-       if (maybe_smart && 5 <= last->len && last->buf[4] == '#') {
-               /* smart HTTP response; validate that the service
+       strbuf_addf(&exp, "application/x-%s-advertisement", service);
+       if (maybe_smart &&
+           (5 <= last->len && last->buf[4] == '#') &&
+           !strbuf_cmp(&exp, &type)) {
+               /*
+                * smart HTTP response; validate that the service
                 * pkt-line matches our request.
                 */
-               struct strbuf exp = STRBUF_INIT;
-
                if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)
                        die("%s has invalid packet header", refs_url);
                if (buffer.len && buffer.buf[buffer.len - 1] == '\n')
                        strbuf_setlen(&buffer, buffer.len - 1);
 
+               strbuf_reset(&exp);
                strbuf_addf(&exp, "# service=%s", service);
                if (strbuf_cmp(&exp, &buffer))
                        die("invalid server response; got '%s'", buffer.buf);
@@ -160,6 +165,8 @@ static struct discovery* discover_refs(const char *service)
        }
 
        free(refs_url);
+       strbuf_release(&exp);
+       strbuf_release(&type);
        strbuf_release(&buffer);
        last_discovery = last;
        return last;

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 02f442bfadbb1239785a456cddc50223da4f3834..895b9258b07ca65b78ac376023e203f482d8ac16 100644 (file)
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -80,6 +80,7 @@ fi
 prepare_httpd() {
        mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
        cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
+       cp "$TEST_PATH"/broken-smart-http.sh "$HTTPD_ROOT_PATH"
 
        ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
 

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index fe76e84b74472a431eef59a1f4936cfc69367003..938b4cf803b3b4ea07be20338425f784b80cdf0c 100644 (file)
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -62,9 +62,13 @@ Alias /auth/dumb/ www/auth/dumb/
        SetEnv GIT_COMMITTER_EMAIL custom@example.com
 </LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
+ScriptAlias /broken_smart/ broken-smart-http.sh/
 <Directory ${GIT_EXEC_PATH}>
        Options FollowSymlinks
 </Directory>
+<Files broken-smart-http.sh>
+       Options ExecCGI
+</Files>
 <Files ${GIT_EXEC_PATH}/git-http-backend>
        Options ExecCGI
 </Files>

diff --git a/t/lib-httpd/broken-smart-http.sh b/t/lib-httpd/broken-smart-http.sh
new file mode 100755 (executable)
index 0000000..f7ebfff
--- /dev/null
+++ b/t/lib-httpd/broken-smart-http.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+printf "Content-Type: text/%s\n" "html"
+echo
+printf "%s\n" "001e# service=git-upload-pack"
+printf "%s"   "0000"
+printf "%s%c%s%s\n" \
+       "00a58681d9f286a48b08f37b3a095330da16689e3693 HEAD" \
+       0 \
+       " include-tag multi_ack_detailed multi_ack ofs-delta" \
+       " side-band side-band-64k thin-pack no-progress shallow no-done "
+printf "%s"   "0000"

diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index c5cd2e348c6fe47af7d94ef6852757c301f8097a..cb95b953b72cc8dc5d56c5c3d3a5c0a8b462f324 100755 (executable)
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -157,6 +157,12 @@ test_expect_success 'GIT_SMART_HTTP can disable smart http' '
         test_must_fail git fetch)
 '
 
+test_expect_success 'invalid Content-Type rejected' '
+       echo "fatal: invalid content-type text/html" >expect
+       test_must_fail git clone $HTTPD_URL/broken_smart/repo.git 2>actual
+       test_cmp expect actual
+'
+
 test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '