From 5088d3b38775f8ac12d7f77636775b16059b67ef Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 22 Sep 2015 18:03:49 -0400 Subject: [PATCH 1/3] transport: refactor protocol whitelist code The current callers only want to die when their transport is prohibited. But future callers want to query the mechanism without dying. Let's break out a few query functions, and also save the results in a static list so we don't have to re-parse for each query. Based-on-a-patch-by: Blake Burkhart Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- transport.c | 40 +++++++++++++++++++++++++++++++--------- transport.h | 15 +++++++++++++-- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/transport.c b/transport.c index 94fe8658f2..647d2c2afa 100644 --- a/transport.c +++ b/transport.c @@ -909,18 +909,40 @@ static int external_specification_len(const char *url) return strchr(url, ':') - url; } +static const struct string_list *protocol_whitelist(void) +{ + static int enabled = -1; + static struct string_list allowed = STRING_LIST_INIT_DUP; + + if (enabled < 0) { + const char *v = getenv("GIT_ALLOW_PROTOCOL"); + if (v) { + string_list_split(&allowed, v, ':', -1); + string_list_sort(&allowed); + enabled = 1; + } else { + enabled = 0; + } + } + + return enabled ? &allowed : NULL; +} + +int is_transport_allowed(const char *type) +{ + const struct string_list *allowed = protocol_whitelist(); + return !allowed || string_list_has_string(allowed, type); +} + void transport_check_allowed(const char *type) { - struct string_list allowed = STRING_LIST_INIT_DUP; - const char *v = getenv("GIT_ALLOW_PROTOCOL"); - - if (!v) - return; - - string_list_split(&allowed, v, ':', -1); - if (!unsorted_string_list_has_string(&allowed, type)) + if (!is_transport_allowed(type)) die("transport '%s' not allowed", type); - string_list_clear(&allowed, 0); +} + +int transport_restrict_protocols(void) +{ + return !!protocol_whitelist(); } struct transport *transport_get(struct remote *remote, const char *url) diff --git a/transport.h b/transport.h index f7df6ec1d2..ed84da2aa4 100644 --- a/transport.h +++ b/transport.h @@ -132,13 +132,24 @@ struct transport { /* Returns a transport suitable for the url */ struct transport *transport_get(struct remote *, const char *); +/* + * Check whether a transport is allowed by the environment. Type should + * generally be the URL scheme, as described in Documentation/git.txt + */ +int is_transport_allowed(const char *type); + /* * Check whether a transport is allowed by the environment, - * and die otherwise. type should generally be the URL scheme, - * as described in Documentation/git.txt + * and die otherwise. */ void transport_check_allowed(const char *type); +/* + * Returns true if the user has attempted to turn on protocol + * restrictions at all. + */ +int transport_restrict_protocols(void); + /* Transport options which apply to git:// and scp-style URLs */ /* The program to use on the remote side to send a pack */ From f4113cac0c88b4f36ee6f3abf3218034440a68e3 Mon Sep 17 00:00:00 2001 From: Blake Burkhart Date: Tue, 22 Sep 2015 18:06:04 -0400 Subject: [PATCH 2/3] http: limit redirection to protocol-whitelist Previously, libcurl would follow redirection to any protocol it was compiled for support with. This is desirable to allow redirection from HTTP to HTTPS. However, it would even successfully allow redirection from HTTP to SFTP, a protocol that git does not otherwise support at all. Furthermore git's new protocol-whitelisting could be bypassed by following a redirect within the remote helper, as it was only enforced at transport selection time. This patch limits redirects within libcurl to HTTP, HTTPS, FTP and FTPS. If there is a protocol-whitelist present, this list is limited to those also allowed by the whitelist. As redirection happens from within libcurl, it is impossible for an HTTP redirect to a protocol implemented within another remote helper. When the curl version git was compiled with is too old to support restrictions on protocol redirection, we warn the user if GIT_ALLOW_PROTOCOL restrictions were requested. This is a little inaccurate, as even without that variable in the environment, we would still restrict SFTP, etc, and we do not warn in that case. But anything else means we would literally warn every time git accesses an http remote. This commit includes a test, but it is not as robust as we would hope. It redirects an http request to ftp, and checks that curl complained about the protocol, which means that we are relying on curl's specific error message to know what happened. Ideally we would redirect to a working ftp server and confirm that we can clone without protocol restrictions, and not with them. But we do not have a portable way of providing an ftp server, nor any other protocol that curl supports (https is the closest, but we would have to deal with certificates). [jk: added test and version warning] Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/git.txt | 5 ----- http.c | 17 +++++++++++++++++ t/lib-httpd/apache.conf | 1 + t/t5812-proto-disable-http.sh | 9 +++++++++ 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index b6a12b32ee..41a09cac7a 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -1071,11 +1071,6 @@ GIT_ICASE_PATHSPECS:: - any external helpers are named by their protocol (e.g., use `hg` to allow the `git-remote-hg` helper) -+ -Note that this controls only git's internal protocol selection. -If libcurl is used (e.g., by the `http` transport), it may -redirect to other protocols. There is not currently any way to -restrict this. Discussion[[Discussion]] diff --git a/http.c b/http.c index 6798620065..5a57bccea9 100644 --- a/http.c +++ b/http.c @@ -8,6 +8,7 @@ #include "credential.h" #include "version.h" #include "pkt-line.h" +#include "transport.h" int active_requests; int http_is_verbose; @@ -303,6 +304,7 @@ static void set_curl_keepalive(CURL *c) static CURL *get_curl_handle(void) { CURL *result = curl_easy_init(); + long allowed_protocols = 0; if (!result) die("curl_easy_init failed"); @@ -355,6 +357,21 @@ static CURL *get_curl_handle(void) #elif LIBCURL_VERSION_NUM >= 0x071101 curl_easy_setopt(result, CURLOPT_POST301, 1); #endif +#if LIBCURL_VERSION_NUM >= 0x071304 + if (is_transport_allowed("http")) + allowed_protocols |= CURLPROTO_HTTP; + if (is_transport_allowed("https")) + allowed_protocols |= CURLPROTO_HTTPS; + if (is_transport_allowed("ftp")) + allowed_protocols |= CURLPROTO_FTP; + if (is_transport_allowed("ftps")) + allowed_protocols |= CURLPROTO_FTPS; + curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols); +#else + if (transport_restrict_protocols()) + warning("protocol restrictions not applied to curl redirects because\n" + "your curl version is too old (>= 7.19.4)"); +#endif if (getenv("GIT_CURL_VERBOSE")) curl_easy_setopt(result, CURLOPT_VERBOSE, 1); diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 0b81a0047b..68ef8adb8e 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -119,6 +119,7 @@ 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] RewriteRule ^/smart-redir-limited/(.*)/info/refs$ /smart/$1/info/refs [R=301] +RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302] LoadModule ssl_module modules/mod_ssl.so diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh index dd5001cbac..6a4f81662d 100755 --- a/t/t5812-proto-disable-http.sh +++ b/t/t5812-proto-disable-http.sh @@ -16,5 +16,14 @@ test_expect_success 'create git-accessible repo' ' 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 clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr && + { + test_i18ngrep "ftp.*disabled" stderr || + test_i18ngrep "your curl version is too old" + } +' + stop_httpd test_done From b258116462399b318c86165c61a5c7123043cfd4 Mon Sep 17 00:00:00 2001 From: Blake Burkhart Date: Tue, 22 Sep 2015 18:06:20 -0400 Subject: [PATCH 3/3] http: limit redirection depth By default, libcurl will follow circular http redirects forever. Let's put a cap on this so that somebody who can trigger an automated fetch of an arbitrary repository (e.g., for CI) cannot convince git to loop infinitely. The value chosen is 20, which is the same default that Firefox uses. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http.c | 1 + t/lib-httpd/apache.conf | 3 +++ t/t5812-proto-disable-http.sh | 4 ++++ 3 files changed, 8 insertions(+) diff --git a/http.c b/http.c index 5a57bccea9..00e3fc80e8 100644 --- a/http.c +++ b/http.c @@ -352,6 +352,7 @@ static CURL *get_curl_handle(void) } 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); #elif LIBCURL_VERSION_NUM >= 0x071101 diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 68ef8adb8e..7d15e6d44c 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -121,6 +121,9 @@ RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301] RewriteRule ^/smart-redir-limited/(.*)/info/refs$ /smart/$1/info/refs [R=301] 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] + LoadModule ssl_module modules/mod_ssl.so diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh index 6a4f81662d..0d105d5417 100755 --- a/t/t5812-proto-disable-http.sh +++ b/t/t5812-proto-disable-http.sh @@ -25,5 +25,9 @@ test_expect_success 'curl redirects respect whitelist' ' } ' +test_expect_success 'curl limits redirects' ' + test_must_fail git clone "$HTTPD_URL/loop-redir/smart/repo.git" +' + stop_httpd test_done