{"thread":{"id":"44583","subject":"[PATCH 3/6] remote-curl: rename shadowed options variable","startedAt":"2016-12-01T09:04:55Z","lastAt":"2016-12-05T13:08:49Z","messageCount":18,"participants":["Jeff King","Ramsay Jones","Brandon Williams","Philip Oakley","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"306679","messageId":"20161201090422.wzyquf4swsv33ti6@sigill.intra.peff.net","threadId":"44583","inReplyTo":"20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net","subject":"[PATCH 3/6] remote-curl: rename shadowed options variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T09:04:22Z","receivedAt":"2016-12-01T09:04:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The discover_refs() function has a local \"options\" variable\nto hold the http_get_options we pass to http_get_strbuf().\nBut this shadows the global \"struct options\" that holds our\nprogram-level options, which cannot be accessed from this\nfunction.\n\nLet's give the local one a more descriptive name so we can\ntell the two apart.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote-curl.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex f14c41f4c..f4145fd5c 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -274,7 +274,7 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tstruct strbuf effective_url = STRBUF_INIT;\n \tstruct discovery *last = last_discovery;\n \tint http_ret, maybe_smart = 0;\n-\tstruct http_get_options options;\n+\tstruct http_get_options http_options;\n \n \tif (last && !strcmp(service, last->service))\n \t\treturn last;\n@@ -291,15 +291,15 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t\tstrbuf_addf(&refs_url, \"service=%s\", service);\n \t}\n \n-\tmemset(&options, 0, sizeof(options));\n-\toptions.content_type = &type;\n-\toptions.charset = &charset;\n-\toptions.effective_url = &effective_url;\n-\toptions.base_url = &url;\n-\toptions.no_cache = 1;\n-\toptions.keep_error = 1;\n+\tmemset(&http_options, 0, sizeof(http_options));\n+\thttp_options.content_type = &type;\n+\thttp_options.charset = &charset;\n+\thttp_options.effective_url = &effective_url;\n+\thttp_options.base_url = &url;\n+\thttp_options.no_cache = 1;\n+\thttp_options.keep_error = 1;\n \n-\thttp_ret = http_get_strbuf(refs_url.buf, &buffer, &options);\n+\thttp_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);\n \tswitch (http_ret) {\n \tcase HTTP_OK:\n \t\tbreak;\n-- \n2.11.0.319.g1f4e1e0\n\n"},{"id":"306680","messageId":"20161201090351.4sy4nwxjhbyvacb6@sigill.intra.peff.net","threadId":"44583","inReplyTo":"20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net","subject":"[PATCH 1/6] http: simplify update_url_from_redirect","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T09:03:52Z","receivedAt":"2016-12-01T09:05:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function looks for a common tail between what we asked\nfor and where we were redirected to, but it open-codes the\ncomparison. We can avoid some confusing subtractions by\nusing strip_suffix_mem().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 4c4a812fc..840dbd1c7 100644\n--- a/http.c\n+++ b/http.c\n@@ -1664,7 +1664,7 @@ static int update_url_from_redirect(struct strbuf *base,\n \t\t\t\t    const struct strbuf *got)\n {\n \tconst char *tail;\n-\tsize_t tail_len;\n+\tsize_t new_len;\n \n \tif (!strcmp(asked, got->buf))\n \t\treturn 0;\n@@ -1673,14 +1673,12 @@ static int update_url_from_redirect(struct strbuf *base,\n \t\tdie(\"BUG: update_url_from_redirect: %s is not a superset of %s\",\n \t\t    asked, base->buf);\n \n-\ttail_len = strlen(tail);\n-\n-\tif (got->len < tail_len ||\n-\t    strcmp(tail, got->buf + got->len - tail_len))\n+\tnew_len = got->len;\n+\tif (!strip_suffix_mem(got->buf, &new_len, tail))\n \t\treturn 0; /* insane redirect scheme */\n \n \tstrbuf_reset(base);\n-\tstrbuf_add(base, got->buf, got->len - tail_len);\n+\tstrbuf_add(base, got->buf, new_len);\n \treturn 1;\n }\n \n-- \n2.11.0.319.g1f4e1e0\n\n"},{"id":"306681","messageId":"20161201090414.zgz7pimgpctghbwu@sigill.intra.peff.net","threadId":"44583","inReplyTo":"20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net","subject":"[PATCH 2/6] http: always update the base URL for redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T09:04:14Z","receivedAt":"2016-12-01T09:05:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a malicious server redirects the initial ref\nadvertisement, it may be able to leak sha1s from other,\nunrelated servers that the client has access to. For\nexample, imagine that Alice is a git user, she has access to\na private repository on a server hosted by Bob, and Mallory\nruns a malicious server and wants to find out about Bob's\nprivate repository.\n\nMallory asks Alice to clone an unrelated repository from her\nover HTTP. When Alice's client contacts Mallory's server for\nthe initial ref advertisement, the server issues an HTTP\nredirect for Bob's server. Alice contacts Bob's server and\ngets the ref advertisement for the private repository. If\nthere is anything to fetch, she then follows up by asking\nthe server for one or more sha1 objects. But who is the\nserver?\n\nIf it is still Mallory's server, then Alice will leak the\nexistence of those sha1s to her.\n\nSince commit c93c92f30 (http: update base URLs when we see\nredirects, 2013-09-28), the client usually rewrites the base\nURL such that all further requests will go to Bob's server.\nBut this is done by textually matching the URL. If we were\noriginally looking for \"http://mallory/repo.git/info/refs\",\nand we got pointed at \"http://bob/other.git/info/refs\", then\nwe know that the right root is \"http://bob/other.git\".\n\nIf the redirect appears to change more than just the root,\nwe punt and continue to use the original server. E.g.,\nimagine the redirect adds a URL component that Bob's server\nwill ignore, like \"http://bob/other.git/info/refs?dummy=1\".\n\nWe can solve this by aborting in this case rather than\nsilently continuing to use Mallory's server. In addition to\nprotecting from sha1 leakage, it's arguably safer and more\nsane to refuse a confusing redirect like that in general.\nFor example, part of the motivation in c93c92f30 is\navoiding accidentally sending credentials over clear http,\njust to get a response that says \"try again over https\". So\neven in a non-malicious case, we'd prefer to err on the side\nof caution.\n\nThe downside is that it's possible this will break a\nlegitimate but complicated server-side redirection scheme.\nThe setup given in the newly added test does work, but it's\nconvoluted enough that we don't need to care about it. A\nmore plausible case would be a server which redirects a\nrequest for \"info/refs?service=git-upload-pack\" to just\n\"info/refs\" (because it does not do smart HTTP, and for some\nreason really dislikes query parameters).  Right now we\nwould transparently downgrade to dumb-http, but with this\npatch, we'd complain (and the user would have to set\nGIT_SMART_HTTP=0 to fetch).\n\nReported-by: Jann Horn <jannh@google.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c                        | 12 ++++++++----\n t/lib-httpd/apache.conf       |  8 ++++++++\n t/t5551-http-fetch-smart.sh   |  4 ++++\n t/t5812-proto-disable-http.sh |  1 +\n 4 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 840dbd1c7..ba03beead 100644\n--- a/http.c\n+++ b/http.c\n@@ -1655,9 +1655,9 @@ static int http_request(const char *url,\n  *\n  * Note that this assumes a sane redirect scheme. It's entirely possible\n  * in the example above to end up at a URL that does not even end in\n- * \"info/refs\".  In such a case we simply punt, as there is not much we can\n- * do (and such a scheme is unlikely to represent a real git repository,\n- * which means we are likely about to abort anyway).\n+ * \"info/refs\".  In such a case we die. There's not much we can do, such a\n+ * scheme is unlikely to represent a real git repository, and failing to\n+ * rewrite the base opens options for malicious redirects to do funny things.\n  */\n static int update_url_from_redirect(struct strbuf *base,\n \t\t\t\t    const char *asked,\n@@ -1675,10 +1675,14 @@ static int update_url_from_redirect(struct strbuf *base,\n \n \tnew_len = got->len;\n \tif (!strip_suffix_mem(got->buf, &new_len, tail))\n-\t\treturn 0; /* insane redirect scheme */\n+\t\tdie(_(\"unable to update url base from redirection:\\n\"\n+\t\t      \"  asked for: %s\\n\"\n+\t\t      \"   redirect: %s\"),\n+\t\t    asked, got->buf);\n \n \tstrbuf_reset(base);\n \tstrbuf_add(base, got->buf, new_len);\n+\n \treturn 1;\n }\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex c3e631394..f98b23a3c 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -132,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]\n RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]\n RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]\n \n+# The first rule issues a client-side redirect to something\n+# that _doesn't_ look like a git repo. The second rule is a\n+# server-side rewrite, so that it turns out the odd-looking\n+# thing _is_ a git repo. The \"[PT]\" tells Apache to match\n+# the usual ScriptAlias rules for /smart.\n+RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]\n+RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]\n+\n # Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.\n # And as RewriteCond does not allow testing for non-matches, we match\n # the desired case first (one has abra, two has cadabra), and let it\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 1ec5b2747..6e5b9e42f 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -119,6 +119,10 @@ test_expect_success 'redirects re-root further requests' '\n \tgit clone $HTTPD_URL/smart-redir-limited/repo.git repo-redir-limited\n '\n \n+test_expect_success 're-rooting dies on insane schemes' '\n+\ttest_must_fail git clone $HTTPD_URL/insane-redir/repo.git insane\n+'\n+\n test_expect_success 'clone from password-protected repository' '\n \techo two >expect &&\n \tset_askpass user@host pass@host &&\ndiff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh\nindex 0d105d541..044cc152f 100755\n--- a/t/t5812-proto-disable-http.sh\n+++ b/t/t5812-proto-disable-http.sh\n@@ -18,6 +18,7 @@ test_proto \"smart http\" http \"$HTTPD_URL/smart/repo.git\"\n \n test_expect_success 'curl redirects respect whitelist' '\n \ttest_must_fail env GIT_ALLOW_PROTOCOL=http:https \\\n+\t\t\t   GIT_SMART_HTTP=0 \\\n \t\tgit clone \"$HTTPD_URL/ftp-redir/repo.git\" 2>stderr &&\n \t{\n \t\ttest_i18ngrep \"ftp.*disabled\" stderr ||\n-- \n2.11.0.319.g1f4e1e0\n\n"},{"id":"306682","messageId":"20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net","threadId":"44583","inReplyTo":null,"subject":"[PATCH 0/6] restricting http redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T09:03:37Z","receivedAt":"2016-12-01T09:05:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Jann Horn brought up on the git-security list some interesting\nsocial-engineering attacks around the way Git handles HTTP redirects.\nThese patches are my attempt to harden our redirect handling against\nthese attacks.\n\nOut of the box, they should make it more obvious to the user when we are\nredirecting, and avoid intermingling objects between multiple dumb-http\nrepositories. There's also a config flag (not on by default) to disable\nredirects entirely if you're operating in a more paranoid environment.\n\nThe individual commits have more details on the attack scenarios.\n\nI gave some thought to how this might interact with the\nbw/transport-protocol-policy topic, which lets you distinguish between\n\"from the user\" and \"from some other system\" when allowing protocols. I\nthink that topic is missing some bits when it comes to HTTP, which I\noutlined elsewhere:\n\n  http://public-inbox.org/git/20161201083005.dui572o4jxsqacas@sigill.intra.peff.net/\n\nI also wondered if the new http.followRedirects option in this series\ncould be replaced by just setting protocol.allow to \"user\".  But it's\nnot quite the same:\n\n  1. That only covers setting http.followRedirects to \"false\". There is\n     a special value \"initial\", which allows redirects on the initial\n     ref advertisement (see patch 4 for details).\n\n  2. The http.* options can be applied on a per-server basis. So you\n     might allow a trusted server to redirect you, but not others. The\n     protocol config is less flexible in that regard (it's less about\n     \"who are you contacting\" and more about \"what situation are you\n     in\").\n\nSo I think it's fine for the two to co-exist. There's some small\noverlap, but which is appropriate depends on what problem you're trying\nto solve.\n\nThanks Jann for the initial report and for good discussion on the\nsecurity list.\n\n  [1/6]: http: simplify update_url_from_redirect\n  [2/6]: http: always update the base URL for redirects\n  [3/6]: remote-curl: rename shadowed options variable\n  [4/6]: http: make redirects more obvious\n  [5/6]: http: treat http-alternates like redirects\n  [6/6]: http-walker: complain about non-404 loose object errors\n\n Documentation/config.txt      | 10 +++++++\n http-walker.c                 | 15 +++++++----\n http.c                        | 56 ++++++++++++++++++++++++++++++---------\n http.h                        | 10 ++++++-\n remote-curl.c                 | 22 +++++++++-------\n t/lib-httpd/apache.conf       | 14 ++++++++++\n t/t5550-http-fetch-dumb.sh    | 61 +++++++++++++++++++++++++++++++++++++++++++\n t/t5551-http-fetch-smart.sh   |  4 +++\n t/t5812-proto-disable-http.sh |  1 +\n 9 files changed, 165 insertions(+), 28 deletions(-)\n\n"},{"id":"306683","messageId":"20161201090436.cwh2pxio7afowtfs@sigill.intra.peff.net","threadId":"44583","inReplyTo":"20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net","subject":"[PATCH 6/6] http-walker: complain about non-404 loose object errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T09:04:37Z","receivedAt":"2016-12-01T09:11:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since commit 17966c0a6 (http: avoid disconnecting on 404s\nfor loose objects, 2016-07-11), we turn off curl's\nFAILONERROR option and instead manually deal with failing\nHTTP codes.\n\nHowever, the logic to do so only recognizes HTTP 404 as a\nfailure. This is probably the most common result, but if we\nwere to get another code, the curl result remains CURLE_OK,\nand we treat it as success. We still end up detecting the\nfailure when we try to zlib-inflate the object (which will\nfail), but instead of reporting the HTTP error, we just\nclaim that the object is corrupt.\n\nInstead, let's catch anything in the 300's or above as an\nerror (300's are redirects which are not an error at the\nHTTP level, but are an indication that we've explicitly\ndisabled redirects, so we should treat them as such; we\ncertainly don't have the resulting object content).\n\nNote that we also fill in req->errorstr, which we didn't do\nbefore. Without FAILONERROR, curl will not have filled this\nin, and it will remain a blank string. This never mattered\nfor the 404 case, because in the logic below we hit the\n\"missing_target()\" branch and print nothing. But for other\nerrors, we'd want to say _something_, if only to fill in the\nblank slot in the error message.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-walker.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex 25a8b1ad4..c2f81cd6a 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -482,10 +482,13 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)\n \t * we turned off CURLOPT_FAILONERROR to avoid losing a\n \t * persistent connection and got CURLE_OK.\n \t */\n-\tif (req->http_code == 404 && req->curl_result == CURLE_OK &&\n+\tif (req->http_code >= 300 && req->curl_result == CURLE_OK &&\n \t\t\t(starts_with(req->url, \"http://\") ||\n-\t\t\t starts_with(req->url, \"https://\")))\n+\t\t\t starts_with(req->url, \"https://\"))) {\n \t\treq->curl_result = CURLE_HTTP_RETURNED_ERROR;\n+\t\txsnprintf(req->errorstr, sizeof(req->errorstr),\n+\t\t\t  \"HTTP request failed\");\n+\t}\n \n \tif (obj_req->state == ABORTED) {\n \t\tret = error(\"Request for %s aborted\", hex);\n-- \n2.11.0.319.g1f4e1e0\n"},{"id":"306684","messageId":"20161201090428.pweq7slwujbne5hg@sigill.intra.peff.net","threadId":"44583","inReplyTo":"20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net","subject":"[PATCH 4/6] http: make redirects more obvious","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T09:04:28Z","receivedAt":"2016-12-01T09:11:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We instruct curl to always follow HTTP redirects. This is\nconvenient, but it creates opportunities for malicious\nservers to create confusing situations. For instance,\nimagine Alice is a git user with access to a private\nrepository on Bob's server. Mallory runs her own server and\nwants to access objects from Bob's repository.\n\nMallory may try a few tricks that involve asking Alice to\nclone from her, build on top, and then push the result:\n\n  1. Mallory may simply redirect all fetch requests to Bob's\n     server. Git will transparently follow those redirects\n     and fetch Bob's history, which Alice may believe she\n     got from Mallory. The subsequent push seems like it is\n     just feeding Mallory back her own objects, but is\n     actually leaking Bob's objects. There is nothing in\n     git's output to indicate that Bob's repository was\n     involved at all.\n\n     The downside (for Mallory) of this attack is that Alice\n     will have received Bob's entire repository, and is\n     likely to notice that when building on top of it.\n\n  2. If Mallory happens to know the sha1 of some object X in\n     Bob's repository, she can instead build her own history\n     that references that object. She then runs a dumb http\n     server, and Alice's client will fetch each object\n     individually. When it asks for X, Mallory redirects her\n     to Bob's server. The end result is that Alice obtains\n     objects from Bob, but they may be buried deep in\n     history. Alice is less likely to notice.\n\nBoth of these attacks are fairly hard to pull off. There's a\nsocial component in getting Mallory to convince Alice to\nwork with her. Alice may be prompted for credentials in\naccessing Bob's repository (but not always, if she is using\na credential helper that caches). Attack (1) requires a\ncertain amount of obliviousness on Alice's part while making\na new commit. Attack (2) requires that Mallory knows a sha1\nin Bob's repository, that Bob's server supports dumb http,\nand that the object in question is loose on Bob's server.\n\nBut we can probably make things a bit more obvious without\nany loss of functionality. This patch does two things to\nthat end.\n\nFirst, when we encounter a whole-repo redirect during the\ninitial ref discovery, we now inform the user on stderr,\nmaking attack (1) much more obvious.\n\nSecond, the decision to follow redirects is now\nconfigurable. The truly paranoid can set the new\nhttp.followRedirects to false to avoid any redirection\nentirely. But for a more practical default, we will disallow\nredirects only after the initial ref discovery. This is\nenough to thwart attacks similar to (2), while still\nallowing the common use of redirects at the repository\nlevel. Since c93c92f30 (http: update base URLs when we see\nredirects, 2013-09-28) we re-root all further requests from\nthe redirect destination, which should generally mean that\nno further redirection is necessary.\n\nAs an escape hatch, in case there really is a server that\nneeds to redirect individual requests, the user can set\nhttp.followRedirects to \"true\" (and this can be done on a\nper-server basis via http.*.followRedirects config).\n\nReported-by: Jann Horn <jannh@google.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt   | 10 ++++++++++\n http.c                     | 33 ++++++++++++++++++++++++++++++---\n http.h                     | 10 +++++++++-\n remote-curl.c              |  4 ++++\n t/lib-httpd/apache.conf    |  6 ++++++\n t/t5550-http-fetch-dumb.sh | 23 +++++++++++++++++++++++\n 6 files changed, 82 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex a0ab66aae..d51182a06 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1891,6 +1891,16 @@ http.userAgent::\n \tof common USER_AGENT strings (but not including those like git/1.7.1).\n \tCan be overridden by the `GIT_HTTP_USER_AGENT` environment variable.\n \n+http.followRedirects::\n+\tWhether git should follow HTTP redirects. If set to `true`, git\n+\twill transparently follow any redirect issued by a server it\n+\tencounters. If set to `false`, git will treat all redirects as\n+\terrors. If set to `initial`, git will follow redirects only for\n+\tthe initial request to a remote, but not for subsequent\n+\tfollow-up HTTP requests. Since git uses the redirected URL as\n+\tthe base for the follow-up requests, this is generally\n+\tsufficient. The default is `initial`.\n+\n http.<url>.*::\n \tAny of the http.* options above can be applied selectively to some URLs.\n \tFor a config key to match a URL, each element of the config key is\ndiff --git a/http.c b/http.c\nindex ba03beead..825118481 100644\n--- a/http.c\n+++ b/http.c\n@@ -111,6 +111,8 @@ static int http_proactive_auth;\n static const char *user_agent;\n static int curl_empty_auth;\n \n+enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;\n+\n #if LIBCURL_VERSION_NUM >= 0x071700\n /* Use CURLOPT_KEYPASSWD as is */\n #elif LIBCURL_VERSION_NUM >= 0x070903\n@@ -366,6 +368,16 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(\"http.followredirects\", var)) {\n+\t\tif (value && !strcmp(value, \"initial\"))\n+\t\t\thttp_follow_config = HTTP_FOLLOW_INITIAL;\n+\t\telse if (git_config_bool(var, value))\n+\t\t\thttp_follow_config = HTTP_FOLLOW_ALWAYS;\n+\t\telse\n+\t\t\thttp_follow_config = HTTP_FOLLOW_NONE;\n+\t\treturn 0;\n+\t}\n+\n \t/* Fall back on the default ones */\n \treturn git_default_config(var, value, cb);\n }\n@@ -717,7 +729,6 @@ static CURL *get_curl_handle(void)\n \t\t\t\t curl_low_speed_time);\n \t}\n \n-\tcurl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);\n \tcurl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);\n #if LIBCURL_VERSION_NUM >= 0x071301\n \tcurl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);\n@@ -1044,6 +1055,16 @@ struct active_request_slot *get_active_slot(void)\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);\n \n+\t/*\n+\t * Default following to off unless \"ALWAYS\" is configured; this gives\n+\t * callers a sane starting point, and they can tweak for individual\n+\t * HTTP_FOLLOW_* cases themselves.\n+\t */\n+\tif (http_follow_config == HTTP_FOLLOW_ALWAYS)\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);\n+\telse\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 0);\n+\n #if LIBCURL_VERSION_NUM >= 0x070a08\n \tcurl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);\n #endif\n@@ -1286,9 +1307,12 @@ static int handle_curl_result(struct slot_results *results)\n \t * If we see a failing http code with CURLE_OK, we have turned off\n \t * FAILONERROR (to keep the server's custom error response), and should\n \t * translate the code into failure here.\n+\t *\n+\t * Likewise, if we see a redirect (30x code), that means we turned off\n+\t * redirect-following, and we should treat the result as an error.\n \t */\n \tif (results->curl_result == CURLE_OK &&\n-\t    results->http_code >= 400) {\n+\t    results->http_code >= 300) {\n \t\tresults->curl_result = CURLE_HTTP_RETURNED_ERROR;\n \t\t/*\n \t\t * Normally curl will already have put the \"reason phrase\"\n@@ -1607,6 +1631,9 @@ static int http_request(const char *url,\n \t\tstrbuf_addstr(&buf, \" no-cache\");\n \tif (options && options->keep_error)\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n+\tif (options && options->initial_request &&\n+\t    http_follow_config == HTTP_FOLLOW_INITIAL)\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);\n \n \theaders = curl_slist_append(headers, buf.buf);\n \n@@ -2030,7 +2057,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n \t\tif (c != CURLE_OK)\n \t\t\tdie(\"BUG: curl_easy_getinfo for HTTP code failed: %s\",\n \t\t\t\tcurl_easy_strerror(c));\n-\t\tif (slot->http_code >= 400)\n+\t\tif (slot->http_code >= 300)\n \t\t\treturn size;\n \t}\n \ndiff --git a/http.h b/http.h\nindex 5ab9d9c32..02bccb7b0 100644\n--- a/http.h\n+++ b/http.h\n@@ -116,6 +116,13 @@ extern struct credential http_auth;\n \n extern char curl_errorstr[CURL_ERROR_SIZE];\n \n+enum http_follow_config {\n+\tHTTP_FOLLOW_NONE,\n+\tHTTP_FOLLOW_ALWAYS,\n+\tHTTP_FOLLOW_INITIAL\n+};\n+extern enum http_follow_config http_follow_config;\n+\n static inline int missing__target(int code, int result)\n {\n \treturn\t/* file:// URL -- do we ever use one??? */\n@@ -139,7 +146,8 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n /* Options for http_get_*() */\n struct http_get_options {\n \tunsigned no_cache:1,\n-\t\t keep_error:1;\n+\t\t keep_error:1,\n+\t\t initial_request:1;\n \n \t/* If non-NULL, returns the content-type of the response. */\n \tstruct strbuf *content_type;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex f4145fd5c..28d9d1063 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -296,6 +296,7 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \thttp_options.charset = &charset;\n \thttp_options.effective_url = &effective_url;\n \thttp_options.base_url = &url;\n+\thttp_options.initial_request = 1;\n \thttp_options.no_cache = 1;\n \thttp_options.keep_error = 1;\n \n@@ -314,6 +315,9 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t\tdie(\"unable to access '%s': %s\", url.buf, curl_errorstr);\n \t}\n \n+\tif (options.verbosity && !starts_with(refs_url.buf, url.buf))\n+\t\twarning(_(\"redirecting to %s\"), url.buf);\n+\n \tlast= xcalloc(1, sizeof(*last_discovery));\n \tlast->service = service;\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex f98b23a3c..69174c6e3 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -123,6 +123,7 @@ ScriptAlias /error/ error.sh/\n </Files>\n \n RewriteEngine on\n+RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]\n RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]\n RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]\n RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]\n@@ -140,6 +141,11 @@ RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]\n RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]\n RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]\n \n+# Serve info/refs internally without redirecting, but\n+# issue a redirect for any object requests.\n+RewriteRule ^/redir-objects/(.*/info/refs)$ /dumb/$1 [PT]\n+RewriteRule ^/redir-objects/(.*/objects/.*)$ /dumb/$1 [R=301]\n+\n # Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.\n # And as RewriteCond does not allow testing for non-matches, we match\n # the desired case first (one has abra, two has cadabra), and let it\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 7641417b4..532507b7c 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -307,5 +307,28 @@ test_expect_success 'remote-http complains cleanly about malformed urls' '\n \ttest_must_fail git remote-http http::/example.com/repo.git\n '\n \n+test_expect_success 'redirects can be forbidden/allowed' '\n+\ttest_must_fail git -c http.followRedirects=false \\\n+\t\tclone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&\n+\tgit -c http.followRedirects=true \\\n+\t\tclone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr\n+'\n+\n+test_expect_success 'redirects are reported to stderr' '\n+\t# just look for a snippet of the redirected-to URL\n+\ttest_i18ngrep /dumb/ stderr\n+'\n+\n+test_expect_success 'non-initial redirects can be forbidden' '\n+\ttest_must_fail git -c http.followRedirects=initial \\\n+\t\tclone $HTTPD_URL/redir-objects/repo.git redir-objects &&\n+\tgit -c http.followRedirects=true \\\n+\t\tclone $HTTPD_URL/redir-objects/repo.git redir-objects\n+'\n+\n+test_expect_success 'http.followRedirects defaults to \"initial\"' '\n+\ttest_must_fail git clone $HTTPD_URL/redir-objects/repo.git default\n+'\n+\n stop_httpd\n test_done\n-- \n2.11.0.319.g1f4e1e0\n\n"},{"id":"306685","messageId":"20161201090432.wtcu2jpacwcf6a4a@sigill.intra.peff.net","threadId":"44583","inReplyTo":"20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net","subject":"[PATCH 5/6] http: treat http-alternates like redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T09:04:33Z","receivedAt":"2016-12-01T09:11:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit made HTTP redirects more obvious and\ntightened up the default behavior. However, there's another\nway for a server to ask a git client to fetch arbitrary\ncontent: by having an http-alternates file (or a regular\nalternates file, which is used as a backup).\n\nSimilar to the HTTP redirect case, a malicious server can\nclaim to have refs pointing at object X, return a 404 when\nthe client asks for X, but point to some other URL via\nhttp-alternates, which the client will transparently fetch.\nThe end result is that it looks from the user's perspective\nlike the objects came from the malicious server, as the\nother URL is not mentioned at all.\n\nWorse, because we feed the new URL to curl ourselves, the\nusual protocol restrictions do not kick in (neither curl's\ndefault of disallowing file://, nor the protocol\nwhitelisting in f4113cac0 (http: limit redirection to\nprotocol-whitelist, 2015-09-22).\n\nLet's apply the same rules here as we do for HTTP redirects.\nNamely:\n\n  - unless http.followRedirects is set to \"always\", we will\n    not follow remote redirects from http-alternates (or\n    alternates) at all\n\n  - set CURLOPT_PROTOCOLS alongside CURLOPT_REDIR_PROTOCOLS\n    restrict ourselves to a known-safe set and respect any\n    user-provided whitelist.\n\n  - mention alternate object stores on stderr so that the\n    user is aware another source of objects may be involved\n\nThe first item may prove to be too restrictive. The most\ncommon use of alternates is to point to another path on the\nsame server. While it's possible for a single-server\nredirect to be an attack, it takes a fairly obscure setup\n(victim and evil repository on the same host, host speaks\ndumb http, and evil repository has access to edit its own\nhttp-alternates file).\n\nSo we could make the checks more specific, and only cover\ncross-server redirects. But that means parsing the URLs\nourselves, rather than letting curl handle them. This patch\ngoes for the simpler approach. Given that they are only used\nwith dumb http, http-alternates are probably pretty rare.\nAnd there's an escape hatch: the user can allow redirects on\na specific server by setting http.<url>.followRedirects to\n\"always\".\n\nReported-by: Jann Horn <jannh@google.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-walker.c              |  8 +++++---\n http.c                     |  1 +\n t/t5550-http-fetch-dumb.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 44 insertions(+), 3 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex 0b2425531..25a8b1ad4 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -274,9 +274,8 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\tstruct strbuf target = STRBUF_INIT;\n \t\t\t\tstrbuf_add(&target, base, serverlen);\n \t\t\t\tstrbuf_add(&target, data + i, posn - i - 7);\n-\t\t\t\tif (walker->get_verbosely)\n-\t\t\t\t\tfprintf(stderr, \"Also look at %s\\n\",\n-\t\t\t\t\t\ttarget.buf);\n+\t\t\t\twarning(\"adding alternate object store: %s\",\n+\t\t\t\t\ttarget.buf);\n \t\t\t\tnewalt = xmalloc(sizeof(*newalt));\n \t\t\t\tnewalt->next = NULL;\n \t\t\t\tnewalt->base = strbuf_detach(&target, NULL);\n@@ -302,6 +301,9 @@ static void fetch_alternates(struct walker *walker, const char *base)\n \tstruct alternates_request alt_req;\n \tstruct walker_data *cdata = walker->data;\n \n+\tif (http_follow_config != HTTP_FOLLOW_ALWAYS)\n+\t\treturn;\n+\n \t/*\n \t * If another request has already started fetching alternates,\n \t * wait for them to arrive and return to processing this request's\ndiff --git a/http.c b/http.c\nindex 825118481..051fe6e5a 100644\n--- a/http.c\n+++ b/http.c\n@@ -745,6 +745,7 @@ static CURL *get_curl_handle(void)\n \tif (is_transport_allowed(\"ftps\"))\n \t\tallowed_protocols |= CURLPROTO_FTPS;\n \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);\n+\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);\n #else\n \tif (transport_restrict_protocols())\n \t\twarning(\"protocol restrictions not applied to curl redirects because\\n\"\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 532507b7c..264a1ab8b 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -330,5 +330,43 @@ test_expect_success 'http.followRedirects defaults to \"initial\"' '\n \ttest_must_fail git clone $HTTPD_URL/redir-objects/repo.git default\n '\n \n+# The goal is for a clone of the \"evil\" repository, which has no objects\n+# itself, to cause the client to fetch objects from the \"victim\" repository.\n+test_expect_success 'set up evil alternates scheme' '\n+\tvictim=$HTTPD_DOCUMENT_ROOT_PATH/victim.git &&\n+\tgit init --bare \"$victim\" &&\n+\tgit -C \"$victim\" --work-tree=. commit --allow-empty -m secret &&\n+\tgit -C \"$victim\" repack -ad &&\n+\tgit -C \"$victim\" update-server-info &&\n+\tsha1=$(git -C \"$victim\" rev-parse HEAD) &&\n+\n+\tevil=$HTTPD_DOCUMENT_ROOT_PATH/evil.git &&\n+\tgit init --bare \"$evil\" &&\n+\t# do this by hand to avoid object existence check\n+\tprintf \"%s\\\\t%s\\\\n\" $sha1 refs/heads/master >\"$evil/info/refs\"\n+'\n+\n+# Here we'll just redirect via HTTP. In a real-world attack these would be on\n+# different servers, but we should reject it either way.\n+test_expect_success 'http-alternates is a non-initial redirect' '\n+\techo \"$HTTPD_URL/dumb/victim.git/objects\" \\\n+\t\t>\"$evil/objects/info/http-alternates\" &&\n+\ttest_must_fail git -c http.followRedirects=initial \\\n+\t\tclone $HTTPD_URL/dumb/evil.git evil-initial &&\n+\tgit -c http.followRedirects=true \\\n+\t\tclone $HTTPD_URL/dumb/evil.git evil-initial\n+'\n+\n+# Curl supports a lot of protocols that we'd prefer not to allow\n+# http-alternates to use, but it's hard to test whether curl has\n+# accessed, say, the SMTP protocol, because we are not running an SMTP server.\n+# But we can check that it does not allow access to file://, which would\n+# otherwise allow this clone to complete.\n+test_expect_success 'http-alternates cannot point at funny protocols' '\n+\techo \"file://$victim/objects\" >\"$evil/objects/info/http-alternates\" &&\n+\ttest_must_fail git -c http.followRedirects=true \\\n+\t\tclone \"$HTTPD_URL/dumb/evil.git\" evil-file\n+'\n+\n stop_httpd\n test_done\n-- \n2.11.0.319.g1f4e1e0\n\n"},{"id":"306689","messageId":"5a47c032-755d-ad11-390f-22ae19ca584a@ramsayjones.plus.com","threadId":"44583","inReplyTo":"20161201090428.pweq7slwujbne5hg@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] http: make redirects more obvious","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-12-01T16:06:51Z","receivedAt":"2016-12-01T16:10:37Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 01/12/16 09:04, Jeff King wrote:\n> We instruct curl to always follow HTTP redirects. This is\n> convenient, but it creates opportunities for malicious\n> servers to create confusing situations. For instance,\n> imagine Alice is a git user with access to a private\n> repository on Bob's server. Mallory runs her own server and\n\nAhem, so Mallory is female? (-blush-) :(\n\nATB,\nRamsay Jones\n\n"},{"id":"306690","messageId":"331124b5-aa2b-773c-23ac-975ad3f50dbf@ramsayjones.plus.com","threadId":"44583","inReplyTo":"20161201090414.zgz7pimgpctghbwu@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] http: always update the base URL for redirects","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-12-01T16:02:37Z","receivedAt":"2016-12-01T16:10:40Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 01/12/16 09:04, Jeff King wrote:\n> If a malicious server redirects the initial ref\n> advertisement, it may be able to leak sha1s from other,\n> unrelated servers that the client has access to. For\n> example, imagine that Alice is a git user, she has access to\n> a private repository on a server hosted by Bob, and Mallory\n> runs a malicious server and wants to find out about Bob's\n> private repository.\n> \n> Mallory asks Alice to clone an unrelated repository from her\n-----------------------------------------------------------^^^\n... from _him_ ? (ie Mallory)\n\n> over HTTP. When Alice's client contacts Mallory's server for\n> the initial ref advertisement, the server issues an HTTP\n> redirect for Bob's server. Alice contacts Bob's server and\n> gets the ref advertisement for the private repository. If\n> there is anything to fetch, she then follows up by asking\n> the server for one or more sha1 objects. But who is the\n> server?\n> \n> If it is still Mallory's server, then Alice will leak the\n> existence of those sha1s to her.\n------------------------------^^^\n... to _him_ ? (again Mallory)\n\nATB,\nRamsay Jones\n"},{"id":"306769","messageId":"20161201225331.GH54082@google.com","threadId":"44583","inReplyTo":"331124b5-aa2b-773c-23ac-975ad3f50dbf@ramsayjones.plus.com","subject":"Re: [PATCH 2/6] http: always update the base URL for redirects","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-01T22:53:31Z","receivedAt":"2016-12-01T22:53:39Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/01, Ramsay Jones wrote:\n> \n> \n> On 01/12/16 09:04, Jeff King wrote:\n> > If a malicious server redirects the initial ref\n> > advertisement, it may be able to leak sha1s from other,\n> > unrelated servers that the client has access to. For\n> > example, imagine that Alice is a git user, she has access to\n> > a private repository on a server hosted by Bob, and Mallory\n> > runs a malicious server and wants to find out about Bob's\n> > private repository.\n> > \n> > Mallory asks Alice to clone an unrelated repository from her\n> -----------------------------------------------------------^^^\n> ... from _him_ ? (ie Mallory)\n> \n> > over HTTP. When Alice's client contacts Mallory's server for\n> > the initial ref advertisement, the server issues an HTTP\n> > redirect for Bob's server. Alice contacts Bob's server and\n> > gets the ref advertisement for the private repository. If\n> > there is anything to fetch, she then follows up by asking\n> > the server for one or more sha1 objects. But who is the\n> > server?\n> > \n> > If it is still Mallory's server, then Alice will leak the\n> > existence of those sha1s to her.\n> ------------------------------^^^\n> ... to _him_ ? (again Mallory)\n> \n> ATB,\n> Ramsay Jones\n\nDepends, I only know Mallorys who are women so her seems appropriate.\n\n-- \nBrandon Williams\n"},{"id":"306771","messageId":"20161201230223.GI54082@google.com","threadId":"44583","inReplyTo":"20161201090432.wtcu2jpacwcf6a4a@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] http: treat http-alternates like redirects","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-01T23:02:23Z","receivedAt":"2016-12-01T23:02:30Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/01, Jeff King wrote:\n>   - set CURLOPT_PROTOCOLS alongside CURLOPT_REDIR_PROTOCOLS\n>     restrict ourselves to a known-safe set and respect any\n>     user-provided whitelist.\n\n\n\n> diff --git a/http.c b/http.c\n> index 825118481..051fe6e5a 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -745,6 +745,7 @@ static CURL *get_curl_handle(void)\n>  \tif (is_transport_allowed(\"ftps\"))\n>  \t\tallowed_protocols |= CURLPROTO_FTPS;\n>  \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);\n> +\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);\n>  #else\n>  \tif (transport_restrict_protocols())\n>  \t\twarning(\"protocol restrictions not applied to curl redirects because\\n\"\n\nBecause I don't know much about how curl works....Only\nhttp/https/ftp/ftps protocols are allowed to be passed to curl?  Is that\nbecause curl only understands those particular protocols?\n\n-- \nBrandon Williams\n"},{"id":"306773","messageId":"2297C36B9A1441748D7E68363A05F8C5@PhilipOakley","threadId":"44583","inReplyTo":"20161201225331.GH54082@google.com","subject":"Re: [PATCH 2/6] http: always update the base URL for redirects","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2016-12-01T23:13:12Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Brandon Williams\" <bmwill@google.com>\n> On 12/01, Ramsay Jones wrote:\n>>\n>>\n>> On 01/12/16 09:04, Jeff King wrote:\n>> > If a malicious server redirects the initial ref\n>> > advertisement, it may be able to leak sha1s from other,\n>> > unrelated servers that the client has access to. For\n>> > example, imagine that Alice is a git user, she has access to\n>> > a private repository on a server hosted by Bob, and Mallory\n>> > runs a malicious server and wants to find out about Bob's\n>> > private repository.\n>> >\n>> > Mallory asks Alice to clone an unrelated repository from her\n>> -----------------------------------------------------------^^^\n>> ... from _him_ ? (ie Mallory)\n>>\n>> > over HTTP. When Alice's client contacts Mallory's server for\n>> > the initial ref advertisement, the server issues an HTTP\n>> > redirect for Bob's server. Alice contacts Bob's server and\n>> > gets the ref advertisement for the private repository. If\n>> > there is anything to fetch, she then follows up by asking\n>> > the server for one or more sha1 objects. But who is the\n>> > server?\n>> >\n>> > If it is still Mallory's server, then Alice will leak the\n>> > existence of those sha1s to her.\n>> ------------------------------^^^\n>> ... to _him_ ? (again Mallory)\n>>\n>> ATB,\n>> Ramsay Jones\n>\n> Depends, I only know Mallorys who are women so her seems appropriate.\n>\n> -- \n> Brandon Williams\n>\nIn a British context \"Mallory and Irvine\" were two (male) climbers who died \non Everest in 1924 (tales of daring...), so it's easy to expect (from this \nside of the pond) that 'Mallory' would be male. However he was really George \nMallory.\n\nMeanwhile that search engine's images shows far more female Mallorys, so \nI've learnt something.\n--\nPhilip\n\n"},{"id":"306778","messageId":"xmqq8trz6wrq.fsf@gitster.mtv.corp.google.com","threadId":"44583","inReplyTo":"2297C36B9A1441748D7E68363A05F8C5@PhilipOakley","subject":"Re: [PATCH 2/6] http: always update the base URL for redirects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-01T23:43:21Z","receivedAt":"2016-12-01T23:48:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" <philipoakley@iee.org> writes:\n\n>> Depends, I only know Mallorys who are women so her seems appropriate.\n>>\n> In a British context \"Mallory and Irvine\" were two (male) climbers who\n> died on Everest in 1924 (tales of daring...), so it's easy to expect\n> (from this side of the pond) that 'Mallory' would be male. However he\n> was really George Mallory.\n>\n> Meanwhile that search engine's images shows far more female Mallorys,\n> so I've learnt something.\n\n\"baby name Mallory\" in search engine gave me several sites, most of\nthem telling me that is a girl's name except for one.\n\nDidn't think of doing image search, but that's a good way ;-)\n"},{"id":"306786","messageId":"20161202000602.xm5gb5gz75hnztki@sigill.intra.peff.net","threadId":"44583","inReplyTo":"20161201230223.GI54082@google.com","subject":"Re: [PATCH 5/6] http: treat http-alternates like redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-02T00:06:02Z","receivedAt":"2016-12-02T00:06:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2016 at 03:02:23PM -0800, Brandon Williams wrote:\n\n> > diff --git a/http.c b/http.c\n> > index 825118481..051fe6e5a 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -745,6 +745,7 @@ static CURL *get_curl_handle(void)\n> >  \tif (is_transport_allowed(\"ftps\"))\n> >  \t\tallowed_protocols |= CURLPROTO_FTPS;\n> >  \tcurl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);\n> > +\tcurl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);\n> >  #else\n> >  \tif (transport_restrict_protocols())\n> >  \t\twarning(\"protocol restrictions not applied to curl redirects because\\n\"\n> \n> Because I don't know much about how curl works....Only\n> http/https/ftp/ftps protocols are allowed to be passed to curl?  Is that\n> because curl only understands those particular protocols?\n\nNo, curl understands more protocols, and that is exactly the problem. We\ndon't want to accidentally have curl access file://, smtp://, or\nsimilar, based on what some server puts in their http-alternates file.\n\nYou should only be able to get to this code-path by calling one of\ngit-remote-{http,https,ftp,ftps}. So there is no problem with\nrestricting the protocol beyond those options. And there should be no\nproblem with restricting within that set; if the protocol we intend to\nfeed to curl had been disallowed by policy, git would have blocked it\nbefore hitting git-remote in the first place.\n\n-Peff\n"},{"id":"306787","messageId":"7cd35131-2e0c-0dff-8864-c099e313251d@ramsayjones.plus.com","threadId":"44583","inReplyTo":"xmqq8trz6wrq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/6] http: always update the base URL for redirects","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-12-02T00:07:50Z","receivedAt":"2016-12-02T00:07:58Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 01/12/16 23:43, Junio C Hamano wrote:\n> \"Philip Oakley\" <philipoakley@iee.org> writes:\n> \n>>> Depends, I only know Mallorys who are women so her seems appropriate.\n>>>\n>> In a British context \"Mallory and Irvine\" were two (male) climbers who\n>> died on Everest in 1924 (tales of daring...), so it's easy to expect\n>> (from this side of the pond) that 'Mallory' would be male. However he\n>> was really George Mallory.\n>>\n>> Meanwhile that search engine's images shows far more female Mallorys,\n>> so I've learnt something.\n> \n> \"baby name Mallory\" in search engine gave me several sites, most of\n> them telling me that is a girl's name except for one.\n> \n> Didn't think of doing image search, but that's a good way ;-)\n\nHeh, I didn't think about any of this. I was remembering the\ndescription of 'Man-in-the-middle Attack' from Applied Cryptography\n(Bruce Schneier) which implies that Mallory is male.\n\nATB,\nRamsay Jones\n\n"},{"id":"306788","messageId":"20161202001852.arasy44d6iczeeez@sigill.intra.peff.net","threadId":"44583","inReplyTo":"7cd35131-2e0c-0dff-8864-c099e313251d@ramsayjones.plus.com","subject":"Re: [PATCH 2/6] http: always update the base URL for redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-02T00:18:52Z","receivedAt":"2016-12-02T00:19:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 02, 2016 at 12:07:50AM +0000, Ramsay Jones wrote:\n\n> >> In a British context \"Mallory and Irvine\" were two (male) climbers who\n> >> died on Everest in 1924 (tales of daring...), so it's easy to expect\n> >> (from this side of the pond) that 'Mallory' would be male. However he\n> >> was really George Mallory.\n> >>\n> >> Meanwhile that search engine's images shows far more female Mallorys,\n> >> so I've learnt something.\n> > \n> > \"baby name Mallory\" in search engine gave me several sites, most of\n> > them telling me that is a girl's name except for one.\n> > \n> > Didn't think of doing image search, but that's a good way ;-)\n> \n> Heh, I didn't think about any of this. I was remembering the\n> description of 'Man-in-the-middle Attack' from Applied Cryptography\n> (Bruce Schneier) which implies that Mallory is male.\n\nI admit that I always assumed Applied Cryptography (and other papers)\nwere always talking about a female. But that's probably because I\nstarted with an assumption about the name in the first place. That\nprobably came from watching the Family Ties sitcom as a kid; the older\ndaughter is named Mallory (and if you google it, you can see some\namazing 80's haircuts and clothes).\n\nWe can call her Marsha if you want, instead evoking Brady Bunch memories\nof 60's clothing and haircuts.\n\n-Peff\n"},{"id":"306790","messageId":"777dfad5-02e2-73a9-fc38-1e6575c100aa@ramsayjones.plus.com","threadId":"44583","inReplyTo":"20161202001852.arasy44d6iczeeez@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] http: always update the base URL for redirects","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-12-02T01:21:37Z","receivedAt":"2016-12-02T01:22:13Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 02/12/16 00:18, Jeff King wrote:\n> On Fri, Dec 02, 2016 at 12:07:50AM +0000, Ramsay Jones wrote:\n> \n>>>> In a British context \"Mallory and Irvine\" were two (male) climbers who\n>>>> died on Everest in 1924 (tales of daring...), so it's easy to expect\n>>>> (from this side of the pond) that 'Mallory' would be male. However he\n>>>> was really George Mallory.\n>>>>\n>>>> Meanwhile that search engine's images shows far more female Mallorys,\n>>>> so I've learnt something.\n>>>\n>>> \"baby name Mallory\" in search engine gave me several sites, most of\n>>> them telling me that is a girl's name except for one.\n>>>\n>>> Didn't think of doing image search, but that's a good way ;-)\n>>\n>> Heh, I didn't think about any of this. I was remembering the\n>> description of 'Man-in-the-middle Attack' from Applied Cryptography\n>> (Bruce Schneier) which implies that Mallory is male.\n> \n> I admit that I always assumed Applied Cryptography (and other papers)\n> were always talking about a female. But that's probably because I\n> started with an assumption about the name in the first place. That\n> probably came from watching the Family Ties sitcom as a kid; the older\n> daughter is named Mallory (and if you google it, you can see some\n> amazing 80's haircuts and clothes).\n> \n> We can call her Marsha if you want, instead evoking Brady Bunch memories\n> of 60's clothing and haircuts.\n\nI'm not sure it matters too much, but if you are going to\nchange the name then Eve is also used in the description\nof Man-in-the-middle (see \"Practical Cryptography\", Ferguson\nand Schneier).\n\nATB,\nRamsay Jones\n\n\n"},{"id":"306932","messageId":"20161205130828.3yk2y6yschthoyx6@sigill.intra.peff.net","threadId":"44583","inReplyTo":"20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] restricting http redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-05T13:08:28Z","receivedAt":"2016-12-05T13:08:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2016 at 04:03:37AM -0500, Jeff King wrote:\n\n> Jann Horn brought up on the git-security list some interesting\n> social-engineering attacks around the way Git handles HTTP redirects.\n> These patches are my attempt to harden our redirect handling against\n> these attacks.\n\nThere's one other possible attack I thought of while discussing [1],\nthat is worth mentioning.\n\nWe limited the number of http redirects in b25811646 (http: limit\nredirection depth, 2015-09-22). But what about http-alternates? Could\nyou redirect to yourself via http-alternates and convince a client to\nloop infinitely?\n\nIt looks like no, because we do not seem to handle recursive\nalternates at all in the http walker. Which means that repositories with\nrecursive local alternates cannot be fetched over dumb-http. But it also\nmeans that we don't have to worry about limiting the recursion depth.\n\n-Peff\n\n[1] http://public-inbox.org/git/fe33de5b5f0b3da68b249cc4a49a6d7@3c843fe6ba8f3c586a21345a2783aa0/\n"}]}