{"thread":{"id":"44630","subject":"[PATCH v2 1/6] http: simplify update_url_from_redirect","startedAt":"2016-12-06T18:25:37Z","lastAt":"2016-12-06T18:31:32Z","messageCount":7,"participants":["Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"307025","messageId":"20161206182429.6jp23tltvlznrh2p@sigill.intra.peff.net","threadId":"44630","inReplyTo":"20161206182414.466uotqfufcimpqb@sigill.intra.peff.net","subject":"[PATCH v2 1/6] http: simplify update_url_from_redirect","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-06T18:24:29Z","receivedAt":"2016-12-06T18:25:37Z","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 d8e427b69..d4034a14b 100644\n--- a/http.c\n+++ b/http.c\n@@ -1500,7 +1500,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@@ -1509,14 +1509,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.191.gdb26c57\n\n"},{"id":"307026","messageId":"20161206182438.fri62lalfopystse@sigill.intra.peff.net","threadId":"44630","inReplyTo":"20161206182414.466uotqfufcimpqb@sigill.intra.peff.net","subject":"[PATCH v2 3/6] remote-curl: rename shadowed options variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-06T18:24:38Z","receivedAt":"2016-12-06T18:25:40Z","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 6b83b7783..e3803daa3 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -254,7 +254,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@@ -271,15 +271,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.191.gdb26c57\n\n"},{"id":"307027","messageId":"20161206182435.3o5viseqbisxmc6b@sigill.intra.peff.net","threadId":"44630","inReplyTo":"20161206182414.466uotqfufcimpqb@sigill.intra.peff.net","subject":"[PATCH v2 2/6] http: always update the base URL for redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-06T18:24:35Z","receivedAt":"2016-12-06T18:25:44Z","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 d4034a14b..718d2109b 100644\n--- a/http.c\n+++ b/http.c\n@@ -1491,9 +1491,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@@ -1511,10 +1511,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 018a83a5a..9a355fb1c 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 2f375eb94..d8826acde 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -110,6 +110,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.191.gdb26c57\n\n"},{"id":"307028","messageId":"20161206182441.ym3laupnv3demie2@sigill.intra.peff.net","threadId":"44630","inReplyTo":"20161206182414.466uotqfufcimpqb@sigill.intra.peff.net","subject":"[PATCH v2 4/6] http: make redirects more obvious","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-06T18:24:41Z","receivedAt":"2016-12-06T18:25:47Z","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                     | 31 +++++++++++++++++++++++++++++--\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, 81 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f4721a048..815333643 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1833,6 +1833,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 718d2109b..b99ade5fa 100644\n--- a/http.c\n+++ b/http.c\n@@ -98,6 +98,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@@ -337,6 +339,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@@ -553,7 +565,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@@ -882,6 +893,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@@ -1122,9 +1143,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@@ -1443,6 +1467,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 \ndiff --git a/http.h b/http.h\nindex 36f558bfb..31b4cc94b 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 e3803daa3..05ae8dead 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -276,6 +276,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@@ -294,6 +295,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 9a355fb1c..5b408d649 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 3484b6f0f..ad94ed7b1 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -299,5 +299,28 @@ test_expect_success 'git client does not send an empty Accept-Language' '\n \t! grep \"^Accept-Language:\" stderr\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.191.gdb26c57\n\n"},{"id":"307029","messageId":"20161206182445.qsudezzqdca233sm@sigill.intra.peff.net","threadId":"44630","inReplyTo":"20161206182414.466uotqfufcimpqb@sigill.intra.peff.net","subject":"[PATCH v2 5/6] http: treat http-alternates like redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-06T18:24:45Z","receivedAt":"2016-12-06T18:26:19Z","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 2c721f0c3..4bff31c44 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -290,9 +290,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@@ -318,6 +317,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 b99ade5fa..a9778bfa4 100644\n--- a/http.c\n+++ b/http.c\n@@ -581,6 +581,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 ad94ed7b1..22011f0b6 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -322,5 +322,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.191.gdb26c57\n\n"},{"id":"307030","messageId":"20161206182539.tiyvf2c3j2acoxqt@sigill.intra.peff.net","threadId":"44630","inReplyTo":"20161206182414.466uotqfufcimpqb@sigill.intra.peff.net","subject":"[PATCH v2 6/6] http-walker: complain about non-404 loose object errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-06T18:25:39Z","receivedAt":"2016-12-06T18:26:22Z","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---\nThe second hunk here is new in v2; earlier it appeared in patch 3. But\narguably it goes better here anyway; I didn't even need to modify the\ncommit message to explain it.\n\n http-walker.c | 7 +++++--\n http.c        | 2 +-\n 2 files changed, 6 insertions(+), 3 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);\ndiff --git a/http.c b/http.c\nindex ff4d88231..c25d1d540 100644\n--- a/http.c\n+++ b/http.c\n@@ -2021,7 +2021,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 \n-- \n2.11.0.191.gdb26c57\n"},{"id":"307032","messageId":"20161206182414.466uotqfufcimpqb@sigill.intra.peff.net","threadId":"44630","inReplyTo":"20161206181008.yaz2md3343pukaov@sigill.intra.peff.net","subject":"[PATCH v2] jk/http-walker-limit-redirect rebased to maint-2.9","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-06T18:24:14Z","receivedAt":"2016-12-06T18:31:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 06, 2016 at 01:10:08PM -0500, Jeff King wrote:\n\n> > I think I merged yours and then Brandon's on jch/pu branches in that\n> > order, and the conflict resolution should look OK.\n> > \n> > I however forked yours on v2.11.0-rc1, which would need to be\n> > rebased to one of the earlier maintenance tracks, before we can\n> > merge it to 'next'.\n> \n> Yeah, I built it on top of master.\n> \n> It does depend on some of the http-walker changes Eric made a few months\n> ago. In particular, 17966c0a6 (http: avoid disconnecting on 404s for\n> loose objects, 2016-07-11) added some checks against the HTTP status\n> code, and my series modifies the checks (mostly so that \">= 400\" becomes\n> \">= 300\").\n> \n> Rebasing on maint-2.9 means omitting those changes. That preserves the\n> security properties, but means that the error handling is worse when we\n> see an illegal redirect. That may be OK, though.\n> \n> Since the resolution is to omit the changes entirely from my series,\n> merging up to v2.11 wouldn't produce any conflicts. We'd need to have a\n> separate set of patches adding those changes back in.\n\nThis actually turned out to be pretty easy. The final patch from my\noriginal series is the one that tweaks the error-handling from\n17966c0a6. The rest of them are _almost_ untouched, except that one of\nthe error-handling tweaks gets bumped to the final patch.\n\nSo here's a resend of the patches, rebased on your maint-2.9 branch.\nPatches 1-5 should be applied directly there.\n\nOn maint-2.10, you can merge up maint-2.9, and then apply patch 6.\n\nHopefully that makes sense, but I've pushed branches\njk/maint-X-http-redirect to https://github.com/peff/git that show the\nfinal configuration (and a diff of jk/maint-2.10-http-redirect shows the\noutcome is identical to applying the original series on top of 2.10).\n\nMerging up to 2.11 should be trivial.\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-Peff\n"}]}