Skip to content

Commit 50d3413

Browse files
peffgitster
authored andcommitted
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 c93c92f (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>
1 parent fcaa6e6 commit 50d3413

File tree

6 files changed

+81
-3
lines changed

6 files changed

+81
-3
lines changed

Documentation/config.txt

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1833,6 +1833,16 @@ http.userAgent::
18331833
of common USER_AGENT strings (but not including those like git/1.7.1).
18341834
Can be overridden by the `GIT_HTTP_USER_AGENT` environment variable.
18351835

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+
18361846
http.<url>.*::
18371847
Any of the http.* options above can be applied selectively to some URLs.
18381848
For a config key to match a URL, each element of the config key is

http.c

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,8 @@ static int http_proactive_auth;
9898
static const char *user_agent;
9999
static int curl_empty_auth;
100100

101+
enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
102+
101103
#if LIBCURL_VERSION_NUM >= 0x071700
102104
/* Use CURLOPT_KEYPASSWD as is */
103105
#elif LIBCURL_VERSION_NUM >= 0x070903
@@ -337,6 +339,16 @@ static int http_options(const char *var, const char *value, void *cb)
337339
return 0;
338340
}
339341

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+
340352
/* Fall back on the default ones */
341353
return git_default_config(var, value, cb);
342354
}
@@ -553,7 +565,6 @@ static CURL *get_curl_handle(void)
553565
curl_low_speed_time);
554566
}
555567

556-
curl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);
557568
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
558569
#if LIBCURL_VERSION_NUM >= 0x071301
559570
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
@@ -882,6 +893,16 @@ struct active_request_slot *get_active_slot(void)
882893
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
883894
curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
884895

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+
885906
#if LIBCURL_VERSION_NUM >= 0x070a08
886907
curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
887908
#endif
@@ -1122,9 +1143,12 @@ static int handle_curl_result(struct slot_results *results)
11221143
* If we see a failing http code with CURLE_OK, we have turned off
11231144
* FAILONERROR (to keep the server's custom error response), and should
11241145
* 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.
11251149
*/
11261150
if (results->curl_result == CURLE_OK &&
1127-
results->http_code >= 400) {
1151+
results->http_code >= 300) {
11281152
results->curl_result = CURLE_HTTP_RETURNED_ERROR;
11291153
/*
11301154
* Normally curl will already have put the "reason phrase"
@@ -1443,6 +1467,9 @@ static int http_request(const char *url,
14431467
strbuf_addstr(&buf, " no-cache");
14441468
if (options && options->keep_error)
14451469
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);
14461473

14471474
headers = curl_slist_append(headers, buf.buf);
14481475

http.h

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,13 @@ extern struct credential http_auth;
116116

117117
extern char curl_errorstr[CURL_ERROR_SIZE];
118118

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+
119126
static inline int missing__target(int code, int result)
120127
{
121128
return /* file:// URL -- do we ever use one??? */
@@ -139,7 +146,8 @@ extern char *get_remote_object_url(const char *url, const char *hex,
139146
/* Options for http_get_*() */
140147
struct http_get_options {
141148
unsigned no_cache:1,
142-
keep_error:1;
149+
keep_error:1,
150+
initial_request:1;
143151

144152
/* If non-NULL, returns the content-type of the response. */
145153
struct strbuf *content_type;

remote-curl.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
276276
http_options.charset = &charset;
277277
http_options.effective_url = &effective_url;
278278
http_options.base_url = &url;
279+
http_options.initial_request = 1;
279280
http_options.no_cache = 1;
280281
http_options.keep_error = 1;
281282

@@ -294,6 +295,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
294295
die("unable to access '%s': %s", url.buf, curl_errorstr);
295296
}
296297

298+
if (options.verbosity && !starts_with(refs_url.buf, url.buf))
299+
warning(_("redirecting to %s"), url.buf);
300+
297301
last= xcalloc(1, sizeof(*last_discovery));
298302
last->service = service;
299303
last->buf_alloc = strbuf_detach(&buffer, &last->len);

t/lib-httpd/apache.conf

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ ScriptAlias /error/ error.sh/
123123
</Files>
124124

125125
RewriteEngine on
126+
RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]
126127
RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]
127128
RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
128129
RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]
@@ -140,6 +141,11 @@ RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
140141
RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
141142
RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
142143

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+
143149
# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
144150
# And as RewriteCond does not allow testing for non-matches, we match
145151
# the desired case first (one has abra, two has cadabra), and let it

t/t5550-http-fetch-dumb.sh

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,5 +299,28 @@ test_expect_success 'git client does not send an empty Accept-Language' '
299299
! grep "^Accept-Language:" stderr
300300
'
301301

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+
302325
stop_httpd
303326
test_done

0 commit comments

Comments
 (0)