| From 4767ee743a3e1a5bd033cffa10a6e6cd82305337 Mon Sep 17 00:00:00 2001 |
| From: Jeff King <peff@peff.net> |
| 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. |
| |
| (cherry picked from commit c716fe4bd917e013bf376a678b3a924447777b2d) |
| Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> |
| --- |
| credential.c | 36 ++++++++++++++++++++++++++++++++++-- |
| credential.h | 16 ++++++++++++++++ |
| t/t0300-credentials.sh | 12 ++++++++++-- |
| 3 files changed, 60 insertions(+), 4 deletions(-) |
| |
| diff --git a/credential.c b/credential.c |
| index a79aff0c9c..24823829f6 100644 |
| --- a/credential.c |
| +++ b/credential.c |
| @@ -324,7 +324,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; |
| |
| @@ -338,7 +353,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, ':'); |
| @@ -373,4 +388,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); |
| + } |
| } |
| diff --git a/credential.h b/credential.h |
| index 6b0cd16be2..122a23cd2f 100644 |
| --- a/credential.h |
| +++ b/credential.h |
| @@ -28,7 +28,23 @@ void credential_reject(struct credential *); |
| |
| int credential_read(struct credential *, FILE *); |
| 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 |
| + * resulting credential will be empty but we'll still return success from the |
| + * "gently" form. |
| + * |
| + * If we encounter a component which cannot be represented as a credential |
| + * value (e.g., because it contains a newline), the "gently" form will return |
| + * an error but leave the broken state in the credential object for further |
| + * examination. The non-gentle form will issue a warning to stderr and return |
| + * an empty credential. |
| + */ |
| void credential_from_url(struct credential *, const char *url); |
| +int credential_from_url_gently(struct credential *, const char *url, int quiet); |
| + |
| int credential_match(const struct credential *have, |
| const struct credential *want); |
| |
| diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh |
| index 26f3c3a972..b9c0f1f279 100755 |
| --- a/t/t0300-credentials.sh |
| +++ b/t/t0300-credentials.sh |
| @@ -308,9 +308,17 @@ test_expect_success 'empty helper spec resets helper list' ' |
| EOF |
| ' |
| |
| -test_expect_success 'url parser rejects embedded newlines' ' |
| - test_must_fail git credential fill <<-\EOF |
| +test_expect_success 'url parser ignores embedded newlines' ' |
| + check fill <<-EOF |
| url=https://one.example.com?%0ahost=two.example.com/ |
| + -- |
| + username=askpass-username |
| + password=askpass-password |
| + -- |
| + 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: |
| EOF |
| ' |
| |
| -- |
| 2.26.0.292.g33ef6b2f38 |
| |