From 21920cbd9af489763a62ebb81e1ca188354f833e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 24 Apr 2020 11:49:50 +0000 Subject: credential: fix grammar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was a lot going on behind the scenes when the vulnerability and possible solutions were discussed. Grammar was not a primary focus, that's why this slipped in. Reviewed-by: Jonathan Nieder Signed-off-by: Johannes Schindelin Reviewed-by: Carlo Marcelo Arenas Belón Signed-off-by: Junio C Hamano --- credential.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/credential.h b/credential.h index 122a23cd2f..5a86502d95 100644 --- a/credential.h +++ b/credential.h @@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *); /* * Parse a url into a credential struct, replacing any existing contents. * - * Ifthe url can't be parsed (e.g., a missing "proto://" component), the + * If the url can't be parsed (e.g., a missing "proto://" component), the * resulting credential will be empty but we'll still return success from the * "gently" form. * -- cgit v1.2.3 From 6828e5972b82f474cc14ca9cb9e01e897f205f4c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 24 Apr 2020 11:49:51 +0000 Subject: credential: optionally allow partial URLs in credential_from_url_gently() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we required from a URL in order to parse it into a `struct credential`. That led to serious vulnerabilities. There was one call site, though, that really needed that leniency: when parsing config settings a la `credential.dev.azure.com.useHTTPPath`. Settings like this might be desired when users want to use, say, a given user name on a given host, regardless of the protocol to be used. In preparation for fixing that bug, let's refactor the code to optionally allow for partial URLs. For the moment, this functionality is only exposed via the now-renamed function `credential_from_url_1()`, but it is not used. The intention is to make it easier to verify that this commit does not change the existing behavior unless explicitly allowing for partial URLs. Please note that this patch does more than just reinstating a way to imitate the behavior before those CVE-2020-11008 fixes: Before that, we would simply ignore URLs without a protocol. In other words, misleadingly, the following setting would be applied to _all_ URLs: [credential "example.com"] username = that-me The obvious intention is to match the host name only. With this patch, we allow precisely that: when parsing the URL with non-zero `allow_partial_url`, we do not simply return success if there was no protocol, but we simply leave the protocol unset and continue parsing the URL. Signed-off-by: Johannes Schindelin Reviewed-by: Carlo Marcelo Arenas Belón Signed-off-by: Junio C Hamano --- credential.c | 42 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/credential.c b/credential.c index 64a841eddc..7dbbf26f17 100644 --- a/credential.c +++ b/credential.c @@ -343,8 +343,31 @@ static int check_url_component(const char *url, int quiet, return -1; } -int credential_from_url_gently(struct credential *c, const char *url, - int quiet) +/* + * Potentially-partial URLs can, but do not have to, contain + * + * - a protocol (or scheme) of the form "://" + * + * - a host name (the part after the protocol and before the first slash after + * that, if any) + * + * - a user name and potentially a password (as "[:]@" part of + * the host name) + * + * - a path (the part after the host name, if any, starting with the slash) + * + * Missing parts will be left unset in `struct credential`. Thus, `https://` + * will have only the `protocol` set, `example.com` only the host name, and + * `/git` only the path. + * + * Note that an empty host name in an otherwise fully-qualified URL (e.g. + * `cert:///path/to/cert.pem`) will be treated as unset if we expect the URL to + * be potentially partial, and only then (otherwise, the empty string is used). + * + * The credential_from_url() function does not allow partial URLs. + */ +static int credential_from_url_1(struct credential *c, const char *url, + int allow_partial_url, int quiet) { const char *at, *colon, *cp, *slash, *host, *proto_end; @@ -357,12 +380,12 @@ int credential_from_url_gently(struct credential *c, const char *url, * (3) proto://:@/... */ proto_end = strstr(url, "://"); - if (!proto_end || proto_end == url) { + if (!allow_partial_url && (!proto_end || proto_end == url)) { if (!quiet) warning(_("url has no scheme: %s"), url); return -1; } - cp = proto_end + 3; + cp = proto_end ? proto_end + 3 : url; at = strchr(cp, '@'); colon = strchr(cp, ':'); slash = strchrnul(cp, '/'); @@ -382,8 +405,10 @@ int credential_from_url_gently(struct credential *c, const char *url, host = at + 1; } - c->protocol = xmemdupz(url, proto_end - url); - c->host = url_decode_mem(host, slash - host); + if (proto_end && proto_end - url > 0) + c->protocol = xmemdupz(url, proto_end - url); + if (!allow_partial_url || slash - host > 0) + c->host = url_decode_mem(host, slash - host); /* Trim leading and trailing slashes from path */ while (*slash == '/') slash++; @@ -405,6 +430,11 @@ int credential_from_url_gently(struct credential *c, const char *url, return 0; } +int credential_from_url_gently(struct credential *c, const char *url, int quiet) +{ + return credential_from_url_1(c, url, 0, quiet); +} + void credential_from_url(struct credential *c, const char *url) { if (credential_from_url_gently(c, url, 0) < 0) -- cgit v1.2.3 From 9a121b0d226dd0017318be0d18120aeb766f1235 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 24 Apr 2020 11:49:52 +0000 Subject: credential: handle `credential..` again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the patches for CVE-2020-11008, the ability to specify credential settings in the config for partial URLs got lost. For example, it used to be possible to specify a credential helper for a specific protocol: [credential "https://"] helper = my-https-helper Likewise, it used to be possible to configure settings for a specific host, e.g.: [credential "dev.azure.com"] useHTTPPath = true Let's reinstate this behavior. While at it, increase the test coverage to document and verify the behavior with a couple other categories of partial URLs. Signed-off-by: Johannes Schindelin Reviewed-by: Carlo Marcelo Arenas Belón Signed-off-by: Junio C Hamano --- credential.c | 18 +++++++++++++++++- t/t0300-credentials.sh | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/credential.c b/credential.c index 7dbbf26f17..c1a9ca4e48 100644 --- a/credential.c +++ b/credential.c @@ -35,6 +35,10 @@ int credential_match(const struct credential *want, #undef CHECK } + +static int credential_from_potentially_partial_url(struct credential *c, + const char *url); + static int credential_config_callback(const char *var, const char *value, void *data) { @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value, char *url = xmemdupz(key, dot - key); int matched; - credential_from_url(&want, url); + if (credential_from_potentially_partial_url(&want, url) < 0) { + warning(_("skipping credential lookup for key: %s"), + var); + credential_clear(&want); + free(url); + return 0; + } matched = credential_match(&want, c); credential_clear(&want); @@ -430,6 +440,12 @@ static int credential_from_url_1(struct credential *c, const char *url, return 0; } +static int credential_from_potentially_partial_url(struct credential *c, + const char *url) +{ + return credential_from_url_1(c, url, 1, 0); +} + int credential_from_url_gently(struct credential *c, const char *url, int quiet) { return credential_from_url_1(c, url, 0, quiet); diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index efed3ea295..da4b315d6d 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -448,4 +448,42 @@ test_expect_success 'credential system refuses to work with missing protocol' ' test_i18ncmp expect stderr ' +test_expect_success 'credential config with partial URLs' ' + echo "echo password=yep" | write_script git-credential-yep && + test_write_lines url=https://user@example.com/repo.git >stdin && + for partial in \ + example.com \ + user@example.com \ + https:// \ + https://example.com \ + https://example.com/ \ + https://user@example.com \ + https://user@example.com/ \ + https://example.com/repo.git \ + https://user@example.com/repo.git \ + /repo.git + do + git -c credential.$partial.helper=yep \ + credential fill stdout && + grep yep stdout || + return 1 + done && + + for partial in \ + dont.use.this \ + http:// \ + /repo + do + git -c credential.$partial.helper=yep \ + credential fill stdout && + ! grep yep stdout || + return 1 + done && + + git -c credential.$partial.helper=yep \ + -c credential.with%0anewline.username=uh-oh \ + credential fill stdout 2>stderr && + test_i18ngrep "skipping credential lookup for key" stderr +' + test_done -- cgit v1.2.3