{"thread":{"id":"32803","subject":"[PATCH] Verify Content-Type from smart HTTP servers","startedAt":"2013-01-31T22:09:40Z","lastAt":"2013-02-06T22:47:01Z","messageCount":12,"participants":["Junio C Hamano","Jeff King","Shawn Pearce","Michael Schubert"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"208413","messageId":"7v38xhf1i3.fsf@alter.siamese.dyndns.org","threadId":"32803","inReplyTo":null,"subject":"[PATCH] Verify Content-Type from smart HTTP servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-31T22:09:40Z","receivedAt":"2013-01-31T22:09:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Before parsing a suspected smart-HTTP response verify the returned\nContent-Type matches the standard. This protects a client from\nattempting to process a payload that smells like a smart-HTTP\nserver response.\n\nJGit has been doing this check on all responses since the dawn of\ntime. I mistakenly failed to include it in git-core when smart HTTP\nwas introduced. At the time I didn't know how to get the Content-Type\nfrom libcurl. I punted, meant to circle back and fix this, and just\nplain forgot about it.\n\nSigned-off-by: Shawn Pearce <spearce@spearce.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * The original was picked up by majordomo taboo rules; resending\n   after minor style fix.\n\n   Was there a report of an attempted attack by malicious server or\n   something that triggered this, or is this just a \"common sense\n   thing to do\" in general?\n\n http-push.c                      |  4 ++--\n http.c                           | 31 ++++++++++++++++++++++---------\n http.h                           |  2 +-\n remote-curl.c                    | 15 +++++++++++----\n t/lib-httpd.sh                   |  1 +\n t/lib-httpd/apache.conf          |  4 ++++\n t/lib-httpd/broken-smart-http.sh | 11 +++++++++++\n t/t5551-http-fetch.sh            |  6 ++++++\n 8 files changed, 58 insertions(+), 16 deletions(-)\n create mode 100755 t/lib-httpd/broken-smart-http.sh\n\ndiff --git a/http-push.c b/http-push.c\nindex 8701c12..ba45b7b 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1560,7 +1560,7 @@ static int remote_exists(const char *path)\n \n \tsprintf(url, \"%s%s\", repo->url, path);\n \n-\tswitch (http_get_strbuf(url, NULL, 0)) {\n+\tswitch (http_get_strbuf(url, NULL, NULL, 0)) {\n \tcase HTTP_OK:\n \t\tret = 1;\n \t\tbreak;\n@@ -1584,7 +1584,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)\n \turl = xmalloc(strlen(repo->url) + strlen(path) + 1);\n \tsprintf(url, \"%s%s\", repo->url, path);\n \n-\tif (http_get_strbuf(url, &buffer, 0) != HTTP_OK)\n+\tif (http_get_strbuf(url, NULL, &buffer, 0) != HTTP_OK)\n \t\tdie(\"Couldn't get %s for remote symref\\n%s\", url,\n \t\t    curl_errorstr);\n \tfree(url);\ndiff --git a/http.c b/http.c\nindex 44f3525..d868d8b 100644\n--- a/http.c\n+++ b/http.c\n@@ -788,7 +788,8 @@ int handle_curl_result(struct slot_results *results)\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n \n-static int http_request(const char *url, void *result, int target, int options)\n+static int http_request(const char *url, struct strbuf *type,\n+\t\t\tvoid *result, int target, int options)\n {\n \tstruct active_request_slot *slot;\n \tstruct slot_results results;\n@@ -838,24 +839,36 @@ static int http_request(const char *url, void *result, int target, int options)\n \t\tret = HTTP_START_FAILED;\n \t}\n \n+\tif (type) {\n+\t\tchar *t;\n+\t\tcurl_easy_getinfo(slot->curl, CURLINFO_CONTENT_TYPE, &t);\n+\t\tif (t)\n+\t\t\tstrbuf_addstr(type, t);\n+\t}\n+\n \tcurl_slist_free_all(headers);\n \tstrbuf_release(&buf);\n \n \treturn ret;\n }\n \n-static int http_request_reauth(const char *url, void *result, int target,\n+static int http_request_reauth(const char *url,\n+\t\t\t       struct strbuf *type,\n+\t\t\t       void *result, int target,\n \t\t\t       int options)\n {\n-\tint ret = http_request(url, result, target, options);\n+\tint ret = http_request(url, type, result, target, options);\n \tif (ret != HTTP_REAUTH)\n \t\treturn ret;\n-\treturn http_request(url, result, target, options);\n+\treturn http_request(url, type, result, target, options);\n }\n \n-int http_get_strbuf(const char *url, struct strbuf *result, int options)\n+int http_get_strbuf(const char *url,\n+\t\t    struct strbuf *type,\n+\t\t    struct strbuf *result, int options)\n {\n-\treturn http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);\n+\treturn http_request_reauth(url, type, result,\n+\t\t\t\t   HTTP_REQUEST_STRBUF, options);\n }\n \n /*\n@@ -878,7 +891,7 @@ static int http_get_file(const char *url, const char *filename, int options)\n \t\tgoto cleanup;\n \t}\n \n-\tret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);\n+\tret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);\n \tfclose(result);\n \n \tif ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename))\n@@ -904,7 +917,7 @@ int http_fetch_ref(const char *base, struct ref *ref)\n \tint ret = -1;\n \n \turl = quote_ref_url(base, ref->name);\n-\tif (http_get_strbuf(url, &buffer, HTTP_NO_CACHE) == HTTP_OK) {\n+\tif (http_get_strbuf(url, NULL, &buffer, HTTP_NO_CACHE) == HTTP_OK) {\n \t\tstrbuf_rtrim(&buffer);\n \t\tif (buffer.len == 40)\n \t\t\tret = get_sha1_hex(buffer.buf, ref->old_sha1);\n@@ -997,7 +1010,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n \tstrbuf_addstr(&buf, \"objects/info/packs\");\n \turl = strbuf_detach(&buf, NULL);\n \n-\tret = http_get_strbuf(url, &buf, HTTP_NO_CACHE);\n+\tret = http_get_strbuf(url, NULL, &buf, HTTP_NO_CACHE);\n \tif (ret != HTTP_OK)\n \t\tgoto cleanup;\n \ndiff --git a/http.h b/http.h\nindex 0a80d30..25d1931 100644\n--- a/http.h\n+++ b/http.h\n@@ -132,7 +132,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n  *\n  * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.\n  */\n-int http_get_strbuf(const char *url, struct strbuf *result, int options);\n+int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);\n \n /*\n  * Prints an error message using error() containing url and curl_errorstr,\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 9a8b123..e6f3b63 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -92,6 +92,8 @@ static void free_discovery(struct discovery *d)\n \n static struct discovery* discover_refs(const char *service)\n {\n+\tstruct strbuf exp = STRBUF_INIT;\n+\tstruct strbuf type = STRBUF_INIT;\n \tstruct strbuf buffer = STRBUF_INIT;\n \tstruct discovery *last = last_discovery;\n \tchar *refs_url;\n@@ -113,7 +115,7 @@ static struct discovery* discover_refs(const char *service)\n \t}\n \trefs_url = strbuf_detach(&buffer, NULL);\n \n-\thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n+\thttp_ret = http_get_strbuf(refs_url, &type, &buffer, HTTP_NO_CACHE);\n \tswitch (http_ret) {\n \tcase HTTP_OK:\n \t\tbreak;\n@@ -133,16 +135,19 @@ static struct discovery* discover_refs(const char *service)\n \tlast->buf = last->buf_alloc;\n \n \tif (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n-\t\t/* smart HTTP response; validate that the service\n+\t\t/*\n+\t\t * smart HTTP response; validate that the service\n \t\t * pkt-line matches our request.\n \t\t */\n-\t\tstruct strbuf exp = STRBUF_INIT;\n-\n+\t\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n+\t\tif (strbuf_cmp(&exp, &type))\n+\t\t\tdie(\"invalid content-type %s\", type.buf);\n \t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n \t\t\tdie(\"%s has invalid packet header\", refs_url);\n \t\tif (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n \t\t\tstrbuf_setlen(&buffer, buffer.len - 1);\n \n+\t\tstrbuf_reset(&exp);\n \t\tstrbuf_addf(&exp, \"# service=%s\", service);\n \t\tif (strbuf_cmp(&exp, &buffer))\n \t\t\tdie(\"invalid server response; got '%s'\", buffer.buf);\n@@ -160,6 +165,8 @@ static struct discovery* discover_refs(const char *service)\n \t}\n \n \tfree(refs_url);\n+\tstrbuf_release(&exp);\n+\tstrbuf_release(&type);\n \tstrbuf_release(&buffer);\n \tlast_discovery = last;\n \treturn last;\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 02f442b..895b925 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -80,6 +80,7 @@ fi\n prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n+\tcp \"$TEST_PATH\"/broken-smart-http.sh \"$HTTPD_ROOT_PATH\"\n \n \tln -s \"$LIB_HTTPD_MODULE_PATH\" \"$HTTPD_ROOT_PATH/modules\"\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex fe76e84..938b4cf 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -62,9 +62,13 @@ Alias /auth/dumb/ www/auth/dumb/\n \tSetEnv GIT_COMMITTER_EMAIL custom@example.com\n </LocationMatch>\n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n+ScriptAlias /broken_smart/ broken-smart-http.sh/\n <Directory ${GIT_EXEC_PATH}>\n \tOptions FollowSymlinks\n </Directory>\n+<Files broken-smart-http.sh>\n+\tOptions ExecCGI\n+</Files>\n <Files ${GIT_EXEC_PATH}/git-http-backend>\n \tOptions ExecCGI\n </Files>\ndiff --git a/t/lib-httpd/broken-smart-http.sh b/t/lib-httpd/broken-smart-http.sh\nnew file mode 100755\nindex 0000000..f7ebfff\n--- /dev/null\n+++ b/t/lib-httpd/broken-smart-http.sh\n@@ -0,0 +1,11 @@\n+#!/bin/sh\n+printf \"Content-Type: text/%s\\n\" \"html\"\n+echo\n+printf \"%s\\n\" \"001e# service=git-upload-pack\"\n+printf \"%s\"   \"0000\"\n+printf \"%s%c%s%s\\n\" \\\n+\t\"00a58681d9f286a48b08f37b3a095330da16689e3693 HEAD\" \\\n+\t0 \\\n+\t\" include-tag multi_ack_detailed multi_ack ofs-delta\" \\\n+\t\" side-band side-band-64k thin-pack no-progress shallow no-done \"\n+printf \"%s\"   \"0000\"\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex c5cd2e3..cb95b95 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -157,6 +157,12 @@ test_expect_success 'GIT_SMART_HTTP can disable smart http' '\n \t test_must_fail git fetch)\n '\n \n+test_expect_success 'invalid Content-Type rejected' '\n+\techo \"fatal: invalid content-type text/html\" >expect\n+\ttest_must_fail git clone $HTTPD_URL/broken_smart/repo.git 2>actual\n+\ttest_cmp expect actual\n+'\n+\n test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n \n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n-- \n1.8.1.2.605.gb99210a\n"},{"id":"208456","messageId":"20130201085248.GA30644@sigill.intra.peff.net","threadId":"32803","inReplyTo":"7v38xhf1i3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-01T08:52:49Z","receivedAt":"2013-02-01T08:52:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 31, 2013 at 02:09:40PM -0800, Junio C Hamano wrote:\n\n> Before parsing a suspected smart-HTTP response verify the returned\n> Content-Type matches the standard. This protects a client from\n> attempting to process a payload that smells like a smart-HTTP\n> server response.\n> \n> JGit has been doing this check on all responses since the dawn of\n> time. I mistakenly failed to include it in git-core when smart HTTP\n> was introduced. At the time I didn't know how to get the Content-Type\n> from libcurl. I punted, meant to circle back and fix this, and just\n> plain forgot about it.\n> \n> Signed-off-by: Shawn Pearce <spearce@spearce.org>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nShould this be \"From:\" Shawn? The tone of the message and the S-O-B\norder makes it look like it.\n\n> @@ -133,16 +135,19 @@ static struct discovery* discover_refs(const char *service)\n>  \tlast->buf = last->buf_alloc;\n>  \n>  \tif (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n> -\t\t/* smart HTTP response; validate that the service\n> +\t\t/*\n> +\t\t * smart HTTP response; validate that the service\n>  \t\t * pkt-line matches our request.\n>  \t\t */\n> -\t\tstruct strbuf exp = STRBUF_INIT;\n> -\n> +\t\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> +\t\tif (strbuf_cmp(&exp, &type))\n> +\t\t\tdie(\"invalid content-type %s\", type.buf);\n\nHmm. I wondered if it is possible for a non-smart server to send us down\nthis code path, which would now complain of the bogus content-type.\nSomething like an info/refs file with:\n\n  # 1\n  # the comment above is meaningless, but puts a \"#\" at position 4.\n\nBut I note that we would already die in the next line:\n\n>  \t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n>  \t\t\tdie(\"%s has invalid packet header\", refs_url);\n\nso I do not think the patch makes anything worse. However, should we\ntake this opportunity to make the \"did we get a smart response\" test\nmore robust? That is, should we actually be checking the content-type\nin the outer conditional, and going down the smart code-path if it is\napplication/x-%s-advertisement, and otherwise treating the result as\ndumb?\n\nIt's probably not a big deal, as the false positive example above is\nquite specific and unlikely, but it just seems cleaner to me.\n\nAs a side note, should we (can we) care about the content-type for dumb\nhttp? It should probably be text/plain or application/octet-stream, but\nI would not be surprised if we get a variety of random junk in the real\nworld, though.\n\n-Peff\n"},{"id":"208476","messageId":"7vip6bc3e1.fsf@alter.siamese.dyndns.org","threadId":"32803","inReplyTo":"20130201085248.GA30644@sigill.intra.peff.net","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-01T18:09:26Z","receivedAt":"2013-02-01T18:09:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Should this be \"From:\" Shawn? The tone of the message and the S-O-B\n> order makes it look like it.\n\nYes. I should have left that line when edited the format-patch\noutput in my MUA to say I was resending something that vger rejected\nand people did not see after tweaking the patch to slip their taboo\nlist.\n\n>> @@ -133,16 +135,19 @@ static struct discovery* discover_refs(const char *service)\n>>  \tlast->buf = last->buf_alloc;\n>>  \n>>  \tif (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n>> -\t\t/* smart HTTP response; validate that the service\n>> +\t\t/*\n>> +\t\t * smart HTTP response; validate that the service\n>>  \t\t * pkt-line matches our request.\n>>  \t\t */\n>> -\t\tstruct strbuf exp = STRBUF_INIT;\n>> -\n>> +\t\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n>> +\t\tif (strbuf_cmp(&exp, &type))\n>> +\t\t\tdie(\"invalid content-type %s\", type.buf);\n>\n> Hmm. I wondered if it is possible for a non-smart server to send us down\n> this code path, which would now complain of the bogus content-type.\n> Something like an info/refs file with:\n>\n>   # 1\n>   # the comment above is meaningless, but puts a \"#\" at position 4.\n>\n> But I note that we would already die in the next line:\n>\n>>  \t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n>>  \t\t\tdie(\"%s has invalid packet header\", refs_url);\n>\n> so I do not think the patch makes anything worse. However, should we\n> take this opportunity to make the \"did we get a smart response\" test\n> more robust? That is, should we actually be checking the content-type\n> in the outer conditional, and going down the smart code-path if it is\n> application/x-%s-advertisement, and otherwise treating the result as\n> dumb?\n\nDoes the outer caller that switches between dumb and smart actually\nknow what service type it is requesting (I am not familiar with the\ncallchain involved)?  Even if it doesn't, it may still make sense.\n\n> As a side note, should we (can we) care about the content-type for dumb\n> http? It should probably be text/plain or application/octet-stream, but\n> I would not be surprised if we get a variety of random junk in the real\n> world, though.\n\nThe design objective of dumb http protocol was to allow working with\nany dumb bit transfer thing, so I'd prefer to keep it lenient and\nallow application/x-git-loose-object-file and somesuch.\n\nThanks.\n"},{"id":"208486","messageId":"20130201185827.GA22919@sigill.intra.peff.net","threadId":"32803","inReplyTo":"7vip6bc3e1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-01T18:58:27Z","receivedAt":"2013-02-01T18:58:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 01, 2013 at 10:09:26AM -0800, Junio C Hamano wrote:\n\n> > so I do not think the patch makes anything worse. However, should we\n> > take this opportunity to make the \"did we get a smart response\" test\n> > more robust? That is, should we actually be checking the content-type\n> > in the outer conditional, and going down the smart code-path if it is\n> > application/x-%s-advertisement, and otherwise treating the result as\n> > dumb?\n> \n> Does the outer caller that switches between dumb and smart actually\n> know what service type it is requesting (I am not familiar with the\n> callchain involved)?  Even if it doesn't, it may still make sense.\n\nI was specifically thinking of this (on top of your patch):\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex e6f3b63..63680a8 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -134,14 +134,12 @@ static struct discovery* discover_refs(const char *service)\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n \tlast->buf = last->buf_alloc;\n \n-\tif (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n+\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n+\tif (maybe_smart && !strbuf_cmp(&exp, &type)) {\n \t\t/*\n \t\t * smart HTTP response; validate that the service\n \t\t * pkt-line matches our request.\n \t\t */\n-\t\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n-\t\tif (strbuf_cmp(&exp, &type))\n-\t\t\tdie(\"invalid content-type %s\", type.buf);\n \t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n \t\t\tdie(\"%s has invalid packet header\", refs_url);\n \t\tif (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n\nTo just follow the dumb path if we don't get the content-type we expect.\nWe may want to keep the '#' format check in addition (packet_get_line\nwill check it and die, anyway, but we may want to drop back to\nconsidering it dumb, just to protect against a badly configured dumb\nserver which uses our mime type, but I do not think it likely).\n\n> > As a side note, should we (can we) care about the content-type for dumb\n> > http? It should probably be text/plain or application/octet-stream, but\n> > I would not be surprised if we get a variety of random junk in the real\n> > world, though.\n> \n> The design objective of dumb http protocol was to allow working with\n> any dumb bit transfer thing, so I'd prefer to keep it lenient and\n> allow application/x-git-loose-object-file and somesuch.\n\nYeah, I do not think it really buys us anything in practice, and we have\nno way of knowing what kind of crap is in the wild. Not worth it.\n\n-Peff\n"},{"id":"208592","messageId":"7va9rk5z02.fsf@alter.siamese.dyndns.org","threadId":"32803","inReplyTo":"20130201185827.GA22919@sigill.intra.peff.net","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-04T07:17:33Z","receivedAt":"2013-02-04T07:17:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I was specifically thinking of this (on top of your patch):\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index e6f3b63..63680a8 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -134,14 +134,12 @@ static struct discovery* discover_refs(const char *service)\n>  \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n>  \tlast->buf = last->buf_alloc;\n>  \n> -\tif (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n> +\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> +\tif (maybe_smart && !strbuf_cmp(&exp, &type)) {\n>  \t\t/*\n>  \t\t * smart HTTP response; validate that the service\n>  \t\t * pkt-line matches our request.\n>  \t\t */\n> -\t\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> -\t\tif (strbuf_cmp(&exp, &type))\n> -\t\t\tdie(\"invalid content-type %s\", type.buf);\n>  \t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n>  \t\t\tdie(\"%s has invalid packet header\", refs_url);\n>  \t\tif (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n>\n> To just follow the dumb path if we don't get the content-type we expect.\n> We may want to keep the '#' format check in addition (packet_get_line\n> will check it and die, anyway, but we may want to drop back to\n> considering it dumb, just to protect against a badly configured dumb\n> server which uses our mime type, but I do not think it likely).\n\nYeah, but it doesn't cost anything to check, so let's do so.\n\nDoes this look good to both of you (relative to Shawn's patch)?\n\n remote-curl.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex e6f3b63..933c69a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -134,14 +134,14 @@ static struct discovery* discover_refs(const char *service)\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n \tlast->buf = last->buf_alloc;\n \n-\tif (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n+\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n+\tif (maybe_smart &&\n+\t    (5 <= last->len && last->buf[4] == '#') &&\n+\t    !strbuf_cmp(&exp, &type)) {\n \t\t/*\n \t\t * smart HTTP response; validate that the service\n \t\t * pkt-line matches our request.\n \t\t */\n-\t\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n-\t\tif (strbuf_cmp(&exp, &type))\n-\t\t\tdie(\"invalid content-type %s\", type.buf);\n \t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n \t\t\tdie(\"%s has invalid packet header\", refs_url);\n \t\tif (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n"},{"id":"208595","messageId":"20130204083824.GB30835@sigill.intra.peff.net","threadId":"32803","inReplyTo":"7va9rk5z02.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-04T08:38:24Z","receivedAt":"2013-02-04T08:38:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 03, 2013 at 11:17:33PM -0800, Junio C Hamano wrote:\n\n> Does this look good to both of you (relative to Shawn's patch)?\n> \n>  remote-curl.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/remote-curl.c b/remote-curl.c\n> index e6f3b63..933c69a 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -134,14 +134,14 @@ static struct discovery* discover_refs(const char *service)\n>  \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n>  \tlast->buf = last->buf_alloc;\n>  \n> -\tif (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n> +\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> +\tif (maybe_smart &&\n> +\t    (5 <= last->len && last->buf[4] == '#') &&\n> +\t    !strbuf_cmp(&exp, &type)) {\n>  \t\t/*\n>  \t\t * smart HTTP response; validate that the service\n>  \t\t * pkt-line matches our request.\n>  \t\t */\n> -\t\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> -\t\tif (strbuf_cmp(&exp, &type))\n> -\t\t\tdie(\"invalid content-type %s\", type.buf);\n>  \t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n>  \t\t\tdie(\"%s has invalid packet header\", refs_url);\n>  \t\tif (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n\nYeah, I think that's fine. Thanks.\n\n-Peff\n"},{"id":"208661","messageId":"CAJo=hJtZ64ER4X+axtFZJ5ArnEg3h_nCVEBdd8KmE0nUpskzBA@mail.gmail.com","threadId":"32803","inReplyTo":"20130204083824.GB30835@sigill.intra.peff.net","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2013-02-04T23:49:35Z","receivedAt":"2013-02-04T23:49:35Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Mon, Feb 4, 2013 at 12:38 AM, Jeff King <peff@peff.net> wrote:\n> On Sun, Feb 03, 2013 at 11:17:33PM -0800, Junio C Hamano wrote:\n>\n> > Does this look good to both of you (relative to Shawn's patch)?\n> >\n> >  remote-curl.c | 8 ++++----\n> >  1 file changed, 4 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/remote-curl.c b/remote-curl.c\n> > index e6f3b63..933c69a 100644\n> > --- a/remote-curl.c\n> > +++ b/remote-curl.c\n> > @@ -134,14 +134,14 @@ static struct discovery* discover_refs(const char *service)\n> >       last->buf_alloc = strbuf_detach(&buffer, &last->len);\n> >       last->buf = last->buf_alloc;\n> >\n> > -     if (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n> > +     strbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> > +     if (maybe_smart &&\n> > +         (5 <= last->len && last->buf[4] == '#') &&\n> > +         !strbuf_cmp(&exp, &type)) {\n> >               /*\n> >                * smart HTTP response; validate that the service\n> >                * pkt-line matches our request.\n> >                */\n> > -             strbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> > -             if (strbuf_cmp(&exp, &type))\n> > -                     die(\"invalid content-type %s\", type.buf);\n> >               if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n> >                       die(\"%s has invalid packet header\", refs_url);\n> >               if (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n>\n> Yeah, I think that's fine. Thanks.\n\nLooks fine to me too, but I think the test won't work now. :-)\n"},{"id":"208665","messageId":"7vip67zk3q.fsf@alter.siamese.dyndns.org","threadId":"32803","inReplyTo":"CAJo=hJtZ64ER4X+axtFZJ5ArnEg3h_nCVEBdd8KmE0nUpskzBA@mail.gmail.com","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-05T00:21:13Z","receivedAt":"2013-02-05T00:21:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce <spearce@spearce.org> writes:\n\n> Looks fine to me too, but I think the test won't work now. :-)\n\nHeh, that's amusing ;-)\n\n t/t5551-http-fetch.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex cb95b95..47eb769 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -158,9 +158,8 @@ test_expect_success 'GIT_SMART_HTTP can disable smart http' '\n '\n \n test_expect_success 'invalid Content-Type rejected' '\n-\techo \"fatal: invalid content-type text/html\" >expect\n \ttest_must_fail git clone $HTTPD_URL/broken_smart/repo.git 2>actual\n-\ttest_cmp expect actual\n+\tgrep \"not valid:\" actual\n '\n \n test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n"},{"id":"208793","messageId":"51122F69.9060704@elegosoft.com","threadId":"32803","inReplyTo":"7v38xhf1i3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2013-02-06T10:24:41Z","receivedAt":"2013-02-06T10:24:41Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"On 01/31/2013 11:09 PM, Junio C Hamano wrote:\n\n>  \n> -static int http_request_reauth(const char *url, void *result, int target,\n> +static int http_request_reauth(const char *url,\n> +\t\t\t       struct strbuf *type,\n> +\t\t\t       void *result, int target,\n>  \t\t\t       int options)\n>  {\n> -\tint ret = http_request(url, result, target, options);\n> +\tint ret = http_request(url, type, result, target, options);\n>  \tif (ret != HTTP_REAUTH)\n>  \t\treturn ret;\n> -\treturn http_request(url, result, target, options);\n> +\treturn http_request(url, type, result, target, options);\n>  }\n\nThis needs something like\n\ndiff --git a/http.c b/http.c\nindex d868d8b..da43be3 100644\n--- a/http.c\n+++ b/http.c\n@@ -860,6 +860,8 @@ static int http_request_reauth(const char *url,\n        int ret = http_request(url, type, result, target, options);\n        if (ret != HTTP_REAUTH)\n                return ret;\n+       if (type)\n+               strbuf_reset(type);\n        return http_request(url, type, result, target, options);\n }\n\non top. Otherwise we get\n\n\"text/plainapplication/x-git-receive-pack-advertisement\"\n\nwhen doing HTTP auth.\n\nThanks.\n\n> -int http_get_strbuf(const char *url, struct strbuf *result, int options)\n> +int http_get_strbuf(const char *url,\n> +\t\t    struct strbuf *type,\n> +\t\t    struct strbuf *result, int options)\n>  {\n> -\treturn http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);\n> +\treturn http_request_reauth(url, type, result,\n> +\t\t\t\t   HTTP_REQUEST_STRBUF, options);\n>  }\n>  \n>  /*\n> @@ -878,7 +891,7 @@ static int http_get_file(const char *url, const char *filename, int options)\n>  \t\tgoto cleanup;\n>  \t}\n>  \n> -\tret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);\n> +\tret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);\n>  \tfclose(result);\n>  \n>  \tif ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename))\n> @@ -904,7 +917,7 @@ int http_fetch_ref(const char *base, struct ref *ref)\n>  \tint ret = -1;\n>  \n>  \turl = quote_ref_url(base, ref->name);\n> -\tif (http_get_strbuf(url, &buffer, HTTP_NO_CACHE) == HTTP_OK) {\n> +\tif (http_get_strbuf(url, NULL, &buffer, HTTP_NO_CACHE) == HTTP_OK) {\n>  \t\tstrbuf_rtrim(&buffer);\n>  \t\tif (buffer.len == 40)\n>  \t\t\tret = get_sha1_hex(buffer.buf, ref->old_sha1);\n> @@ -997,7 +1010,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n>  \tstrbuf_addstr(&buf, \"objects/info/packs\");\n>  \turl = strbuf_detach(&buf, NULL);\n>  \n> -\tret = http_get_strbuf(url, &buf, HTTP_NO_CACHE);\n> +\tret = http_get_strbuf(url, NULL, &buf, HTTP_NO_CACHE);\n>  \tif (ret != HTTP_OK)\n>  \t\tgoto cleanup;\n>  \n> diff --git a/http.h b/http.h\n> index 0a80d30..25d1931 100644\n> --- a/http.h\n> +++ b/http.h\n> @@ -132,7 +132,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n>   *\n>   * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.\n>   */\n> -int http_get_strbuf(const char *url, struct strbuf *result, int options);\n> +int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);\n>  \n>  /*\n>   * Prints an error message using error() containing url and curl_errorstr,\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 9a8b123..e6f3b63 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -92,6 +92,8 @@ static void free_discovery(struct discovery *d)\n>  \n>  static struct discovery* discover_refs(const char *service)\n>  {\n> +\tstruct strbuf exp = STRBUF_INIT;\n> +\tstruct strbuf type = STRBUF_INIT;\n>  \tstruct strbuf buffer = STRBUF_INIT;\n>  \tstruct discovery *last = last_discovery;\n>  \tchar *refs_url;\n> @@ -113,7 +115,7 @@ static struct discovery* discover_refs(const char *service)\n>  \t}\n>  \trefs_url = strbuf_detach(&buffer, NULL);\n>  \n> -\thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n> +\thttp_ret = http_get_strbuf(refs_url, &type, &buffer, HTTP_NO_CACHE);\n>  \tswitch (http_ret) {\n>  \tcase HTTP_OK:\n>  \t\tbreak;\n> @@ -133,16 +135,19 @@ static struct discovery* discover_refs(const char *service)\n>  \tlast->buf = last->buf_alloc;\n>  \n>  \tif (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n> -\t\t/* smart HTTP response; validate that the service\n> +\t\t/*\n> +\t\t * smart HTTP response; validate that the service\n>  \t\t * pkt-line matches our request.\n>  \t\t */\n> -\t\tstruct strbuf exp = STRBUF_INIT;\n> -\n> +\t\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> +\t\tif (strbuf_cmp(&exp, &type))\n> +\t\t\tdie(\"invalid content-type %s\", type.buf);\n>  \t\tif (packet_get_line(&buffer, &last->buf, &last->len) <= 0)\n>  \t\t\tdie(\"%s has invalid packet header\", refs_url);\n>  \t\tif (buffer.len && buffer.buf[buffer.len - 1] == '\\n')\n>  \t\t\tstrbuf_setlen(&buffer, buffer.len - 1);\n>  \n> +\t\tstrbuf_reset(&exp);\n>  \t\tstrbuf_addf(&exp, \"# service=%s\", service);\n>  \t\tif (strbuf_cmp(&exp, &buffer))\n>  \t\t\tdie(\"invalid server response; got '%s'\", buffer.buf);\n> @@ -160,6 +165,8 @@ static struct discovery* discover_refs(const char *service)\n>  \t}\n>  \n>  \tfree(refs_url);\n> +\tstrbuf_release(&exp);\n> +\tstrbuf_release(&type);\n>  \tstrbuf_release(&buffer);\n>  \tlast_discovery = last;\n>  \treturn last;\n"},{"id":"208794","messageId":"20130206103952.GA5267@sigill.intra.peff.net","threadId":"32803","inReplyTo":"51122F69.9060704@elegosoft.com","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-06T10:39:52Z","receivedAt":"2013-02-06T10:39:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2013 at 11:24:41AM +0100, Michael Schubert wrote:\n\n> On 01/31/2013 11:09 PM, Junio C Hamano wrote:\n> \n> >  \n> > -static int http_request_reauth(const char *url, void *result, int target,\n> > +static int http_request_reauth(const char *url,\n> > +\t\t\t       struct strbuf *type,\n> > +\t\t\t       void *result, int target,\n> >  \t\t\t       int options)\n> >  {\n> > -\tint ret = http_request(url, result, target, options);\n> > +\tint ret = http_request(url, type, result, target, options);\n> >  \tif (ret != HTTP_REAUTH)\n> >  \t\treturn ret;\n> > -\treturn http_request(url, result, target, options);\n> > +\treturn http_request(url, type, result, target, options);\n> >  }\n> \n> This needs something like\n> \n> diff --git a/http.c b/http.c\n> index d868d8b..da43be3 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -860,6 +860,8 @@ static int http_request_reauth(const char *url,\n>         int ret = http_request(url, type, result, target, options);\n>         if (ret != HTTP_REAUTH)\n>                 return ret;\n> +       if (type)\n> +               strbuf_reset(type);\n>         return http_request(url, type, result, target, options);\n>  }\n> \n> on top. Otherwise we get\n> \n> \"text/plainapplication/x-git-receive-pack-advertisement\"\n> \n> when doing HTTP auth.\n\nGood catch. It probably makes sense to put it in http_request, so that\nwe also protect against any existing cruft from the callers of\nhttp_get_*, like:\n\n-- >8 --\nSubject: [PATCH] http_request: reset \"type\" strbuf before adding\n\nCallers may pass us a strbuf which we use to record the\ncontent-type of the response. However, we simply appended to\nit rather than overwriting its contents, meaning that cruft\nin the strbuf gave us a bogus type. E.g., the multiple\nrequests triggered by http_request could yield a type like\n\"text/plainapplication/x-git-receive-pack-advertisement\".\n\nReported-by: Michael Schubert <mschub@elegosoft.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIs it worth having a strbuf_set* family of functions to match the\nstrbuf_add*? We seem to have these sorts of errors with strbuf from time\nto time, and I wonder if that would make it easier (and more readable)\nto do the right thing.\n\n http.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/http.c b/http.c\nindex d868d8b..d9d1aad 100644\n--- a/http.c\n+++ b/http.c\n@@ -841,6 +841,7 @@ static int http_request(const char *url, struct strbuf *type,\n \n \tif (type) {\n \t\tchar *t;\n+\t\tstrbuf_reset(type);\n \t\tcurl_easy_getinfo(slot->curl, CURLINFO_CONTENT_TYPE, &t);\n \t\tif (t)\n \t\t\tstrbuf_addstr(type, t);\n-- \n1.8.1.2.11.g1a2f572\n"},{"id":"208813","messageId":"7v4nhpo2qv.fsf@alter.siamese.dyndns.org","threadId":"32803","inReplyTo":"20130206103952.GA5267@sigill.intra.peff.net","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-06T15:56:08Z","receivedAt":"2013-02-06T15:56:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Is it worth having a strbuf_set* family of functions to match the\n> strbuf_add*? We seem to have these sorts of errors with strbuf from time\n> to time, and I wonder if that would make it easier (and more readable)\n> to do the right thing.\n\nPossibly.\n\nThe callsite below may be a poor example, though; you would need the\n_reset() even if you change the _addstr() we can see in the context\nto _setstr() to make sure later strbuf_*(type) will start from a\nclean slate when !t anyway, no?\n\n>\n>  http.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/http.c b/http.c\n> index d868d8b..d9d1aad 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -841,6 +841,7 @@ static int http_request(const char *url, struct strbuf *type,\n>  \n>  \tif (type) {\n>  \t\tchar *t;\n> +\t\tstrbuf_reset(type);\n>  \t\tcurl_easy_getinfo(slot->curl, CURLINFO_CONTENT_TYPE, &t);\n>  \t\tif (t)\n>  \t\t\tstrbuf_addstr(type, t);\n"},{"id":"208880","messageId":"20130206224701.GH27507@sigill.intra.peff.net","threadId":"32803","inReplyTo":"7v4nhpo2qv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Verify Content-Type from smart HTTP servers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-06T22:47:01Z","receivedAt":"2013-02-06T22:47:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2013 at 07:56:08AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Is it worth having a strbuf_set* family of functions to match the\n> > strbuf_add*? We seem to have these sorts of errors with strbuf from time\n> > to time, and I wonder if that would make it easier (and more readable)\n> > to do the right thing.\n> \n> Possibly.\n> \n> The callsite below may be a poor example, though; you would need the\n> _reset() even if you change the _addstr() we can see in the context\n> to _setstr() to make sure later strbuf_*(type) will start from a\n> clean slate when !t anyway, no?\n\nAh, true. Let's not worry about it, then.\n\n-Peff\n"}]}