From 7725b8100ffbbff2750ee4d61a0fcc1f53a086e8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 30 Oct 2024 13:26:10 +0100 Subject: credential: sanitize the user prompt When asking the user interactively for credentials, we want to avoid misleading them e.g. via control sequences that pretend that the URL targets a trusted host when it does not. While Git learned, over the course of the preceding commits, to disallow URLs containing URL-encoded control characters by default, credential helpers are still allowed to specify values very freely (apart from Line Feed and NUL characters, anything is allowed), and this would allow, say, a username containing control characters to be specified that would then be displayed in the interactive terminal prompt asking the user for the password, potentially sending those control characters directly to the terminal. This is undesirable because control characters can be used to mislead users to divulge secret information to untrusted sites. To prevent such an attack vector, let's add a `git_prompt()` that forces the displayed text to be sanitized, i.e. displaying question marks instead of control characters. Note: While this commit's diff changes a lot of `user@host` strings to `user%40host`, which may look suspicious on the surface, there is a good reason for that: this string specifies a user name, not a @ combination! In the context of t5541, the actual combination looks like this: `user%40@127.0.0.1:5541`. Therefore, these string replacements document a net improvement introduced by this commit, as `user@host@127.0.0.1` could have left readers wondering where the user name ends and where the host name begins. Hinted-at-by: Jeff King Signed-off-by: Johannes Schindelin --- credential.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'credential.h') diff --git a/credential.h b/credential.h index 935b28a70f..0364d436d2 100644 --- a/credential.h +++ b/credential.h @@ -119,7 +119,8 @@ struct credential { configured:1, quit:1, use_http_path:1, - username_from_proto:1; + username_from_proto:1, + sanitize_prompt:1; char *username; char *password; @@ -132,6 +133,7 @@ struct credential { #define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ .password_expiry_utc = TIME_MAX, \ + .sanitize_prompt = 1, \ } /* Initialize a credential structure, setting all fields to empty. */ -- cgit v1.3 From b01b9b81d36759cdcd07305e78765199e1bc2060 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 4 Nov 2024 14:48:22 +0100 Subject: 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(-) (limited to 'credential.h') diff --git a/Documentation/config/credential.txt b/Documentation/config/credential.txt index fd8113d6d4..9cadca7f73 100644 --- 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 diff --git a/credential.c b/credential.c index 1392a54d5c..b76a730901 100644 --- a/credential.c +++ b/credential.c @@ -69,6 +69,8 @@ static int credential_config_callback(const char *var, const char *value, 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; } @@ -262,7 +264,8 @@ int credential_read(struct credential *c, FILE *fp) 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) @@ -271,19 +274,23 @@ static void credential_write_item(FILE *fp, const char *key, const char *value, 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); } void credential_write(const struct credential *c, FILE *fp) { - 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(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); 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); } } diff --git a/credential.h b/credential.h index 0364d436d2..2c0b39a925 100644 --- a/credential.h +++ b/credential.h @@ -120,7 +120,8 @@ struct credential { quit:1, use_http_path:1, username_from_proto:1, - sanitize_prompt:1; + sanitize_prompt:1, + protect_protocol:1; char *username; char *password; @@ -134,6 +135,7 @@ struct credential { .helpers = STRING_LIST_INIT_DUP, \ .password_expiry_utc = TIME_MAX, \ .sanitize_prompt = 1, \ + .protect_protocol = 1, \ } /* Initialize a credential structure, setting all fields to empty. */ diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index b62c70c193..168ae76550 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -720,6 +720,22 @@ test_expect_success 'url parser rejects embedded newlines' ' 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 -- cgit v1.3 From 6c27d22276b754e2214242de7a200b372aa611f6 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 17 Dec 2024 07:43:56 +0100 Subject: credential: stop using `the_repository` Stop using `the_repository` in the "credential" subsystem by passing in a repository when filling, approving or rejecting credentials. Adjust callers accordingly by using `the_repository`. While there may be some callers that have a repository available in their context, this trivial conversion allows for easier verification and bubbles up the use of `the_repository` by one level. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/credential.c | 6 +++--- credential.c | 34 +++++++++++++++++----------------- credential.h | 11 +++++++---- http.c | 24 ++++++++++++------------ imap-send.c | 10 +++++----- remote-curl.c | 4 ++-- 6 files changed, 46 insertions(+), 43 deletions(-) (limited to 'credential.h') diff --git a/builtin/credential.c b/builtin/credential.c index 14c8c6608b..614b195b75 100644 --- a/builtin/credential.c +++ b/builtin/credential.c @@ -32,15 +32,15 @@ int cmd_credential(int argc, die("unable to read credential from stdin"); if (!strcmp(op, "fill")) { - credential_fill(&c, 0); + credential_fill(the_repository, &c, 0); credential_next_state(&c); credential_write(&c, stdout, CREDENTIAL_OP_RESPONSE); } else if (!strcmp(op, "approve")) { credential_set_all_capabilities(&c, CREDENTIAL_OP_HELPER); - credential_approve(&c); + credential_approve(the_repository, &c); } else if (!strcmp(op, "reject")) { credential_set_all_capabilities(&c, CREDENTIAL_OP_HELPER); - credential_reject(&c); + credential_reject(the_repository, &c); } else { usage(usage_msg); } diff --git a/credential.c b/credential.c index a995031c5f..b3f87b5b2f 100644 --- a/credential.c +++ b/credential.c @@ -1,4 +1,3 @@ -#define USE_THE_REPOSITORY_VARIABLE #define DISABLE_SIGN_COMPARE_WARNINGS #include "git-compat-util.h" @@ -166,7 +165,7 @@ static int match_partial_url(const char *url, void *cb) return matches; } -static void credential_apply_config(struct credential *c) +static void credential_apply_config(struct repository *r, struct credential *c) { char *normalized_url; struct urlmatch_config config = URLMATCH_CONFIG_INIT; @@ -191,7 +190,7 @@ static void credential_apply_config(struct credential *c) credential_format(c, &url); normalized_url = url_normalize(url.buf, &config.url); - git_config(urlmatch_config_entry, &config); + repo_config(r, urlmatch_config_entry, &config); string_list_clear(&config.vars, 1); free(normalized_url); urlmatch_config_release(&config); @@ -254,34 +253,34 @@ static char *credential_ask_one(const char *what, struct credential *c, return xstrdup(r); } -static int credential_getpass(struct credential *c) +static int credential_getpass(struct repository *r, struct credential *c) { int interactive; char *value; - if (!git_config_get_maybe_bool("credential.interactive", &interactive) && + if (!repo_config_get_maybe_bool(r, "credential.interactive", &interactive) && !interactive) { - trace2_data_intmax("credential", the_repository, + trace2_data_intmax("credential", r, "interactive/skipped", 1); return -1; } - if (!git_config_get_string("credential.interactive", &value)) { + if (!repo_config_get_string(r, "credential.interactive", &value)) { int same = !strcmp(value, "never"); free(value); if (same) { - trace2_data_intmax("credential", the_repository, + trace2_data_intmax("credential", r, "interactive/skipped", 1); return -1; } } - trace2_region_enter("credential", "interactive", the_repository); + trace2_region_enter("credential", "interactive", r); if (!c->username) c->username = credential_ask_one("Username", c, PROMPT_ASKPASS|PROMPT_ECHO); if (!c->password) c->password = credential_ask_one("Password", c, PROMPT_ASKPASS); - trace2_region_leave("credential", "interactive", the_repository); + trace2_region_leave("credential", "interactive", r); return 0; } @@ -489,7 +488,8 @@ static int credential_do(struct credential *c, const char *helper, return r; } -void credential_fill(struct credential *c, int all_capabilities) +void credential_fill(struct repository *r, + struct credential *c, int all_capabilities) { int i; @@ -499,7 +499,7 @@ void credential_fill(struct credential *c, int all_capabilities) credential_next_state(c); c->multistage = 0; - credential_apply_config(c); + credential_apply_config(r, c); if (all_capabilities) credential_set_all_capabilities(c, CREDENTIAL_OP_INITIAL); @@ -526,12 +526,12 @@ void credential_fill(struct credential *c, int all_capabilities) c->helpers.items[i].string); } - if (credential_getpass(c) || + if (credential_getpass(r, c) || (!c->username && !c->password && !c->credential)) die("unable to get password from user"); } -void credential_approve(struct credential *c) +void credential_approve(struct repository *r, struct credential *c) { int i; @@ -542,20 +542,20 @@ void credential_approve(struct credential *c) credential_next_state(c); - credential_apply_config(c); + credential_apply_config(r, c); for (i = 0; i < c->helpers.nr; i++) credential_do(c, c->helpers.items[i].string, "store"); c->approved = 1; } -void credential_reject(struct credential *c) +void credential_reject(struct repository *r, struct credential *c) { int i; credential_next_state(c); - credential_apply_config(c); + credential_apply_config(r, c); for (i = 0; i < c->helpers.nr; i++) credential_do(c, c->helpers.items[i].string, "erase"); diff --git a/credential.h b/credential.h index 5f9e6ff2ef..1e6b0dc5b0 100644 --- a/credential.h +++ b/credential.h @@ -4,6 +4,8 @@ #include "string-list.h" #include "strvec.h" +struct repository; + /** * The credentials API provides an abstracted way of gathering * authentication credentials from the user. @@ -65,7 +67,7 @@ * // Fill in the username and password fields by contacting * // helpers and/or asking the user. The function will die if it * // fails. - * credential_fill(&c); + * credential_fill(repo, &c); * * // Otherwise, we have a username and password. Try to use it. * @@ -218,7 +220,8 @@ void credential_clear(struct credential *); * If all_capabilities is set, this is an internal user that is prepared * to deal with all known capabilities, and we should advertise that fact. */ -void credential_fill(struct credential *, int all_capabilities); +void credential_fill(struct repository *, struct credential *, + int all_capabilities); /** * Inform the credential subsystem that the provided credentials @@ -227,7 +230,7 @@ void credential_fill(struct credential *, int all_capabilities); * that they may store the result to be used again. Any errors * from helpers are ignored. */ -void credential_approve(struct credential *); +void credential_approve(struct repository *, struct credential *); /** * Inform the credential subsystem that the provided credentials @@ -239,7 +242,7 @@ void credential_approve(struct credential *); * for another call to `credential_fill`). Any errors from helpers * are ignored. */ -void credential_reject(struct credential *); +void credential_reject(struct repository *, struct credential *); /** * Enable all of the supported credential flags in this credential. diff --git a/http.c b/http.c index c8fc15aa11..f08b2ae474 100644 --- a/http.c +++ b/http.c @@ -609,7 +609,7 @@ static void init_curl_http_auth(CURL *result) } } - credential_fill(&http_auth, 1); + credential_fill(the_repository, &http_auth, 1); if (http_auth.password) { if (always_auth_proactively()) { @@ -652,7 +652,7 @@ static void init_curl_proxy_auth(CURL *result) { if (proxy_auth.username) { if (!proxy_auth.password && !proxy_auth.credential) - credential_fill(&proxy_auth, 1); + credential_fill(the_repository, &proxy_auth, 1); set_proxyauth_name_password(result); } @@ -686,7 +686,7 @@ static int has_cert_password(void) cert_auth.host = xstrdup(""); cert_auth.username = xstrdup(""); cert_auth.path = xstrdup(ssl_cert); - credential_fill(&cert_auth, 0); + credential_fill(the_repository, &cert_auth, 0); } return 1; } @@ -700,7 +700,7 @@ static int has_proxy_cert_password(void) proxy_cert_auth.host = xstrdup(""); proxy_cert_auth.username = xstrdup(""); proxy_cert_auth.path = xstrdup(http_proxy_ssl_cert); - credential_fill(&proxy_cert_auth, 0); + credential_fill(the_repository, &proxy_cert_auth, 0); } return 1; } @@ -1784,9 +1784,9 @@ static int handle_curl_result(struct slot_results *results) curl_errorstr, sizeof(curl_errorstr)); if (results->curl_result == CURLE_OK) { - credential_approve(&http_auth); - credential_approve(&proxy_auth); - credential_approve(&cert_auth); + credential_approve(the_repository, &http_auth); + credential_approve(the_repository, &proxy_auth); + credential_approve(the_repository, &cert_auth); return HTTP_OK; } else if (results->curl_result == CURLE_SSL_CERTPROBLEM) { /* @@ -1795,7 +1795,7 @@ static int handle_curl_result(struct slot_results *results) * with the certificate. So we reject the credential to * avoid caching or saving a bad password. */ - credential_reject(&cert_auth); + credential_reject(the_repository, &cert_auth); return HTTP_NOAUTH; } else if (results->curl_result == CURLE_SSL_PINNEDPUBKEYNOTMATCH) { return HTTP_NOMATCHPUBLICKEY; @@ -1808,7 +1808,7 @@ static int handle_curl_result(struct slot_results *results) credential_clear_secrets(&http_auth); return HTTP_REAUTH; } - credential_reject(&http_auth); + credential_reject(the_repository, &http_auth); if (always_auth_proactively()) http_proactive_auth = PROACTIVE_AUTH_NONE; return HTTP_NOAUTH; @@ -1822,7 +1822,7 @@ static int handle_curl_result(struct slot_results *results) } } else { if (results->http_connectcode == 407) - credential_reject(&proxy_auth); + credential_reject(the_repository, &proxy_auth); if (!curl_errorstr[0]) strlcpy(curl_errorstr, curl_easy_strerror(results->curl_result), @@ -2210,7 +2210,7 @@ static int http_request_reauth(const char *url, int ret; if (always_auth_proactively()) - credential_fill(&http_auth, 1); + credential_fill(the_repository, &http_auth, 1); ret = http_request(url, result, target, options); @@ -2251,7 +2251,7 @@ static int http_request_reauth(const char *url, BUG("Unknown http_request target"); } - credential_fill(&http_auth, 1); + credential_fill(the_repository, &http_auth, 1); ret = http_request(url, result, target, options); } diff --git a/imap-send.c b/imap-send.c index 68ab1aea83..6c8f84e836 100644 --- a/imap-send.c +++ b/imap-send.c @@ -922,7 +922,7 @@ static void server_fill_credential(struct imap_server_conf *srvc, struct credent cred->username = xstrdup_or_null(srvc->user); cred->password = xstrdup_or_null(srvc->pass); - credential_fill(cred, 1); + credential_fill(the_repository, cred, 1); if (!srvc->user) srvc->user = xstrdup(cred->username); @@ -1123,7 +1123,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c } /* !preauth */ if (cred.username) - credential_approve(&cred); + credential_approve(the_repository, &cred); credential_clear(&cred); /* check the target mailbox exists */ @@ -1150,7 +1150,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c bail: if (cred.username) - credential_reject(&cred); + credential_reject(the_repository, &cred); credential_clear(&cred); out: @@ -1492,9 +1492,9 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server, if (cred.username) { if (res == CURLE_OK) - credential_approve(&cred); + credential_approve(the_repository, &cred); else if (res == CURLE_LOGIN_DENIED) - credential_reject(&cred); + credential_reject(the_repository, &cred); } credential_clear(&cred); diff --git a/remote-curl.c b/remote-curl.c index a24e3a8b9a..1273507a96 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -942,7 +942,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece do { err = probe_rpc(rpc, &results); if (err == HTTP_REAUTH) - credential_fill(&http_auth, 0); + credential_fill(the_repository, &http_auth, 0); } while (err == HTTP_REAUTH); if (err != HTTP_OK) return -1; @@ -1064,7 +1064,7 @@ retry: rpc->any_written = 0; err = run_slot(slot, NULL); if (err == HTTP_REAUTH && !large_request) { - credential_fill(&http_auth, 0); + credential_fill(the_repository, &http_auth, 0); curl_slist_free_all(headers); goto retry; } -- cgit v1.3