Git ransom campaign incident report - May 2019

Git ransom campaign incident report - May 2019

[Martin Langhoff]

Spotted this on the internet...

https://github.blog/2019-05-14-git-ransom-campaign-incident-report/

Haven't hacked on git for a while, and I am not affiliated with any of
the stakeholders. However, reading it, I wanted to slam my head on the
desk.

IIRC, git will sanely store a password elsewhere if it gets to prompt
for it. Should we be trying to unpack usernames/passwords from HTTP
urls, and DTRT with them?

Are there other ways this could be made better?

cheers,


martin
-- 
 martin.langhoff@gmail.com
 - ask interesting questions  ~  http://linkedin.com/in/martinlanghoff
 - don't be distracted        ~  http://github.com/martin-langhoff
   by shiny stuff

Re: Git ransom campaign incident report - May 2019

[Ævar Arnfjörð Bjarmason]

On Wed, May 15 2019, Martin Langhoff wrote:

> Spotted this on the internet...
>
> https://github.blog/2019-05-14-git-ransom-campaign-incident-report/
>
> Haven't hacked on git for a while, and I am not affiliated with any of
> the stakeholders. However, reading it, I wanted to slam my head on the
> desk.
>
> IIRC, git will sanely store a password elsewhere if it gets to prompt
> for it. Should we be trying to unpack usernames/passwords from HTTP
> urls, and DTRT with them?
>
> Are there other ways this could be made better?

I think we should do nothing.

The linked blog post really manages to bury the lead. I guess you'll get
that when PR at three different companies gets a say. For those looking
for a brief summary, here's mine:

    Some people using git hosting sites "git clone"'d https URLs to
    their repos with username/passwords in them. They then pointed a
    webserver at their checked-out directory, and got pwned by someone
    scraping "/.git/config" from public websites looking for
    credentials.

Trying to mitigate this in git is just going to annoy users who are
doing this in the context of an otherwise secure workflow. The users who
were affected by this are probably also the sort of users who are
hardcoding their AWS password in some JavaScript checked into their
project or whatever, there's only so much you can do.

It's probably more productive to say convince whoever maintains the
default nginx/apache etc. docker image to default to some Fisher-Price
mode where dotfiles aren't served up by default.

Or, for GitLab/GitHub etc. to discourage use of https API tokens in
favor of SSH deploy keys. OpenSSH goes out of its way to not allow you
to provide paswords in URLs, on the command-line etc. in anticipation of
exactly this sort of scenario. Even then I've seen users write say
docker images where they manage to hardcode an SSH private key in a
public image out of convenience or lazyness (say needing "git clone"
something during the image build).

Re: Git ransom campaign incident report - May 2019

[Jeff King]

On Wed, May 15, 2019 at 08:59:47PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 
> On Wed, May 15 2019, Martin Langhoff wrote:
> 
> > Spotted this on the internet...
> >
> > https://github.blog/2019-05-14-git-ransom-campaign-incident-report/
> >
> > Haven't hacked on git for a while, and I am not affiliated with any of
> > the stakeholders. However, reading it, I wanted to slam my head on the
> > desk.
> >
> > IIRC, git will sanely store a password elsewhere if it gets to prompt
> > for it. Should we be trying to unpack usernames/passwords from HTTP
> > urls, and DTRT with them?
> >
> > Are there other ways this could be made better?
> 
> I think we should do nothing.

I think so, too.

But just brainstorming, one thing we _could_ do is issue a warning when
we see a password in a URL and say "hey, what you're doing isn't
fantastic; considering using a credential helper".

Of course I suspect there are many cases where people _do_ need to store
the password in plaintext, because an automated system needs to fetch
with it. They can use the plaintext git-credential-store, but it's
slightly more hassle. And it doesn't really _solve_ the problem (though
perhaps it would be harder to accidentally expose it with your web
server!).

> Or, for GitLab/GitHub etc. to discourage use of https API tokens in
> favor of SSH deploy keys. OpenSSH goes out of its way to not allow you
> to provide paswords in URLs, on the command-line etc. in anticipation of
> exactly this sort of scenario. Even then I've seen users write say
> docker images where they manage to hardcode an SSH private key in a
> public image out of convenience or lazyness (say needing "git clone"
> something during the image build).

You can have limited-access https tokens, too. But I don't think those
or deploy keys fix the fundamental problem, because that read access is
sufficient. The ransom is not "give us bitcoin or we will delete your
code, lock you out of your account, etc". Those things can easily be
fixed by complaining to the service provider, who has backups. The
ransom is "give me bitcoin or I will share your code with the world".
So whatever token the server has to do a fetch is enough to steal the
code (though I think deploy keys can be repo-specific, and I'm not sure
if http tokens are; that expands the attack to _all_ of the user's
repos, not just the one the server cares about).

I don't know how many people who git by this actually even want to fetch
to the server, though. They might just have blindly rsync'd up their
local checkout, and the deploy server actually never needed to know
about git in the first place.

-Peff

Re: Git ransom campaign incident report - May 2019

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]

Hi,

On Thu, 16 May 2019, Jeff King wrote:

> On Wed, May 15, 2019 at 08:59:47PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> >
> > On Wed, May 15 2019, Martin Langhoff wrote:
> >
> > > Spotted this on the internet...
> > >
> > > https://github.blog/2019-05-14-git-ransom-campaign-incident-report/
> > >
> > > Haven't hacked on git for a while, and I am not affiliated with any of
> > > the stakeholders. However, reading it, I wanted to slam my head on the
> > > desk.
> > >
> > > IIRC, git will sanely store a password elsewhere if it gets to prompt
> > > for it. Should we be trying to unpack usernames/passwords from HTTP
> > > urls, and DTRT with them?
> > >
> > > Are there other ways this could be made better?
> >
> > I think we should do nothing.
>
> I think so, too.
>
> But just brainstorming, one thing we _could_ do is issue a warning when
> we see a password in a URL and say "hey, what you're doing isn't
> fantastic; considering using a credential helper".
>
> Of course I suspect there are many cases where people _do_ need to store
> the password in plaintext, because an automated system needs to fetch
> with it. They can use the plaintext git-credential-store, but it's
> slightly more hassle. And it doesn't really _solve_ the problem (though
> perhaps it would be harder to accidentally expose it with your web
> server!).

One thing that we actually *could* do here is to anonymize the URLs stored
under remote.origin.url when cloning. In no other circumstance that I can
think of do we take an URL from some command-line parameter that is not
*explicitly* intended for storing in the config.

Combined with that warning "You cloned via a URL that contains
credentials; for security reasons, the credentials were scrubbed before
storing this in your Git config. Please consider using a credential
manager instead of storing secrets in your Git config." this should
provide a reasonable compromise.

Judging from looking at my own automated jobs, it does not appear that you
would *ever* need to store such credentials in the Git config, anyway. If
you need to, say, push to a repository, you can always store the full URL
(or the credentials) in a secret variable.

Ciao,
Dscho

Re: Git ransom campaign incident report - May 2019

[Jeff King]

On Fri, May 17, 2019 at 09:39:55PM +0200, Johannes Schindelin wrote:

> > Of course I suspect there are many cases where people _do_ need to store
> > the password in plaintext, because an automated system needs to fetch
> > with it. They can use the plaintext git-credential-store, but it's
> > slightly more hassle. And it doesn't really _solve_ the problem (though
> > perhaps it would be harder to accidentally expose it with your web
> > server!).
> 
> One thing that we actually *could* do here is to anonymize the URLs stored
> under remote.origin.url when cloning. In no other circumstance that I can
> think of do we take an URL from some command-line parameter that is not
> *explicitly* intended for storing in the config.

Good point. That's a plausible way that these end up in config even when
they're not needed and it wasn't intended by the user.

> Combined with that warning "You cloned via a URL that contains
> credentials; for security reasons, the credentials were scrubbed before
> storing this in your Git config. Please consider using a credential
> manager instead of storing secrets in your Git config." this should
> provide a reasonable compromise.

Yeah, we'd definitely want to warn the user. Some people might find it
irritating because a follow-up fetch would require an interactive
password. We might need to make sure the escape hatch is pretty turnkey.
The simplest thing is just a "--save-password" option to override this,
but it would be nice if there's an out-of-the-box solution with a
credential helper.

We can use credential-store as a general solution. That's still
sticking the password in the filesystem in plaintext, but the file is
0700 by default, and it's not in the repo directory).

Unfortunately converting:

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

into a safer use of credential-store is a little tricky, because seeding
the store is generally only done interactively. What if we did this:

  1. Do not ever write the password part of a URL into config.

  2. When we extract the user/pass out of a URL, put them into the
     credential struct, so that when we successfully authenticate, we
     trigger storage. This _might_ actually happen already, but we
     should definitely confirm it.

  3. If the user has no credential helper defined, then do one of:

     a. Warn them that the credential was not recorded, but that they
        can use "git clone -c credential.helper=store" as a fallback
	(but probably worded in a way to recommend using something
	stronger if possible).

	This is slightly annoying because following the advice requires
	re-cloning. Fixing it up from there is more like:

	  git config credential.helper store
	  git fetch
	  [interactively input password]

     b. Just use credential-store. Optionally notify them of what
         happened (and that they might want to choose a better helper).

I hate the magical-ness of 3b, because credential-store really _isn't_
the best choice. It's just better than the current behavior. At the same
time, by doing it automatically, the existing flow they were using just
works, and is moderately better.

> Judging from looking at my own automated jobs, it does not appear that you
> would *ever* need to store such credentials in the Git config, anyway. If
> you need to, say, push to a repository, you can always store the full URL
> (or the credentials) in a secret variable.

Yes, that's definitely the way you _should_ do it. I think the problem
is that handling secret storage is tricky and system-specific, and the
people who are affected here are ones who just didn't think about it. If
we can make even a moderate improvement without the user having to do
anything differently, it's worth considering.

-Peff

Re: Git ransom campaign incident report - May 2019

[Martin Langhoff]

On Fri, May 17, 2019 at 6:20 PM Jeff King <peff@peff.net> wrote:
> I hate the magical-ness of 3b, because credential-store really _isn't_
> the best choice. It's just better than the current behavior. At the same
> time, by doing it automatically, the existing flow they were using just
> works, and is moderately better.

Quite a bit better. It sits in a different directory, and with tight
permissions.

Overall -- thank you! That's the process I was picturing. Even just
scrubbing the credentials -- your "step 1" -- would be a significant
improvement, if a bit unfriendly.

> > Judging from looking at my own automated jobs, it does not appear that you
> > would *ever* need to store such credentials in the Git config, anyway. If
> > you need to, say, push to a repository, you can always store the full URL
> > (or the credentials) in a secret variable.
>
> Yes, that's definitely the way you _should_ do it. I think the problem

The key thing are the credentials, and there are much better solutions
for this -- ssh keys, etc.

This isn't for thoughtful users, this is to save unaware users from
themselves. Maybe they'll and hurt themselves with something else, but
that's part of removing sharp edges from a product.

cheers,


m




--
 martin.langhoff@gmail.com
 - ask interesting questions  ~  http://linkedin.com/in/martinlanghoff
 - don't be distracted        ~  http://github.com/martin-langhoff
   by shiny stuff

Re: Git ransom campaign incident report - May 2019

[Jeff King]

On Fri, May 17, 2019 at 06:20:31PM -0400, Jeff King wrote:

> What if we did this:
> 
>   1. Do not ever write the password part of a URL into config.
> 
>   2. When we extract the user/pass out of a URL, put them into the
>      credential struct, so that when we successfully authenticate, we
>      trigger storage. This _might_ actually happen already, but we
>      should definitely confirm it.
> 
>   3. If the user has no credential helper defined, then do one of:
> 
>      a. Warn them that the credential was not recorded, but that they
>         can use "git clone -c credential.helper=store" as a fallback
> 	(but probably worded in a way to recommend using something
> 	stronger if possible).
> 
> 	This is slightly annoying because following the advice requires
> 	re-cloning. Fixing it up from there is more like:
> 
> 	  git config credential.helper store
> 	  git fetch
> 	  [interactively input password]
> 
>      b. Just use credential-store. Optionally notify them of what
>          happened (and that they might want to choose a better helper).

So here are patches to do that. Step 2 is indeed how things work
already, so nothing was needed there (there are still 3 patches because
there was a bit of prep-work ;) ).

I did 3b here: automagically enabling credential-store. I'm still on the
fence on whether that's a good idea or not.

I don't have any data on how many victims of this ransom campaign might
have been helped by this. But it certainly seems like a decent best
practice. I'd hope that _most_ people have moved on to using a
credential helper and supplying the initial password interactively these
days; this is really just aimed at people who don't know better. So the
goal is making them a bit more secure, but also educating them about the
other options. Hopefully without breaking any workflows. :)

-Peff

  [1/3]: transport_anonymize_url(): support retaining username
  [2/3]: clone: avoid storing URL passwords in config
  [3/3]: clone: auto-enable git-credential-store when necessary

 builtin/clone.c            | 37 +++++++++++++++++++++++++++++++++++--
 credential.c               | 13 +++++++++++++
 credential.h               |  6 ++++++
 t/t5550-http-fetch-dumb.sh | 12 ++++++++++++
 transport.c                | 21 ++++++++++++++-------
 transport.h                | 11 ++++++++++-
 6 files changed, 90 insertions(+), 10 deletions(-)

[PATCH 1/3] transport_anonymize_url(): support retaining username

[Jeff King]

When we anonymize URLs to show in messages, we strip out both the
username and password (if any). But there are also contexts where we
should strip out the password (to avoid leaking it) but retain the
username.

Let's generalize transport_anonymize_url() to support both cases. We'll
give it a new name since the password-only mode isn't really
"anonymizing", but keep the old name as a synonym to avoid disrupting
existing callers.

Note that there are actually three places we parse URLs, and this
functionality _could_ go into any of them:

  - transport_anonymize_url(), which we modify here

  - the urlmatch.c code parses a URL into its constituent parts, from
    which we could easily remove the elements we want to drop and
    re-format it as a single URL. But its parsing also normalizes
    elements (e.g., downcasing hostnames).  This isn't wrong, but it's
    more friendly if we can leave the rest of the URL untouched.

  - credential_form_url() parses a URL and decodes the specific
    elements, but it's hard to convert it back into a regular URL. It
    treats "host:port" as a single unit, meaning it needs to be
    re-encoded specially (since a colon would otherwise end
    percent-encoded).

Since transport_anonymize_url() seemed closest to what we want here, I
used that as the base.

Signed-off-by: Jeff King <peff@peff.net>
---
I think it would be beneficial to unify these three cases under a single
parser, but it seemed like too big a rabbit hole for this topic. Of the
three, the urlmatch one seems the most mature. I think if we could
simply separate the normalization from the parsing/decoding, the others
could build on top of it. It might also require some careful thinking
about how pseudo-urls like ssh "host:path" interact.

I won't call that a #leftoverbits, because it's more of a feast. :)

 transport.c | 21 ++++++++++++++-------
 transport.h | 11 ++++++++++-
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/transport.c b/transport.c
index f1fcd2c4b0..ba61e57295 100644
--- a/transport.c
+++ b/transport.c
@@ -1335,11 +1335,7 @@ int transport_disconnect(struct transport *transport)
 	return ret;
 }
 
-/*
- * Strip username (and password) from a URL and return
- * it in a newly allocated string.
- */
-char *transport_anonymize_url(const char *url)
+char *transport_strip_url(const char *url, int strip_user)
 {
 	char *scheme_prefix, *anon_part;
 	size_t anon_len, prefix_len = 0;
@@ -1348,7 +1344,10 @@ char *transport_anonymize_url(const char *url)
 	if (url_is_local_not_ssh(url) || !anon_part)
 		goto literal_copy;
 
-	anon_len = strlen(++anon_part);
+	anon_len = strlen(anon_part);
+	if (strip_user)
+		anon_part++;
+
 	scheme_prefix = strstr(url, "://");
 	if (!scheme_prefix) {
 		if (!strchr(anon_part, ':'))
@@ -1373,7 +1372,15 @@ char *transport_anonymize_url(const char *url)
 		cp = strchr(scheme_prefix + 3, '/');
 		if (cp && cp < anon_part)
 			goto literal_copy;
-		prefix_len = scheme_prefix - url + 3;
+
+		if (strip_user)
+			prefix_len = scheme_prefix - url + 3;
+		else {
+			cp = strchr(scheme_prefix + 3, ':');
+			if (cp && cp > anon_part)
+				goto literal_copy; /* username only */
+			prefix_len = cp - url;
+		}
 	}
 	return xstrfmt("%.*s%.*s", (int)prefix_len, url,
 		       (int)anon_len, anon_part);
diff --git a/transport.h b/transport.h
index 06e06d3d89..6d8c99ac91 100644
--- a/transport.h
+++ b/transport.h
@@ -243,10 +243,19 @@ const struct ref *transport_get_remote_refs(struct transport *transport,
 int transport_fetch_refs(struct transport *transport, struct ref *refs);
 void transport_unlock_pack(struct transport *transport);
 int transport_disconnect(struct transport *transport);
-char *transport_anonymize_url(const char *url);
 void transport_take_over(struct transport *transport,
 			 struct child_process *child);
 
+/*
+ * Strip password and optionally username from a URL and return
+ * it in a newly allocated string (even if nothing was stripped).
+ */
+char *transport_strip_url(const char *url, int strip_username);
+static inline char *transport_anonymize_url(const char *url)
+{
+	return transport_strip_url(url, 1);
+}
+
 int transport_connect(struct transport *transport, const char *name,
 		      const char *exec, int fd[2]);
 
-- 
2.22.0.rc0.583.g23d90da2b3

Re: [PATCH 1/3] transport_anonymize_url(): support retaining username

[Eric Sunshine]

On Sun, May 19, 2019 at 2:37 PM Jeff King <peff@peff.net> wrote:
> [...]
>   - credential_form_url() parses a URL and decodes the specific

s/form/from/

>     elements, but it's hard to convert it back into a regular URL. It
>     treats "host:port" as a single unit, meaning it needs to be
>     re-encoded specially (since a colon would otherwise end
>     percent-encoded).
> [...]
> Signed-off-by: Jeff King <peff@peff.net>

Re: [PATCH 1/3] transport_anonymize_url(): support retaining username

[René Scharfe]

Am 19.05.19 um 07:10 schrieb Jeff King:
> diff --git a/transport.c b/transport.c
> index f1fcd2c4b0..ba61e57295 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1373,7 +1372,15 @@ char *transport_anonymize_url(const char *url)
>  		cp = strchr(scheme_prefix + 3, '/');
>  		if (cp && cp < anon_part)
>  			goto literal_copy;
> -		prefix_len = scheme_prefix - url + 3;
> +
> +		if (strip_user)
> +			prefix_len = scheme_prefix - url + 3;
> +		else {
> +			cp = strchr(scheme_prefix + 3, ':');
> +			if (cp && cp > anon_part)

Don't you mean this?

			if (!cp || cp > anon_part)

Or the search could stop at anon_part in the first place:

			assert(scheme_prefix + 3 < anon_part);
			cp = memchr(schema_prefix + 3, ':', anon_part - schema_prefix - 3);
			if (!cp)

That whole thing looks fragile.  I wonder if using the official regex
(https://tools.ietf.org/html/rfc3986#appendix-B) would make it easier
and more robust.

> +				goto literal_copy; /* username only */
> +			prefix_len = cp - url;

Anyway, you don't want cp == NULL here.

> +		}
>  	}
>  	return xstrfmt("%.*s%.*s", (int)prefix_len, url,
>  		       (int)anon_len, anon_part);

Re: [PATCH 1/3] transport_anonymize_url(): support retaining username

[Johannes Schindelin]

Hi Peff,

On Sun, 19 May 2019, Jeff King wrote:

> When we anonymize URLs to show in messages, we strip out both the
> username and password (if any). But there are also contexts where we
> should strip out the password (to avoid leaking it) but retain the
> username.
>
> Let's generalize transport_anonymize_url() to support both cases. We'll
> give it a new name since the password-only mode isn't really
> "anonymizing", but keep the old name as a synonym to avoid disrupting
> existing callers.
>
> Note that there are actually three places we parse URLs, and this
> functionality _could_ go into any of them:
>
>   - transport_anonymize_url(), which we modify here
>
>   - the urlmatch.c code parses a URL into its constituent parts, from
>     which we could easily remove the elements we want to drop and
>     re-format it as a single URL. But its parsing also normalizes
>     elements (e.g., downcasing hostnames).  This isn't wrong, but it's
>     more friendly if we can leave the rest of the URL untouched.

I have not looked into it at all, but I seem to vaguely remember that the
result of this code might be used to look up `url.<url>.insteadOf`
settings, where the middle part *is* case-sensitive.

>   - credential_form_url() parses a URL and decodes the specific
>     elements, but it's hard to convert it back into a regular URL. It
>     treats "host:port" as a single unit, meaning it needs to be
>     re-encoded specially (since a colon would otherwise end
>     percent-encoded).
>
> Since transport_anonymize_url() seemed closest to what we want here, I
> used that as the base.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I think it would be beneficial to unify these three cases under a single
> parser, but it seemed like too big a rabbit hole for this topic. Of the
> three, the urlmatch one seems the most mature. I think if we could
> simply separate the normalization from the parsing/decoding, the others
> could build on top of it. It might also require some careful thinking
> about how pseudo-urls like ssh "host:path" interact.

In light of what I mentioned above, I am not sure that we should go there
in the first place...

Thanks,
Dscho

> I won't call that a #leftoverbits, because it's more of a feast. :)
>
>  transport.c | 21 ++++++++++++++-------
>  transport.h | 11 ++++++++++-
>  2 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index f1fcd2c4b0..ba61e57295 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1335,11 +1335,7 @@ int transport_disconnect(struct transport *transport)
>  	return ret;
>  }
>
> -/*
> - * Strip username (and password) from a URL and return
> - * it in a newly allocated string.
> - */
> -char *transport_anonymize_url(const char *url)
> +char *transport_strip_url(const char *url, int strip_user)
>  {
>  	char *scheme_prefix, *anon_part;
>  	size_t anon_len, prefix_len = 0;
> @@ -1348,7 +1344,10 @@ char *transport_anonymize_url(const char *url)
>  	if (url_is_local_not_ssh(url) || !anon_part)
>  		goto literal_copy;
>
> -	anon_len = strlen(++anon_part);
> +	anon_len = strlen(anon_part);
> +	if (strip_user)
> +		anon_part++;
> +
>  	scheme_prefix = strstr(url, "://");
>  	if (!scheme_prefix) {
>  		if (!strchr(anon_part, ':'))
> @@ -1373,7 +1372,15 @@ char *transport_anonymize_url(const char *url)
>  		cp = strchr(scheme_prefix + 3, '/');
>  		if (cp && cp < anon_part)
>  			goto literal_copy;
> -		prefix_len = scheme_prefix - url + 3;
> +
> +		if (strip_user)
> +			prefix_len = scheme_prefix - url + 3;
> +		else {
> +			cp = strchr(scheme_prefix + 3, ':');
> +			if (cp && cp > anon_part)
> +				goto literal_copy; /* username only */
> +			prefix_len = cp - url;
> +		}
>  	}
>  	return xstrfmt("%.*s%.*s", (int)prefix_len, url,
>  		       (int)anon_len, anon_part);
> diff --git a/transport.h b/transport.h
> index 06e06d3d89..6d8c99ac91 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -243,10 +243,19 @@ const struct ref *transport_get_remote_refs(struct transport *transport,
>  int transport_fetch_refs(struct transport *transport, struct ref *refs);
>  void transport_unlock_pack(struct transport *transport);
>  int transport_disconnect(struct transport *transport);
> -char *transport_anonymize_url(const char *url);
>  void transport_take_over(struct transport *transport,
>  			 struct child_process *child);
>
> +/*
> + * Strip password and optionally username from a URL and return
> + * it in a newly allocated string (even if nothing was stripped).
> + */
> +char *transport_strip_url(const char *url, int strip_username);
> +static inline char *transport_anonymize_url(const char *url)
> +{
> +	return transport_strip_url(url, 1);
> +}
> +
>  int transport_connect(struct transport *transport, const char *name,
>  		      const char *exec, int fd[2]);
>
> --
> 2.22.0.rc0.583.g23d90da2b3
>
>

Re: [PATCH 1/3] transport_anonymize_url(): support retaining username

[Johannes Schindelin]

Whoops. Meant to comment on this:

On Sun, 19 May 2019, Jeff King wrote:

> diff --git a/transport.c b/transport.c
> index f1fcd2c4b0..ba61e57295 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1335,11 +1335,7 @@ int transport_disconnect(struct transport *transport)
>  	return ret;
>  }
>
> -/*
> - * Strip username (and password) from a URL and return
> - * it in a newly allocated string.
> - */
> -char *transport_anonymize_url(const char *url)
> +char *transport_strip_url(const char *url, int strip_user)

It might make more sense to skip the "transport_" prefix here, and maybe
use a slightly more descriptive name? My current favorite would be
`strip_credentials_from_url(const char *url, int keep_user)`.

>  {
>  	char *scheme_prefix, *anon_part;
>  	size_t anon_len, prefix_len = 0;
> @@ -1348,7 +1344,10 @@ char *transport_anonymize_url(const char *url)
>  	if (url_is_local_not_ssh(url) || !anon_part)
>  		goto literal_copy;
>
> -	anon_len = strlen(++anon_part);
> +	anon_len = strlen(anon_part);
> +	if (strip_user)
> +		anon_part++;
> +
>  	scheme_prefix = strstr(url, "://");
>  	if (!scheme_prefix) {
>  		if (!strchr(anon_part, ':'))
> @@ -1373,7 +1372,15 @@ char *transport_anonymize_url(const char *url)
>  		cp = strchr(scheme_prefix + 3, '/');
>  		if (cp && cp < anon_part)
>  			goto literal_copy;
> -		prefix_len = scheme_prefix - url + 3;
> +
> +		if (strip_user)
> +			prefix_len = scheme_prefix - url + 3;
> +		else {
> +			cp = strchr(scheme_prefix + 3, ':');

How about `scheme_prefix += 3;` (actually quite a bit earlier than here),
followed by `memchr(scheme_prefix, ':', anon_part - scheme_prefix)`?

Ah, I see you just copied that part from above...

Thanks,
Dscho

> +			if (cp && cp > anon_part)
> +				goto literal_copy; /* username only */
> +			prefix_len = cp - url;
> +		}
>  	}
>  	return xstrfmt("%.*s%.*s", (int)prefix_len, url,
>  		       (int)anon_len, anon_part);
> diff --git a/transport.h b/transport.h
> index 06e06d3d89..6d8c99ac91 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -243,10 +243,19 @@ const struct ref *transport_get_remote_refs(struct transport *transport,
>  int transport_fetch_refs(struct transport *transport, struct ref *refs);
>  void transport_unlock_pack(struct transport *transport);
>  int transport_disconnect(struct transport *transport);
> -char *transport_anonymize_url(const char *url);
>  void transport_take_over(struct transport *transport,
>  			 struct child_process *child);
>
> +/*
> + * Strip password and optionally username from a URL and return
> + * it in a newly allocated string (even if nothing was stripped).
> + */
> +char *transport_strip_url(const char *url, int strip_username);
> +static inline char *transport_anonymize_url(const char *url)
> +{
> +	return transport_strip_url(url, 1);
> +}
> +
>  int transport_connect(struct transport *transport, const char *name,
>  		      const char *exec, int fd[2]);
>
> --
> 2.22.0.rc0.583.g23d90da2b3
>
>

[PATCH 2/3] clone: avoid storing URL passwords in config

[Jeff King]

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

[PATCH 3/3] clone: auto-enable git-credential-store when necessary

[Jeff King]

If the user clones with a URL containing a password and has no
credential helper configured, we're stuck. We don't want to write the
password into .git/config because that risks accidentally disclosing it.
But if we don't record it somewhere, subsequent fetches will fail unless
the user is there to input the password.

The best advice we can give the user is to set up a credential helper.
But we can actually go a step further and enable the "store" helper for
them. This still records the password in plaintext, but:

  1. It's not inside the repo directory, which makes it slightly less
     likely to be disclosed.

  2. The permissions on the storage file are tighter than what would be
     on .git/config.

So this is generally a security win over the old behavior of writing it
into .git/config. And it's a usability win over the more recent behavior
of just forgetting the password entirely.

The biggest downside is that it's a bit magical from the user's
perspective, because now the password is off in some other file (usually
~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which
complicates things if they want to purge the repo and password, for
example, because now they can't just delete the repository directory.

The file location is documented, though, and we point people to the
documentation. So perhaps it will be enough (and better still, may lead
to them configuring a more secure helper).

Signed-off-by: Jeff King <peff@peff.net>
---
If we do decide this is too magical, I think the best alternate path is
to advise them on credential helpers, and how to seed the password into
the helper (basically configure the helper and then "git fetch"
should work).

One other thing I wondered: if they do have a helper configured this
patch will omit the advice entirely, but we'll still print the warning.
Is that useful (to remind them that passwords in URLs are a bad thing),
or is it just annoying?

 builtin/clone.c            | 19 ++++++++++++++++---
 credential.c               | 13 +++++++++++++
 credential.h               |  6 ++++++
 t/t5550-http-fetch-dumb.sh |  2 +-
 4 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 15fffc3268..94d2659154 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -31,6 +31,7 @@
 #include "packfile.h"
 #include "list-objects-filter-options.h"
 #include "object-store.h"
+#include "credential.h"
 
 /*
  * Overall FIXMEs:
@@ -894,8 +895,14 @@ static int dir_exists(const char *path)
 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"
+"the password will not be recorded in the repository config.\n"
+"Since you have no credential helper configured, the \"store\" helper\n"
+"has been enabled for this repository, and will provide the password\n"
+"for further fetches.\n"
+"\n"
+"Note that the password is still stored in plaintext in the filesystem;\n"
+"consider configuring a more secure helper. See \"git help gitcredentials\"\n"
+"and \"git help git-credential-store\" for details.\n"
 );
 
 int cmd_clone(int argc, const char **argv, const char *prefix)
@@ -1090,7 +1097,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	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));
+		if (!url_has_credential_helper(sanitized_repo)) {
+			strbuf_addf(&key, "credential.%s.helper",
+				    sanitized_repo);
+			git_config_set(key.buf, "store");
+			strbuf_reset(&key);
+			advise(_(sanitized_url_advice));
+		}
 	}
 	strbuf_addf(&key, "remote.%s.url", option_origin);
 	git_config_set(key.buf, sanitized_repo);
diff --git a/credential.c b/credential.c
index 62be651b03..0a70edcee5 100644
--- a/credential.c
+++ b/credential.c
@@ -372,3 +372,16 @@ void credential_from_url(struct credential *c, const char *url)
 			*p-- = '\0';
 	}
 }
+
+int url_has_credential_helper(const char *url)
+{
+	struct credential c = CREDENTIAL_INIT;
+	int ret;
+
+	credential_from_url(&c, url);
+	credential_apply_config(&c);
+	ret = c.helpers.nr > 0;
+
+	credential_clear(&c);
+	return ret;
+}
diff --git a/credential.h b/credential.h
index 6b0cd16be2..e6d6d6fa40 100644
--- a/credential.h
+++ b/credential.h
@@ -32,4 +32,10 @@ void credential_from_url(struct credential *, const char *url);
 int credential_match(const struct credential *have,
 		     const struct credential *want);
 
+/*
+ * Return true if feeding "url" to the credential system would trigger one
+ * or more helpers.
+ */
+int url_has_credential_helper(const char *url);
+
 #endif /* CREDENTIAL_H */
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index d8c85f3126..2723e91ae0 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -70,7 +70,7 @@ test_expect_success 'username is retained in URL, password is not' '
 	! grep pass url
 '
 
-test_expect_failure 'fetch of password-URL clone uses stored auth' '
+test_expect_success 'fetch of password-URL clone uses stored auth' '
 	set_askpass wrong &&
 	git -C clone-auth-none fetch &&
 	expect_askpass none
-- 
2.22.0.rc0.583.g23d90da2b3

Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary

[Eric Sunshine]

On Sun, May 19, 2019 at 2:42 PM Jeff King <peff@peff.net> wrote:
> If the user clones with a URL containing a password and has no
> credential helper configured, we're stuck. We don't want to write the
> password into .git/config because that risks accidentally disclosing it.
> But if we don't record it somewhere, subsequent fetches will fail unless
> the user is there to input the password.
>
> But we can actually go a step further and enable the "store" helper for
> them. [...]
>
> The biggest downside is that it's a bit magical from the user's
> perspective, because now the password is off in some other file (usually
> ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which
> complicates things if they want to purge the repo and password, for
> example, because now they can't just delete the repository directory.
>
> The file location is documented, though, and we point people to the
> documentation. So perhaps it will be enough (and better still, may lead
> to them configuring a more secure helper).

I'm trying to decide how I feel about this based upon my own
experience recently of having my password magically stored by Git for
Windows without warning or consent on a computer which was not my own
but on which I needed to access a private GitHub repository. Although
the situation is not perfectly analogous, the concern of having one's
password magically squirreled-away _somewhere_ unexpectedly is the
same. Being unfamiliar with Git for Windows's credential helper or
Windows credential management in general, I experienced more than a
few minutes of consternation and alarm before finally figuring out
where Git for Windows had stored my password and how to remove it. The
sense of alarm and discomfort likely would have not arisen had the
credential helper given me the opportunity to approve or deny the
action.

>  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 in the repository config.\n"
> +"Since you have no credential helper configured, the \"store\" helper\n"
> +"has been enabled for this repository, and will provide the password\n"
> +"for further fetches.\n"
> +"\n"
> +"Note that the password is still stored in plaintext in the filesystem;\n"
> +"consider configuring a more secure helper. See \"git help gitcredentials\"\n"
> +"and \"git help git-credential-store\" for details.\n"
>  );

Give the above experience, one way to mitigate such feelings of alarm
might, at a minimum, be for this message to say where the password is
being stored (and, possibly, how to remove it) so the user can do so
immediately if desired. Prompting the user to approve or deny the
action might also go a long way toward making this more palatable
(assuming the session is interactive).

Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary

[Jeff King]

On Mon, May 20, 2019 at 07:28:08AM -0400, Eric Sunshine wrote:

> > The biggest downside is that it's a bit magical from the user's
> > perspective, because now the password is off in some other file (usually
> > ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which
> > complicates things if they want to purge the repo and password, for
> > example, because now they can't just delete the repository directory.
> >
> > The file location is documented, though, and we point people to the
> > documentation. So perhaps it will be enough (and better still, may lead
> > to them configuring a more secure helper).
> 
> I'm trying to decide how I feel about this based upon my own
> experience recently of having my password magically stored by Git for
> Windows without warning or consent on a computer which was not my own
> but on which I needed to access a private GitHub repository. Although
> the situation is not perfectly analogous, the concern of having one's
> password magically squirreled-away _somewhere_ unexpectedly is the
> same. Being unfamiliar with Git for Windows's credential helper or
> Windows credential management in general, I experienced more than a
> few minutes of consternation and alarm before finally figuring out
> where Git for Windows had stored my password and how to remove it. The
> sense of alarm and discomfort likely would have not arisen had the
> credential helper given me the opportunity to approve or deny the
> action.

Thanks, that's a good elaboration of the uneasiness I was feeling. This
patch is better than the status quo in that your password was already
being squirreled away in plaintext, and now it's at least locked down
with filesystem permissions. But it's clearly not as far as we could go.
I was mostly just afraid to break existing workflows, but maybe we
should be more opinionated.

Or maybe we just need to give more specific details about how to back
out the change.

> > +"Note that the password is still stored in plaintext in the filesystem;\n"
> > +"consider configuring a more secure helper. See \"git help gitcredentials\"\n"
> > +"and \"git help git-credential-store\" for details.\n"
> >  );
> 
> Give the above experience, one way to mitigate such feelings of alarm
> might, at a minimum, be for this message to say where the password is
> being stored (and, possibly, how to remove it) so the user can do so
> immediately if desired. Prompting the user to approve or deny the
> action might also go a long way toward making this more palatable
> (assuming the session is interactive).

I actually thought about pointing to the file, but it's non-trivial to
do so (there's a bunch of internal logic in credential-store to decide
between $HOME and XDG locations).

I think what we really need are better commands to manage credentials
independent of helpers, and then we could recommend a simple command to
clear a credential that you don't want to have stored. Right now I think
the best you can do is:

  echo url=https://example.com | git credential reject

but:

  - "reject" is a funny term; this comes from the C code, which thinks
    of it in terms of the server approving/rejecting (and that makes
    sense for scripts calling this command). But at the helper level,
    the operations are really store/erase. We probably ought to support
    those names, too.

  - piping the credential protocol is slightly awkward; we probably
    ought to allow a url on the command line, and avoid reading stdin if
    we get one.

That would give us:

  git credential erase https://example.com

which is really quite readable. :)

Likewise, if we choose not to auto-enable the store helper, we'd
probably want to give advice on seeding your password. Right now that
is:

  echo url=https://example.com | git credential fill | git credential approve

which is...not intuitive. It would make sense to me to have a "seed"
operation which does a fill/approve together. Maybe that should just be
what "store" does, which would allow:

  $ git credential store https://example.com
  Username for 'https://example.com':
  Password for 'https://user@example.com':

(Of course you can also just "git fetch" to get prompted, but it seems
like this shouldn't require a network operation if you don't want it
to).

-Peff

Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary

[Johannes Schindelin]

Hi Peff,

On Mon, 20 May 2019, Jeff King wrote:

> On Mon, May 20, 2019 at 07:28:08AM -0400, Eric Sunshine wrote:
>
> > > The biggest downside is that it's a bit magical from the user's
> > > perspective, because now the password is off in some other file
> > > (usually ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME).
> > > Which complicates things if they want to purge the repo and
> > > password, for example, because now they can't just delete the
> > > repository directory.
> > >
> > > The file location is documented, though, and we point people to the
> > > documentation. So perhaps it will be enough (and better still, may
> > > lead to them configuring a more secure helper).
> >
> > I'm trying to decide how I feel about this based upon my own
> > experience recently of having my password magically stored by Git for
> > Windows without warning or consent on a computer which was not my own
> > but on which I needed to access a private GitHub repository. Although
> > the situation is not perfectly analogous, the concern of having one's
> > password magically squirreled-away _somewhere_ unexpectedly is the
> > same. Being unfamiliar with Git for Windows's credential helper or
> > Windows credential management in general, I experienced more than a
> > few minutes of consternation and alarm before finally figuring out
> > where Git for Windows had stored my password and how to remove it. The
> > sense of alarm and discomfort likely would have not arisen had the
> > credential helper given me the opportunity to approve or deny the
> > action.
>
> Thanks, that's a good elaboration of the uneasiness I was feeling. This
> patch is better than the status quo in that your password was already
> being squirreled away in plaintext, and now it's at least locked down
> with filesystem permissions. But it's clearly not as far as we could go.
> I was mostly just afraid to break existing workflows, but maybe we
> should be more opinionated.
>
> Or maybe we just need to give more specific details about how to back
> out the change.

I think that would make the most sense. "If you did not mean for this
password to be stored, call [...]".

> > > +"Note that the password is still stored in plaintext in the filesystem;\n"
> > > +"consider configuring a more secure helper. See \"git help gitcredentials\"\n"
> > > +"and \"git help git-credential-store\" for details.\n"
> > >  );
> >
> > Give the above experience, one way to mitigate such feelings of alarm
> > might, at a minimum, be for this message to say where the password is
> > being stored (and, possibly, how to remove it) so the user can do so
> > immediately if desired. Prompting the user to approve or deny the
> > action might also go a long way toward making this more palatable
> > (assuming the session is interactive).
>
> I actually thought about pointing to the file, but it's non-trivial to
> do so (there's a bunch of internal logic in credential-store to decide
> between $HOME and XDG locations).
>
> I think what we really need are better commands to manage credentials
> independent of helpers, and then we could recommend a simple command to
> clear a credential that you don't want to have stored. Right now I think
> the best you can do is:
>
>   echo url=https://example.com | git credential reject
>
> but:
>
>   - "reject" is a funny term; this comes from the C code, which thinks
>     of it in terms of the server approving/rejecting (and that makes
>     sense for scripts calling this command). But at the helper level,
>     the operations are really store/erase. We probably ought to support
>     those names, too.
>
>   - piping the credential protocol is slightly awkward; we probably
>     ought to allow a url on the command line, and avoid reading stdin if
>     we get one.
>
> That would give us:
>
>   git credential erase https://example.com
>
> which is really quite readable. :)
>
> Likewise, if we choose not to auto-enable the store helper, we'd
> probably want to give advice on seeding your password. Right now that
> is:
>
>   echo url=https://example.com | git credential fill | git credential approve
>
> which is...not intuitive. It would make sense to me to have a "seed"
> operation which does a fill/approve together. Maybe that should just be
> what "store" does, which would allow:
>
>   $ git credential store https://example.com
>   Username for 'https://example.com':
>   Password for 'https://user@example.com':
>
> (Of course you can also just "git fetch" to get prompted, but it seems
> like this shouldn't require a network operation if you don't want it
> to).

There is nothing stopping us from adding a new command to `git
credential`: `git credential forget <url>` where the URL may contain the
user name for further differentiation (and maybe even a password that will
be ignored, for copy/pasting convenience).

Ciao,
Dscho

Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary

[Ævar Arnfjörð Bjarmason]

On Sun, May 19 2019, Jeff King wrote:

> If the user clones with a URL containing a password and has no
> credential helper configured, we're stuck. We don't want to write the
> password into .git/config because that risks accidentally disclosing it.
> But if we don't record it somewhere, subsequent fetches will fail unless
> the user is there to input the password.
>
> The best advice we can give the user is to set up a credential helper.
> But we can actually go a step further and enable the "store" helper for
> them. This still records the password in plaintext, but:
>
>   1. It's not inside the repo directory, which makes it slightly less
>      likely to be disclosed.
>
>   2. The permissions on the storage file are tighter than what would be
>      on .git/config.
>
> So this is generally a security win over the old behavior of writing it
> into .git/config. And it's a usability win over the more recent behavior
> of just forgetting the password entirely.
>
> The biggest downside is that it's a bit magical from the user's
> perspective, because now the password is off in some other file (usually
> ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which
> complicates things if they want to purge the repo and password, for
> example, because now they can't just delete the repository directory.
>
> The file location is documented, though, and we point people to the
> documentation. So perhaps it will be enough (and better still, may lead
> to them configuring a more secure helper).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> If we do decide this is too magical, I think the best alternate path is
> to advise them on credential helpers, and how to seed the password into
> the helper (basically configure the helper and then "git fetch"
> should work).
>
> One other thing I wondered: if they do have a helper configured this
> patch will omit the advice entirely, but we'll still print the warning.
> Is that useful (to remind them that passwords in URLs are a bad thing),
> or is it just annoying?
>
>  builtin/clone.c            | 19 ++++++++++++++++---
>  credential.c               | 13 +++++++++++++
>  credential.h               |  6 ++++++
>  t/t5550-http-fetch-dumb.sh |  2 +-
>  4 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 15fffc3268..94d2659154 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -31,6 +31,7 @@
>  #include "packfile.h"
>  #include "list-objects-filter-options.h"
>  #include "object-store.h"
> +#include "credential.h"
>
>  /*
>   * Overall FIXMEs:
> @@ -894,8 +895,14 @@ static int dir_exists(const char *path)
>  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"
> +"the password will not be recorded in the repository config.\n"
> +"Since you have no credential helper configured, the \"store\" helper\n"
> +"has been enabled for this repository, and will provide the password\n"
> +"for further fetches.\n"
> +"\n"
> +"Note that the password is still stored in plaintext in the filesystem;\n"
> +"consider configuring a more secure helper. See \"git help gitcredentials\"\n"
> +"and \"git help git-credential-store\" for details.\n"
>  );
>
>  int cmd_clone(int argc, const char **argv, const char *prefix)
> @@ -1090,7 +1097,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	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));
> +		if (!url_has_credential_helper(sanitized_repo)) {
> +			strbuf_addf(&key, "credential.%s.helper",
> +				    sanitized_repo);
> +			git_config_set(key.buf, "store");
> +			strbuf_reset(&key);
> +			advise(_(sanitized_url_advice));
> +		}
>  	}
>  	strbuf_addf(&key, "remote.%s.url", option_origin);
>  	git_config_set(key.buf, sanitized_repo);
> diff --git a/credential.c b/credential.c
> index 62be651b03..0a70edcee5 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -372,3 +372,16 @@ void credential_from_url(struct credential *c, const char *url)
>  			*p-- = '\0';
>  	}
>  }
> +
> +int url_has_credential_helper(const char *url)
> +{
> +	struct credential c = CREDENTIAL_INIT;
> +	int ret;
> +
> +	credential_from_url(&c, url);
> +	credential_apply_config(&c);
> +	ret = c.helpers.nr > 0;
> +
> +	credential_clear(&c);
> +	return ret;
> +}
> diff --git a/credential.h b/credential.h
> index 6b0cd16be2..e6d6d6fa40 100644
> --- a/credential.h
> +++ b/credential.h
> @@ -32,4 +32,10 @@ void credential_from_url(struct credential *, const char *url);
>  int credential_match(const struct credential *have,
>  		     const struct credential *want);
>
> +/*
> + * Return true if feeding "url" to the credential system would trigger one
> + * or more helpers.
> + */
> +int url_has_credential_helper(const char *url);
> +
>  #endif /* CREDENTIAL_H */
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index d8c85f3126..2723e91ae0 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -70,7 +70,7 @@ test_expect_success 'username is retained in URL, password is not' '
>  	! grep pass url
>  '
>
> -test_expect_failure 'fetch of password-URL clone uses stored auth' '
> +test_expect_success 'fetch of password-URL clone uses stored auth' '
>  	set_askpass wrong &&
>  	git -C clone-auth-none fetch &&
>  	expect_askpass none

I've only looked at this very briefly, there's a regression here where
you're assuming that having a configured credential helper means it
works.

I.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other
what doesn't exist on my VPS in my ~/.gitconfig, cloning just warns
about it being missing, but will store the password in the repo.

With this you detect that I have the helper, don't store it, but then my
helper doesn't work, whereas this worked before.

That's an X-Y problem of config includes being rather limited (now I'm
just putting up with the error), but I expect this'll apply to others.

Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary

[Jeff King]

On Mon, May 20, 2019 at 03:56:14PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > -test_expect_failure 'fetch of password-URL clone uses stored auth' '
> > +test_expect_success 'fetch of password-URL clone uses stored auth' '
> >  	set_askpass wrong &&
> >  	git -C clone-auth-none fetch &&
> >  	expect_askpass none
> 
> I've only looked at this very briefly, there's a regression here where
> you're assuming that having a configured credential helper means it
> works.
> 
> I.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other
> what doesn't exist on my VPS in my ~/.gitconfig, cloning just warns
> about it being missing, but will store the password in the repo.
> 
> With this you detect that I have the helper, don't store it, but then my
> helper doesn't work, whereas this worked before.

There are more cases beyond that, too. You might have a helper defined
which doesn't actually store passwords, but just sometimes tries to
provide one. My thinking was that if you're clueful enough to have
configured helpers, you can probably deal with the fallout. But you're
right that it may still be a regression in the sense that the user may
still have to actually _do_ something to get their fetch to work.

I guess a more robust version of this is that _after_ the successful
clone, we could ask the credential system "hey, do you have the
credential for $URL?". And if it can't answer, then we can take action
(whether that action is setting up credential-store and seeding it with
the password, or just advising the user about the situation).

-Peff

Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary

[Ævar Arnfjörð Bjarmason]

On Mon, May 20 2019, Jeff King wrote:

> On Mon, May 20, 2019 at 03:56:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > -test_expect_failure 'fetch of password-URL clone uses stored auth' '
>> > +test_expect_success 'fetch of password-URL clone uses stored auth' '
>> >  	set_askpass wrong &&
>> >  	git -C clone-auth-none fetch &&
>> >  	expect_askpass none
>>
>> I've only looked at this very briefly, there's a regression here where
>> you're assuming that having a configured credential helper means it
>> works.
>>
>> I.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other
>> what doesn't exist on my VPS in my ~/.gitconfig, cloning just warns
>> about it being missing, but will store the password in the repo.
>>
>> With this you detect that I have the helper, don't store it, but then my
>> helper doesn't work, whereas this worked before.
>
> There are more cases beyond that, too. You might have a helper defined
> which doesn't actually store passwords, but just sometimes tries to
> provide one. My thinking was that if you're clueful enough to have
> configured helpers, you can probably deal with the fallout. But you're
> right that it may still be a regression in the sense that the user may
> still have to actually _do_ something to get their fetch to work.
>
> I guess a more robust version of this is that _after_ the successful
> clone, we could ask the credential system "hey, do you have the
> credential for $URL?". And if it can't answer, then we can take action
> (whether that action is setting up credential-store and seeding it with
> the password, or just advising the user about the situation).
>
> -Peff

Yeah I don't mean deal with some there-but-broken helper, but this:

    /usr/share/doc/git/contrib/credential/gnome-keyring/git-credential-gnome-keyring:
    not found

Until then the observable effect of that has been to make the
credential.helper config a noop, but now it's causing "we have a helper"
behavior.

Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary

[Jeff King]

On Mon, May 20, 2019 at 05:17:20PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > There are more cases beyond that, too. You might have a helper defined
> > which doesn't actually store passwords, but just sometimes tries to
> > provide one. My thinking was that if you're clueful enough to have
> > configured helpers, you can probably deal with the fallout. But you're
> > right that it may still be a regression in the sense that the user may
> > still have to actually _do_ something to get their fetch to work.
> >
> > I guess a more robust version of this is that _after_ the successful
> > clone, we could ask the credential system "hey, do you have the
> > credential for $URL?". And if it can't answer, then we can take action
> > (whether that action is setting up credential-store and seeding it with
> > the password, or just advising the user about the situation).
> >
> > -Peff
> 
> Yeah I don't mean deal with some there-but-broken helper, but this:
> 
>     /usr/share/doc/git/contrib/credential/gnome-keyring/git-credential-gnome-keyring:
>     not found
> 
> Until then the observable effect of that has been to make the
> credential.helper config a noop, but now it's causing "we have a helper"
> behavior.

Right, I understood. The other case I mean is not one that's broken, but
a helper that's designed to provide a password from a read-only store
(which presumably doesn't have _this_ password, else why would they be
providing it in the URL?).

It is not going to help that the clone will feed the password to such a
helper because it will (correctly, and by design) ignore any "store"
requests.

In other words, I am agreeing with you and indicating that there are
even more cases where a non-empty helper config will mislead us.

I'm going to try to re-work the patch to do this check-at-the-end
technique, and probably try to make the UI for clearing and seeding
passwords a bit more friendly, too.

-Peff

Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary

[Ævar Arnfjörð Bjarmason]

On Sun, May 19 2019, Jeff King wrote:

> If the user clones with a URL containing a password and has no
> credential helper configured, we're stuck. We don't want to write the
> password into .git/config because that risks accidentally disclosing it.
> But if we don't record it somewhere, subsequent fetches will fail unless
> the user is there to input the password.
>
> The best advice we can give the user is to set up a credential helper.
> But we can actually go a step further and enable the "store" helper for
> them. This still records the password in plaintext, but:
>
>   1. It's not inside the repo directory, which makes it slightly less
>      likely to be disclosed.
>
>   2. The permissions on the storage file are tighter than what would be
>      on .git/config.
>
> So this is generally a security win over the old behavior of writing it
> into .git/config. And it's a usability win over the more recent behavior
> of just forgetting the password entirely.
>
> The biggest downside is that it's a bit magical from the user's
> perspective, because now the password is off in some other file (usually
> ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which
> complicates things if they want to purge the repo and password, for
> example, because now they can't just delete the repository directory.
>
> The file location is documented, though, and we point people to the
> documentation. So perhaps it will be enough (and better still, may lead
> to them configuring a more secure helper).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> If we do decide this is too magical, I think the best alternate path is
> to advise them on credential helpers, and how to seed the password into
> the helper (basically configure the helper and then "git fetch"
> should work).
>
> One other thing I wondered: if they do have a helper configured this
> patch will omit the advice entirely, but we'll still print the warning.
> Is that useful (to remind them that passwords in URLs are a bad thing),
> or is it just annoying?

A few things, in no particular order, sorry for the braindump. Please
don't take this as "this all sucks" just trying to help by poking holes
in it.

 1. I'm a bit uneasy about us acting on an incident where the details of
    what happened / frequency haven't been released. I.e. GitHub
    supposedly has ~100 million repos, one source claims on the order of
    ~300 of these (public)[1], none of the sites said anything on that,
    that 300 number is third-party speculation.

    Let's say it's 100x that counting private repos. That's still a very
    small percentage of 100 million.

    But more importantly, it doesn't give us any insight on how to
    perhaps better solve this problem, e.g. maybe 80% of it is people
    using some build system (docker?) where we'd expose /home too. Not
    saying it is, just that we basically have a partial bug report here.

    I don't have a good sense for what the state is there. Is this all
    repos cloned in /var/www, then this is sufficient, or something
    else?

 2. We're now making the trade-off of auto-storing the password in ~/,
    is that a worse default now in the wild? We're defaulting to storing
    for a longer time (e.g. on a shared *nix server) in workflows where
    you'd clone && rm -rf.

    So yeah, we might handle this *specific* incident, but are we just
    making it worse for others?

 3. There's seemingly no way to just say "I want it the old way, make it
    so!" without creating an auth helper that does that.

    Storing passwords in config isn't per-se insecure, neither is having
    passwords in (https) URLs. Yeah in combination with *other* stuff
    the might be insecure, but often not (but may actually become
    insecure by auto-storing in ~/)

 4. Re #3: We have existing users in the wild who do this, e.g. GitLab
    CI, the user/password token (lives for about an hour IIRC) is stored
    in .gitconfig, there's no writable /home there IIRC (or might not be
    in similar CI environments), and I daresay the people who set that
    up understand the security trade-offs.

 5. Given #2 && #3 && #4 I'd be much more comfortable with something
    where we just make this fail outright, and force the user to eitiher
    say "Yeah I want passwords in .git/config here" or "oh thanks, I'll
    use a credential helper" via some core.* or credentials.* config.

    I.e. let's not make #3 impossible, and for users who are clueless
    about security we're doing them a disservice by auto-using a crappy
    fallback ~/ helper, whereas they might have an *actual* helper they
    can use that doesn't suck (i.e. OS pwd store) if it was an error.

 6. We'll still happily let this new behavior pass by on init && config
    fetch. Should we care? I was going to say this categorically breaks
    core.sharedRepository=group with a password in the repo, it does in
    the 'git -c core.sharedRepository=group clone' case, but you can
    manually set it up still, you just can't use "clone" anymore.

 7. We still accept passwords on the command-line for cloning, so if
    we're trying to help novice users we're still open to the shared
    server attack where someone just runs "ps auxf" in a loop to scrape
    passwords.

    To me this is another good argument for some variant of #3. We could
    provide ssh-like security by outright refusing passwords on the
    command-line (could take them interactively, or via some FD/file),
    then allow relaxing that to the level of this auto-helper, or all
    the way down to the old behavior.

1. https://hub.packtpub.com/atlassian-bitbucket-github-and-gitlab-take-collective-steps-against-the-git-ransomware-attack/

Re: Git ransom campaign incident report - May 2019

[Johannes Schindelin]

Hi Peff,

On Sun, 19 May 2019, Jeff King wrote:

> On Fri, May 17, 2019 at 06:20:31PM -0400, Jeff King wrote:
>
> > What if we did this:
> >
> >   1. Do not ever write the password part of a URL into config.
> >
> >   2. When we extract the user/pass out of a URL, put them into the
> >      credential struct, so that when we successfully authenticate, we
> >      trigger storage. This _might_ actually happen already, but we
> >      should definitely confirm it.
> >
> >   3. If the user has no credential helper defined, then do one of:
> >
> >      a. Warn them that the credential was not recorded, but that they
> >         can use "git clone -c credential.helper=store" as a fallback
> > 	(but probably worded in a way to recommend using something
> > 	stronger if possible).
> >
> > 	This is slightly annoying because following the advice requires
> > 	re-cloning. Fixing it up from there is more like:
> >
> > 	  git config credential.helper store
> > 	  git fetch
> > 	  [interactively input password]
> >
> >      b. Just use credential-store. Optionally notify them of what
> >          happened (and that they might want to choose a better helper).
>
> So here are patches to do that. Step 2 is indeed how things work
> already, so nothing was needed there (there are still 3 patches because
> there was a bit of prep-work ;) ).

Thank you for working on this!!!

> I did 3b here: automagically enabling credential-store. I'm still on the
> fence on whether that's a good idea or not.

I think you're right, it is a good idea. It is built by default (because
it has no 3rd-party dependencies), and it should be "safe enough".

I'll just have to look into mapping the Unix-y `chmod()` to a
Windows-appropriate ACL action, I guess. AFAICT we don't do that yet.

> I don't have any data on how many victims of this ransom campaign might
> have been helped by this. But it certainly seems like a decent best
> practice. I'd hope that _most_ people have moved on to using a
> credential helper and supplying the initial password interactively these
> days; this is really just aimed at people who don't know better. So the
> goal is making them a bit more secure, but also educating them about the
> other options. Hopefully without breaking any workflows. :)

Well, let's also not underestimate the side effect of educating people
(even transitively) by the mere action of doing something about it and
talking about that.

Ciao,
Dscho

>
> -Peff
>
>   [1/3]: transport_anonymize_url(): support retaining username
>   [2/3]: clone: avoid storing URL passwords in config
>   [3/3]: clone: auto-enable git-credential-store when necessary
>
>  builtin/clone.c            | 37 +++++++++++++++++++++++++++++++++++--
>  credential.c               | 13 +++++++++++++
>  credential.h               |  6 ++++++
>  t/t5550-http-fetch-dumb.sh | 12 ++++++++++++
>  transport.c                | 21 ++++++++++++++-------
>  transport.h                | 11 ++++++++++-
>  6 files changed, 90 insertions(+), 10 deletions(-)
>