From 7725b8100ffbbff2750ee4d61a0fcc1f53a086e8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 30 Oct 2024 13:26:10 +0100 Subject: [PATCH 2/2] 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 --- Documentation/config/credential.txt | 6 ++++++ credential.c | 7 ++++++- credential.h | 4 +++- t/t0300-credentials.sh | 20 ++++++++++++++++++++ t/t5541-http-push-smart.sh | 6 +++--- t/t5550-http-fetch-dumb.sh | 14 +++++++------- t/t5551-http-fetch-smart.sh | 16 ++++++++-------- 7 files changed, 53 insertions(+), 20 deletions(-) Index: b/Documentation/config/credential.txt =================================================================== --- a/Documentation/config/credential.txt +++ b/Documentation/config/credential.txt @@ -14,6 +14,12 @@ credential.useHttpPath:: or https URL to be important. Defaults to false. See linkgit:gitcredentials[7] for more information. +credential.sanitizePrompt:: + By default, user names and hosts that are shown as part of the + password prompt are not allowed to contain control characters (they + will be URL-encoded by default). Configure this setting to `false` to + override that behavior. + 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 @@ -125,6 +125,8 @@ static int credential_config_callback(co } else if (!strcmp(key, "usehttppath")) c->use_http_path = git_config_bool(var, value); + else if (!strcmp(key, "sanitizeprompt")) + c->sanitize_prompt = git_config_bool(var, value); return 0; } @@ -237,7 +239,10 @@ static char *credential_ask_one(const ch struct strbuf prompt = STRBUF_INIT; char *r; - credential_describe(c, &desc); + if (c->sanitize_prompt) + credential_format(c, &desc); + else + credential_describe(c, &desc); if (desc.len) strbuf_addf(&prompt, "%s for '%s': ", what, desc.buf); else Index: b/credential.h =================================================================== --- a/credential.h +++ b/credential.h @@ -168,7 +168,8 @@ struct credential { multistage: 1, quit:1, use_http_path:1, - username_from_proto:1; + username_from_proto:1, + sanitize_prompt:1; struct credential_capability capa_authtype; struct credential_capability capa_state; @@ -195,6 +196,7 @@ struct credential { .wwwauth_headers = STRVEC_INIT, \ .state_headers = STRVEC_INIT, \ .state_headers_to_send = STRVEC_INIT, \ + .sanitize_prompt = 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 @@ -77,6 +77,10 @@ test_expect_success 'setup helper script test -z "$pexpiry" || echo password_expiry_utc=$pexpiry EOF + write_script git-credential-cntrl-in-username <<-\EOF && + printf "username=\\007latrix Lestrange\\n" + EOF + PATH="$PWD:$PATH" ' @@ -1008,4 +1012,20 @@ test_expect_success 'credential config w test_grep "skipping credential lookup for key" stderr ' +BEL="$(printf '\007')" + +test_expect_success 'interactive prompt is sanitized' ' + check fill cntrl-in-username <<-EOF + protocol=https + host=example.org + -- + protocol=https + host=example.org + username=${BEL}latrix Lestrange + password=askpass-password + -- + askpass: Password for ${SQ}https://%07latrix%20Lestrange@example.org${SQ}: + EOF +' + test_done Index: b/t/t5541-http-push-smart.sh =================================================================== --- a/t/t5541-http-push-smart.sh +++ b/t/t5541-http-push-smart.sh @@ -343,7 +343,7 @@ test_expect_success 'push over smart htt git push "$HTTPD_URL"/auth/smart/test_repo.git && git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \ log -1 --format=%s >actual && - expect_askpass both user@host && + expect_askpass both user%40host && test_cmp expect actual ' @@ -355,7 +355,7 @@ test_expect_success 'push to auth-only-f git push "$HTTPD_URL"/auth-push/smart/test_repo.git && git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \ log -1 --format=%s >actual && - expect_askpass both user@host && + expect_askpass both user%40host && test_cmp expect actual ' @@ -385,7 +385,7 @@ test_expect_success 'push into half-auth git push "$HTTPD_URL/half-auth-complete/smart/half-auth.git" && git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/half-auth.git" \ log -1 --format=%s >actual && - expect_askpass both user@host && + expect_askpass both user%40host && test_cmp expect actual ' Index: b/t/t5550-http-fetch-dumb.sh =================================================================== --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -111,13 +111,13 @@ test_expect_success 'http auth can use u test_expect_success 'http auth can use just user in URL' ' set_askpass wrong pass@host && git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-pass && - expect_askpass pass user@host + expect_askpass pass user%40host ' test_expect_success 'http auth can request both user and pass' ' set_askpass user@host pass@host && git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-both && - expect_askpass both user@host + expect_askpass both user%40host ' test_expect_success 'http auth respects credential helper config' ' @@ -135,14 +135,14 @@ test_expect_success 'http auth can get u test_config_global "credential.$HTTPD_URL.username" user@host && set_askpass wrong pass@host && git clone "$HTTPD_URL/auth/dumb/repo.git" clone-auth-user && - expect_askpass pass user@host + expect_askpass pass user%40host ' test_expect_success 'configured username does not override URL' ' test_config_global "credential.$HTTPD_URL.username" wrong && set_askpass wrong pass@host && git clone "$HTTPD_URL_USER/auth/dumb/repo.git" clone-auth-user2 && - expect_askpass pass user@host + expect_askpass pass user%40host ' test_expect_success 'set up repo with http submodules' ' @@ -163,7 +163,7 @@ test_expect_success 'cmdline credential set_askpass wrong pass@host && git -c "credential.$HTTPD_URL.username=user@host" \ clone --recursive super super-clone && - expect_askpass pass user@host + expect_askpass pass user%40host ' test_expect_success 'cmdline credential config passes submodule via fetch' ' @@ -174,7 +174,7 @@ test_expect_success 'cmdline credential git -C super-clone \ -c "credential.$HTTPD_URL.username=user@host" \ fetch --recurse-submodules && - expect_askpass pass user@host + expect_askpass pass user%40host ' test_expect_success 'cmdline credential config passes submodule update' ' @@ -191,7 +191,7 @@ test_expect_success 'cmdline credential git -C super-clone \ -c "credential.$HTTPD_URL.username=user@host" \ submodule update && - expect_askpass pass user@host + expect_askpass pass user%40host ' test_expect_success 'fetch changes via http' ' Index: b/t/t5551-http-fetch-smart.sh =================================================================== --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -181,7 +181,7 @@ test_expect_success 'clone from password echo two >expect && set_askpass user@host pass@host && git clone --bare "$HTTPD_URL/auth/smart/repo.git" smart-auth && - expect_askpass both user@host && + expect_askpass both user%40host && git --git-dir=smart-auth log -1 --format=%s >actual && test_cmp expect actual ' @@ -199,7 +199,7 @@ test_expect_success 'clone from auth-onl echo two >expect && set_askpass user@host pass@host && git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth && - expect_askpass both user@host && + expect_askpass both user%40host && git --git-dir=half-auth log -1 --format=%s >actual && test_cmp expect actual ' @@ -224,14 +224,14 @@ test_expect_success 'redirects send auth set_askpass user@host pass@host && git -c credential.useHttpPath=true \ clone $HTTPD_URL/smart-redir-auth/repo.git repo-redir-auth && - expect_askpass both user@host auth/smart/repo.git + expect_askpass both user%40host auth/smart/repo.git ' test_expect_success 'GIT_TRACE_CURL redacts auth details' ' rm -rf redact-auth trace && set_askpass user@host pass@host && GIT_TRACE_CURL="$(pwd)/trace" git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth && - expect_askpass both user@host && + expect_askpass both user%40host && # Ensure that there is no "Basic" followed by a base64 string, but that # the auth details are redacted @@ -243,7 +243,7 @@ test_expect_success 'GIT_CURL_VERBOSE re rm -rf redact-auth trace && set_askpass user@host pass@host && GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth 2>trace && - expect_askpass both user@host && + expect_askpass both user%40host && # Ensure that there is no "Basic" followed by a base64 string, but that # the auth details are redacted @@ -256,7 +256,7 @@ test_expect_success 'GIT_TRACE_CURL does set_askpass user@host pass@host && GIT_TRACE_REDACT=0 GIT_TRACE_CURL="$(pwd)/trace" \ git clone --bare "$HTTPD_URL/auth/smart/repo.git" redact-auth && - expect_askpass both user@host && + expect_askpass both user%40host && grep -i "Authorization: Basic [0-9a-zA-Z+/]" trace ' @@ -570,7 +570,7 @@ test_expect_success 'http auth remembers # the first request prompts the user... set_askpass user@host pass@host && git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null && - expect_askpass both user@host && + expect_askpass both user%40host && # ...and the second one uses the stored value rather than # prompting the user. @@ -601,7 +601,7 @@ test_expect_success 'http auth forgets b # us to prompt the user again. set_askpass user@host pass@host && git ls-remote "$HTTPD_URL/auth/smart/repo.git" >/dev/null && - expect_askpass both user@host + expect_askpass both user%40host ' test_expect_success 'client falls back from v2 to v0 to match server' '