Skip to content

Commit 8a2882f

Browse files
gitstergitster
committed
Merge branch 'jk/http-walker-limit-redirect-2.9'
Transport with dumb http can be fooled into following foreign URLs that the end user does not intend to, especially with the server side redirects and http-alternates mechanism, which can lead to security issues. Tighten the redirection and make it more obvious to the end user when it happens. * jk/http-walker-limit-redirect-2.9: http: treat http-alternates like redirects http: make redirects more obvious remote-curl: rename shadowed options variable http: always update the base URL for redirects http: simplify update_url_from_redirect
2 parents 73e494f + cb4d2d3 commit 8a2882f

File tree

9 files changed

+159
-25
lines changed

9 files changed

+159
-25
lines changed

‎Documentation/config.txt‎

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

1894+
http.followRedirects::
1895+
Whether git should follow HTTP redirects. If set to `true`, git
1896+
will transparently follow any redirect issued by a server it
1897+
encounters. If set to `false`, git will treat all redirects as
1898+
errors. If set to `initial`, git will follow redirects only for
1899+
the initial request to a remote, but not for subsequent
1900+
follow-up HTTP requests. Since git uses the redirected URL as
1901+
the base for the follow-up requests, this is generally
1902+
sufficient. The default is `initial`.
1903+
18941904
http.<url>.*::
18951905
Any of the http.* options above can be applied selectively to some URLs.
18961906
For a config key to match a URL, each element of the config key is

‎http-walker.c‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -274,9 +274,8 @@ static void process_alternates_response(void *callback_data)
274274
struct strbuf target = STRBUF_INIT;
275275
strbuf_add(&target, base, serverlen);
276276
strbuf_add(&target, data + i, posn - i - 7);
277-
if (walker->get_verbosely)
278-
fprintf(stderr, "Also look at %s\n",
279-
target.buf);
277+
warning("adding alternate object store: %s",
278+
target.buf);
280279
newalt = xmalloc(sizeof(*newalt));
281280
newalt->next = NULL;
282281
newalt->base = strbuf_detach(&target, NULL);
@@ -302,6 +301,9 @@ static void fetch_alternates(struct walker *walker, const char *base)
302301
struct alternates_request alt_req;
303302
struct walker_data *cdata = walker->data;
304303

304+
if (http_follow_config != HTTP_FOLLOW_ALWAYS)
305+
return;
306+
305307
/*
306308
* If another request has already started fetching alternates,
307309
* wait for them to arrive and return to processing this request's

‎http.c‎

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,8 @@ static int http_proactive_auth;
111111
static const char *user_agent;
112112
static int curl_empty_auth;
113113

114+
enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
115+
114116
#if LIBCURL_VERSION_NUM >= 0x071700
115117
/* Use CURLOPT_KEYPASSWD as is */
116118
#elif LIBCURL_VERSION_NUM >= 0x070903
@@ -366,6 +368,16 @@ static int http_options(const char *var, const char *value, void *cb)
366368
return 0;
367369
}
368370

371+
if (!strcmp("http.followredirects", var)) {
372+
if (value && !strcmp(value, "initial"))
373+
http_follow_config = HTTP_FOLLOW_INITIAL;
374+
else if (git_config_bool(var, value))
375+
http_follow_config = HTTP_FOLLOW_ALWAYS;
376+
else
377+
http_follow_config = HTTP_FOLLOW_NONE;
378+
return 0;
379+
}
380+
369381
/* Fall back on the default ones */
370382
return git_default_config(var, value, cb);
371383
}
@@ -717,7 +729,6 @@ static CURL *get_curl_handle(void)
717729
curl_low_speed_time);
718730
}
719731

720-
curl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);
721732
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
722733
#if LIBCURL_VERSION_NUM >= 0x071301
723734
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
@@ -734,6 +745,7 @@ static CURL *get_curl_handle(void)
734745
if (is_transport_allowed("ftps"))
735746
allowed_protocols |= CURLPROTO_FTPS;
736747
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
748+
curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
737749
#else
738750
if (transport_restrict_protocols())
739751
warning("protocol restrictions not applied to curl redirects because\n"
@@ -1044,6 +1056,16 @@ struct active_request_slot *get_active_slot(void)
10441056
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
10451057
curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
10461058

1059+
/*
1060+
* Default following to off unless "ALWAYS" is configured; this gives
1061+
* callers a sane starting point, and they can tweak for individual
1062+
* HTTP_FOLLOW_* cases themselves.
1063+
*/
1064+
if (http_follow_config == HTTP_FOLLOW_ALWAYS)
1065+
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
1066+
else
1067+
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 0);
1068+
10471069
#if LIBCURL_VERSION_NUM >= 0x070a08
10481070
curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
10491071
#endif
@@ -1286,9 +1308,12 @@ static int handle_curl_result(struct slot_results *results)
12861308
* If we see a failing http code with CURLE_OK, we have turned off
12871309
* FAILONERROR (to keep the server's custom error response), and should
12881310
* translate the code into failure here.
1311+
*
1312+
* Likewise, if we see a redirect (30x code), that means we turned off
1313+
* redirect-following, and we should treat the result as an error.
12891314
*/
12901315
if (results->curl_result == CURLE_OK &&
1291-
results->http_code >= 400) {
1316+
results->http_code >= 300) {
12921317
results->curl_result = CURLE_HTTP_RETURNED_ERROR;
12931318
/*
12941319
* Normally curl will already have put the "reason phrase"
@@ -1607,6 +1632,9 @@ static int http_request(const char *url,
16071632
strbuf_addstr(&buf, " no-cache");
16081633
if (options && options->keep_error)
16091634
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
1635+
if (options && options->initial_request &&
1636+
http_follow_config == HTTP_FOLLOW_INITIAL)
1637+
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
16101638

16111639
headers = curl_slist_append(headers, buf.buf);
16121640

@@ -1655,16 +1683,16 @@ static int http_request(const char *url,
16551683
*
16561684
* Note that this assumes a sane redirect scheme. It's entirely possible
16571685
* in the example above to end up at a URL that does not even end in
1658-
* "info/refs". In such a case we simply punt, as there is not much we can
1659-
* do (and such a scheme is unlikely to represent a real git repository,
1660-
* which means we are likely about to abort anyway).
1686+
* "info/refs". In such a case we die. There's not much we can do, such a
1687+
* scheme is unlikely to represent a real git repository, and failing to
1688+
* rewrite the base opens options for malicious redirects to do funny things.
16611689
*/
16621690
static int update_url_from_redirect(struct strbuf *base,
16631691
const char *asked,
16641692
const struct strbuf *got)
16651693
{
16661694
const char *tail;
1667-
size_t tail_len;
1695+
size_t new_len;
16681696

16691697
if (!strcmp(asked, got->buf))
16701698
return 0;
@@ -1673,14 +1701,16 @@ static int update_url_from_redirect(struct strbuf *base,
16731701
die("BUG: update_url_from_redirect: %s is not a superset of %s",
16741702
asked, base->buf);
16751703

1676-
tail_len = strlen(tail);
1677-
1678-
if (got->len < tail_len ||
1679-
strcmp(tail, got->buf + got->len - tail_len))
1680-
return 0; /* insane redirect scheme */
1704+
new_len = got->len;
1705+
if (!strip_suffix_mem(got->buf, &new_len, tail))
1706+
die(_("unable to update url base from redirection:\n"
1707+
" asked for: %s\n"
1708+
" redirect: %s"),
1709+
asked, got->buf);
16811710

16821711
strbuf_reset(base);
1683-
strbuf_add(base, got->buf, got->len - tail_len);
1712+
strbuf_add(base, got->buf, new_len);
1713+
16841714
return 1;
16851715
}
16861716

‎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: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
274274
struct strbuf effective_url = STRBUF_INIT;
275275
struct discovery *last = last_discovery;
276276
int http_ret, maybe_smart = 0;
277-
struct http_get_options options;
277+
struct http_get_options http_options;
278278

279279
if (last && !strcmp(service, last->service))
280280
return last;
@@ -291,15 +291,16 @@ static struct discovery *discover_refs(const char *service, int for_push)
291291
strbuf_addf(&refs_url, "service=%s", service);
292292
}
293293

294-
memset(&options, 0, sizeof(options));
295-
options.content_type = &type;
296-
options.charset = &charset;
297-
options.effective_url = &effective_url;
298-
options.base_url = &url;
299-
options.no_cache = 1;
300-
options.keep_error = 1;
294+
memset(&http_options, 0, sizeof(http_options));
295+
http_options.content_type = &type;
296+
http_options.charset = &charset;
297+
http_options.effective_url = &effective_url;
298+
http_options.base_url = &url;
299+
http_options.initial_request = 1;
300+
http_options.no_cache = 1;
301+
http_options.keep_error = 1;
301302

302-
http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
303+
http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
303304
switch (http_ret) {
304305
case HTTP_OK:
305306
break;
@@ -314,6 +315,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
314315
die("unable to access '%s': %s", url.buf, curl_errorstr);
315316
}
316317

318+
if (options.verbosity && !starts_with(refs_url.buf, url.buf))
319+
warning(_("redirecting to %s"), url.buf);
320+
317321
last= xcalloc(1, sizeof(*last_discovery));
318322
last->service = service;
319323
last->buf_alloc = strbuf_detach(&buffer, &last->len);

‎t/lib-httpd/apache.conf‎

Lines changed: 14 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]
@@ -132,6 +133,19 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
132133
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]
133134
RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
134135

136+
# The first rule issues a client-side redirect to something
137+
# that _doesn't_ look like a git repo. The second rule is a
138+
# server-side rewrite, so that it turns out the odd-looking
139+
# thing _is_ a git repo. The "[PT]" tells Apache to match
140+
# the usual ScriptAlias rules for /smart.
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+
135149
# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
136150
# And as RewriteCond does not allow testing for non-matches, we match
137151
# the desired case first (one has abra, two has cadabra), and let it

‎t/t5550-http-fetch-dumb.sh‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,5 +307,66 @@ test_expect_success 'remote-http complains cleanly about malformed urls' '
307307
test_must_fail git remote-http http::/example.com/repo.git
308308
'
309309

310+
test_expect_success 'redirects can be forbidden/allowed' '
311+
test_must_fail git -c http.followRedirects=false \
312+
clone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&
313+
git -c http.followRedirects=true \
314+
clone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr
315+
'
316+
317+
test_expect_success 'redirects are reported to stderr' '
318+
# just look for a snippet of the redirected-to URL
319+
test_i18ngrep /dumb/ stderr
320+
'
321+
322+
test_expect_success 'non-initial redirects can be forbidden' '
323+
test_must_fail git -c http.followRedirects=initial \
324+
clone $HTTPD_URL/redir-objects/repo.git redir-objects &&
325+
git -c http.followRedirects=true \
326+
clone $HTTPD_URL/redir-objects/repo.git redir-objects
327+
'
328+
329+
test_expect_success 'http.followRedirects defaults to "initial"' '
330+
test_must_fail git clone $HTTPD_URL/redir-objects/repo.git default
331+
'
332+
333+
# The goal is for a clone of the "evil" repository, which has no objects
334+
# itself, to cause the client to fetch objects from the "victim" repository.
335+
test_expect_success 'set up evil alternates scheme' '
336+
victim=$HTTPD_DOCUMENT_ROOT_PATH/victim.git &&
337+
git init --bare "$victim" &&
338+
git -C "$victim" --work-tree=. commit --allow-empty -m secret &&
339+
git -C "$victim" repack -ad &&
340+
git -C "$victim" update-server-info &&
341+
sha1=$(git -C "$victim" rev-parse HEAD) &&
342+
343+
evil=$HTTPD_DOCUMENT_ROOT_PATH/evil.git &&
344+
git init --bare "$evil" &&
345+
# do this by hand to avoid object existence check
346+
printf "%s\\t%s\\n" $sha1 refs/heads/master >"$evil/info/refs"
347+
'
348+
349+
# Here we'll just redirect via HTTP. In a real-world attack these would be on
350+
# different servers, but we should reject it either way.
351+
test_expect_success 'http-alternates is a non-initial redirect' '
352+
echo "$HTTPD_URL/dumb/victim.git/objects" \
353+
>"$evil/objects/info/http-alternates" &&
354+
test_must_fail git -c http.followRedirects=initial \
355+
clone $HTTPD_URL/dumb/evil.git evil-initial &&
356+
git -c http.followRedirects=true \
357+
clone $HTTPD_URL/dumb/evil.git evil-initial
358+
'
359+
360+
# Curl supports a lot of protocols that we'd prefer not to allow
361+
# http-alternates to use, but it's hard to test whether curl has
362+
# accessed, say, the SMTP protocol, because we are not running an SMTP server.
363+
# But we can check that it does not allow access to file://, which would
364+
# otherwise allow this clone to complete.
365+
test_expect_success 'http-alternates cannot point at funny protocols' '
366+
echo "file://$victim/objects" >"$evil/objects/info/http-alternates" &&
367+
test_must_fail git -c http.followRedirects=true \
368+
clone "$HTTPD_URL/dumb/evil.git" evil-file
369+
'
370+
310371
stop_httpd
311372
test_done

‎t/t5551-http-fetch-smart.sh‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,10 @@ test_expect_success 'redirects re-root further requests' '
119119
git clone $HTTPD_URL/smart-redir-limited/repo.git repo-redir-limited
120120
'
121121

122+
test_expect_success 're-rooting dies on insane schemes' '
123+
test_must_fail git clone $HTTPD_URL/insane-redir/repo.git insane
124+
'
125+
122126
test_expect_success 'clone from password-protected repository' '
123127
echo two >expect &&
124128
set_askpass user@host pass@host &&

‎t/t5812-proto-disable-http.sh‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ test_proto "smart http" http "$HTTPD_URL/smart/repo.git"
1818

1919
test_expect_success 'curl redirects respect whitelist' '
2020
test_must_fail env GIT_ALLOW_PROTOCOL=http:https \
21+
GIT_SMART_HTTP=0 \
2122
git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
2223
{
2324
test_i18ngrep "ftp.*disabled" stderr ||

0 commit comments

Comments
 (0)