git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Martin Langhoff" <martin.langhoff@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>
Subject: [PATCH 2/3] clone: avoid storing URL passwords in config
Date: Sun, 19 May 2019 01:12:19 -0400	[thread overview]
Message-ID: <20190519051218.GB19434@sigill.intra.peff.net> (raw)
In-Reply-To: <20190519050724.GA26179@sigill.intra.peff.net>

If you clone a URL with a password, like:

  git clone https://user:pass@example.com/repo.git

we'll write that literal URL, including the password into .git/config.
This can lead to accidentally disclosing it, since .git/config isn't
generally assumed to be a secret. In particular, it's very prone to
accidentally exposing by a webserver:

  1. It's actually _in_ the repo directory, and it's not uncommon for
     people to clone (or copy) the contents to a web-accessible
     directory.

  2. The filesystem permissions are not restrictive. So cloning as user
     "bob" would usually produce a config file readable by user "httpd".

Let's strip the password out before storing it. There are two things to
note:

  - we must (and do) retain the username here. Both as a convenience so
    the user does not have to input it again, but also because
    credential helpers use it as part of the lookup key (which matters
    if you use two different usernames with the same host).

  - since we don't record the password anywhere, a follow-up fetch will
    fail. This is a regression for some workflows, of course, but one
    we'll fix in a future commit. For now we'll warn the user about
    what's happening and add a failing test to show the problem.

Idea-stolen-from: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
---
If we stop here, we probably ought to be pointing users at credential
helpers in the advice. I didn't bother here, because the next commit
ends up rewriting most of this advice anyway.

 builtin/clone.c            | 24 ++++++++++++++++++++++--
 t/t5550-http-fetch-dumb.sh | 12 ++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index ffdd94e8f6..15fffc3268 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -891,10 +891,18 @@ static int dir_exists(const char *path)
 	return !stat(path, &sb);
 }
 
+static const char sanitized_url_advice[] = N_(
+"The URL you provided to Git contains a password. It will be\n"
+"used to clone the repository, but to avoid accidental disclosure\n"
+"the password will not be recorded. Further fetches from the remote\n"
+"may require you to provide the password interactively.\n"
+);
+
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
 	const char *repo_name, *repo, *work_tree, *git_dir;
+	char *sanitized_repo;
 	char *path, *dir;
 	int dest_exists;
 	const struct ref *refs, *remote_head;
@@ -1079,9 +1087,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
 	}
 
+	sanitized_repo = transport_strip_url(repo, 0);
+	if (strcmp(repo, sanitized_repo)) {
+		warning(_("omitting password while storing URL in on-disk config"));
+		advise(_(sanitized_url_advice));
+	}
 	strbuf_addf(&key, "remote.%s.url", option_origin);
-	git_config_set(key.buf, repo);
+	git_config_set(key.buf, sanitized_repo);
 	strbuf_reset(&key);
+	FREE_AND_NULL(sanitized_repo);
 
 	if (option_no_tags) {
 		strbuf_addf(&key, "remote.%s.tagOpt", option_origin);
@@ -1098,7 +1112,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		    branch_top.buf);
 	refspec_append(&remote->fetch, default_refspec.buf);
 
-	transport = transport_get(remote, remote->url[0]);
+	/*
+	 * Override remote->url here, since that will be the sanitized version
+	 * we wrote to the config. If there was a password, we need to use the
+	 * version that has it (and if there isn't, the two are identical
+	 * anyway).
+	 */
+	transport = transport_get(remote, repo);
 	transport_set_verbosity(transport, option_verbosity, option_progress);
 	transport->family = family;
 
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index b811d89cfd..d8c85f3126 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -64,6 +64,18 @@ test_expect_success 'http auth can use user/pass in URL' '
 	expect_askpass none
 '
 
+test_expect_success 'username is retained in URL, password is not' '
+	git -C clone-auth-none config remote.origin.url >url &&
+	grep user url &&
+	! grep pass url
+'
+
+test_expect_failure 'fetch of password-URL clone uses stored auth' '
+	set_askpass wrong &&
+	git -C clone-auth-none fetch &&
+	expect_askpass none
+'
+
 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 &&
-- 
2.22.0.rc0.583.g23d90da2b3


  parent reply	other threads:[~2019-05-19 18:39 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-15 17:49 Git ransom campaign incident report - May 2019 Martin Langhoff
2019-05-15 18:59 ` Ævar Arnfjörð Bjarmason
2019-05-16  4:27   ` Jeff King
2019-05-17 19:39     ` Johannes Schindelin
2019-05-17 22:20       ` Jeff King
2019-05-17 23:13         ` Martin Langhoff
2019-05-19  5:07         ` Jeff King
2019-05-19  5:10           ` [PATCH 1/3] transport_anonymize_url(): support retaining username Jeff King
2019-05-19 23:28             ` Eric Sunshine
2019-05-20 16:14             ` René Scharfe
2019-05-20 16:36             ` Johannes Schindelin
2019-05-20 16:43             ` Johannes Schindelin
2019-05-19  5:12           ` Jeff King [this message]
2019-05-19  5:16           ` [PATCH 3/3] clone: auto-enable git-credential-store when necessary Jeff King
2019-05-20 11:28             ` Eric Sunshine
2019-05-20 12:31               ` Jeff King
2019-05-20 16:48                 ` Johannes Schindelin
2019-05-20 13:56             ` Ævar Arnfjörð Bjarmason
2019-05-20 14:08               ` Jeff King
2019-05-20 15:17                 ` Ævar Arnfjörð Bjarmason
2019-05-20 15:24                   ` Jeff King
2019-05-20 17:08             ` Ævar Arnfjörð Bjarmason
2019-05-20 14:43           ` Git ransom campaign incident report - May 2019 Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190519051218.GB19434@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=martin.langhoff@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).