@matheusbsbs / matheus-bagliano / commits / c716fe4bd9

Fork 0

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.

Jeff King committed Mar 12, 2020 at 01:31 UTC c716fe4bd917e013bf376a678b3a924447777b2d

3 files changed +60 -4

credential.c

+34 -2

@@ -321,7 +321,22 @@ void credential_reject(struct credential *c)

321 c->approved = 0;

322 }

323

324 -void credential_from_url(struct credential *c, const char *url)

324 +static int check_url_component(const char *url, int quiet,

325 + const char *name, const char *value)

326 +{

327 + if (!value)

328 + return 0;

329 + if (!strchr(value, '\n'))

330 + return 0;

331 +

332 + if (!quiet)

333 + warning(_("url contains a newline in its %s component: %s"),

334 + name, url);

335 + return -1;

336 +}

337 +

338 +int credential_from_url_gently(struct credential *c, const char *url,

339 + int quiet)

340 {

341 const char *at, *colon, *cp, *slash, *host, *proto_end;

342

@@ -335,7 +350,7 @@ void credential_from_url(struct credential *c, const char *url)

350 */

351 proto_end = strstr(url, "://");

352 if (!proto_end)

338 - return;

353 + return 0;

354 cp = proto_end + 3;

355 at = strchr(cp, '@');

356 colon = strchr(cp, ':');

@@ -370,4 +385,21 @@ void credential_from_url(struct credential *c, const char *url)

385 while (p > c->path && *p == '/')

386 *p-- = '\0';

387 }

388 +

389 + if (check_url_component(url, quiet, "username", c->username) < 0 ||

390 + check_url_component(url, quiet, "password", c->password) < 0 ||

391 + check_url_component(url, quiet, "protocol", c->protocol) < 0 ||

392 + check_url_component(url, quiet, "host", c->host) < 0 ||

393 + check_url_component(url, quiet, "path", c->path) < 0)

394 + return -1;

395 +

396 + return 0;

397 +}

398 +

399 +void credential_from_url(struct credential *c, const char *url)

400 +{

401 + if (credential_from_url_gently(c, url, 0) < 0) {

402 + warning(_("skipping credential lookup for url: %s"), url);

403 + credential_clear(c);

404 + }

405 }

credential.h

+16

@@ -28,7 +28,23 @@ void credential_reject(struct credential *);

29 int credential_read(struct credential *, FILE *);

30 void credential_write(const struct credential *, FILE *);

31 +

32 +/*

33 + * Parse a url into a credential struct, replacing any existing contents.

34 + *

35 + * Ifthe url can't be parsed (e.g., a missing "proto://" component), the

36 + * resulting credential will be empty but we'll still return success from the

37 + * "gently" form.

38 + *

39 + * If we encounter a component which cannot be represented as a credential

40 + * value (e.g., because it contains a newline), the "gently" form will return

41 + * an error but leave the broken state in the credential object for further

42 + * examination. The non-gentle form will issue a warning to stderr and return

43 + * an empty credential.

44 + */

45 void credential_from_url(struct credential *, const char *url);

46 +int credential_from_url_gently(struct credential *, const char *url, int quiet);

47 +

48 int credential_match(const struct credential *have,

49 const struct credential *want);

t/t0300-credentials.sh

+10 -2

@@ -309,9 +309,17 @@ test_expect_success 'empty helper spec resets helper list' '

309 EOF

310 '

311

312 -test_expect_success 'url parser rejects embedded newlines' '

313 - test_must_fail git credential fill <<-\EOF

312 +test_expect_success 'url parser ignores embedded newlines' '

313 + check fill <<-EOF

314 url=https://one.example.com?%0ahost=two.example.com/

315 + --

316 + username=askpass-username

317 + password=askpass-password

318 + --

319 + warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/

320 + warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/

321 + askpass: Username:

322 + askpass: Password:

323 EOF

324 '

325