From 24036686c4af84c9e84e486ef3debab6e6d8e6b5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Apr 2020 20:48:05 -0700 Subject: 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 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'credential.c') 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++; -- cgit v1.2.3 From 8ba8ed568e2a3b75ee84c49ddffb026fde1a0a91 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Apr 2020 20:50:48 -0700 Subject: 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 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) (limited to 'credential.c') 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, -- cgit v1.2.3 From fe29a9b7b0236d3d45c254965580d6aff7fa8504 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 18 Apr 2020 20:53:09 -0700 Subject: 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 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'credential.c') 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); } -- cgit v1.2.3 From c44088ecc4b0722636e0a305f9608d3047197282 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 18 Apr 2020 20:54:13 -0700 Subject: 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 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'credential.c') 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, ':'); -- cgit v1.2.3 From e7fab62b736cca3416660636e46f0be8386a5030 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sat, 18 Apr 2020 20:54:57 -0700 Subject: 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 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'credential.c') 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 == '/') -- cgit v1.2.3