From b01b9b81d36759cdcd07305e78765199e1bc2060 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 4 Nov 2024 14:48:22 +0100 Subject: [PATCH] credential: disallow Carriage Returns in the protocol by default While Git has documented that the credential protocol is line-based, with newlines as terminators, the exact shape of a newline has not been documented. From Git's perspective, which is firmly rooted in the Linux ecosystem, it is clear that "a newline" means a Line Feed character. However, even Git's credential protocol respects Windows line endings (a Carriage Return character followed by a Line Feed character, "CR/LF") by virtue of using `strbuf_getline()`. There is a third category of line endings that has been used originally by MacOS, and that is respected by the default line readers of .NET and node.js: bare Carriage Returns. Git cannot handle those, and what is worse: Git's remedy against CVE-2020-5260 does not catch when credential helpers are used that interpret bare Carriage Returns as newlines. Git Credential Manager addressed this as CVE-2024-50338, but other credential helpers may still be vulnerable. So let's not only disallow Line Feed characters as part of the values in the credential protocol, but also disallow Carriage Return characters. In the unlikely event that a credential helper relies on Carriage Returns in the protocol, introduce an escape hatch via the `credential.protectProtocol` config setting. This addresses CVE-2024-52006. Signed-off-by: Johannes Schindelin --- Documentation/config/credential.txt | 5 +++++ credential.c | 21 ++++++++++++++------- credential.h | 4 +++- t/t0300-credentials.sh | 16 ++++++++++++++++ 4 files changed, 38 insertions(+), 8 deletions(-) Index: b/Documentation/config/credential.txt =================================================================== --- a/Documentation/config/credential.txt +++ b/Documentation/config/credential.txt @@ -20,6 +20,11 @@ credential.sanitizePrompt:: will be URL-encoded by default). Configure this setting to `false` to override that behavior. +credential.protectProtocol:: + By default, Carriage Return characters are not allowed in the protocol + that is used when Git talks to a credential helper. This setting allows + users to override this default. + credential.username:: If no username is set for a network authentication, use this username by default. See credential..* below, and Index: b/credential.c =================================================================== --- a/credential.c +++ b/credential.c @@ -127,6 +127,8 @@ static int credential_config_callback(co c->use_http_path = git_config_bool(var, value); else if (!strcmp(key, "sanitizeprompt")) c->sanitize_prompt = git_config_bool(var, value); + else if (!strcmp(key, "protectprotocol")) + c->protect_protocol = git_config_bool(var, value); return 0; } @@ -361,7 +363,8 @@ int credential_read(struct credential *c return 0; } -static void credential_write_item(FILE *fp, const char *key, const char *value, +static void credential_write_item(const struct credential *c, + FILE *fp, const char *key, const char *value, int required) { if (!value && required) @@ -370,6 +373,10 @@ static void credential_write_item(FILE * return; if (strchr(value, '\n')) die("credential value for %s contains newline", key); + if (c->protect_protocol && strchr(value, '\r')) + die("credential value for %s contains carriage return\n" + "If this is intended, set `credential.protectProtocol=false`", + key); fprintf(fp, "%s=%s\n", key, value); } @@ -377,34 +384,34 @@ void credential_write(const struct crede enum credential_op_type op_type) { if (credential_has_capability(&c->capa_authtype, op_type)) - credential_write_item(fp, "capability[]", "authtype", 0); + credential_write_item(c, fp, "capability[]", "authtype", 0); if (credential_has_capability(&c->capa_state, op_type)) - credential_write_item(fp, "capability[]", "state", 0); + credential_write_item(c, fp, "capability[]", "state", 0); if (credential_has_capability(&c->capa_authtype, op_type)) { - credential_write_item(fp, "authtype", c->authtype, 0); - credential_write_item(fp, "credential", c->credential, 0); + credential_write_item(c, fp, "authtype", c->authtype, 0); + credential_write_item(c, fp, "credential", c->credential, 0); if (c->ephemeral) - credential_write_item(fp, "ephemeral", "1", 0); + credential_write_item(c, fp, "ephemeral", "1", 0); } - credential_write_item(fp, "protocol", c->protocol, 1); - credential_write_item(fp, "host", c->host, 1); - credential_write_item(fp, "path", c->path, 0); - credential_write_item(fp, "username", c->username, 0); - credential_write_item(fp, "password", c->password, 0); - credential_write_item(fp, "oauth_refresh_token", c->oauth_refresh_token, 0); + credential_write_item(c, fp, "protocol", c->protocol, 1); + credential_write_item(c, fp, "host", c->host, 1); + credential_write_item(c, fp, "path", c->path, 0); + credential_write_item(c, fp, "username", c->username, 0); + credential_write_item(c, fp, "password", c->password, 0); + credential_write_item(c, fp, "oauth_refresh_token", c->oauth_refresh_token, 0); if (c->password_expiry_utc != TIME_MAX) { char *s = xstrfmt("%"PRItime, c->password_expiry_utc); - credential_write_item(fp, "password_expiry_utc", s, 0); + credential_write_item(c, fp, "password_expiry_utc", s, 0); free(s); } for (size_t i = 0; i < c->wwwauth_headers.nr; i++) - credential_write_item(fp, "wwwauth[]", c->wwwauth_headers.v[i], 0); + credential_write_item(c, fp, "wwwauth[]", c->wwwauth_headers.v[i], 0); if (credential_has_capability(&c->capa_state, op_type)) { if (c->multistage) - credential_write_item(fp, "continue", "1", 0); + credential_write_item(c, fp, "continue", "1", 0); for (size_t i = 0; i < c->state_headers_to_send.nr; i++) - credential_write_item(fp, "state[]", c->state_headers_to_send.v[i], 0); + credential_write_item(c, fp, "state[]", c->state_headers_to_send.v[i], 0); } } Index: b/credential.h =================================================================== --- a/credential.h +++ b/credential.h @@ -169,7 +169,8 @@ struct credential { quit:1, use_http_path:1, username_from_proto:1, - sanitize_prompt:1; + sanitize_prompt:1, + protect_protocol:1; struct credential_capability capa_authtype; struct credential_capability capa_state; @@ -197,6 +198,7 @@ struct credential { .state_headers = STRVEC_INIT, \ .state_headers_to_send = STRVEC_INIT, \ .sanitize_prompt = 1, \ + .protect_protocol = 1, \ } /* Initialize a credential structure, setting all fields to empty. */ Index: b/t/t0300-credentials.sh =================================================================== --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -903,6 +903,22 @@ test_expect_success 'url parser rejects test_cmp expect stderr ' +test_expect_success 'url parser rejects embedded carriage returns' ' + test_config credential.helper "!true" && + test_must_fail git credential fill 2>stderr <<-\EOF && + url=https://example%0d.com/ + EOF + cat >expect <<-\EOF && + fatal: credential value for host contains carriage return + If this is intended, set `credential.protectProtocol=false` + EOF + test_cmp expect stderr && + GIT_ASKPASS=true \ + git -c credential.protectProtocol=false credential fill <<-\EOF + url=https://example%0d.com/ + EOF +' + test_expect_success 'host-less URLs are parsed as empty host' ' check fill "verbatim foo bar" <<-\EOF url=cert:///path/to/cert.pem