@matheusbsbs / matheus-bagliano / commits / 8ba8ed568e
Fork 0

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 <carenas@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Jeff King committed Apr 18, 2020 at 20:50 UTC 8ba8ed568e2a3b75ee84c49ddffb026fde1a0a91
2 files changed +40 -14
credential.c
+14 -6
@@ -88,6 +88,11 @@ static int proto_is_http(const char *s)
88
89 static void credential_apply_config(struct credential *c)
90 {
91 + if (!c->host)
92 + die(_("refusing to work with credential missing host field"));
93 + if (!c->protocol)
94 + die(_("refusing to work with credential missing protocol field"));
95 +
96 if (c->configured)
97 return;
98 git_config(credential_config_callback, c);
@@ -190,8 +195,11 @@ int credential_read(struct credential *c, FILE *fp)
195 return 0;
196 }
197
193 -static void credential_write_item(FILE *fp, const char *key, const char *value)
198 +static void credential_write_item(FILE *fp, const char *key, const char *value,
199 + int required)
200 {
201 + if (!value && required)
202 + BUG("credential value for %s is missing", key);
203 if (!value)
204 return;
205 if (strchr(value, '\n'))
@@ -201,11 +209,11 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)
209
210 void credential_write(const struct credential *c, FILE *fp)
211 {
204 - credential_write_item(fp, "protocol", c->protocol);
205 - credential_write_item(fp, "host", c->host);
206 - credential_write_item(fp, "path", c->path);
207 - credential_write_item(fp, "username", c->username);
208 - credential_write_item(fp, "password", c->password);
212 + credential_write_item(fp, "protocol", c->protocol, 1);
213 + credential_write_item(fp, "host", c->host, 1);
214 + credential_write_item(fp, "path", c->path, 0);
215 + credential_write_item(fp, "username", c->username, 0);
216 + credential_write_item(fp, "password", c->password, 0);
217 }
218
219 static int run_credential_helper(struct credential *c,
t/t0300-credentials.sh
+26 -8
@@ -400,18 +400,16 @@ test_expect_success 'empty helper spec resets helper list' '
400 EOF
401 '
402
403 -test_expect_success 'url parser ignores embedded newlines' '
404 - check fill <<-EOF
403 +test_expect_success 'url parser rejects embedded newlines' '
404 + test_must_fail git credential fill 2>stderr <<-\EOF &&
405 url=https://one.example.com?%0ahost=two.example.com/
406 - --
407 - username=askpass-username
408 - password=askpass-password
409 - --
406 + EOF
407 + cat >expect <<-\EOF &&
408 warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/
409 warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/
412 - askpass: Username:
413 - askpass: Password:
410 + fatal: refusing to work with credential missing host field
411 EOF
412 + test_i18ncmp expect stderr
413 '
414
415 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' '
429 EOF
430 '
431
432 +test_expect_success 'credential system refuses to work with missing host' '
433 + test_must_fail git credential fill 2>stderr <<-\EOF &&
434 + protocol=http
435 + EOF
436 + cat >expect <<-\EOF &&
437 + fatal: refusing to work with credential missing host field
438 + EOF
439 + test_i18ncmp expect stderr
440 +'
441 +
442 +test_expect_success 'credential system refuses to work with missing protocol' '
443 + test_must_fail git credential fill 2>stderr <<-\EOF &&
444 + host=example.com
445 + EOF
446 + cat >expect <<-\EOF &&
447 + fatal: refusing to work with credential missing protocol field
448 + EOF
449 + test_i18ncmp expect stderr
450 +'
451 +
452 test_done