From 7725b8100ffbbff2750ee4d61a0fcc1f53a086e8 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
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
<username>@<hostname> 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 <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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
@@ -10,6 +10,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.<context>.* below, and
Index: b/credential.c
===================================================================
--- a/credential.c
+++ b/credential.c
@@ -62,6 +62,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;
 }
@@ -155,7 +157,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
@@ -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;
@@ -128,7 +129,10 @@ struct credential {
 	char *path;
 };
 
-#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }
+#define CREDENTIAL_INIT { \
+	.helpers = STRING_LIST_INIT_DUP, \
+	.sanitize_prompt = 1, \
+}
 
 /* Initialize a credential structure, setting all fields to empty. */
 void credential_init(struct credential *);
Index: b/t/t0300-credentials.sh
===================================================================
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -35,6 +35,10 @@ test_expect_success 'setup helper script
 	test -z "$pass" || echo password=$pass
 	EOF
 
+	write_script git-credential-cntrl-in-username <<-\EOF &&
+	printf "username=\\007latrix Lestrange\\n"
+	EOF
+
 	PATH="$PWD:$PATH"
 '
 
@@ -633,4 +637,20 @@ test_expect_success 'credential system r
 	test_i18ncmp expect 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
@@ -352,7 +352,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
 '
 
@@ -364,7 +364,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
 '
 
@@ -394,7 +394,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
@@ -67,13 +67,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' '
@@ -91,14 +91,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' '
@@ -119,7 +119,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' '
@@ -130,7 +130,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' '
@@ -147,7 +147,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
@@ -139,7 +139,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
 '
@@ -157,7 +157,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
 '
@@ -182,7 +182,7 @@ 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 'disable dumb http on server' '
