http: make redirects more obvious

We instruct curl to always follow HTTP redirects. This is convenient, but it creates opportunities for malicious servers to create confusing situations. For instance, imagine Alice is a git user with access to a private repository on Bob's server. Mallory runs her own server and wants to access objects from Bob's repository. Mallory may try a few tricks that involve asking Alice to clone from her, build on top, and then push the result: 1. Mallory may simply redirect all fetch requests to Bob's server. Git will transparently follow those redirects and fetch Bob's history, which Alice may believe she got from Mallory. The subsequent push seems like it is just feeding Mallory back her own objects, but is actually leaking Bob's objects. There is nothing in git's output to indicate that Bob's repository was involved at all. The downside (for Mallory) of this attack is that Alice will have received Bob's entire repository, and is likely to notice that when building on top of it. 2. If Mallory happens to know the sha1 of some object X in Bob's repository, she can instead build her own history that references that object. She then runs a dumb http server, and Alice's client will fetch each object individually. When it asks for X, Mallory redirects her to Bob's server. The end result is that Alice obtains objects from Bob, but they may be buried deep in history. Alice is less likely to notice. Both of these attacks are fairly hard to pull off. There's a social component in getting Mallory to convince Alice to work with her. Alice may be prompted for credentials in accessing Bob's repository (but not always, if she is using a credential helper that caches). Attack (1) requires a certain amount of obliviousness on Alice's part while making a new commit. Attack (2) requires that Mallory knows a sha1 in Bob's repository, that Bob's server supports dumb http, and that the object in question is loose on Bob's server. But we can probably make things a bit more obvious without any loss of functionality. This patch does two things to that end. First, when we encounter a whole-repo redirect during the initial ref discovery, we now inform the user on stderr, making attack (1) much more obvious. Second, the decision to follow redirects is now configurable. The truly paranoid can set the new http.followRedirects to false to avoid any redirection entirely. But for a more practical default, we will disallow redirects only after the initial ref discovery. This is enough to thwart attacks similar to (2), while still allowing the common use of redirects at the repository level. Since c93c92f30 (http: update base URLs when we see redirects, 2013-09-28) we re-root all further requests from the redirect destination, which should generally mean that no further redirection is necessary. As an escape hatch, in case there really is a server that needs to redirect individual requests, the user can set http.followRedirects to "true" (and this can be done on a per-server basis via http.*.followRedirects config). Reported-by: Jann Horn <jannh@google.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Dec 6, 2016 at 13:24 UTC 50d3413740d1da599cdc0106e6e916741394cc98
6 files changed +81 -3
Documentation/config.txt
+10
@@ -1833,6 +1833,16 @@ http.userAgent::
1833 of common USER_AGENT strings (but not including those like git/1.7.1).
1834 Can be overridden by the `GIT_HTTP_USER_AGENT` environment variable.
1835
1836 +http.followRedirects::
1837 + Whether git should follow HTTP redirects. If set to `true`, git
1838 + will transparently follow any redirect issued by a server it
1839 + encounters. If set to `false`, git will treat all redirects as
1840 + errors. If set to `initial`, git will follow redirects only for
1841 + the initial request to a remote, but not for subsequent
1842 + follow-up HTTP requests. Since git uses the redirected URL as
1843 + the base for the follow-up requests, this is generally
1844 + sufficient. The default is `initial`.
1845 +
1846 http.<url>.*::
1847 Any of the http.* options above can be applied selectively to some URLs.
1848 For a config key to match a URL, each element of the config key is
http.c
+29 -2
@@ -98,6 +98,8 @@ static int http_proactive_auth;
98 static const char *user_agent;
99 static int curl_empty_auth;
100
101 +enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
102 +
103 #if LIBCURL_VERSION_NUM >= 0x071700
104 /* Use CURLOPT_KEYPASSWD as is */
105 #elif LIBCURL_VERSION_NUM >= 0x070903
@@ -337,6 +339,16 @@ static int http_options(const char *var, const char *value, void *cb)
339 return 0;
340 }
341
342 + if (!strcmp("http.followredirects", var)) {
343 + if (value && !strcmp(value, "initial"))
344 + http_follow_config = HTTP_FOLLOW_INITIAL;
345 + else if (git_config_bool(var, value))
346 + http_follow_config = HTTP_FOLLOW_ALWAYS;
347 + else
348 + http_follow_config = HTTP_FOLLOW_NONE;
349 + return 0;
350 + }
351 +
352 /* Fall back on the default ones */
353 return git_default_config(var, value, cb);
354 }
@@ -553,7 +565,6 @@ static CURL *get_curl_handle(void)
565 curl_low_speed_time);
566 }
567
556 - curl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);
568 curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
569 #if LIBCURL_VERSION_NUM >= 0x071301
570 curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
@@ -882,6 +893,16 @@ struct active_request_slot *get_active_slot(void)
893 curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
894 curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
895
896 + /*
897 + * Default following to off unless "ALWAYS" is configured; this gives
898 + * callers a sane starting point, and they can tweak for individual
899 + * HTTP_FOLLOW_* cases themselves.
900 + */
901 + if (http_follow_config == HTTP_FOLLOW_ALWAYS)
902 + curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
903 + else
904 + curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 0);
905 +
906 #if LIBCURL_VERSION_NUM >= 0x070a08
907 curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
908 #endif
@@ -1122,9 +1143,12 @@ static int handle_curl_result(struct slot_results *results)
1143 * If we see a failing http code with CURLE_OK, we have turned off
1144 * FAILONERROR (to keep the server's custom error response), and should
1145 * translate the code into failure here.
1146 + *
1147 + * Likewise, if we see a redirect (30x code), that means we turned off
1148 + * redirect-following, and we should treat the result as an error.
1149 */
1150 if (results->curl_result == CURLE_OK &&
1127 - results->http_code >= 400) {
1151 + results->http_code >= 300) {
1152 results->curl_result = CURLE_HTTP_RETURNED_ERROR;
1153 /*
1154 * Normally curl will already have put the "reason phrase"
@@ -1443,6 +1467,9 @@ static int http_request(const char *url,
1467 strbuf_addstr(&buf, " no-cache");
1468 if (options && options->keep_error)
1469 curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
1470 + if (options && options->initial_request &&
1471 + http_follow_config == HTTP_FOLLOW_INITIAL)
1472 + curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
1473
1474 headers = curl_slist_append(headers, buf.buf);
1475
http.h
+9 -1
@@ -116,6 +116,13 @@ extern struct credential http_auth;
116
117 extern char curl_errorstr[CURL_ERROR_SIZE];
118
119 +enum http_follow_config {
120 + HTTP_FOLLOW_NONE,
121 + HTTP_FOLLOW_ALWAYS,
122 + HTTP_FOLLOW_INITIAL
123 +};
124 +extern enum http_follow_config http_follow_config;
125 +
126 static inline int missing__target(int code, int result)
127 {
128 return /* file:// URL -- do we ever use one??? */
@@ -139,7 +146,8 @@ extern char *get_remote_object_url(const char *url, const char *hex,
146 /* Options for http_get_*() */
147 struct http_get_options {
148 unsigned no_cache:1,
142 - keep_error:1;
149 + keep_error:1,
150 + initial_request:1;
151
152 /* If non-NULL, returns the content-type of the response. */
153 struct strbuf *content_type;
remote-curl.c
+4
@@ -276,6 +276,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
276 http_options.charset = &charset;
277 http_options.effective_url = &effective_url;
278 http_options.base_url = &url;
279 + http_options.initial_request = 1;
280 http_options.no_cache = 1;
281 http_options.keep_error = 1;
282
@@ -294,6 +295,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
295 die("unable to access '%s': %s", url.buf, curl_errorstr);
296 }
297
298 + if (options.verbosity && !starts_with(refs_url.buf, url.buf))
299 + warning(_("redirecting to %s"), url.buf);
300 +
301 last= xcalloc(1, sizeof(*last_discovery));
302 last->service = service;
303 last->buf_alloc = strbuf_detach(&buffer, &last->len);
t/lib-httpd/apache.conf
+6
@@ -123,6 +123,7 @@ ScriptAlias /error/ error.sh/
123 </Files>
124
125 RewriteEngine on
126 +RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]
127 RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]
128 RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
129 RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]
@@ -140,6 +141,11 @@ RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
141 RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
142 RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
143
144 +# Serve info/refs internally without redirecting, but
145 +# issue a redirect for any object requests.
146 +RewriteRule ^/redir-objects/(.*/info/refs)$ /dumb/$1 [PT]
147 +RewriteRule ^/redir-objects/(.*/objects/.*)$ /dumb/$1 [R=301]
148 +
149 # Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
150 # And as RewriteCond does not allow testing for non-matches, we match
151 # the desired case first (one has abra, two has cadabra), and let it
t/t5550-http-fetch-dumb.sh
+23
@@ -299,5 +299,28 @@ test_expect_success 'git client does not send an empty Accept-Language' '
299 ! grep "^Accept-Language:" stderr
300 '
301
302 +test_expect_success 'redirects can be forbidden/allowed' '
303 + test_must_fail git -c http.followRedirects=false \
304 + clone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&
305 + git -c http.followRedirects=true \
306 + clone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr
307 +'
308 +
309 +test_expect_success 'redirects are reported to stderr' '
310 + # just look for a snippet of the redirected-to URL
311 + test_i18ngrep /dumb/ stderr
312 +'
313 +
314 +test_expect_success 'non-initial redirects can be forbidden' '
315 + test_must_fail git -c http.followRedirects=initial \
316 + clone $HTTPD_URL/redir-objects/repo.git redir-objects &&
317 + git -c http.followRedirects=true \
318 + clone $HTTPD_URL/redir-objects/repo.git redir-objects
319 +'
320 +
321 +test_expect_success 'http.followRedirects defaults to "initial"' '
322 + test_must_fail git clone $HTTPD_URL/redir-objects/repo.git default
323 +'
324 +
325 stop_httpd
326 test_done