[PATCH 0/6] restricting http redirects

[PATCH 0/6] restricting http redirects

[Jeff King]

Jann Horn brought up on the git-security list some interesting
social-engineering attacks around the way Git handles HTTP redirects.
These patches are my attempt to harden our redirect handling against
these attacks.

Out of the box, they should make it more obvious to the user when we are
redirecting, and avoid intermingling objects between multiple dumb-http
repositories. There's also a config flag (not on by default) to disable
redirects entirely if you're operating in a more paranoid environment.

The individual commits have more details on the attack scenarios.

I gave some thought to how this might interact with the
bw/transport-protocol-policy topic, which lets you distinguish between
"from the user" and "from some other system" when allowing protocols. I
think that topic is missing some bits when it comes to HTTP, which I
outlined elsewhere:

  http://public-inbox.org/git/20161201083005.dui572o4jxsqacas@sigill.intra.peff.net/

I also wondered if the new http.followRedirects option in this series
could be replaced by just setting protocol.allow to "user".  But it's
not quite the same:

  1. That only covers setting http.followRedirects to "false". There is
     a special value "initial", which allows redirects on the initial
     ref advertisement (see patch 4 for details).

  2. The http.* options can be applied on a per-server basis. So you
     might allow a trusted server to redirect you, but not others. The
     protocol config is less flexible in that regard (it's less about
     "who are you contacting" and more about "what situation are you
     in").

So I think it's fine for the two to co-exist. There's some small
overlap, but which is appropriate depends on what problem you're trying
to solve.

Thanks Jann for the initial report and for good discussion on the
security list.

  [1/6]: http: simplify update_url_from_redirect
  [2/6]: http: always update the base URL for redirects
  [3/6]: remote-curl: rename shadowed options variable
  [4/6]: http: make redirects more obvious
  [5/6]: http: treat http-alternates like redirects
  [6/6]: http-walker: complain about non-404 loose object errors

 Documentation/config.txt      | 10 +++++++
 http-walker.c                 | 15 +++++++----
 http.c                        | 56 ++++++++++++++++++++++++++++++---------
 http.h                        | 10 ++++++-
 remote-curl.c                 | 22 +++++++++-------
 t/lib-httpd/apache.conf       | 14 ++++++++++
 t/t5550-http-fetch-dumb.sh    | 61 +++++++++++++++++++++++++++++++++++++++++++
 t/t5551-http-fetch-smart.sh   |  4 +++
 t/t5812-proto-disable-http.sh |  1 +
 9 files changed, 165 insertions(+), 28 deletions(-)

[PATCH 1/6] http: simplify update_url_from_redirect

[Jeff King]

This function looks for a common tail between what we asked
for and where we were redirected to, but it open-codes the
comparison. We can avoid some confusing subtractions by
using strip_suffix_mem().

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/http.c b/http.c
index 4c4a812fc..840dbd1c7 100644
--- a/http.c
+++ b/http.c
@@ -1664,7 +1664,7 @@ static int update_url_from_redirect(struct strbuf *base,
 				    const struct strbuf *got)
 {
 	const char *tail;
-	size_t tail_len;
+	size_t new_len;
 
 	if (!strcmp(asked, got->buf))
 		return 0;
@@ -1673,14 +1673,12 @@ static int update_url_from_redirect(struct strbuf *base,
 		die("BUG: update_url_from_redirect: %s is not a superset of %s",
 		    asked, base->buf);
 
-	tail_len = strlen(tail);
-
-	if (got->len < tail_len ||
-	    strcmp(tail, got->buf + got->len - tail_len))
+	new_len = got->len;
+	if (!strip_suffix_mem(got->buf, &new_len, tail))
 		return 0; /* insane redirect scheme */
 
 	strbuf_reset(base);
-	strbuf_add(base, got->buf, got->len - tail_len);
+	strbuf_add(base, got->buf, new_len);
 	return 1;
 }
 
-- 
2.11.0.319.g1f4e1e0

[PATCH 2/6] http: always update the base URL for redirects

[Jeff King]

If a malicious server redirects the initial ref
advertisement, it may be able to leak sha1s from other,
unrelated servers that the client has access to. For
example, imagine that Alice is a git user, she has access to
a private repository on a server hosted by Bob, and Mallory
runs a malicious server and wants to find out about Bob's
private repository.

Mallory asks Alice to clone an unrelated repository from her
over HTTP. When Alice's client contacts Mallory's server for
the initial ref advertisement, the server issues an HTTP
redirect for Bob's server. Alice contacts Bob's server and
gets the ref advertisement for the private repository. If
there is anything to fetch, she then follows up by asking
the server for one or more sha1 objects. But who is the
server?

If it is still Mallory's server, then Alice will leak the
existence of those sha1s to her.

Since commit c93c92f30 (http: update base URLs when we see
redirects, 2013-09-28), the client usually rewrites the base
URL such that all further requests will go to Bob's server.
But this is done by textually matching the URL. If we were
originally looking for "http://mallory/repo.git/info/refs",
and we got pointed at "http://bob/other.git/info/refs", then
we know that the right root is "http://bob/other.git".

If the redirect appears to change more than just the root,
we punt and continue to use the original server. E.g.,
imagine the redirect adds a URL component that Bob's server
will ignore, like "http://bob/other.git/info/refs?dummy=1".

We can solve this by aborting in this case rather than
silently continuing to use Mallory's server. In addition to
protecting from sha1 leakage, it's arguably safer and more
sane to refuse a confusing redirect like that in general.
For example, part of the motivation in c93c92f30 is
avoiding accidentally sending credentials over clear http,
just to get a response that says "try again over https". So
even in a non-malicious case, we'd prefer to err on the side
of caution.

The downside is that it's possible this will break a
legitimate but complicated server-side redirection scheme.
The setup given in the newly added test does work, but it's
convoluted enough that we don't need to care about it. A
more plausible case would be a server which redirects a
request for "info/refs?service=git-upload-pack" to just
"info/refs" (because it does not do smart HTTP, and for some
reason really dislikes query parameters).  Right now we
would transparently downgrade to dumb-http, but with this
patch, we'd complain (and the user would have to set
GIT_SMART_HTTP=0 to fetch).

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 http.c                        | 12 ++++++++----
 t/lib-httpd/apache.conf       |  8 ++++++++
 t/t5551-http-fetch-smart.sh   |  4 ++++
 t/t5812-proto-disable-http.sh |  1 +
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index 840dbd1c7..ba03beead 100644
--- a/http.c
+++ b/http.c
@@ -1655,9 +1655,9 @@ static int http_request(const char *url,
  *
  * Note that this assumes a sane redirect scheme. It's entirely possible
  * in the example above to end up at a URL that does not even end in
- * "info/refs".  In such a case we simply punt, as there is not much we can
- * do (and such a scheme is unlikely to represent a real git repository,
- * which means we are likely about to abort anyway).
+ * "info/refs".  In such a case we die. There's not much we can do, such a
+ * scheme is unlikely to represent a real git repository, and failing to
+ * rewrite the base opens options for malicious redirects to do funny things.
  */
 static int update_url_from_redirect(struct strbuf *base,
 				    const char *asked,
@@ -1675,10 +1675,14 @@ static int update_url_from_redirect(struct strbuf *base,
 
 	new_len = got->len;
 	if (!strip_suffix_mem(got->buf, &new_len, tail))
-		return 0; /* insane redirect scheme */
+		die(_("unable to update url base from redirection:\n"
+		      "  asked for: %s\n"
+		      "   redirect: %s"),
+		    asked, got->buf);
 
 	strbuf_reset(base);
 	strbuf_add(base, got->buf, new_len);
+
 	return 1;
 }
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index c3e631394..f98b23a3c 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -132,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
 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]
 RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
 
+# The first rule issues a client-side redirect to something
+# that _doesn't_ look like a git repo. The second rule is a
+# server-side rewrite, so that it turns out the odd-looking
+# thing _is_ a git repo. The "[PT]" tells Apache to match
+# the usual ScriptAlias rules for /smart.
+RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
+RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
+
 # Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
 # And as RewriteCond does not allow testing for non-matches, we match
 # the desired case first (one has abra, two has cadabra), and let it
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 1ec5b2747..6e5b9e42f 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -119,6 +119,10 @@ test_expect_success 'redirects re-root further requests' '
 	git clone $HTTPD_URL/smart-redir-limited/repo.git repo-redir-limited
 '
 
+test_expect_success 're-rooting dies on insane schemes' '
+	test_must_fail git clone $HTTPD_URL/insane-redir/repo.git insane
+'
+
 test_expect_success 'clone from password-protected repository' '
 	echo two >expect &&
 	set_askpass user@host pass@host &&
diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 0d105d541..044cc152f 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -18,6 +18,7 @@ test_proto "smart http" http "$HTTPD_URL/smart/repo.git"
 
 test_expect_success 'curl redirects respect whitelist' '
 	test_must_fail env GIT_ALLOW_PROTOCOL=http:https \
+			   GIT_SMART_HTTP=0 \
 		git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
 	{
 		test_i18ngrep "ftp.*disabled" stderr ||
-- 
2.11.0.319.g1f4e1e0

[PATCH 3/6] remote-curl: rename shadowed options variable

[Jeff King]

The discover_refs() function has a local "options" variable
to hold the http_get_options we pass to http_get_strbuf().
But this shadows the global "struct options" that holds our
program-level options, which cannot be accessed from this
function.

Let's give the local one a more descriptive name so we can
tell the two apart.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index f14c41f4c..f4145fd5c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -274,7 +274,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	struct strbuf effective_url = STRBUF_INIT;
 	struct discovery *last = last_discovery;
 	int http_ret, maybe_smart = 0;
-	struct http_get_options options;
+	struct http_get_options http_options;
 
 	if (last && !strcmp(service, last->service))
 		return last;
@@ -291,15 +291,15 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		strbuf_addf(&refs_url, "service=%s", service);
 	}
 
-	memset(&options, 0, sizeof(options));
-	options.content_type = &type;
-	options.charset = &charset;
-	options.effective_url = &effective_url;
-	options.base_url = &url;
-	options.no_cache = 1;
-	options.keep_error = 1;
+	memset(&http_options, 0, sizeof(http_options));
+	http_options.content_type = &type;
+	http_options.charset = &charset;
+	http_options.effective_url = &effective_url;
+	http_options.base_url = &url;
+	http_options.no_cache = 1;
+	http_options.keep_error = 1;
 
-	http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
+	http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
 	switch (http_ret) {
 	case HTTP_OK:
 		break;
-- 
2.11.0.319.g1f4e1e0

[PATCH 4/6] http: make redirects more obvious

[Jeff King]

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>
---
 Documentation/config.txt   | 10 ++++++++++
 http.c                     | 33 ++++++++++++++++++++++++++++++---
 http.h                     | 10 +++++++++-
 remote-curl.c              |  4 ++++
 t/lib-httpd/apache.conf    |  6 ++++++
 t/t5550-http-fetch-dumb.sh | 23 +++++++++++++++++++++++
 6 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a0ab66aae..d51182a06 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1891,6 +1891,16 @@ http.userAgent::
 	of common USER_AGENT strings (but not including those like git/1.7.1).
 	Can be overridden by the `GIT_HTTP_USER_AGENT` environment variable.
 
+http.followRedirects::
+	Whether git should follow HTTP redirects. If set to `true`, git
+	will transparently follow any redirect issued by a server it
+	encounters. If set to `false`, git will treat all redirects as
+	errors. If set to `initial`, git will follow redirects only for
+	the initial request to a remote, but not for subsequent
+	follow-up HTTP requests. Since git uses the redirected URL as
+	the base for the follow-up requests, this is generally
+	sufficient. The default is `initial`.
+
 http.<url>.*::
 	Any of the http.* options above can be applied selectively to some URLs.
 	For a config key to match a URL, each element of the config key is
diff --git a/http.c b/http.c
index ba03beead..825118481 100644
--- a/http.c
+++ b/http.c
@@ -111,6 +111,8 @@ static int http_proactive_auth;
 static const char *user_agent;
 static int curl_empty_auth;
 
+enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
+
 #if LIBCURL_VERSION_NUM >= 0x071700
 /* Use CURLOPT_KEYPASSWD as is */
 #elif LIBCURL_VERSION_NUM >= 0x070903
@@ -366,6 +368,16 @@ static int http_options(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp("http.followredirects", var)) {
+		if (value && !strcmp(value, "initial"))
+			http_follow_config = HTTP_FOLLOW_INITIAL;
+		else if (git_config_bool(var, value))
+			http_follow_config = HTTP_FOLLOW_ALWAYS;
+		else
+			http_follow_config = HTTP_FOLLOW_NONE;
+		return 0;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
@@ -717,7 +729,6 @@ static CURL *get_curl_handle(void)
 				 curl_low_speed_time);
 	}
 
-	curl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);
 	curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
 #if LIBCURL_VERSION_NUM >= 0x071301
 	curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
@@ -1044,6 +1055,16 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
 	curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
 
+	/*
+	 * Default following to off unless "ALWAYS" is configured; this gives
+	 * callers a sane starting point, and they can tweak for individual
+	 * HTTP_FOLLOW_* cases themselves.
+	 */
+	if (http_follow_config == HTTP_FOLLOW_ALWAYS)
+		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
+	else
+		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 0);
+
 #if LIBCURL_VERSION_NUM >= 0x070a08
 	curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
 #endif
@@ -1286,9 +1307,12 @@ static int handle_curl_result(struct slot_results *results)
 	 * If we see a failing http code with CURLE_OK, we have turned off
 	 * FAILONERROR (to keep the server's custom error response), and should
 	 * translate the code into failure here.
+	 *
+	 * Likewise, if we see a redirect (30x code), that means we turned off
+	 * redirect-following, and we should treat the result as an error.
 	 */
 	if (results->curl_result == CURLE_OK &&
-	    results->http_code >= 400) {
+	    results->http_code >= 300) {
 		results->curl_result = CURLE_HTTP_RETURNED_ERROR;
 		/*
 		 * Normally curl will already have put the "reason phrase"
@@ -1607,6 +1631,9 @@ static int http_request(const char *url,
 		strbuf_addstr(&buf, " no-cache");
 	if (options && options->keep_error)
 		curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
+	if (options && options->initial_request &&
+	    http_follow_config == HTTP_FOLLOW_INITIAL)
+		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
 
 	headers = curl_slist_append(headers, buf.buf);
 
@@ -2030,7 +2057,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
 		if (c != CURLE_OK)
 			die("BUG: curl_easy_getinfo for HTTP code failed: %s",
 				curl_easy_strerror(c));
-		if (slot->http_code >= 400)
+		if (slot->http_code >= 300)
 			return size;
 	}
 
diff --git a/http.h b/http.h
index 5ab9d9c32..02bccb7b0 100644
--- a/http.h
+++ b/http.h
@@ -116,6 +116,13 @@ extern struct credential http_auth;
 
 extern char curl_errorstr[CURL_ERROR_SIZE];
 
+enum http_follow_config {
+	HTTP_FOLLOW_NONE,
+	HTTP_FOLLOW_ALWAYS,
+	HTTP_FOLLOW_INITIAL
+};
+extern enum http_follow_config http_follow_config;
+
 static inline int missing__target(int code, int result)
 {
 	return	/* file:// URL -- do we ever use one??? */
@@ -139,7 +146,8 @@ extern char *get_remote_object_url(const char *url, const char *hex,
 /* Options for http_get_*() */
 struct http_get_options {
 	unsigned no_cache:1,
-		 keep_error:1;
+		 keep_error:1,
+		 initial_request:1;
 
 	/* If non-NULL, returns the content-type of the response. */
 	struct strbuf *content_type;
diff --git a/remote-curl.c b/remote-curl.c
index f4145fd5c..28d9d1063 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -296,6 +296,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	http_options.charset = &charset;
 	http_options.effective_url = &effective_url;
 	http_options.base_url = &url;
+	http_options.initial_request = 1;
 	http_options.no_cache = 1;
 	http_options.keep_error = 1;
 
@@ -314,6 +315,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		die("unable to access '%s': %s", url.buf, curl_errorstr);
 	}
 
+	if (options.verbosity && !starts_with(refs_url.buf, url.buf))
+		warning(_("redirecting to %s"), url.buf);
+
 	last= xcalloc(1, sizeof(*last_discovery));
 	last->service = service;
 	last->buf_alloc = strbuf_detach(&buffer, &last->len);
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index f98b23a3c..69174c6e3 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -123,6 +123,7 @@ ScriptAlias /error/ error.sh/
 </Files>
 
 RewriteEngine on
+RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]
 RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]
 RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
 RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]
@@ -140,6 +141,11 @@ RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
 RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
 RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
 
+# Serve info/refs internally without redirecting, but
+# issue a redirect for any object requests.
+RewriteRule ^/redir-objects/(.*/info/refs)$ /dumb/$1 [PT]
+RewriteRule ^/redir-objects/(.*/objects/.*)$ /dumb/$1 [R=301]
+
 # Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
 # And as RewriteCond does not allow testing for non-matches, we match
 # the desired case first (one has abra, two has cadabra), and let it
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 7641417b4..532507b7c 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -307,5 +307,28 @@ test_expect_success 'remote-http complains cleanly about malformed urls' '
 	test_must_fail git remote-http http::/example.com/repo.git
 '
 
+test_expect_success 'redirects can be forbidden/allowed' '
+	test_must_fail git -c http.followRedirects=false \
+		clone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&
+	git -c http.followRedirects=true \
+		clone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr
+'
+
+test_expect_success 'redirects are reported to stderr' '
+	# just look for a snippet of the redirected-to URL
+	test_i18ngrep /dumb/ stderr
+'
+
+test_expect_success 'non-initial redirects can be forbidden' '
+	test_must_fail git -c http.followRedirects=initial \
+		clone $HTTPD_URL/redir-objects/repo.git redir-objects &&
+	git -c http.followRedirects=true \
+		clone $HTTPD_URL/redir-objects/repo.git redir-objects
+'
+
+test_expect_success 'http.followRedirects defaults to "initial"' '
+	test_must_fail git clone $HTTPD_URL/redir-objects/repo.git default
+'
+
 stop_httpd
 test_done
-- 
2.11.0.319.g1f4e1e0

[PATCH 5/6] http: treat http-alternates like redirects

[Jeff King]

The previous commit made HTTP redirects more obvious and
tightened up the default behavior. However, there's another
way for a server to ask a git client to fetch arbitrary
content: by having an http-alternates file (or a regular
alternates file, which is used as a backup).

Similar to the HTTP redirect case, a malicious server can
claim to have refs pointing at object X, return a 404 when
the client asks for X, but point to some other URL via
http-alternates, which the client will transparently fetch.
The end result is that it looks from the user's perspective
like the objects came from the malicious server, as the
other URL is not mentioned at all.

Worse, because we feed the new URL to curl ourselves, the
usual protocol restrictions do not kick in (neither curl's
default of disallowing file://, nor the protocol
whitelisting in f4113cac0 (http: limit redirection to
protocol-whitelist, 2015-09-22).

Let's apply the same rules here as we do for HTTP redirects.
Namely:

  - unless http.followRedirects is set to "always", we will
    not follow remote redirects from http-alternates (or
    alternates) at all

  - set CURLOPT_PROTOCOLS alongside CURLOPT_REDIR_PROTOCOLS
    restrict ourselves to a known-safe set and respect any
    user-provided whitelist.

  - mention alternate object stores on stderr so that the
    user is aware another source of objects may be involved

The first item may prove to be too restrictive. The most
common use of alternates is to point to another path on the
same server. While it's possible for a single-server
redirect to be an attack, it takes a fairly obscure setup
(victim and evil repository on the same host, host speaks
dumb http, and evil repository has access to edit its own
http-alternates file).

So we could make the checks more specific, and only cover
cross-server redirects. But that means parsing the URLs
ourselves, rather than letting curl handle them. This patch
goes for the simpler approach. Given that they are only used
with dumb http, http-alternates are probably pretty rare.
And there's an escape hatch: the user can allow redirects on
a specific server by setting http.<url>.followRedirects to
"always".

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 http-walker.c              |  8 +++++---
 http.c                     |  1 +
 t/t5550-http-fetch-dumb.sh | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index 0b2425531..25a8b1ad4 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -274,9 +274,8 @@ static void process_alternates_response(void *callback_data)
 				struct strbuf target = STRBUF_INIT;
 				strbuf_add(&target, base, serverlen);
 				strbuf_add(&target, data + i, posn - i - 7);
-				if (walker->get_verbosely)
-					fprintf(stderr, "Also look at %s\n",
-						target.buf);
+				warning("adding alternate object store: %s",
+					target.buf);
 				newalt = xmalloc(sizeof(*newalt));
 				newalt->next = NULL;
 				newalt->base = strbuf_detach(&target, NULL);
@@ -302,6 +301,9 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	struct alternates_request alt_req;
 	struct walker_data *cdata = walker->data;
 
+	if (http_follow_config != HTTP_FOLLOW_ALWAYS)
+		return;
+
 	/*
 	 * If another request has already started fetching alternates,
 	 * wait for them to arrive and return to processing this request's
diff --git a/http.c b/http.c
index 825118481..051fe6e5a 100644
--- a/http.c
+++ b/http.c
@@ -745,6 +745,7 @@ static CURL *get_curl_handle(void)
 	if (is_transport_allowed("ftps"))
 		allowed_protocols |= CURLPROTO_FTPS;
 	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
+	curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
 #else
 	if (transport_restrict_protocols())
 		warning("protocol restrictions not applied to curl redirects because\n"
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 532507b7c..264a1ab8b 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -330,5 +330,43 @@ test_expect_success 'http.followRedirects defaults to "initial"' '
 	test_must_fail git clone $HTTPD_URL/redir-objects/repo.git default
 '
 
+# The goal is for a clone of the "evil" repository, which has no objects
+# itself, to cause the client to fetch objects from the "victim" repository.
+test_expect_success 'set up evil alternates scheme' '
+	victim=$HTTPD_DOCUMENT_ROOT_PATH/victim.git &&
+	git init --bare "$victim" &&
+	git -C "$victim" --work-tree=. commit --allow-empty -m secret &&
+	git -C "$victim" repack -ad &&
+	git -C "$victim" update-server-info &&
+	sha1=$(git -C "$victim" rev-parse HEAD) &&
+
+	evil=$HTTPD_DOCUMENT_ROOT_PATH/evil.git &&
+	git init --bare "$evil" &&
+	# do this by hand to avoid object existence check
+	printf "%s\\t%s\\n" $sha1 refs/heads/master >"$evil/info/refs"
+'
+
+# Here we'll just redirect via HTTP. In a real-world attack these would be on
+# different servers, but we should reject it either way.
+test_expect_success 'http-alternates is a non-initial redirect' '
+	echo "$HTTPD_URL/dumb/victim.git/objects" \
+		>"$evil/objects/info/http-alternates" &&
+	test_must_fail git -c http.followRedirects=initial \
+		clone $HTTPD_URL/dumb/evil.git evil-initial &&
+	git -c http.followRedirects=true \
+		clone $HTTPD_URL/dumb/evil.git evil-initial
+'
+
+# Curl supports a lot of protocols that we'd prefer not to allow
+# http-alternates to use, but it's hard to test whether curl has
+# accessed, say, the SMTP protocol, because we are not running an SMTP server.
+# But we can check that it does not allow access to file://, which would
+# otherwise allow this clone to complete.
+test_expect_success 'http-alternates cannot point at funny protocols' '
+	echo "file://$victim/objects" >"$evil/objects/info/http-alternates" &&
+	test_must_fail git -c http.followRedirects=true \
+		clone "$HTTPD_URL/dumb/evil.git" evil-file
+'
+
 stop_httpd
 test_done
-- 
2.11.0.319.g1f4e1e0

[PATCH 6/6] http-walker: complain about non-404 loose object errors

[Jeff King]

Since commit 17966c0a6 (http: avoid disconnecting on 404s
for loose objects, 2016-07-11), we turn off curl's
FAILONERROR option and instead manually deal with failing
HTTP codes.

However, the logic to do so only recognizes HTTP 404 as a
failure. This is probably the most common result, but if we
were to get another code, the curl result remains CURLE_OK,
and we treat it as success. We still end up detecting the
failure when we try to zlib-inflate the object (which will
fail), but instead of reporting the HTTP error, we just
claim that the object is corrupt.

Instead, let's catch anything in the 300's or above as an
error (300's are redirects which are not an error at the
HTTP level, but are an indication that we've explicitly
disabled redirects, so we should treat them as such; we
certainly don't have the resulting object content).

Note that we also fill in req->errorstr, which we didn't do
before. Without FAILONERROR, curl will not have filled this
in, and it will remain a blank string. This never mattered
for the 404 case, because in the logic below we hit the
"missing_target()" branch and print nothing. But for other
errors, we'd want to say _something_, if only to fill in the
blank slot in the error message.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-walker.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index 25a8b1ad4..c2f81cd6a 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -482,10 +482,13 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)
 	 * we turned off CURLOPT_FAILONERROR to avoid losing a
 	 * persistent connection and got CURLE_OK.
 	 */
-	if (req->http_code == 404 && req->curl_result == CURLE_OK &&
+	if (req->http_code >= 300 && req->curl_result == CURLE_OK &&
 			(starts_with(req->url, "http://") ||
-			 starts_with(req->url, "https://")))
+			 starts_with(req->url, "https://"))) {
 		req->curl_result = CURLE_HTTP_RETURNED_ERROR;
+		xsnprintf(req->errorstr, sizeof(req->errorstr),
+			  "HTTP request failed");
+	}
 
 	if (obj_req->state == ABORTED) {
 		ret = error("Request for %s aborted", hex);
-- 
2.11.0.319.g1f4e1e0

Re: [PATCH 2/6] http: always update the base URL for redirects

[Ramsay Jones]

On 01/12/16 09:04, Jeff King wrote:
> If a malicious server redirects the initial ref
> advertisement, it may be able to leak sha1s from other,
> unrelated servers that the client has access to. For
> example, imagine that Alice is a git user, she has access to
> a private repository on a server hosted by Bob, and Mallory
> runs a malicious server and wants to find out about Bob's
> private repository.
> 
> Mallory asks Alice to clone an unrelated repository from her
-----------------------------------------------------------^^^
... from _him_ ? (ie Mallory)

> over HTTP. When Alice's client contacts Mallory's server for
> the initial ref advertisement, the server issues an HTTP
> redirect for Bob's server. Alice contacts Bob's server and
> gets the ref advertisement for the private repository. If
> there is anything to fetch, she then follows up by asking
> the server for one or more sha1 objects. But who is the
> server?
> 
> If it is still Mallory's server, then Alice will leak the
> existence of those sha1s to her.
------------------------------^^^
... to _him_ ? (again Mallory)

ATB,
Ramsay Jones

Re: [PATCH 4/6] http: make redirects more obvious

[Ramsay Jones]

On 01/12/16 09:04, Jeff King wrote:
> 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

Ahem, so Mallory is female? (-blush-) :(

ATB,
Ramsay Jones

Re: [PATCH 2/6] http: always update the base URL for redirects

[Brandon Williams]

On 12/01, Ramsay Jones wrote:
> 
> 
> On 01/12/16 09:04, Jeff King wrote:
> > If a malicious server redirects the initial ref
> > advertisement, it may be able to leak sha1s from other,
> > unrelated servers that the client has access to. For
> > example, imagine that Alice is a git user, she has access to
> > a private repository on a server hosted by Bob, and Mallory
> > runs a malicious server and wants to find out about Bob's
> > private repository.
> > 
> > Mallory asks Alice to clone an unrelated repository from her
> -----------------------------------------------------------^^^
> ... from _him_ ? (ie Mallory)
> 
> > over HTTP. When Alice's client contacts Mallory's server for
> > the initial ref advertisement, the server issues an HTTP
> > redirect for Bob's server. Alice contacts Bob's server and
> > gets the ref advertisement for the private repository. If
> > there is anything to fetch, she then follows up by asking
> > the server for one or more sha1 objects. But who is the
> > server?
> > 
> > If it is still Mallory's server, then Alice will leak the
> > existence of those sha1s to her.
> ------------------------------^^^
> ... to _him_ ? (again Mallory)
> 
> ATB,
> Ramsay Jones

Depends, I only know Mallorys who are women so her seems appropriate.

-- 
Brandon Williams

Re: [PATCH 5/6] http: treat http-alternates like redirects

[Brandon Williams]

On 12/01, Jeff King wrote:
>   - set CURLOPT_PROTOCOLS alongside CURLOPT_REDIR_PROTOCOLS
>     restrict ourselves to a known-safe set and respect any
>     user-provided whitelist.



> diff --git a/http.c b/http.c
> index 825118481..051fe6e5a 100644
> --- a/http.c
> +++ b/http.c
> @@ -745,6 +745,7 @@ static CURL *get_curl_handle(void)
>  	if (is_transport_allowed("ftps"))
>  		allowed_protocols |= CURLPROTO_FTPS;
>  	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
> +	curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
>  #else
>  	if (transport_restrict_protocols())
>  		warning("protocol restrictions not applied to curl redirects because\n"

Because I don't know much about how curl works....Only
http/https/ftp/ftps protocols are allowed to be passed to curl?  Is that
because curl only understands those particular protocols?

-- 
Brandon Williams

Re: [PATCH 2/6] http: always update the base URL for redirects

[Philip Oakley]

From: "Brandon Williams" <bmwill@google.com>
> On 12/01, Ramsay Jones wrote:
>>
>>
>> On 01/12/16 09:04, Jeff King wrote:
>> > If a malicious server redirects the initial ref
>> > advertisement, it may be able to leak sha1s from other,
>> > unrelated servers that the client has access to. For
>> > example, imagine that Alice is a git user, she has access to
>> > a private repository on a server hosted by Bob, and Mallory
>> > runs a malicious server and wants to find out about Bob's
>> > private repository.
>> >
>> > Mallory asks Alice to clone an unrelated repository from her
>> -----------------------------------------------------------^^^
>> ... from _him_ ? (ie Mallory)
>>
>> > over HTTP. When Alice's client contacts Mallory's server for
>> > the initial ref advertisement, the server issues an HTTP
>> > redirect for Bob's server. Alice contacts Bob's server and
>> > gets the ref advertisement for the private repository. If
>> > there is anything to fetch, she then follows up by asking
>> > the server for one or more sha1 objects. But who is the
>> > server?
>> >
>> > If it is still Mallory's server, then Alice will leak the
>> > existence of those sha1s to her.
>> ------------------------------^^^
>> ... to _him_ ? (again Mallory)
>>
>> ATB,
>> Ramsay Jones
>
> Depends, I only know Mallorys who are women so her seems appropriate.
>
> -- 
> Brandon Williams
>
In a British context "Mallory and Irvine" were two (male) climbers who died 
on Everest in 1924 (tales of daring...), so it's easy to expect (from this 
side of the pond) that 'Mallory' would be male. However he was really George 
Mallory.

Meanwhile that search engine's images shows far more female Mallorys, so 
I've learnt something.
--
Philip

Re: [PATCH 2/6] http: always update the base URL for redirects

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

>> Depends, I only know Mallorys who are women so her seems appropriate.
>>
> In a British context "Mallory and Irvine" were two (male) climbers who
> died on Everest in 1924 (tales of daring...), so it's easy to expect
> (from this side of the pond) that 'Mallory' would be male. However he
> was really George Mallory.
>
> Meanwhile that search engine's images shows far more female Mallorys,
> so I've learnt something.

"baby name Mallory" in search engine gave me several sites, most of
them telling me that is a girl's name except for one.

Didn't think of doing image search, but that's a good way ;-)

Re: [PATCH 5/6] http: treat http-alternates like redirects

[Jeff King]

On Thu, Dec 01, 2016 at 03:02:23PM -0800, Brandon Williams wrote:

> > diff --git a/http.c b/http.c
> > index 825118481..051fe6e5a 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -745,6 +745,7 @@ static CURL *get_curl_handle(void)
> >  	if (is_transport_allowed("ftps"))
> >  		allowed_protocols |= CURLPROTO_FTPS;
> >  	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
> > +	curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
> >  #else
> >  	if (transport_restrict_protocols())
> >  		warning("protocol restrictions not applied to curl redirects because\n"
> 
> Because I don't know much about how curl works....Only
> http/https/ftp/ftps protocols are allowed to be passed to curl?  Is that
> because curl only understands those particular protocols?

No, curl understands more protocols, and that is exactly the problem. We
don't want to accidentally have curl access file://, smtp://, or
similar, based on what some server puts in their http-alternates file.

You should only be able to get to this code-path by calling one of
git-remote-{http,https,ftp,ftps}. So there is no problem with
restricting the protocol beyond those options. And there should be no
problem with restricting within that set; if the protocol we intend to
feed to curl had been disallowed by policy, git would have blocked it
before hitting git-remote in the first place.

-Peff

Re: [PATCH 2/6] http: always update the base URL for redirects

[Ramsay Jones]

On 01/12/16 23:43, Junio C Hamano wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
> 
>>> Depends, I only know Mallorys who are women so her seems appropriate.
>>>
>> In a British context "Mallory and Irvine" were two (male) climbers who
>> died on Everest in 1924 (tales of daring...), so it's easy to expect
>> (from this side of the pond) that 'Mallory' would be male. However he
>> was really George Mallory.
>>
>> Meanwhile that search engine's images shows far more female Mallorys,
>> so I've learnt something.
> 
> "baby name Mallory" in search engine gave me several sites, most of
> them telling me that is a girl's name except for one.
> 
> Didn't think of doing image search, but that's a good way ;-)

Heh, I didn't think about any of this. I was remembering the
description of 'Man-in-the-middle Attack' from Applied Cryptography
(Bruce Schneier) which implies that Mallory is male.

ATB,
Ramsay Jones

Re: [PATCH 2/6] http: always update the base URL for redirects

[Jeff King]

On Fri, Dec 02, 2016 at 12:07:50AM +0000, Ramsay Jones wrote:

> >> In a British context "Mallory and Irvine" were two (male) climbers who
> >> died on Everest in 1924 (tales of daring...), so it's easy to expect
> >> (from this side of the pond) that 'Mallory' would be male. However he
> >> was really George Mallory.
> >>
> >> Meanwhile that search engine's images shows far more female Mallorys,
> >> so I've learnt something.
> > 
> > "baby name Mallory" in search engine gave me several sites, most of
> > them telling me that is a girl's name except for one.
> > 
> > Didn't think of doing image search, but that's a good way ;-)
> 
> Heh, I didn't think about any of this. I was remembering the
> description of 'Man-in-the-middle Attack' from Applied Cryptography
> (Bruce Schneier) which implies that Mallory is male.

I admit that I always assumed Applied Cryptography (and other papers)
were always talking about a female. But that's probably because I
started with an assumption about the name in the first place. That
probably came from watching the Family Ties sitcom as a kid; the older
daughter is named Mallory (and if you google it, you can see some
amazing 80's haircuts and clothes).

We can call her Marsha if you want, instead evoking Brady Bunch memories
of 60's clothing and haircuts.

-Peff

Re: [PATCH 2/6] http: always update the base URL for redirects

[Ramsay Jones]

On 02/12/16 00:18, Jeff King wrote:
> On Fri, Dec 02, 2016 at 12:07:50AM +0000, Ramsay Jones wrote:
> 
>>>> In a British context "Mallory and Irvine" were two (male) climbers who
>>>> died on Everest in 1924 (tales of daring...), so it's easy to expect
>>>> (from this side of the pond) that 'Mallory' would be male. However he
>>>> was really George Mallory.
>>>>
>>>> Meanwhile that search engine's images shows far more female Mallorys,
>>>> so I've learnt something.
>>>
>>> "baby name Mallory" in search engine gave me several sites, most of
>>> them telling me that is a girl's name except for one.
>>>
>>> Didn't think of doing image search, but that's a good way ;-)
>>
>> Heh, I didn't think about any of this. I was remembering the
>> description of 'Man-in-the-middle Attack' from Applied Cryptography
>> (Bruce Schneier) which implies that Mallory is male.
> 
> I admit that I always assumed Applied Cryptography (and other papers)
> were always talking about a female. But that's probably because I
> started with an assumption about the name in the first place. That
> probably came from watching the Family Ties sitcom as a kid; the older
> daughter is named Mallory (and if you google it, you can see some
> amazing 80's haircuts and clothes).
> 
> We can call her Marsha if you want, instead evoking Brady Bunch memories
> of 60's clothing and haircuts.

I'm not sure it matters too much, but if you are going to
change the name then Eve is also used in the description
of Man-in-the-middle (see "Practical Cryptography", Ferguson
and Schneier).

ATB,
Ramsay Jones

Re: [PATCH 0/6] restricting http redirects

[Jeff King]

On Thu, Dec 01, 2016 at 04:03:37AM -0500, Jeff King wrote:

> Jann Horn brought up on the git-security list some interesting
> social-engineering attacks around the way Git handles HTTP redirects.
> These patches are my attempt to harden our redirect handling against
> these attacks.

There's one other possible attack I thought of while discussing [1],
that is worth mentioning.

We limited the number of http redirects in b25811646 (http: limit
redirection depth, 2015-09-22). But what about http-alternates? Could
you redirect to yourself via http-alternates and convince a client to
loop infinitely?

It looks like no, because we do not seem to handle recursive
alternates at all in the http walker. Which means that repositories with
recursive local alternates cannot be fetched over dumb-http. But it also
means that we don't have to worry about limiting the recursion depth.

-Peff

[1] http://public-inbox.org/git/fe33de5b5f0b3da68b249cc4a49a6d7@3c843fe6ba8f3c586a21345a2783aa0/