Subject: [PATCH] urlmatch: do not allow passwords in URLs by default

From: "Derrick Stolee via GitGitGadget" <[email-masked]>
Date: Fri, 30 Apr 2021 18:37:24 +0000

From: Derrick Stolee <[email-masked]>

Git allows URLs of the following pattern:

  https://username:password@domain/route

These URLs are then parsed to pull out the username and password for use
when authenticating with the URL. Git is careful to anonymize the URL in
status messages with transport_anonymize_url(), but it stores the URL as
plaintext in the .git/config file. The password may leak in other ways.

This is not a recommended way to store credentials, especially
authentication tokens that do more than simply allow access to a
repository.

Users should be aware that when they provide a URL in this form that
they are not being incredibly secure. It is much better to use a
credential manager to securely store passwords. Even better, some
credential managers use more sophisticated authentication strategies
including multi-factor authentication. This does not stop users from
continuing to do this.

Some Git hosting providers are working to completely drop
username/password credential strategies, which will make URLs of this
form stop working. However, that requires certain changes to credential
managers that need to be released and sufficiently adopted before making
such a server-side change.

In the meantime, it might be helpful to alert users that they are doing
something insecure with these URLs.

Create a new config option, core.allowUsernamePasswordUrls, which is
disabled by default. If Git attempts to parse a password from a URL in
this form, it will die() if this config is not enabled. This affects a
few test scripts, but enabling the config in those places is relatively
simple.

This will cause a significant change in behavior for users who rely upon
this username:password pattern. The error message describes the config
that they must enable to continue working with these URLs. This has a
significant chance of breaking some automated workflows that use URLs in
this fashion, but even those workflows would be better off using a
different mechanism for credentials.

I cannot understate the care in which we should consider this change.
The impact of this change in a Git release could be significant. We
should advertise this very clearly in the release notes.

Signed-off-by: Derrick Stolee <[email-masked]>
---
    Reject passwords in URLs
    
    I received multiple messages "alerting" me to the issue of users
    supplying server-side tokens into the username:password field of a URL.
    This is not a secure way to handle these tokens.
    
    On the one hand, this is user error: Users should not supply a token to
    a location where they do not know what will happen to it. In Git's
    defense, its behavior is completely open about storing the URL in the
    .git/config file as a plain-text string and users should know that when
    using this feature.
    
    However, users just. keep. doing it.
    
    There is some expectation that since this portion of the URL is a
    password, then Git is responsible for tracking that password securely.
    I'm not sure we should venture down that road, since we already have a
    pretty good solution by using the credential helper interface.
    
    Here is my best effort to find a compromise here: start failing when
    parsing a password from a URL like this, with a config option to
    re-enable the existing behavior.
    
    I completely understand if this is too much of a breaking change. I
    wonder if there is anything we can do to assist users into being more
    careful with their secrets.
    
    Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-945%2Fderrickstolee%2Freject-passwords-in-urls-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-945/derrickstolee/reject-passwords-in-urls-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/945

 Documentation/config/core.txt     |  7 +++++++
 t/t0110-urlmatch-normalization.sh |  4 ++++
 t/t5541-http-push-smart.sh        |  9 +++++++--
 t/t5550-http-fetch-dumb.sh        |  3 ++-
 t/t5601-clone.sh                  |  5 +++++
 urlmatch.c                        | 14 ++++++++++++++
 6 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index c04f62a54a15..807dc30e7321 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -628,3 +628,10 @@ core.abbrev::
 	If set to "no", no abbreviation is made and the object names
 	are shown in their full length.
 	The minimum length is 4.
+
+core.allowUsernamePasswordUrls::
+	If enabled, allow parsing URLs that contain plain-text usernames
+	and passwords using `username:password@<url>` text. Defaults to
+	`false`, and will cause Git to fail when parsing such a URL.
+	*WARNING:* Storing passwords and tokens in plaintext is insecure
+	and should be avoided if at all possible.
diff --git a/t/t0110-urlmatch-normalization.sh b/t/t0110-urlmatch-normalization.sh
index f99529d83853..66352775497a 100755
--- a/t/t0110-urlmatch-normalization.sh
+++ b/t/t0110-urlmatch-normalization.sh
@@ -6,6 +6,10 @@ test_description='urlmatch URL normalization'
 # The base name of the test url files
 tu="$TEST_DIRECTORY/t0110/url"
 
+test_expect_success 'enable username:password urls' '
+	git config --global core.allowUsernamePasswordUrls true
+'
+
 # Note that only file: URLs should be allowed without a host
 
 test_expect_success 'url scheme' '
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index c024fa281831..3ffc367bae43 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -460,6 +460,7 @@ test_expect_success GPG 'push with post-receive to inspect certificate' '
 
 test_expect_success 'push status output scrubs password' '
 	cd "$ROOT_PATH/test_repo_clone" &&
+	git config core.allowUsernamePasswordUrls true &&
 	git push --porcelain \
 		"$HTTPD_URL_USER_PASS/smart/test_repo.git" \
 		+HEAD:scrub >status &&
@@ -469,9 +470,11 @@ test_expect_success 'push status output scrubs password' '
 
 test_expect_success 'clone/fetch scrubs password from reflogs' '
 	cd "$ROOT_PATH" &&
-	git clone "$HTTPD_URL_USER_PASS/smart/test_repo.git" \
+	git -c core.allowUsernamePasswordUrls=true clone \
+		"$HTTPD_URL_USER_PASS/smart/test_repo.git" \
 		reflog-test &&
 	cd reflog-test &&
+	git config core.allowUsernamePasswordUrls true &&
 	test_commit prepare-for-force-fetch &&
 	git switch -c away &&
 	git fetch "$HTTPD_URL_USER_PASS/smart/test_repo.git" \
@@ -484,8 +487,10 @@ test_expect_success 'clone/fetch scrubs password from reflogs' '
 
 test_expect_success 'Non-ASCII branch name can be used with --force-with-lease' '
 	cd "$ROOT_PATH" &&
-	git clone "$HTTPD_URL_USER_PASS/smart/test_repo.git" non-ascii &&
+	git -c core.allowUsernamePasswordUrls=true clone \
+		"$HTTPD_URL_USER_PASS/smart/test_repo.git" non-ascii &&
 	cd non-ascii &&
+	git config core.allowUsernamePasswordUrls true &&
 	git checkout -b rama-de-árbol &&
 	test_commit F &&
 	git push --force-with-lease origin rama-de-árbol &&
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 6d9142afc3b2..342538e85e60 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -81,7 +81,8 @@ test_expect_success 'cloning password-protected repository can fail' '
 
 test_expect_success 'http auth can use user/pass in URL' '
 	set_askpass wrong &&
-	git clone "$HTTPD_URL_USER_PASS/auth/dumb/repo.git" clone-auth-none &&
+	git -c core.allowUsernamePasswordUrls=true clone \
+		"$HTTPD_URL_USER_PASS/auth/dumb/repo.git" clone-auth-none &&
 	expect_askpass none
 '
 
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 329ae599fd3c..fd7cabbafa53 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -71,6 +71,11 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
 
 '
 
+test_expect_success 'clone fails when using username:password' '
+	test_must_fail git clone https://username:[email-masked] 2>err &&
+	test_i18ngrep "attempted to parse a URL with a plain-text username and password" err
+'
+
 test_expect_success 'clone from hooks' '
 
 	test_create_repo r0 &&
diff --git a/urlmatch.c b/urlmatch.c
index 33a2ccd306b6..e81ec9e1fc0b 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "urlmatch.h"
+#include "config.h"
 
 #define URL_ALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
 #define URL_DIGIT "0123456789"
@@ -106,6 +107,18 @@ static int match_host(const struct url_info *url_info,
 	return (!url_len && !pat_len);
 }
 
+static void die_if_username_password_not_allowed(void)
+{
+	int opt_in = 0;
+	if (!git_config_get_bool("core.allowusernamepasswordurls", &opt_in) &&
+	    opt_in)
+		return;
+
+	die(_("attempted to parse a URL with a plain-text username and password! "
+	      "This is insecure! "
+	      "Enable core.allowUsernamePasswordUrls to avoid this error"));
+}
+
 static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)
 {
 	/*
@@ -191,6 +204,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
 			}
 			colon_ptr = strchr(norm.buf + scheme_len + 3, ':');
 			if (colon_ptr) {
+				die_if_username_password_not_allowed();
 				passwd_off = (colon_ptr + 1) - norm.buf;
 				passwd_len = norm.len - passwd_off;
 				user_len = (passwd_off - 1) - (scheme_len + 3);

base-commit: 7e391989789db82983665667013a46eabc6fc570
-- 
gitgitgadget