Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-06 Thread Michael Schubert 
On 01/31/2013 11:09 PM, Junio C Hamano wrote:

  
 -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);
  }

This needs something like

diff --git a/http.c b/http.c
index d868d8b..da43be3 100644
--- a/http.c
+++ b/http.c
@@ -860,6 +860,8 @@ static int http_request_reauth(const char *url,
int ret = http_request(url, type, result, target, options);
if (ret != HTTP_REAUTH)
return ret;
+   if (type)
+   strbuf_reset(type);
return http_request(url, type, result, target, options);
 }

on top. Otherwise we get

text/plainapplication/x-git-receive-pack-advertisement

when doing HTTP auth.

Thanks.

 -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 0a80d30..25d1931 100644
 --- 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 9a8b123..e6f3b63 100644
 --- 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;
 @@ -133,16 +135,19 @@ static struct discovery* discover_refs(const char 
 *service)
   last-buf = last-buf_alloc;
  
   if (maybe_smart  5 = last-len  last-buf[4] == '#') {
 - /* smart HTTP response; validate that the service
 + /*
 +  * smart HTTP response; validate that the service
* pkt-line matches our request.
*/
 - struct strbuf exp = STRBUF_INIT;
 -
 + strbuf_addf(exp, application/x-%s-advertisement, service);
 + if (strbuf_cmp(exp, type))
 + die(invalid content-type %s, type.buf);
   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')

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-06 Thread Jeff King 
On Wed, Feb 06, 2013 at 11:24:41AM +0100, Michael Schubert wrote:

 On 01/31/2013 11:09 PM, Junio C Hamano wrote:
 
   
  -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);
   }
 
 This needs something like
 
 diff --git a/http.c b/http.c
 index d868d8b..da43be3 100644
 --- a/http.c
 +++ b/http.c
 @@ -860,6 +860,8 @@ static int http_request_reauth(const char *url,
 int ret = http_request(url, type, result, target, options);
 if (ret != HTTP_REAUTH)
 return ret;
 +   if (type)
 +   strbuf_reset(type);
 return http_request(url, type, result, target, options);
  }
 
 on top. Otherwise we get
 
 text/plainapplication/x-git-receive-pack-advertisement
 
 when doing HTTP auth.

Good catch. It probably makes sense to put it in http_request, so that
we also protect against any existing cruft from the callers of
http_get_*, like:

-- 8 --
Subject: [PATCH] http_request: reset type strbuf before adding

Callers may pass us a strbuf which we use to record the
content-type of the response. However, we simply appended to
it rather than overwriting its contents, meaning that cruft
in the strbuf gave us a bogus type. E.g., the multiple
requests triggered by http_request could yield a type like
text/plainapplication/x-git-receive-pack-advertisement.

Reported-by: Michael Schubert msc...@elegosoft.com
Signed-off-by: Jeff King p...@peff.net
---
Is it worth having a strbuf_set* family of functions to match the
strbuf_add*? We seem to have these sorts of errors with strbuf from time
to time, and I wonder if that would make it easier (and more readable)
to do the right thing.

 http.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/http.c b/http.c
index d868d8b..d9d1aad 100644
--- a/http.c
+++ b/http.c
@@ -841,6 +841,7 @@ static int http_request(const char *url, struct strbuf 
*type,
 
if (type) {
char *t;
+   strbuf_reset(type);
curl_easy_getinfo(slot-curl, CURLINFO_CONTENT_TYPE, t);
if (t)
strbuf_addstr(type, t);
-- 
1.8.1.2.11.g1a2f572

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-06 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 Is it worth having a strbuf_set* family of functions to match the
 strbuf_add*? We seem to have these sorts of errors with strbuf from time
 to time, and I wonder if that would make it easier (and more readable)
 to do the right thing.

Possibly.

The callsite below may be a poor example, though; you would need the
_reset() even if you change the _addstr() we can see in the context
to _setstr() to make sure later strbuf_*(type) will start from a
clean slate when !t anyway, no?


  http.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/http.c b/http.c
 index d868d8b..d9d1aad 100644
 --- a/http.c
 +++ b/http.c
 @@ -841,6 +841,7 @@ static int http_request(const char *url, struct strbuf 
 *type,
  
   if (type) {
   char *t;
 + strbuf_reset(type);
   curl_easy_getinfo(slot-curl, CURLINFO_CONTENT_TYPE, t);
   if (t)
   strbuf_addstr(type, t);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-06 Thread Jeff King 
On Wed, Feb 06, 2013 at 07:56:08AM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  Is it worth having a strbuf_set* family of functions to match the
  strbuf_add*? We seem to have these sorts of errors with strbuf from time
  to time, and I wonder if that would make it easier (and more readable)
  to do the right thing.
 
 Possibly.
 
 The callsite below may be a poor example, though; you would need the
 _reset() even if you change the _addstr() we can see in the context
 to _setstr() to make sure later strbuf_*(type) will start from a
 clean slate when !t anyway, no?

Ah, true. Let's not worry about it, then.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-04 Thread Jeff King 
On Sun, Feb 03, 2013 at 11:17:33PM -0800, Junio C Hamano wrote:

 Does this look good to both of you (relative to Shawn's patch)?
 
  remote-curl.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/remote-curl.c b/remote-curl.c
 index e6f3b63..933c69a 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -134,14 +134,14 @@ 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] == '#') {
 + 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.
*/
 - strbuf_addf(exp, application/x-%s-advertisement, service);
 - if (strbuf_cmp(exp, type))
 - die(invalid content-type %s, type.buf);
   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')

Yeah, I think that's fine. Thanks.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-04 Thread Shawn Pearce 
On Mon, Feb 4, 2013 at 12:38 AM, Jeff King p...@peff.net wrote:
 On Sun, Feb 03, 2013 at 11:17:33PM -0800, Junio C Hamano wrote:

  Does this look good to both of you (relative to Shawn's patch)?
 
   remote-curl.c | 8 
   1 file changed, 4 insertions(+), 4 deletions(-)
 
  diff --git a/remote-curl.c b/remote-curl.c
  index e6f3b63..933c69a 100644
  --- a/remote-curl.c
  +++ b/remote-curl.c
  @@ -134,14 +134,14 @@ 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] == '#') {
  + 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.
 */
  - strbuf_addf(exp, application/x-%s-advertisement, service);
  - if (strbuf_cmp(exp, type))
  - die(invalid content-type %s, type.buf);
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')

 Yeah, I think that's fine. Thanks.

Looks fine to me too, but I think the test won't work now. :-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-04 Thread Junio C Hamano 
Shawn Pearce spea...@spearce.org writes:

 Looks fine to me too, but I think the test won't work now. :-)

Heh, that's amusing ;-)

 t/t5551-http-fetch.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index cb95b95..47eb769 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -158,9 +158,8 @@ test_expect_success 'GIT_SMART_HTTP can disable smart http' 
'
 '
 
 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 2actual
-   test_cmp expect actual
+   grep not valid: actual
 '
 
 test -n $GIT_TEST_LONG  test_set_prereq EXPENSIVE

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-03 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 I was specifically thinking of this (on top of your patch):

 diff --git a/remote-curl.c b/remote-curl.c
 index e6f3b63..63680a8 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -134,14 +134,12 @@ 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] == '#') {
 + strbuf_addf(exp, application/x-%s-advertisement, service);
 + if (maybe_smart  !strbuf_cmp(exp, type)) {
   /*
* smart HTTP response; validate that the service
* pkt-line matches our request.
*/
 - strbuf_addf(exp, application/x-%s-advertisement, service);
 - if (strbuf_cmp(exp, type))
 - die(invalid content-type %s, type.buf);
   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')

 To just follow the dumb path if we don't get the content-type we expect.
 We may want to keep the '#' format check in addition (packet_get_line
 will check it and die, anyway, but we may want to drop back to
 considering it dumb, just to protect against a badly configured dumb
 server which uses our mime type, but I do not think it likely).

Yeah, but it doesn't cost anything to check, so let's do so.

Does this look good to both of you (relative to Shawn's patch)?

 remote-curl.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index e6f3b63..933c69a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -134,14 +134,14 @@ 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] == '#') {
+   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.
 */
-   strbuf_addf(exp, application/x-%s-advertisement, service);
-   if (strbuf_cmp(exp, type))
-   die(invalid content-type %s, type.buf);
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')
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-01 Thread Jeff King 
On Thu, Jan 31, 2013 at 02:09:40PM -0800, Junio C Hamano wrote:

 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 spea...@spearce.org
 Signed-off-by: Junio C Hamano gits...@pobox.com

Should this be From: Shawn? The tone of the message and the S-O-B
order makes it look like it.

 @@ -133,16 +135,19 @@ static struct discovery* discover_refs(const char 
 *service)
   last-buf = last-buf_alloc;
  
   if (maybe_smart  5 = last-len  last-buf[4] == '#') {
 - /* smart HTTP response; validate that the service
 + /*
 +  * smart HTTP response; validate that the service
* pkt-line matches our request.
*/
 - struct strbuf exp = STRBUF_INIT;
 -
 + strbuf_addf(exp, application/x-%s-advertisement, service);
 + if (strbuf_cmp(exp, type))
 + die(invalid content-type %s, type.buf);

Hmm. I wondered if it is possible for a non-smart server to send us down
this code path, which would now complain of the bogus content-type.
Something like an info/refs file with:

  # 1
  # the comment above is meaningless, but puts a # at position 4.

But I note that we would already die in the next line:

   if (packet_get_line(buffer, last-buf, last-len) = 0)
   die(%s has invalid packet header, refs_url);

so I do not think the patch makes anything worse. However, should we
take this opportunity to make the did we get a smart response test
more robust? That is, should we actually be checking the content-type
in the outer conditional, and going down the smart code-path if it is
application/x-%s-advertisement, and otherwise treating the result as
dumb?

It's probably not a big deal, as the false positive example above is
quite specific and unlikely, but it just seems cleaner to me.

As a side note, should we (can we) care about the content-type for dumb
http? It should probably be text/plain or application/octet-stream, but
I would not be surprised if we get a variety of random junk in the real
world, though.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-01 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 Should this be From: Shawn? The tone of the message and the S-O-B
 order makes it look like it.

Yes. I should have left that line when edited the format-patch
output in my MUA to say I was resending something that vger rejected
and people did not see after tweaking the patch to slip their taboo
list.

 @@ -133,16 +135,19 @@ static struct discovery* discover_refs(const char 
 *service)
  last-buf = last-buf_alloc;
  
  if (maybe_smart  5 = last-len  last-buf[4] == '#') {
 -/* smart HTTP response; validate that the service
 +/*
 + * smart HTTP response; validate that the service
   * pkt-line matches our request.
   */
 -struct strbuf exp = STRBUF_INIT;
 -
 +strbuf_addf(exp, application/x-%s-advertisement, service);
 +if (strbuf_cmp(exp, type))
 +die(invalid content-type %s, type.buf);

 Hmm. I wondered if it is possible for a non-smart server to send us down
 this code path, which would now complain of the bogus content-type.
 Something like an info/refs file with:

   # 1
   # the comment above is meaningless, but puts a # at position 4.

 But I note that we would already die in the next line:

  if (packet_get_line(buffer, last-buf, last-len) = 0)
  die(%s has invalid packet header, refs_url);

 so I do not think the patch makes anything worse. However, should we
 take this opportunity to make the did we get a smart response test
 more robust? That is, should we actually be checking the content-type
 in the outer conditional, and going down the smart code-path if it is
 application/x-%s-advertisement, and otherwise treating the result as
 dumb?

Does the outer caller that switches between dumb and smart actually
know what service type it is requesting (I am not familiar with the
callchain involved)?  Even if it doesn't, it may still make sense.

 As a side note, should we (can we) care about the content-type for dumb
 http? It should probably be text/plain or application/octet-stream, but
 I would not be surprised if we get a variety of random junk in the real
 world, though.

The design objective of dumb http protocol was to allow working with
any dumb bit transfer thing, so I'd prefer to keep it lenient and
allow application/x-git-loose-object-file and somesuch.

Thanks.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-02-01 Thread Jeff King 
On Fri, Feb 01, 2013 at 10:09:26AM -0800, Junio C Hamano wrote:

  so I do not think the patch makes anything worse. However, should we
  take this opportunity to make the did we get a smart response test
  more robust? That is, should we actually be checking the content-type
  in the outer conditional, and going down the smart code-path if it is
  application/x-%s-advertisement, and otherwise treating the result as
  dumb?
 
 Does the outer caller that switches between dumb and smart actually
 know what service type it is requesting (I am not familiar with the
 callchain involved)?  Even if it doesn't, it may still make sense.

I was specifically thinking of this (on top of your patch):

diff --git a/remote-curl.c b/remote-curl.c
index e6f3b63..63680a8 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -134,14 +134,12 @@ 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] == '#') {
+   strbuf_addf(exp, application/x-%s-advertisement, service);
+   if (maybe_smart  !strbuf_cmp(exp, type)) {
/*
 * smart HTTP response; validate that the service
 * pkt-line matches our request.
 */
-   strbuf_addf(exp, application/x-%s-advertisement, service);
-   if (strbuf_cmp(exp, type))
-   die(invalid content-type %s, type.buf);
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')

To just follow the dumb path if we don't get the content-type we expect.
We may want to keep the '#' format check in addition (packet_get_line
will check it and die, anyway, but we may want to drop back to
considering it dumb, just to protect against a badly configured dumb
server which uses our mime type, but I do not think it likely).

  As a side note, should we (can we) care about the content-type for dumb
  http? It should probably be text/plain or application/octet-stream, but
  I would not be surprised if we get a variety of random junk in the real
  world, though.
 
 The design objective of dumb http protocol was to allow working with
 any dumb bit transfer thing, so I'd prefer to keep it lenient and
 allow application/x-git-loose-object-file and somesuch.

Yeah, I do not think it really buys us anything in practice, and we have
no way of knowing what kind of crap is in the wild. Not worth it.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Verify Content-Type from smart HTTP servers

2013-01-31 Thread Junio C Hamano 
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 spea...@spearce.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 * The original was picked up by majordomo taboo rules; resending
   after minor style fix.

   Was there a report of an attempted attack by malicious server or
   something that triggered this, or is this just a common sense
   thing to do in general?

 http-push.c  |  4 ++--
 http.c   | 31 ++-
 http.h   |  2 +-
 remote-curl.c| 15 +++
 t/lib-httpd.sh   |  1 +
 t/lib-httpd/apache.conf  |  4 
 t/lib-httpd/broken-smart-http.sh | 11 +++
 t/t5551-http-fetch.sh|  6 ++
 8 files changed, 58 insertions(+), 16 deletions(-)
 create mode 100755 t/lib-httpd/broken-smart-http.sh

diff --git a/http-push.c b/http-push.c
index 8701c12..ba45b7b 100644
--- 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 44f3525..d868d8b 100644
--- a/http.c
+++ b/http.c
@@ -788,7 +788,8 @@ int handle_curl_result(struct slot_results *results)
 #define HTTP_REQUEST_STRBUF0
 #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 =

Re: [PATCH] Verify Content-Type from smart HTTP servers

2013-01-31 Thread Shawn Pearce 
On Thu, Jan 31, 2013 at 1:59 PM, Junio C Hamano gits...@pobox.com wrote:
 Shawn Pearce spea...@spearce.org writes:

 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 spea...@spearce.org
 ---

 Sounds sensible.  Was there a report of attack attempts by malicious
 servers or something, or is it just a general common sense thing?

Common-sense cleanup.

I had a report a while ago about JGit not working with the Git servers
at Codeplex. This failure was caused by their HTTP servers returning
an invalid Content-Type, making JGit refuse to continue parsing. This
has since been fixed, I verified this morning that Codeplex is
returning the correct Content-Type.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html