Merge branch 'disallow-control-characters-in-credential-urls-by-default'

This addresses two vulnerabilities:

- CVE-2024-50349:

	Printing unsanitized URLs when asking for credentials made the
	user susceptible to crafted URLs (e.g. in recursive clones) that
	mislead the user into typing in passwords for trusted sites that
	would then be sent to untrusted sites instead.

- CVE-2024-52006

	Git may pass on Carriage Returns via the credential protocol to
	credential helpers which use line-reading functions that
	interpret said Carriage Returns as line endings, even though Git
	did not intend that.

Signed-off-by: Johannes Schindelin <[email protected]>

Author: dscho

Documentation/config/credential.txt

@@ -14,6 +14,17 @@ 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.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.<context>.* below, and

credential.c

@@ -67,6 +67,10 @@ static int credential_config_callback(const char *var, const char *value,
 	}
 	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);
+	else if (!strcmp(key, "protectprotocol"))
+		c->protect_protocol = git_config_bool(var, value);
 
 	return 0;
 }
@@ -164,7 +168,8 @@ static void credential_format(struct credential *c, struct strbuf *out)
 		strbuf_addch(out, '@');
 	}
 	if (c->host)
-		strbuf_addstr(out, c->host);
+		strbuf_add_percentencode(out, c->host,
+					 STRBUF_ENCODE_HOST_AND_PORT);
 	if (c->path) {
 		strbuf_addch(out, '/');
 		strbuf_add_percentencode(out, c->path, 0);
@@ -178,7 +183,10 @@ static char *credential_ask_one(const char *what, struct credential *c,
 	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
@@ -256,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)
@@ -265,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);
 	}
 }

credential.h

@@ -119,7 +119,9 @@ struct credential {
 		 configured:1,
 		 quit:1,
 		 use_http_path:1,
-		 username_from_proto:1;
+		 username_from_proto:1,
+		 sanitize_prompt:1,
+		 protect_protocol:1;
 
 	char *username;
 	char *password;
@@ -132,6 +134,8 @@ struct credential {
 #define CREDENTIAL_INIT { \
 	.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. */

strbuf.c

@@ -492,7 +492,9 @@ void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)
 		unsigned char ch = src[i];
 		if (ch <= 0x1F || ch >= 0x7F ||
 		    (ch == '/' && (flags & STRBUF_ENCODE_SLASH)) ||
-		    strchr(URL_UNSAFE_CHARS, ch))
+		    ((flags & STRBUF_ENCODE_HOST_AND_PORT) ?
+		     !isalnum(ch) && !strchr("-.:[]", ch) :
+		     !!strchr(URL_UNSAFE_CHARS, ch)))
 			strbuf_addf(dst, "%%%02X", (unsigned char)ch);
 		else
 			strbuf_addch(dst, ch);

strbuf.h

@@ -380,6 +380,7 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,
 void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);
 
 #define STRBUF_ENCODE_SLASH 1
+#define STRBUF_ENCODE_HOST_AND_PORT 2
 
 /**
  * Append the contents of a string to a strbuf, percent-encoding any characters

t/t0300-credentials.sh

@@ -45,6 +45,10 @@ test_expect_success 'setup helper scripts' '
 	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"
 '
 
@@ -514,6 +518,19 @@ test_expect_success 'match percent-encoded values in username' '
 	EOF
 '
 
+test_expect_success 'match percent-encoded values in hostname' '
+	test_config "credential.https://a%20b%20c/.helper" "$HELPER" &&
+	check fill <<-\EOF
+	url=https://a b c/
+	--
+	protocol=https
+	host=a b c
+	username=foo
+	password=bar
+	--
+	EOF
+'
+
 test_expect_success 'fetch with multiple path components' '
 	test_unconfig credential.helper &&
 	test_config credential.https://example.com/foo/repo.git.helper "verbatim foo bar" &&
@@ -703,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
@@ -812,4 +845,20 @@ test_expect_success 'credential config with partial URLs' '
 	test_i18ngrep "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%[email protected]${SQ}:
+	EOF
+'
+
 test_done

t/t5541-http-push-smart.sh

@@ -351,7 +351,7 @@ test_expect_success 'push over smart http with auth' '
 	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
 '
 
@@ -363,7 +363,7 @@ test_expect_success 'push to auth-only-for-push repo' '
 	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
 '
 
@@ -393,7 +393,7 @@ test_expect_success 'push into half-auth-complete requires password' '
 	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
 '
 

t/t5550-http-fetch-dumb.sh

@@ -90,13 +90,13 @@ test_expect_success 'http auth can use user/pass in URL' '
 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' '
@@ -114,14 +114,14 @@ test_expect_success 'http auth can get username from config' '
 	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' '
@@ -142,7 +142,7 @@ test_expect_success 'cmdline credential config passes to submodule via clone' '
 	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' '
@@ -153,7 +153,7 @@ test_expect_success 'cmdline credential config passes submodule via fetch' '
 	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' '
@@ -170,7 +170,7 @@ test_expect_success 'cmdline credential config passes submodule update' '
 	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' '

t/t5551-http-fetch-smart.sh

@@ -181,7 +181,7 @@ test_expect_success 'clone from password-protected repository' '
 	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-only-for-objects repository' '
 	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 to new location' '
 	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 redacts auth details' '
 	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 not redact auth details if GIT_TRACE_RE
 	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
 '
@@ -568,7 +568,7 @@ test_expect_success 'http auth remembers successful credentials' '
 	# 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.
@@ -599,7 +599,7 @@ test_expect_success 'http auth forgets bogus credentials' '
 	# 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' '