From a88dbd2f8c7fd8c1e2f63483da03bd6928e8791f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 19 Apr 2020 01:36:02 -0400 Subject: [PATCH 01/10] t0300: make "quit" helper more realistic We test a toy credential helper that writes "quit=1" and confirms that we stop running other helpers. However, that helper is unrealistic in that it does not bother to read its stdin at all. For now we don't send any input to it, because we feed git-credential a blank credential. But that will change in the next patch, which will cause this test to racily fail, as git-credential will get SIGPIPE writing to the helper rather than exiting because it was asked to. Let's make this one-off helper more like our other sample helpers, and have it source the "dump" script. That will read stdin, fixing the SIGPIPE problem. But it will also write what it sees to stderr. We can make the test more robust by checking that output, which confirms that we do run the quit helper, don't run any other helpers, and exit for the reason we expected. Signed-off-by: Jeff King Signed-off-by: Jonathan Nieder --- t/t0300-credentials.sh | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 3bec445cac..0206b3b103 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -22,6 +22,11 @@ test_expect_success 'setup helper scripts' ' exit 0 EOF + write_script git-credential-quit <<-\EOF && + . ./dump + echo quit=1 + EOF + write_script git-credential-verbatim <<-\EOF && user=$1; shift pass=$1; shift @@ -291,11 +296,16 @@ test_expect_success 'http paths can be part of context' ' test_expect_success 'helpers can abort the process' ' test_must_fail git \ - -c credential.helper="!f() { echo quit=1; }; f" \ + -c credential.helper=quit \ -c credential.helper="verbatim foo bar" \ - credential fill >stdout && + credential fill >stdout 2>stderr && >expect && - test_cmp expect stdout + test_cmp expect stdout && + cat >expect <<-\EOF && + quit: get + fatal: credential helper '\''quit'\'' told us to quit + EOF + test_i18ncmp expect stderr ' test_expect_success 'empty helper spec resets helper list' ' From 73aafe9bc27585554181c58871a25e6d0f58a3dc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Apr 2020 20:47:30 -0700 Subject: [PATCH 02/10] t0300: use more realistic inputs Many of the tests in t0300 give partial inputs to git-credential, omitting a protocol or hostname. We're checking only high-level things like whether and how helpers are invoked at all, and we don't care about specific hosts. However, in preparation for tightening up the rules about when we're willing to run a helper, let's start using input that's a bit more realistic: pretend as if http://example.com is being examined. This shouldn't change the point of any of the tests, but do note we have to adjust the expected output to accommodate this (filling a credential will repeat back the protocol/host fields to stdout, and the helper debug messages and askpass prompt will change on stderr). Signed-off-by: Jeff King Reviewed-by: Taylor Blau Signed-off-by: Jonathan Nieder --- t/t0300-credentials.sh | 89 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 85 insertions(+), 4 deletions(-) diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 0206b3b103..f4c5d7ff91 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -40,43 +40,71 @@ test_expect_success 'setup helper scripts' ' test_expect_success 'credential_fill invokes helper' ' check fill "verbatim foo bar" <<-\EOF + protocol=http + host=example.com -- + protocol=http + host=example.com username=foo password=bar -- verbatim: get + verbatim: protocol=http + verbatim: host=example.com EOF ' test_expect_success 'credential_fill invokes multiple helpers' ' check fill useless "verbatim foo bar" <<-\EOF + protocol=http + host=example.com -- + protocol=http + host=example.com username=foo password=bar -- useless: get + useless: protocol=http + useless: host=example.com verbatim: get + verbatim: protocol=http + verbatim: host=example.com EOF ' test_expect_success 'credential_fill stops when we get a full response' ' check fill "verbatim one two" "verbatim three four" <<-\EOF + protocol=http + host=example.com -- + protocol=http + host=example.com username=one password=two -- verbatim: get + verbatim: protocol=http + verbatim: host=example.com EOF ' test_expect_success 'credential_fill continues through partial response' ' check fill "verbatim one \"\"" "verbatim two three" <<-\EOF + protocol=http + host=example.com -- + protocol=http + host=example.com username=two password=three -- verbatim: get + verbatim: protocol=http + verbatim: host=example.com verbatim: get + verbatim: protocol=http + verbatim: host=example.com verbatim: username=one EOF ' @@ -102,14 +130,20 @@ test_expect_success 'credential_fill passes along metadata' ' test_expect_success 'credential_approve calls all helpers' ' check approve useless "verbatim one two" <<-\EOF + protocol=http + host=example.com username=foo password=bar -- -- useless: store + useless: protocol=http + useless: host=example.com useless: username=foo useless: password=bar verbatim: store + verbatim: protocol=http + verbatim: host=example.com verbatim: username=foo verbatim: password=bar EOF @@ -117,6 +151,8 @@ test_expect_success 'credential_approve calls all helpers' ' test_expect_success 'do not bother storing password-less credential' ' check approve useless <<-\EOF + protocol=http + host=example.com username=foo -- -- @@ -126,14 +162,20 @@ test_expect_success 'do not bother storing password-less credential' ' test_expect_success 'credential_reject calls all helpers' ' check reject useless "verbatim one two" <<-\EOF + protocol=http + host=example.com username=foo password=bar -- -- useless: erase + useless: protocol=http + useless: host=example.com useless: username=foo useless: password=bar verbatim: erase + verbatim: protocol=http + verbatim: host=example.com verbatim: username=foo verbatim: password=bar EOF @@ -141,33 +183,49 @@ test_expect_success 'credential_reject calls all helpers' ' test_expect_success 'usernames can be preserved' ' check fill "verbatim \"\" three" <<-\EOF + protocol=http + host=example.com username=one -- + protocol=http + host=example.com username=one password=three -- verbatim: get + verbatim: protocol=http + verbatim: host=example.com verbatim: username=one EOF ' test_expect_success 'usernames can be overridden' ' check fill "verbatim two three" <<-\EOF + protocol=http + host=example.com username=one -- + protocol=http + host=example.com username=two password=three -- verbatim: get + verbatim: protocol=http + verbatim: host=example.com verbatim: username=one EOF ' test_expect_success 'do not bother completing already-full credential' ' check fill "verbatim three four" <<-\EOF + protocol=http + host=example.com username=one password=two -- + protocol=http + host=example.com username=one password=two -- @@ -179,23 +237,31 @@ test_expect_success 'do not bother completing already-full credential' ' # askpass helper is run, we know the internal getpass is working. test_expect_success 'empty helper list falls back to internal getpass' ' check fill <<-\EOF + protocol=http + host=example.com -- + protocol=http + host=example.com username=askpass-username password=askpass-password -- - askpass: Username: - askpass: Password: + askpass: Username for '\''http://example.com'\'': + askpass: Password for '\''http://askpass-username@example.com'\'': EOF ' test_expect_success 'internal getpass does not ask for known username' ' check fill <<-\EOF + protocol=http + host=example.com username=foo -- + protocol=http + host=example.com username=foo password=askpass-password -- - askpass: Password: + askpass: Password for '\''http://foo@example.com'\'': EOF ' @@ -207,7 +273,11 @@ HELPER="!f() { test_expect_success 'respect configured credentials' ' test_config credential.helper "$HELPER" && check fill <<-\EOF + protocol=http + host=example.com -- + protocol=http + host=example.com username=foo password=bar -- @@ -298,11 +368,16 @@ test_expect_success 'helpers can abort the process' ' test_must_fail git \ -c credential.helper=quit \ -c credential.helper="verbatim foo bar" \ - credential fill >stdout 2>stderr && + credential fill >stdout 2>stderr <<-\EOF && + protocol=http + host=example.com + EOF >expect && test_cmp expect stdout && cat >expect <<-\EOF && quit: get + quit: protocol=http + quit: host=example.com fatal: credential helper '\''quit'\'' told us to quit EOF test_i18ncmp expect stderr @@ -311,11 +386,17 @@ test_expect_success 'helpers can abort the process' ' test_expect_success 'empty helper spec resets helper list' ' test_config credential.helper "verbatim file file" && check fill "" "verbatim cmdline cmdline" <<-\EOF + protocol=http + host=example.com -- + protocol=http + host=example.com username=cmdline password=cmdline -- verbatim: get + verbatim: protocol=http + verbatim: host=example.com EOF ' From 24036686c4af84c9e84e486ef3debab6e6d8e6b5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Apr 2020 20:48:05 -0700 Subject: [PATCH 03/10] credential: parse URL without host as empty host, not unset We may feed a URL like "cert:///path/to/cert.pem" into the credential machinery to get the key for a client-side certificate. That credential has no hostname field, which is about to be disallowed (to avoid confusion with protocols where a helper _would_ expect a hostname). This means as of the next patch, credential helpers won't work for unlocking certs. Let's fix that by doing two things: - when we parse a url with an empty host, set the host field to the empty string (asking only to match stored entries with an empty host) rather than NULL (asking to match _any_ host). - when we build a cert:// credential by hand, similarly assign an empty string It's the latter that is more likely to impact real users in practice, since it's what's used for http connections. But we don't have good infrastructure to test it. The url-parsing version will help anybody using git-credential in a script, and is easy to test. Signed-off-by: Jeff King Reviewed-by: Taylor Blau Signed-off-by: Jonathan Nieder --- credential.c | 3 +-- http.c | 1 + t/t0300-credentials.sh | 17 +++++++++++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/credential.c b/credential.c index eeeac3242e..d1bb71b41a 100644 --- a/credential.c +++ b/credential.c @@ -373,8 +373,7 @@ int credential_from_url_gently(struct credential *c, const char *url, if (proto_end - url > 0) c->protocol = xmemdupz(url, proto_end - url); - if (slash - host > 0) - c->host = url_decode_mem(host, slash - host); + c->host = url_decode_mem(host, slash - host); /* Trim leading and trailing slashes from path */ while (*slash == '/') slash++; diff --git a/http.c b/http.c index a5bd5d62c2..29de6bbb23 100644 --- a/http.c +++ b/http.c @@ -524,6 +524,7 @@ static int has_cert_password(void) return 0; if (!cert_auth.password) { cert_auth.protocol = xstrdup("cert"); + cert_auth.host = xstrdup(""); cert_auth.username = xstrdup(""); cert_auth.path = xstrdup(ssl_cert); credential_fill(&cert_auth); diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index f4c5d7ff91..1c1010bc54 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -414,4 +414,21 @@ test_expect_success 'url parser ignores embedded newlines' ' EOF ' +test_expect_success 'host-less URLs are parsed as empty host' ' + check fill "verbatim foo bar" <<-\EOF + url=cert:///path/to/cert.pem + -- + protocol=cert + host= + path=path/to/cert.pem + username=foo + password=bar + -- + verbatim: get + verbatim: protocol=cert + verbatim: host= + verbatim: path=path/to/cert.pem + EOF +' + test_done From 8ba8ed568e2a3b75ee84c49ddffb026fde1a0a91 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Apr 2020 20:50:48 -0700 Subject: [PATCH 04/10] credential: refuse to operate when missing host or protocol The credential helper protocol was designed to be very flexible: the fields it takes as input are treated as a pattern, and any missing fields are taken as wildcards. This allows unusual things like: echo protocol=https | git credential reject to delete all stored https credentials (assuming the helpers themselves treat the input that way). But when helpers are invoked automatically by Git, this flexibility works against us. If for whatever reason we don't have a "host" field, then we'd match _any_ host. When you're filling a credential to send to a remote server, this is almost certainly not what you want. Prevent this at the layer that writes to the credential helper. Add a check to the credential API that the host and protocol are always passed in, and add an assertion to the credential_write function that speaks credential helper protocol to be doubly sure. There are a few ways this can be triggered in practice: - the "git credential" command passes along arbitrary credential parameters it reads from stdin. - until the previous patch, when the host field of a URL is empty, we would leave it unset (rather than setting it to the empty string) - a URL like "example.com/foo.git" is treated by curl as if "http://" was present, but our parser sees it as a non-URL and leaves all fields unset - the recent fix for URLs with embedded newlines blanks the URL but otherwise continues. Rather than having the desired effect of looking up no credential at all, many helpers will return _any_ credential Our earlier test for an embedded newline didn't catch this because it only checked that the credential was cleared, but didn't configure an actual helper. Configuring the "verbatim" helper in the test would show that it is invoked (it's obviously a silly helper which doesn't look at its input, but the point is that it shouldn't be run at all). Since we're switching this case to die(), we don't need to bother with a helper. We can see the new behavior just by checking that the operation fails. We'll add new tests covering partial input as well (these can be triggered through various means with url-parsing, but it's simpler to just check them directly, as we know we are covered even if the url parser changes behavior in the future). [jn: changed to die() instead of logging and showing a manual username/password prompt] Reported-by: Carlo Arenas Signed-off-by: Jeff King Signed-off-by: Jonathan Nieder --- credential.c | 20 ++++++++++++++------ t/t0300-credentials.sh | 34 ++++++++++++++++++++++++++-------- 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/credential.c b/credential.c index d1bb71b41a..39b7cb0c0b 100644 --- a/credential.c +++ b/credential.c @@ -88,6 +88,11 @@ static int proto_is_http(const char *s) static void credential_apply_config(struct credential *c) { + if (!c->host) + die(_("refusing to work with credential missing host field")); + if (!c->protocol) + die(_("refusing to work with credential missing protocol field")); + if (c->configured) return; git_config(credential_config_callback, c); @@ -190,8 +195,11 @@ int credential_read(struct credential *c, FILE *fp) return 0; } -static void credential_write_item(FILE *fp, const char *key, const char *value) +static void credential_write_item(FILE *fp, const char *key, const char *value, + int required) { + if (!value && required) + BUG("credential value for %s is missing", key); if (!value) return; if (strchr(value, '\n')) @@ -201,11 +209,11 @@ static void credential_write_item(FILE *fp, const char *key, const char *value) void credential_write(const struct credential *c, FILE *fp) { - credential_write_item(fp, "protocol", c->protocol); - credential_write_item(fp, "host", c->host); - credential_write_item(fp, "path", c->path); - credential_write_item(fp, "username", c->username); - credential_write_item(fp, "password", c->password); + credential_write_item(fp, "protocol", c->protocol, 1); + credential_write_item(fp, "host", c->host, 1); + credential_write_item(fp, "path", c->path, 0); + credential_write_item(fp, "username", c->username, 0); + credential_write_item(fp, "password", c->password, 0); } static int run_credential_helper(struct credential *c, diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 1c1010bc54..646f8456ff 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -400,18 +400,16 @@ test_expect_success 'empty helper spec resets helper list' ' EOF ' -test_expect_success 'url parser ignores embedded newlines' ' - check fill <<-EOF +test_expect_success 'url parser rejects embedded newlines' ' + test_must_fail git credential fill 2>stderr <<-\EOF && url=https://one.example.com?%0ahost=two.example.com/ - -- - username=askpass-username - password=askpass-password - -- + EOF + cat >expect <<-\EOF && warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/ warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/ - askpass: Username: - askpass: Password: + fatal: refusing to work with credential missing host field EOF + test_i18ncmp expect stderr ' test_expect_success 'host-less URLs are parsed as empty host' ' @@ -431,4 +429,24 @@ test_expect_success 'host-less URLs are parsed as empty host' ' EOF ' +test_expect_success 'credential system refuses to work with missing host' ' + test_must_fail git credential fill 2>stderr <<-\EOF && + protocol=http + EOF + cat >expect <<-\EOF && + fatal: refusing to work with credential missing host field + EOF + test_i18ncmp expect stderr +' + +test_expect_success 'credential system refuses to work with missing protocol' ' + test_must_fail git credential fill 2>stderr <<-\EOF && + host=example.com + EOF + cat >expect <<-\EOF && + fatal: refusing to work with credential missing protocol field + EOF + test_i18ncmp expect stderr +' + test_done From a2b26ffb1a81aa23dd14453f4db05d8fe24ee7cc Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 18 Apr 2020 20:52:34 -0700 Subject: [PATCH 05/10] fsck: convert gitmodules url to URL passed to curl In 07259e74ec1 (fsck: detect gitmodules URLs with embedded newlines, 2020-03-11), git fsck learned to check whether URLs in .gitmodules could be understood by the credential machinery when they are handled by git-remote-curl. However, the check is overbroad: it checks all URLs instead of only URLs that would be passed to git-remote-curl. In principle a git:// or file:/// URL does not need to follow the same conventions as an http:// URL; in particular, git:// and file:// protocols are not succeptible to issues in the credential API because they do not support attaching credentials. In the HTTP case, the URL in .gitmodules does not always match the URL that would be passed to git-remote-curl and the credential machinery: Git's URL syntax allows specifying a remote helper followed by a "::" delimiter and a URL to be passed to it, so that git ls-remote http::https://example.com/repo.git invokes git-remote-http with https://example.com/repo.git as its URL argument. With today's checks, that distinction does not make a difference, but for a check we are about to introduce (for empty URL schemes) it will matter. .gitmodules files also support relative URLs. To ensure coverage for the https based embedded-newline attack, urldecode and check them directly for embedded newlines. Helped-by: Jeff King Signed-off-by: Jonathan Nieder Reviewed-by: Jeff King --- fsck.c | 94 +++++++++++++++++++++++++++++++++-- t/t7416-submodule-dash-url.sh | 29 +++++++++++ 2 files changed, 118 insertions(+), 5 deletions(-) diff --git a/fsck.c b/fsck.c index 5b437c260c..4e3bc8622f 100644 --- a/fsck.c +++ b/fsck.c @@ -7,6 +7,7 @@ #include "tag.h" #include "fsck.h" #include "refs.h" +#include "url.h" #include "utf8.h" #include "sha1-array.h" #include "decorate.h" @@ -942,17 +943,100 @@ static int fsck_tag(struct tag *tag, const char *data, return fsck_tag_buffer(tag, data, size, options); } +/* + * Like builtin/submodule--helper.c's starts_with_dot_slash, but without + * relying on the platform-dependent is_dir_sep helper. + * + * This is for use in checking whether a submodule URL is interpreted as + * relative to the current directory on any platform, since \ is a + * directory separator on Windows but not on other platforms. + */ +static int starts_with_dot_slash(const char *str) +{ + return str[0] == '.' && (str[1] == '/' || str[1] == '\\'); +} + +/* + * Like starts_with_dot_slash, this is a variant of submodule--helper's + * helper of the same name with the twist that it accepts backslash as a + * directory separator even on non-Windows platforms. + */ +static int starts_with_dot_dot_slash(const char *str) +{ + return str[0] == '.' && starts_with_dot_slash(str + 1); +} + +static int submodule_url_is_relative(const char *url) +{ + return starts_with_dot_slash(url) || starts_with_dot_dot_slash(url); +} + +/* + * Check whether a transport is implemented by git-remote-curl. + * + * If it is, returns 1 and writes the URL that would be passed to + * git-remote-curl to the "out" parameter. + * + * Otherwise, returns 0 and leaves "out" untouched. + * + * Examples: + * http::https://example.com/repo.git -> 1, https://example.com/repo.git + * https://example.com/repo.git -> 1, https://example.com/repo.git + * git://example.com/repo.git -> 0 + * + * This is for use in checking for previously exploitable bugs that + * required a submodule URL to be passed to git-remote-curl. + */ +static int url_to_curl_url(const char *url, const char **out) +{ + /* + * We don't need to check for case-aliases, "http.exe", and so + * on because in the default configuration, is_transport_allowed + * prevents URLs with those schemes from being cloned + * automatically. + */ + if (skip_prefix(url, "http::", out) || + skip_prefix(url, "https::", out) || + skip_prefix(url, "ftp::", out) || + skip_prefix(url, "ftps::", out)) + return 1; + if (starts_with(url, "http://") || + starts_with(url, "https://") || + starts_with(url, "ftp://") || + starts_with(url, "ftps://")) { + *out = url; + return 1; + } + return 0; +} + static int check_submodule_url(const char *url) { - struct credential c = CREDENTIAL_INIT; - int ret; + const char *curl_url; if (looks_like_command_line_option(url)) return -1; - ret = credential_from_url_gently(&c, url, 1); - credential_clear(&c); - return ret; + if (submodule_url_is_relative(url)) { + /* + * This could be appended to an http URL and url-decoded; + * check for malicious characters. + */ + char *decoded = url_decode(url); + int has_nl = !!strchr(decoded, '\n'); + free(decoded); + if (has_nl) + return -1; + } + + else if (url_to_curl_url(url, &curl_url)) { + struct credential c = CREDENTIAL_INIT; + int ret = credential_from_url_gently(&c, curl_url, 1); + credential_clear(&c); + return ret; + } + + return 0; } struct fsck_gitmodules_data { diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh index 41431b1ac3..afdd2553d9 100755 --- a/t/t7416-submodule-dash-url.sh +++ b/t/t7416-submodule-dash-url.sh @@ -60,6 +60,20 @@ test_expect_success 'trailing backslash is handled correctly' ' test_i18ngrep ! "unknown option" err ' +test_expect_success 'fsck permits embedded newline with unrecognized scheme' ' + git checkout --orphan newscheme && + cat >.gitmodules <<-\EOF && + [submodule "foo"] + url = "data://acjbkd%0akajfdickajkd" + EOF + git add .gitmodules && + git commit -m "gitmodules with unrecognized scheme" && + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + git push dst HEAD +' + test_expect_success 'fsck rejects embedded newline in url' ' # create an orphan branch to avoid existing .gitmodules objects git checkout --orphan newline && @@ -76,4 +90,19 @@ test_expect_success 'fsck rejects embedded newline in url' ' grep gitmodulesUrl err ' +test_expect_success 'fsck rejects embedded newline in relative url' ' + git checkout --orphan relative-newline && + cat >.gitmodules <<-\EOF && + [submodule "foo"] + url = "./%0ahost=two.example.com/foo.git" + EOF + git add .gitmodules && + git commit -m "relative url with newline" && + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + test_must_fail git push dst HEAD 2>err && + grep gitmodulesUrl err +' + test_done From fe29a9b7b0236d3d45c254965580d6aff7fa8504 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Apr 2020 20:53:09 -0700 Subject: [PATCH 06/10] credential: die() when parsing invalid urls When we try to initialize credential loading by URL and find that the URL is invalid, we set all fields to NULL in order to avoid acting on malicious input. Later when we request credentials, we diagonse the erroneous input: fatal: refusing to work with credential missing host field This is problematic in two ways: - The message doesn't tell the user *why* we are missing the host field, so they can't tell from this message alone how to recover. There can be intervening messages after the original warning of bad input, so the user may not have the context to put two and two together. - The error only occurs when we actually need to get a credential. If the URL permits anonymous access, the only encouragement the user gets to correct their bogus URL is a quiet warning. This is inconsistent with the check we perform in fsck, where any use of such a URL as a submodule is an error. When we see such a bogus URL, let's not try to be nice and continue without helpers. Instead, die() immediately. This is simpler and obviously safe. And there's very little chance of disrupting a normal workflow. It's _possible_ that somebody has a legitimate URL with a raw newline in it. It already wouldn't work with credential helpers, so this patch steps that up from an inconvenience to "we will refuse to work with it at all". If such a case does exist, we should figure out a way to work with it (especially if the newline is only in the path component, which we normally don't even pass to helpers). But until we see a real report, we're better off being defensive. Reported-by: Carlo Arenas Signed-off-by: Jeff King Signed-off-by: Jonathan Nieder --- credential.c | 6 ++---- t/t0300-credentials.sh | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/credential.c b/credential.c index 39b7cb0c0b..7d43359591 100644 --- a/credential.c +++ b/credential.c @@ -405,8 +405,6 @@ int credential_from_url_gently(struct credential *c, const char *url, void credential_from_url(struct credential *c, const char *url) { - if (credential_from_url_gently(c, url, 0) < 0) { - warning(_("skipping credential lookup for url: %s"), url); - credential_clear(c); - } + if (credential_from_url_gently(c, url, 0) < 0) + die(_("credential url cannot be parsed: %s"), url); } diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 646f8456ff..efed3ea295 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -406,8 +406,7 @@ test_expect_success 'url parser rejects embedded newlines' ' EOF cat >expect <<-\EOF && warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/ - warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/ - fatal: refusing to work with credential missing host field + fatal: credential url cannot be parsed: https://one.example.com?%0ahost=two.example.com/ EOF test_i18ncmp expect stderr ' From c44088ecc4b0722636e0a305f9608d3047197282 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 18 Apr 2020 20:54:13 -0700 Subject: [PATCH 07/10] credential: treat URL without scheme as invalid libcurl permits making requests without a URL scheme specified. In this case, it guesses the URL from the hostname, so I can run git ls-remote http::ftp.example.com/path/to/repo and it would make an FTP request. Any user intentionally using such a URL is likely to have made a typo. Unfortunately, credential_from_url is not able to determine the host and protocol in order to determine appropriate credentials to send, and until "credential: refuse to operate when missing host or protocol", this resulted in another host's credentials being leaked to the named host. Teach credential_from_url_gently to consider such a URL to be invalid so that fsck can detect and block gitmodules files with such URLs, allowing server operators to avoid serving them to downstream users running older versions of Git. This also means that when such URLs are passed on the command line, Git will print a clearer error so affected users can switch to the simpler URL that explicitly specifies the host and protocol they intend. One subtlety: .gitmodules files can contain relative URLs, representing a URL relative to the URL they were cloned from. The relative URL resolver used for .gitmodules can follow ".." components out of the path part and past the host part of a URL, meaning that such a relative URL can be used to traverse from a https://foo.example.com/innocent superproject to a https::attacker.example.com/exploit submodule. Fortunately a leading ':' in the first path component after a series of leading './' and '../' components is unlikely to show up in other contexts, so we can catch this by detecting that pattern. Reported-by: Jeff King Signed-off-by: Jonathan Nieder Reviewed-by: Jeff King --- credential.c | 7 ++++-- fsck.c | 47 +++++++++++++++++++++++++++++++++-- t/t5550-http-fetch-dumb.sh | 7 ++---- t/t7416-submodule-dash-url.sh | 32 ++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 9 deletions(-) diff --git a/credential.c b/credential.c index 7d43359591..aedb64574d 100644 --- a/credential.c +++ b/credential.c @@ -357,8 +357,11 @@ int credential_from_url_gently(struct credential *c, const char *url, * (3) proto://:@/... */ proto_end = strstr(url, "://"); - if (!proto_end) - return 0; + if (!proto_end) { + if (!quiet) + warning(_("url has no scheme: %s"), url); + return -1; + } cp = proto_end + 3; at = strchr(cp, '@'); colon = strchr(cp, ':'); diff --git a/fsck.c b/fsck.c index 4e3bc8622f..41af5c0d5f 100644 --- a/fsck.c +++ b/fsck.c @@ -971,6 +971,34 @@ static int submodule_url_is_relative(const char *url) return starts_with_dot_slash(url) || starts_with_dot_dot_slash(url); } +/* + * Count directory components that a relative submodule URL should chop + * from the remote_url it is to be resolved against. + * + * In other words, this counts "../" components at the start of a + * submodule URL. + * + * Returns the number of directory components to chop and writes a + * pointer to the next character of url after all leading "./" and + * "../" components to out. + */ +static int count_leading_dotdots(const char *url, const char **out) +{ + int result = 0; + while (1) { + if (starts_with_dot_dot_slash(url)) { + result++; + url += strlen("../"); + continue; + } + if (starts_with_dot_slash(url)) { + url += strlen("./"); + continue; + } + *out = url; + return result; + } +} /* * Check whether a transport is implemented by git-remote-curl. * @@ -1018,15 +1046,30 @@ static int check_submodule_url(const char *url) return -1; if (submodule_url_is_relative(url)) { + char *decoded; + const char *next; + int has_nl; + /* * This could be appended to an http URL and url-decoded; * check for malicious characters. */ - char *decoded = url_decode(url); - int has_nl = !!strchr(decoded, '\n'); + decoded = url_decode(url); + has_nl = !!strchr(decoded, '\n'); + free(decoded); if (has_nl) return -1; + + /* + * URLs which escape their root via "../" can overwrite + * the host field and previous components, resolving to + * URLs like https::example.com/submodule.git that were + * susceptible to CVE-2020-11008. + */ + if (count_leading_dotdots(url, &next) > 0 && + *next == ':') + return -1; } else if (url_to_curl_url(url, &curl_url)) { diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index 8552184e74..517202e477 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -310,11 +310,8 @@ test_expect_success 'git client does not send an empty Accept-Language' ' ' test_expect_success 'remote-http complains cleanly about malformed urls' ' - # do not actually issue "list" or other commands, as we do not - # want to rely on what curl would actually do with such a broken - # URL. This is just about making sure we do not segfault during - # initialization. - test_must_fail git remote-http http::/example.com/repo.git + test_must_fail git remote-http http::/example.com/repo.git 2>stderr && + test_i18ngrep "url has no scheme" stderr ' test_expect_success 'redirects can be forbidden/allowed' ' diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh index afdd2553d9..249dc3d1d4 100755 --- a/t/t7416-submodule-dash-url.sh +++ b/t/t7416-submodule-dash-url.sh @@ -60,6 +60,38 @@ test_expect_success 'trailing backslash is handled correctly' ' test_i18ngrep ! "unknown option" err ' +test_expect_success 'fsck rejects missing URL scheme' ' + git checkout --orphan missing-scheme && + cat >.gitmodules <<-\EOF && + [submodule "foo"] + url = http::one.example.com/foo.git + EOF + git add .gitmodules && + test_tick && + git commit -m "gitmodules with missing URL scheme" && + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + test_must_fail git push dst HEAD 2>err && + grep gitmodulesUrl err +' + +test_expect_success 'fsck rejects relative URL resolving to missing scheme' ' + git checkout --orphan relative-missing-scheme && + cat >.gitmodules <<-\EOF && + [submodule "foo"] + url = "..\\../.\\../:one.example.com/foo.git" + EOF + git add .gitmodules && + test_tick && + git commit -m "gitmodules with relative URL that strips off scheme" && + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + test_must_fail git push dst HEAD 2>err && + grep gitmodulesUrl err +' + test_expect_success 'fsck permits embedded newline with unrecognized scheme' ' git checkout --orphan newscheme && cat >.gitmodules <<-\EOF && From e7fab62b736cca3416660636e46f0be8386a5030 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 18 Apr 2020 20:54:57 -0700 Subject: [PATCH 08/10] credential: treat URL with empty scheme as invalid Until "credential: refuse to operate when missing host or protocol", Git's credential handling code interpreted URLs with empty scheme to mean "give me credentials matching this host for any protocol". Luckily libcurl does not recognize such URLs (it tries to look for a protocol named "" and fails). Just in case that changes, let's reject them within Git as well. This way, credential_from_url is guaranteed to always produce a "struct credential" with protocol and host set. Signed-off-by: Jonathan Nieder --- credential.c | 5 ++--- t/t5550-http-fetch-dumb.sh | 9 +++++++++ t/t7416-submodule-dash-url.sh | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/credential.c b/credential.c index aedb64574d..64a841eddc 100644 --- a/credential.c +++ b/credential.c @@ -357,7 +357,7 @@ int credential_from_url_gently(struct credential *c, const char *url, * (3) proto://:@/... */ proto_end = strstr(url, "://"); - if (!proto_end) { + if (!proto_end || proto_end == url) { if (!quiet) warning(_("url has no scheme: %s"), url); return -1; @@ -382,8 +382,7 @@ int credential_from_url_gently(struct credential *c, const char *url, host = at + 1; } - if (proto_end - url > 0) - c->protocol = xmemdupz(url, proto_end - url); + c->protocol = xmemdupz(url, proto_end - url); c->host = url_decode_mem(host, slash - host); /* Trim leading and trailing slashes from path */ while (*slash == '/') diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index 517202e477..86f6eeaf71 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -314,6 +314,15 @@ test_expect_success 'remote-http complains cleanly about malformed urls' ' test_i18ngrep "url has no scheme" stderr ' +# NEEDSWORK: Writing commands to git-remote-curl can race against the latter +# erroring out, producing SIGPIPE. Remove "ok=sigpipe" once transport-helper has +# learned to handle early remote helper failures more cleanly. +test_expect_success 'remote-http complains cleanly about empty scheme' ' + test_must_fail ok=sigpipe git ls-remote \ + http::${HTTPD_URL#http}/dumb/repo.git 2>stderr && + test_i18ngrep "url has no scheme" stderr +' + 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 && diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh index 249dc3d1d4..9309040373 100755 --- a/t/t7416-submodule-dash-url.sh +++ b/t/t7416-submodule-dash-url.sh @@ -92,6 +92,38 @@ test_expect_success 'fsck rejects relative URL resolving to missing scheme' ' grep gitmodulesUrl err ' +test_expect_success 'fsck rejects empty URL scheme' ' + git checkout --orphan empty-scheme && + cat >.gitmodules <<-\EOF && + [submodule "foo"] + url = http::://one.example.com/foo.git + EOF + git add .gitmodules && + test_tick && + git commit -m "gitmodules with empty URL scheme" && + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + test_must_fail git push dst HEAD 2>err && + grep gitmodulesUrl err +' + +test_expect_success 'fsck rejects relative URL resolving to empty scheme' ' + git checkout --orphan relative-empty-scheme && + cat >.gitmodules <<-\EOF && + [submodule "foo"] + url = ../../../:://one.example.com/foo.git + EOF + git add .gitmodules && + test_tick && + git commit -m "relative gitmodules URL resolving to empty scheme" && + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + test_must_fail git push dst HEAD 2>err && + grep gitmodulesUrl err +' + test_expect_success 'fsck permits embedded newline with unrecognized scheme' ' git checkout --orphan newscheme && cat >.gitmodules <<-\EOF && From 1a3609e402a062ef7b11f197fe96c28cabca132c Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 18 Apr 2020 20:57:22 -0700 Subject: [PATCH 09/10] fsck: reject URL with empty host in .gitmodules Git's URL parser interprets https:///example.com/repo.git to have no host and a path of "example.com/repo.git". Curl, on the other hand, internally redirects it to https://example.com/repo.git. As a result, until "credential: parse URL without host as empty host, not unset", tricking a user into fetching from such a URL would cause Git to send credentials for another host to example.com. Teach fsck to block and detect .gitmodules files using such a URL to prevent sharing them with Git versions that are not yet protected. A relative URL in a .gitmodules file could also be used to trigger this. The relative URL resolver used for .gitmodules does not normalize sequences of slashes and can follow ".." components out of the path part and to the host part of a URL, meaning that such a relative URL can be used to traverse from a https://foo.example.com/innocent superproject to a https:///attacker.example.com/exploit submodule. Fortunately, redundant extra slashes in .gitmodules are rare, so we can catch this by detecting one after a leading sequence of "./" and "../" components. Helped-by: Jeff King Signed-off-by: Jonathan Nieder Reviewed-by: Jeff King --- fsck.c | 10 +++++++--- t/t7416-submodule-dash-url.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/fsck.c b/fsck.c index 41af5c0d5f..31b5be05f5 100644 --- a/fsck.c +++ b/fsck.c @@ -1064,17 +1064,21 @@ static int check_submodule_url(const char *url) /* * URLs which escape their root via "../" can overwrite * the host field and previous components, resolving to - * URLs like https::example.com/submodule.git that were + * URLs like https::example.com/submodule.git and + * https:///example.com/submodule.git that were * susceptible to CVE-2020-11008. */ if (count_leading_dotdots(url, &next) > 0 && - *next == ':') + (*next == ':' || *next == '/')) return -1; } else if (url_to_curl_url(url, &curl_url)) { struct credential c = CREDENTIAL_INIT; - int ret = credential_from_url_gently(&c, curl_url, 1); + int ret = 0; + if (credential_from_url_gently(&c, curl_url, 1) || + !*c.host) + ret = -1; credential_clear(&c); return ret; } diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh index 9309040373..eec96e0ba9 100755 --- a/t/t7416-submodule-dash-url.sh +++ b/t/t7416-submodule-dash-url.sh @@ -124,6 +124,38 @@ test_expect_success 'fsck rejects relative URL resolving to empty scheme' ' grep gitmodulesUrl err ' +test_expect_success 'fsck rejects empty hostname' ' + git checkout --orphan empty-host && + cat >.gitmodules <<-\EOF && + [submodule "foo"] + url = http:///one.example.com/foo.git + EOF + git add .gitmodules && + test_tick && + git commit -m "gitmodules with extra slashes" && + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + test_must_fail git push dst HEAD 2>err && + grep gitmodulesUrl err +' + +test_expect_success 'fsck rejects relative url that produced empty hostname' ' + git checkout --orphan messy-relative && + cat >.gitmodules <<-\EOF && + [submodule "foo"] + url = ../../..//one.example.com/foo.git + EOF + git add .gitmodules && + test_tick && + git commit -m "gitmodules abusing relative_path" && + test_when_finished "rm -rf dst" && + git init --bare dst && + git -C dst config transfer.fsckObjects true && + test_must_fail git push dst HEAD 2>err && + grep gitmodulesUrl err +' + test_expect_success 'fsck permits embedded newline with unrecognized scheme' ' git checkout --orphan newscheme && cat >.gitmodules <<-\EOF && From df5be6dc3fd18c294ec93a9af0321334e3f92c9c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 19 Apr 2020 02:34:55 -0400 Subject: [PATCH 10/10] Git 2.17.5 Signed-off-by: Jeff King Signed-off-by: Jonathan Nieder --- Documentation/RelNotes/2.17.5.txt | 22 ++++++++++++++++++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.17.5.txt diff --git a/Documentation/RelNotes/2.17.5.txt b/Documentation/RelNotes/2.17.5.txt new file mode 100644 index 0000000000..2abb821a73 --- /dev/null +++ b/Documentation/RelNotes/2.17.5.txt @@ -0,0 +1,22 @@ +Git v2.17.5 Release Notes +========================= + +This release is to address a security issue: CVE-2020-11008 + +Fixes since v2.17.4 +------------------- + + * With a crafted URL that contains a newline or empty host, or lacks + a scheme, the credential helper machinery can be fooled into + providing credential information that is not appropriate for the + protocol in use and host being contacted. + + Unlike the vulnerability CVE-2020-5260 fixed in v2.17.4, the + credentials are not for a host of the attacker's choosing; instead, + they are for some unspecified host (based on how the configured + credential helper handles an absent "host" parameter). + + The attack has been made impossible by refusing to work with + under-specified credential patterns. + +Credit for finding the vulnerability goes to Carlo Arenas. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index cdb21b3311..85d9db5600 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.17.4 +DEF_VER=v2.17.5 LF=' ' diff --git a/RelNotes b/RelNotes index 196ab8077e..07012e884f 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.17.4.txt \ No newline at end of file +Documentation/RelNotes/2.17.5.txt \ No newline at end of file