From 9a6bbee8006c24b46a85d29e7b38cfa79e9ab21b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 11 Mar 2020 17:53:41 -0400 Subject: credential: avoid writing values with newlines The credential protocol that we use to speak to helpers can't represent values with newlines in them. This was an intentional design choice to keep the protocol simple, since none of the values we pass should generally have newlines. However, if we _do_ encounter a newline in a value, we blindly transmit it in credential_write(). Such values may break the protocol syntax, or worse, inject new valid lines into the protocol stream. The most likely way for a newline to end up in a credential struct is by decoding a URL with a percent-encoded newline. However, since the bug occurs at the moment we write the value to the protocol, we'll catch it there. That should leave no possibility of accidentally missing a code path that can trigger the problem. At this level of the code we have little choice but to die(). However, since we'd not ever expect to see this case outside of a malicious URL, that's an acceptable outcome. Reported-by: Felix Wilhelm --- credential.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'credential.c') diff --git a/credential.c b/credential.c index 9747f47b18..00ee4d62db 100644 --- a/credential.c +++ b/credential.c @@ -194,6 +194,8 @@ static void credential_write_item(FILE *fp, const char *key, const char *value) { if (!value) return; + if (strchr(value, '\n')) + die("credential value for %s contains newline", key); fprintf(fp, "%s=%s\n", key, value); } -- cgit v1.2.3 From c716fe4bd917e013bf376a678b3a924447777b2d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 12 Mar 2020 01:31:11 -0400 Subject: credential: detect unrepresentable values when parsing urls The credential protocol can't represent newlines in values, but URLs can embed percent-encoded newlines in various components. A previous commit taught the low-level writing routines to die() when encountering this, but we can be a little friendlier to the user by detecting them earlier and handling them gracefully. This patch teaches credential_from_url() to notice such components, issue a warning, and blank the credential (which will generally result in prompting the user for a username and password). We blank the whole credential in this case. Another option would be to blank only the invalid component. However, we're probably better off not feeding a partially-parsed URL result to a credential helper. We don't know how a given helper would handle it, so we're better off to err on the side of matching nothing rather than something unexpected. The die() call in credential_write() is _probably_ impossible to reach after this patch. Values should end up in credential structs only by URL parsing (which is covered here), or by reading credential protocol input (which by definition cannot read a newline into a value). But we should definitely keep the low-level check, as it's our final and most accurate line of defense against protocol injection attacks. Arguably it could become a BUG(), but it probably doesn't matter much either way. Note that the public interface of credential_from_url() grows a little more than we need here. We'll use the extra flexibility in a future patch to help fsck catch these cases. --- credential.c | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) (limited to 'credential.c') diff --git a/credential.c b/credential.c index 00ee4d62db..eeeac3242e 100644 --- a/credential.c +++ b/credential.c @@ -321,7 +321,22 @@ void credential_reject(struct credential *c) c->approved = 0; } -void credential_from_url(struct credential *c, const char *url) +static int check_url_component(const char *url, int quiet, + const char *name, const char *value) +{ + if (!value) + return 0; + if (!strchr(value, '\n')) + return 0; + + if (!quiet) + warning(_("url contains a newline in its %s component: %s"), + name, url); + return -1; +} + +int credential_from_url_gently(struct credential *c, const char *url, + int quiet) { const char *at, *colon, *cp, *slash, *host, *proto_end; @@ -335,7 +350,7 @@ void credential_from_url(struct credential *c, const char *url) */ proto_end = strstr(url, "://"); if (!proto_end) - return; + return 0; cp = proto_end + 3; at = strchr(cp, '@'); colon = strchr(cp, ':'); @@ -370,4 +385,21 @@ void credential_from_url(struct credential *c, const char *url) while (p > c->path && *p == '/') *p-- = '\0'; } + + if (check_url_component(url, quiet, "username", c->username) < 0 || + check_url_component(url, quiet, "password", c->password) < 0 || + check_url_component(url, quiet, "protocol", c->protocol) < 0 || + check_url_component(url, quiet, "host", c->host) < 0 || + check_url_component(url, quiet, "path", c->path) < 0) + return -1; + + return 0; +} + +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); + } } -- cgit v1.2.3