From: Derrick Stolee <dstolee@xxxxxxxxxxxxx> 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 <dstolee@xxxxxxxxxxxxx> --- 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:password@xxxxxxxxx 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
