credential: handle partial URLs in config settings again

This fixes the problem illustrated by Peff's example
[https://lore.kernel.org/git/[email protected]/]
, in maint-2.17:

  $ echo url=https://example.com |
    git -c credential.example.com.username=foo credential fill
  warning: url has no scheme: example.com
  fatal: credential url cannot be parsed: example.com

The fix is necessarily different than what was proposed by brian
[https://lore.kernel.org/git/[email protected]/]
because that fix targets v2.26.x which has 46fd7b390034 (credential: allow wildcard patterns when matching config, 2020-02-20).

This patch series targets maint-2.17 instead (and might actually not be able to fix maint due to that wildcard pattern patch; I haven't had the time to check yet).

Please note that Git v2.17.4 will not do what we would expect here: if any host name (without protocol) is specified, e.g. -c credential.golli.wog.username = boo, it will actually ignore the host name. That is, this will populate the username:

  $ echo url=https://example.com |
    git -c credential.totally.bog.us.username=foo credential fill

Obviously, this is unexpected, as a Git config like this would leave the last specified user name as "winner":

[credential "first.com"]
username = Whos On
[credential "second.com"]
username = Who

This patch series fixes this. The quoted part of [credential "<value>"] will be interpreted as a partial URL:

* It can start with a protocol followed by ://, but does not have to.
* If it starts with a protocol, the host name will always be set (if the :// is followed immediately by yet another slash, then it will be set to the empty string).
* If it starts without a protocol, it is treated as a path if the value starts with a slash (and the host will be left unset).
* If it starts without a protocol and the first character is not a slash, it will be treated as a host name, optionally followed by a slash and the path.

Johannes Schindelin (3):
credential: fix grammar
credential: teach credential_from_url() a non-strict mode
credential: handle credential.<partial-URL>.<key> again

 credential.c           | 21 ++++++++++++++-------
 credential.h           |  8 ++++++--
 fsck.c                 |  2 +-
 t/t0300-credentials.sh | 13 +++++++++++++
 4 files changed, 34 insertions(+), 10 deletions(-)

base-commit: df5be6dc3fd18c294ec93a9af0321334e3f92c9c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-615%2Fdscho%2Fcredential-config-partial-url-maint-2.17-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-615/dscho/credential-config-partial-url-maint-2.17-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/615

gitgitgadget

From: Johannes Schindelin <[email protected]>

There was a lot going on behind the scenes when the vulnerability and possible solutions were discussed. Grammar was not a primary focus, that's why this slipped in.

---
 credential.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 122a23cd2f1..5a86502d95c 100644
@@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *);
 /*
  * Parse a url into a credential struct, replacing any existing contents.
  *
- * Ifthe url can't be parsed (e.g., a missing "proto://" component), the
+ * If the url can't be parsed (e.g., a missing "proto://" component), the
  * resulting credential will be empty but we'll still return success from the
  * "gently" form.
  *

gitgitgadget

From: Johannes Schindelin <[email protected]>

Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we required from a URL in order to parse it into a struct credential. That led to serious vulnerabilities.

There was one call site, though, that really needed that leniency: when parsing config settings a la credential.dev.azure.com.useHTTPPath.

In preparation for fixing that regression, let's add a parameter called strict to the credential_from_url() function and convert the existing callers to enforce that strict mode.

---
 credential.c | 14 ++++++++------
 credential.h |  6 +++++-
 fsck.c       |  2 +-
 3 files changed, 14 insertions(+), 8 deletions(-)

index 64a841eddca..c73260ac40f 100644
@@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,
 }
 
 int credential_from_url_gently(struct credential *c, const char *url,
-			       int quiet)
+			       int strict, int quiet)
 {
 	const char *at, *colon, *cp, *slash, *host, *proto_end;
 

@@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,
 	 *   (3) proto://<user>:<pass>@<host>/...
 	 */
 	proto_end = strstr(url, "://");
-	if (!proto_end || proto_end == url) {
+	if (strict && (!proto_end || proto_end == url)) {
 		if (!quiet)
 			warning(_("url has no scheme: %s"), url);
 		return -1;
 	}
-	cp = proto_end + 3;
+	cp = proto_end ? proto_end + 3 : url;
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');

@@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
 		host = at + 1;
 	}
 
-	c->protocol = xmemdupz(url, proto_end - url);
-	c->host = url_decode_mem(host, slash - host);
+	if (proto_end && proto_end - url > 0)
+		c->protocol = xmemdupz(url, proto_end - url);
+	if (slash - url > 0)
+		c->host = url_decode_mem(host, slash - host);
 	/* Trim leading and trailing slashes from path */
 	while (*slash == '/')
 		slash++;

@@ -407,6 +409,6 @@ int credential_from_url_gently(struct credential *c, const char *url,
 
 void credential_from_url(struct credential *c, const char *url)
 {
-	if (credential_from_url_gently(c, url, 0) < 0)
+	if (credential_from_url_gently(c, url, 1, 0) < 0)
 		die(_("credential url cannot be parsed: %s"), url);
 }

index 5a86502d95c..823ec2caf35 100644
@@ -41,9 +41,13 @@ void credential_write(const struct credential *, FILE *);
  * an error but leave the broken state in the credential object for further
  * examination.  The non-gentle form will issue a warning to stderr and return
  * an empty credential.
+ *
+ * In strict mode, an empty protocol or an empty host name are not allowed.
+ * The credential_from_url() function enforces strict mode.
  */
 void credential_from_url(struct credential *, const char *url);
-int credential_from_url_gently(struct credential *, const char *url, int quiet);
+int credential_from_url_gently(struct credential *, const char *url,
+			       int strict, int quiet);
 
 int credential_match(const struct credential *have,
 		     const struct credential *want);

index 31b5be05f54..aa66dc1e742 100644
@@ -1076,7 +1076,7 @@ static int check_submodule_url(const char *url)
 	else if (url_to_curl_url(url, &curl_url)) {
 		struct credential c = CREDENTIAL_INIT;
 		int ret = 0;
-		if (credential_from_url_gently(&c, curl_url, 1) ||
+		if (credential_from_url_gently(&c, curl_url, 1, 1) ||
 		    !*c.host)
 			ret = -1;
 		credential_clear(&c);

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Hi Junio,

Right. I debated whether to set it to NULL explicitly, or whether to leave it alone. At the end, I settled with the version in v2.17.4.

It is not obvious from the diff: url is never changed. It would still point at the first h, as you said.

I did not describe well enough in 0/3, my apologies.

Again, I tried to go with the version from v2.17.4 here, i.e. to set c->host if we get a value, but not set it to NULL otherwise (instead leave as-is).

That would have been my preferred solution, too. But there is some subtlety at work: for a url like /this/is/my/path, we want to leave c->host as-is, but for cert:///... we want to set it to "" (t0300.23 and t7416.12 test for this explicitly).

I guess a better condition would be if (proto_end || slash - host > 0). What do you think?

Thanks,
Dscho

Yeah, that would probably be more intuitive to read. What triggered my reaction was the mismatch between "slash - url" in the condition and "slash - host" that specifies the length of the memory region.

Thanks for tackling this.

Can the commit message say a little more about the semantics and when someone would use this?

Is it a shortcut for

[credential "http://dev.azure.com"]
useHttpPath = true

[credential "https://dev.azure.com"]
useHttpPath = true

?

I suspect this would be easier to read squashed with patch 3. That would also mean that the functionality and test coverage come at the same time.

[...]

	int credential_from_url_gently(struct credential *c, const char *url,
				       int quiet)
	{
		return parse_credential_url(c, url, 1, quiet);
	}

	int credential_from_url_nonstrict(struct credential *c, const char *url,
					  int quiet)
	{
		return parse_credential_url(c, url, 0, quiet);
	}

the previous code was ensuring protocol was always at least "" (albeit it might had been easier to understand with a comment)

removing the proto_end - url check would have the same effect, but then we will need to also add a explicit xmemdupz("") for the nonstrict part or audit (and with certainty add) checks to prevent a NULL protocol to introduce regressions

Carlo

Hi Carlo,

If you mean v2.17.5 by "the previous code", then yes.

However, what I wanted to reinstate was the behavior of v2.17.4, at least the behavior that was obviously intended by this code:

	int credential_match(const struct credential *want,
			     const struct credential *have)
	{
	#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
		return CHECK(protocol) &&
		       CHECK(host) &&
		       CHECK(path) &&
		       CHECK(username);
	#undef CHECK
	}

What speaks loudly to me is that _any_ of these can be NULL in want. Any of them. Even protocol.

Remember, the call that I modified here to be more lenient populates that want, not that have. The have still uses the strict mode, and that is very much by design.

I also saw reports where users try to set useHTTPPath for hosts, not for URLs, and it kind of makes sense. So I wanted to make that work, too.

Except that I uncovered the bug during my investigation that v2.17.4 handled credential.<hostname>.useHTTPPath as if no <hostname> was provided at all!

So I wanted to fix that bug as well.

What I do _not_ want is to enforce protocol to be set (falling back to an empty string if not specified). See below as to why.

I fear that my patches did not make it clear that the lenient mode is _only_ used in the config parsing, in which case we very much do not want to have the unspecified parts be interpreted as empty strings.

For example, if I specify

	[credential "http://"]
		helper = prevent

to catch _all_ http:// URLs, I definitely do not want that to be used only for URLs with an empty host name!

Likewise, if I specify

[credential "dev.azure.com"]
useHTTPPath = true

I positively want to use the path part of the URL to specify what credential to use _when accessing http://dev.azure.com/... _or_
https://dev.azure.com/... URLs, I do not expect that setting to be used only for URLs with an empty protocol (which are now forbidden, anyway, IIUC)!

So no, that xmemdupz("") would introduce very much undesirable behavior, I believe.

Ciao,
Dscho

I think the concern raised was that since we are using the same function in both cases there might be unintended consequences on changing the semantics for the other case.

the argument made by Jonathan to use something else for configuration (as is done in master) will help in that direction, and might be needed anyway as your code it otherwise broken for current maint and master, but if not possible (maybe later?) something like this could probably help :

index 88612e583c..f972fcc895 100644
@@ -389,8 +389,9 @@ int credential_from_url_gently(struct credential *c, const char *url,
 
 	if (proto_end && proto_end - url > 0)
 		c->protocol = xmemdupz(url, proto_end - url);
-	if (slash - url > 0)
+	if (strict || slash > url)
 		c->host = url_decode_mem(host, slash - host);
+
 	/* Trim leading and trailing slashes from path */
 	while (*slash == '/')
 		slash++;

changing the condition there as you suggested to Junio would be a plus IMHO as well as getting some test that would excercise the new warning that was introduced in credential.c:57

Carlo

Hi Carlo,

Indeed, I share that concern. That's why I wanted to be extra careful there to make sure that introducing this lenient mode does _not_ change the non-lenient mode in the least, i.e. it is the reason why I kept 2/3 separate from 3/3.

I am in general not a fan of the idea to have two separate parsers for essentially the same thing. In this instance, the difference between the lenient mode and the non-lenient mode is so obvious that I'd rather reuse the same code (think: Don't Repeat Yourself).

Yes (modulo doing "greater than" comparison on pointers which is IIRC not permitted in C in general).

Ciao,
Dscho

Of course, people write a loop like this

	char *cp, *ep = strchr(string, '\n');

	for (cp = string; cp < ep; cp++)
		...

all the time, and forbidding pointer comparison would make the language impossible to use ;-)

I think you are confused with a different rule---what is not kosher is to compare two pointers that do not point into elements of the same array. Whether the comparison is done in (ptr1 < ptr2) way, or (ptr2 - ptr1 < 0) way, does not change the equation.

Having said that, between

1.	if (strict || slash - url > 0)
2.	if (strict || slash > url)

c->host = url_decode_mem(host, slash - host);

I think the former is moderately easier to read. It still has the same "Huh?" factor that a comparison between slash and URL guards the size of the region being decoded, which is slash - host, and makes the reader wonder how these two variables, URL and host, relate to each other at this point in the code, though, either way the comparison is spelled.

Thanks.

Hi Junio,

Yep, that's my confusion all right.

I fully agree! That's why I use strict || slash - host > 0 in my next iteration (actually, I decided to rename strict, but that's beside the point).

Ciao,
Dscho

Hi Jonathan,

I don't really want to be too verbose about this in _this_ commit message. But I do see your point. This is my current version of the commit message:

credential: optionally allow partial URLs in credential_from_url_gently()

Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we required from a URL in order to parse it into a struct credential. That led to serious vulnerabilities.

There was one call site, though, that really needed that leniency: when parsing config settings a la credential.dev.azure.com.useHTTPPath. Settings like this might be desired when users want to use, say, a given user name on a given host, regardless of the protocol to be used.

In preparation for fixing that bug, let's add a parameter called allow_partial_url to the credential_from_url_gently() function and convert the existing callers to set that parameter to 0, i.e. do not change the existing behavior, just add the option to be more lenient.

Please note that this patch does more than just reinstating a way to imitate the behavior before those CVE-2020-11008 fixes: Before that, we would simply ignore URLs without a protocol. In other words, misleadingly, the following setting would be applied to _all_ URLs:

[credential "example.com"]
username = that-me

The obvious intention is to match the host name only. With this patch, we allow precisely that: when parsing the URL with non-zero allow_partial_url, we do not simply return success if there was no protocol, but we simply leave the protocol unset and continue parsing the URL.

As you see, I added substantially more information there.

On the contrary, I _really_ want them to be separate, so that it is easier to review. I know that _I_ had an easier time looking over the patch that introduces the non-strict mode, to make sure that it does not change a thing in strict mode (and yes, v1 made that harder than necessary by _not_ using strict || in the condition that guards the c->host assignment).

In short: it is my intention to keep the two patches separate, with the main goal to make sure that I don't introduce any of those regressions Peff is worried about.

I resisted the temptation because I don't want this to be a big patch series, but a fast one. The reports keep coming in, and the current plan seems to be for GitHub Desktop to release another version on Monday, for which I need to release a MinGit backport probably tomorrow, otherwise it won't work timewise.

Meaning: I don't really want to do "nice to have" work like this flag.

Looks good, but there are only three callers (and I don't expect more). And the only caller interested in the lenient mode (read: the only now-complicated call) lives in the very same file as the called function. So it strikes me as quite a bit of an overkill to introduce two new functions.

Almost. My intention was to handle missing host names, too: /repo.git should also match https://example.com/repo.git.

The user name is optional already, anyway.

BTW I realized (while working on these patches) that it would probably be a good idea to warn about passwords in these credential.<URL>.<key>
settings: the password will be ignored as far as credential_match() is concerned, and they should probably not live in the config.

But for aforementioned reasons, I decided against including a patch that makes that happen.

Again, the sole caller of this lenient mode is the config parser which then wants to match the (partial) URL against the actual URL, using this function:

	int credential_match(const struct credential *want,
			     const struct credential *have)
	{
	#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
		return CHECK(protocol) &&
		       CHECK(host) &&
		       CHECK(path) &&
		       CHECK(username);
	#undef CHECK
	}

The lenient parser is supposed to populate the want of this function. In other words, the code _expects_ protocol (or for that matter, _any_ attribute of want) to be optional.

Oh... I thought it was obvious that a partial URL starting with a slash would refer to the path part only... Does that not make sense?

Ciao,
Dscho

From: Johannes Schindelin <[email protected]>

In the patches for CVE-2020-11008, the ability to specify credential settings in the config for partial URLs got lost. For example, it used to be possible to specify a credential helper for a specific protocol:

	[credential "https://"]
		helper = my-https-helper

Likewise, it used to be possible to configure settings for a specific host, e.g.:

[credential "dev.azure.com"]
useHTTPPath = true

Let's reinstate this behavior.

---
 credential.c           |  7 ++++++-
 t/t0300-credentials.sh | 13 +++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

index c73260ac40f..b9e8daa5406 100644
@@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value,
 		char *url = xmemdupz(key, dot - key);
 		int matched;
 
-		credential_from_url(&want, url);
+		if (credential_from_url_gently(&want, url, 0, 0) < 0) {
+			warning(_("skipping credential lookup for url: %s"), url);
+			credential_clear(c);
+			free(url);
+			return 0;
+		}
 		matched = credential_match(&want, c);
 
 		credential_clear(&want);

index efed3ea2955..9dcba6a7ad9 100755
@@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' '
 	test_i18ncmp expect stderr
 '
 
+test_expect_success 'credential config accepts partial URLs' '
+	echo url=https://example.com |
+	git -c credential.example.com.username=boo \
+		credential fill >actual &&
+	cat >expect <<-EOF &&
+	protocol=https
+	host=example.com
+	username=boo
+	password=askpass-password
+	EOF
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

Ah, I see.

[...]

	[credential "example.com"]
		# settings for the host
		...

	[credential "[email protected]"] # maybe
		# settings for the host/user combination
		...

	[credential "https://"]
		# settings for the scheme
		...

	[credential "https://example.com"]
		# settings for the host/scheme combination
		...

	[credential "https://example.com/"]
		# likewise
		...

	[credential "https://[email protected]"] # maybe
		# settings for the host/scheme/user combination
		...

	[credential "https://[email protected]/"] # maybe
		# likewise
		...

	[credential "https://example.com/repo.git"]
		# settings for the host/scheme/path combination
		...

	[credential "https://[email protected]/repo.git"] # maybe
		# settings for the host/scheme/user/path combination
		...

Hi Jonathan,

I suppose you're right.

BTW I just realized a much worse issue: credential_clear(c);. This clears the wrong struct. It should clear want, not c.

Yes, my bad! The next iteration will have a test for that.

That is actually not what the code does, at least not in maint-2.17. It very much warns about %0a in config keys. The test I am adding to verify that the warning above is exercised correctly looks like this:

	git -c credential.$partial.helper=yep \
                -c credential.with%0anewline.username=uh-oh \
                credential fill >output 2>error &&
        test_i18ngrep "skipping credential lookup for url" error

That is literally the only way I get credential_from_url_gently() to return -1 in the lenient mode.

Yes, I fix that, too. It is a bad usability bug, in my eyes, and I think it is better to fix it while I'm in the space anyway.

I think we can have _all_ of this _without_ violating the DRY principle.

Remember: my main motivation for keeping 2/3 apart from 3/3 was so that it is really easy to verify that 2/3 does not break the callers that _need_ the strict mode.

And since that is the case, we can then enjoy the benefit of the shared code for the one caller that wants to match also partial URLs.

I added tests for all of those. They all work as a naive user (like me) would expect them to.

I threw in another test, too:

	[credential "/repo.git"]
		...

And I also threw in negative tests, to verify that non-matching protocol or host name or path mean that the config setting is ignored.

Thank you for your thorough review, it really helps me, Dscho

Yes, this will remove the die() in all versions, but in v2.26.0 and up, that config would be silently ignored (and your new test will fail).

That seems scary. What if it is not .username, but:

[credential "example.com"]
username = foo
helper = "!echo password=bar"

? (Or you can imagine a helper that is pulling from a read-only store, like "pass"). That would send the credential to the wrong host.

I think any fix we do here needs to make sure we are not any reintroducing wrong-host problems (though I do admit that the usage like my example above is probably way less common than vanilla helpers that do the host-selection themselves).

-Peff

Hi Peff,

Thanks for testing!

It would. But I am not aware of any implications regarding .gitmodules (for some reason I now expect every bug to open an attack vector via submodules, I wonder why that is), so that's at least good.

There do seem to be a few projects that set a credential.<hostname>.useHTTPPath as part of their build, but I doubt that that could be exploited.

Yes. For the record, I tried to be very careful here. The _only_ code path that is affected by this change is the config reading. Reading .gitmodules (also in fsck.c) should behave identically to the way v2.17.5 behaves: they should not allow empty protocol or hostname. If you spot any difference in behavior in this regard, please do let me know:
that would be a bug.

And I do not think that these patches could re-introduce the problems you
fixed: the credential_match() tries to work against a struct credential that has been sent to credential_apply_config() and as per your fixes, this function now verifies that neither protocol nor hostname are unset.

But again, I would love another pair of eyes on this, to confirm my analysis.

Thanks,
Dscho

Hi,

Submodules are only one of many ways that people end up cloning from an attacker-controlled URL. In submodules we're careful not to use --recurse-submodules by default at clone time. So I'll mentally subsitute "attacker-controlled URLs" where you say "submodules". ;-)

I agree with Peff's concern about the unpatched state: since there are people who use [credential "host.example.com"] helper and there are credential helpers that ignore the host passed in, the combination can hurt people. (Fortunately, there aren't many credential helpers in that category that are commonly used.)

[...]

[...]

As mentioned above, the config reading is sensitive, too. That said, I suspect you got it to do basically the right thing.

Reading through the patches. Thank you.

Jonathan

Hi Jonathan,

Yet the vast majority of the vulnerabilities in Git that we fixed over the last years has involved submodules in their attack vector.

Say what you will about this, but the practical reality seems to be that most of the security bugs in Git affected submodule users.

Somebody even muttered the suggestion to me that submodules should be deprecated already.

Yes. That's why I want to make sure that the URL parser we use here is strict by default, and lenient _only_ when parsing the config (because then, it is more like a URL *pattern* used for matching, the parsed URL is never used directly).

Thank you very much!
Dscho

This fixes the problem illustrated by Peff's example
[https://lore.kernel.org/git/[email protected]/]
, in maint-2.17:

  $ echo url=https://example.com |
    git -c credential.example.com.username=foo credential fill
  warning: url has no scheme: example.com
  fatal: credential url cannot be parsed: example.com

The fix is necessarily different than what was proposed by brian
[https://lore.kernel.org/git/[email protected]/]
because that fix targets v2.26.x which has 46fd7b390034 (credential: allow wildcard patterns when matching config, 2020-02-20).

This patch series targets maint-2.17 instead (and might actually not be able to fix maint due to that wildcard pattern patch; I haven't had the time to check yet).

Please note that Git v2.17.4 will not do what we would expect here: if any host name (without protocol) is specified, e.g. -c credential.golli.wog.username = boo, it will actually ignore the host name. That is, this will populate the username:

  $ echo url=https://example.com |
    git -c credential.totally.bog.us.username=foo credential fill

Obviously, this is unexpected, as a Git config like this would leave the last specified user name as "winner":

[credential "first.com"]
username = Whos On
[credential "second.com"]
username = Who

This patch series fixes this. The quoted part of [credential "<value>"] will be interpreted as a partial URL:

* It can start with a protocol followed by ://, but does not have to.
* If it starts with a protocol, the host name will always be set (if the :// is followed immediately by yet another slash, then it will be set to the empty string).
* If it starts without a protocol, it is treated as a path if the value starts with a slash (and the host will be left unset).
* If it starts without a protocol and the first character is not a slash, it will be treated as a host name, optionally followed by a slash and the path.

Changes since v1:

* The condition when the c->host field is set was made more intuitive.
* The "fix grammar" commit now has Jonathan's "reviewed-by" footer.
* Inverted the meaning of the parameter strict and renamed it to allow_partial_urls, to clarify its role.
* Enhanced the commit message of the second patch to illustrate the motivation for the lenient mode a bit better.
* [Not a change] I did leave the second and the third patch separate from one another. This makes it a lot easier to follow the iteration and to keep the reviews straight: it separates the "how do we make URL parts optional?" from the "where do we need URL parts to be optional?"
* A partial URL https:// is now correctly interpreted as having only the protocol set, but not host name nor path.
* The lenient mode is now explained a lot more verbosely in the code comment in credential.h.
* When skipping a config setting, we now show the config key, not just the URL (which might be incomplete, i.e. not actually a URL).
* When skipping a config setting, the correct struct credential is cleared (i.e. the just-parsed one, not the one against which we wanted to match the just-parsed one).
* Added a ton more partial URLs to the test, and the test now also verifies that the warning comes out all right.

Johannes Schindelin (3):
credential: fix grammar
credential: optionally allow partial URLs in
credential_from_url_gently()
credential: handle credential.<partial-URL>.<key> again

 credential.c           | 22 +++++++++++++++-------
 credential.h           | 28 ++++++++++++++++++++++++++--
 fsck.c                 |  2 +-
 t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 81 insertions(+), 10 deletions(-)

base-commit: df5be6dc3fd18c294ec93a9af0321334e3f92c9c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-615%2Fdscho%2Fcredential-config-partial-url-maint-2.17-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-615/dscho/credential-config-partial-url-maint-2.17-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/615

Range-diff vs v1:

1: 2bf781081d9 ! 1: 2c1c0ae91eb credential: fix grammar
@@ Commit message
possible solutions were discussed. Grammar was not a primary focus, that's why this slipped in.

+ Reviewed-by: Jonathan Nieder <[email protected]>

## credential.h ##
2: 1081841b16d ! 2: fc772d21b74 credential: teach credential_from_url() a non-strict mode
@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- credential: teach credential_from_url() a non-strict mode
+ credential: optionally allow partial URLs in credential_from_url_gently()

Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we required from a URL in order to parse it into a struct credential.
@@ Commit message

There was one call site, though, that really needed that leniency: when parsing config settings a la credential.dev.azure.com.useHTTPPath.
+ Settings like this might be desired when users want to use, say, a given
+ user name on a given host, regardless of the protocol to be used.

- In preparation for fixing that regression, let's add a parameter called
- strict to the credential_from_url() function and convert the
- existing callers to enforce that strict mode.
+ In preparation for fixing that bug, let's add a parameter called
+ allow_partial_url to the credential_from_url_gently() function and
+ convert the existing callers to set that parameter to 0, i.e. do not
+ change the existing behavior, just add the option to be more lenient.
+
+ Please note that this patch does more than just reinstating a way to
+ imitate the behavior before those CVE-2020-11008 fixes: Before that, we
+ would simply ignore URLs without a protocol. In other words,
+ misleadingly, the following setting would be applied to _all_ URLs:
+
+ [credential "example.com"]
+ username = that-me
+
+ The obvious intention is to match the host name only. With this patch,
+ we allow precisely that: when parsing the URL with non-zero
+ allow_partial_url, we do not simply return success if there was no
+ protocol, but we simply leave the protocol unset and continue parsing
+ the URL.

@@ credential.c: static int check_url_component(const char *url, int quiet,

int credential_from_url_gently(struct credential *c, const char *url,
- int quiet)
-+ int strict, int quiet)
++ int allow_partial_url, int quiet)
{
const char *at, *colon, *cp, *slash, *host, *proto_end;

@@ credential.c: int credential_from_url_gently(struct credential *c, const char *u
*/
proto_end = strstr(url, "://");
- if (!proto_end || proto_end == url) {
-+ if (strict && (!proto_end || proto_end == url)) {
++ if (!allow_partial_url && (!proto_end || proto_end == url)) {
if (!quiet)
warning(_("url has no scheme: %s"), url);
return -1;
@@ credential.c: int credential_from_url_gently(struct credential *c, const char *u
- c->host = url_decode_mem(host, slash - host);
+ if (proto_end && proto_end - url > 0)
+ c->protocol = xmemdupz(url, proto_end - url);
-+ if (slash - url > 0)
++ if (!allow_partial_url || slash - host > 0)
+ c->host = url_decode_mem(host, slash - host);
/* Trim leading and trailing slashes from path */
while (*slash == '/')
@@ credential.c: int credential_from_url_gently(struct credential *c, const char *u
void credential_from_url(struct credential *c, const char *url)
{
- if (credential_from_url_gently(c, url, 0) < 0)
-+ if (credential_from_url_gently(c, url, 1, 0) < 0)
++ if (credential_from_url_gently(c, url, 0, 0) < 0)
die(_("credential url cannot be parsed: %s"), url);
}

@@ credential.h: void credential_write(const struct credential *, FILE *);
* examination. The non-gentle form will issue a warning to stderr and return
* an empty credential.
+ *
-+ * In strict mode, an empty protocol or an empty host name are not allowed. -+ * The credential_from_url() function enforces strict mode.
++ * If allow_partial_url is non-zero, partial URLs are allowed, i.e. it can, but ++ * does not have to, contain
++ *
++ * - a protocol (or scheme) of the form "<protocol>://"
++ *
++ * - a host name (the part after the protocol and before the first slash after
++ * that, if any)
++ *
++ * - a user name and potentially a password (as "<user>[:<password>]@" part of
++ * the host name)
++ *
++ * - a path (the part after the host name, if any, starting with the slash) ++ *
++ * Missing parts will be left unset in struct credential. Thus, https:// ++ * will have only the protocol set, example.com only the host name, and ++ * /git only the path.
++ *
++ * Note that an empty host name in an otherwise fully-qualified URL will be ++ * treated as unset when allow_partial_url is non-zero (and only then, ++ * otherwise it is treated as the empty string).
++ *
++ * The credential_from_url() function does not allow partial URLs.
*/
void credential_from_url(struct credential *, const char *url);
-int credential_from_url_gently(struct credential *, const char *url, int quiet); +int credential_from_url_gently(struct credential *, const char *url,
-+ int strict, int quiet);
++ int allow_partial_url, int quiet);

int credential_match(const struct credential *have,
const struct credential *want);
@@ fsck.c: static int check_submodule_url(const char *url)
struct credential c = CREDENTIAL_INIT;
int ret = 0;
- if (credential_from_url_gently(&c, curl_url, 1) ||
-+ if (credential_from_url_gently(&c, curl_url, 1, 1) ||
++ if (credential_from_url_gently(&c, curl_url, 0, 1) ||
!*c.host)
ret = -1;
credential_clear(&c);
3: 66823c735b1 ! 3: daedaffe960 credential: handle credential.<partial-URL>.<key> again

     @@ Commit message
      
          Let's reinstate this behavior.
      
     -    Original-test-case-by: Jeff King <[email protected]>
     +    While at it, increase the test coverage to document and verify the
     +    behavior with a couple other categories of partial URLs.
     +
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## credential.c ##
     @@ credential.c: static int credential_config_callback(const char *var, const char
       		int matched;
       
      -		credential_from_url(&want, url);
     -+		if (credential_from_url_gently(&want, url, 0, 0) < 0) {
     -+			warning(_("skipping credential lookup for url: %s"), url);
     -+			credential_clear(c);
     ++		if (credential_from_url_gently(&want, url, 1, 0) < 0) {
     ++			warning(_("skipping credential lookup for key: %s"),
     ++				var);
     ++			credential_clear(&want);
      +			free(url);
      +			return 0;
      +		}
     @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w
       	test_i18ncmp expect stderr
       '
       
     -+test_expect_success 'credential config accepts partial URLs' '
     -+	echo url=https://example.com |
     -+	git -c credential.example.com.username=boo \
     -+		credential fill >actual &&
     -+	cat >expect <<-EOF &&
     -+	protocol=https
     -+	host=example.com
     -+	username=boo
     -+	password=askpass-password
     -+	EOF
     -+	test_cmp expect actual
     ++test_expect_success 'credential config with partial URLs' '
     ++	echo "echo password=yep" | write_script git-credential-yep &&
     ++	test_write_lines url=https://[email protected]/repo.git >input &&
     ++	for partial in \
     ++		example.com \
     ++		[email protected] \
     ++		https:// \
     ++		https://example.com \
     ++		https://example.com/ \
     ++		https://[email protected] \
     ++		https://[email protected]/ \
     ++		https://example.com/repo.git \
     ++		https://[email protected]/repo.git \
     ++		/repo.git
     ++	do
     ++		git -c credential.$partial.helper=yep \
     ++			credential fill <input >output &&
     ++		grep yep output ||
     ++		return 1
     ++	done &&
     ++
     ++	for partial in \
     ++		dont.use.this \
     ++		http:// \
     ++		/repo
     ++	do
     ++		git -c credential.$partial.helper=yep \
     ++			credential fill <input >output &&
     ++		! grep yep output ||
     ++		return 1
     ++	done &&
     ++
     ++	git -c credential.$partial.helper=yep \
     ++		-c credential.with%0anewline.username=uh-oh \
     ++		credential fill <input >output 2>error &&
     ++	test_i18ngrep "skipping credential lookup for key" error
     ++
      +'
      +
       test_done

gitgitgadget

From: Johannes Schindelin <[email protected]>

There was a lot going on behind the scenes when the vulnerability and possible solutions were discussed. Grammar was not a primary focus, that's why this slipped in.

---
 credential.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 122a23cd2f1..5a86502d95c 100644
@@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *);
 /*
  * Parse a url into a credential struct, replacing any existing contents.
  *
- * Ifthe url can't be parsed (e.g., a missing "proto://" component), the
+ * If the url can't be parsed (e.g., a missing "proto://" component), the
  * resulting credential will be empty but we'll still return success from the
  * "gently" form.
  *

gitgitgadget

From: Johannes Schindelin <[email protected]>

Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we required from a URL in order to parse it into a struct credential. That led to serious vulnerabilities.

There was one call site, though, that really needed that leniency: when parsing config settings a la credential.dev.azure.com.useHTTPPath. Settings like this might be desired when users want to use, say, a given user name on a given host, regardless of the protocol to be used.

In preparation for fixing that bug, let's add a parameter called allow_partial_url to the credential_from_url_gently() function and convert the existing callers to set that parameter to 0, i.e. do not change the existing behavior, just add the option to be more lenient.

Please note that this patch does more than just reinstating a way to imitate the behavior before those CVE-2020-11008 fixes: Before that, we would simply ignore URLs without a protocol. In other words, misleadingly, the following setting would be applied to _all_ URLs:

[credential "example.com"]
username = that-me

The obvious intention is to match the host name only. With this patch, we allow precisely that: when parsing the URL with non-zero allow_partial_url, we do not simply return success if there was no protocol, but we simply leave the protocol unset and continue parsing the URL.

---
 credential.c | 14 ++++++++------
 credential.h | 26 +++++++++++++++++++++++++-
 fsck.c       |  2 +-
 3 files changed, 34 insertions(+), 8 deletions(-)

index 64a841eddca..52965a5122c 100644
@@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,
 }
 
 int credential_from_url_gently(struct credential *c, const char *url,
-			       int quiet)
+			       int allow_partial_url, int quiet)
 {
 	const char *at, *colon, *cp, *slash, *host, *proto_end;
 

@@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,
 	 *   (3) proto://<user>:<pass>@<host>/...
 	 */
 	proto_end = strstr(url, "://");
-	if (!proto_end || proto_end == url) {
+	if (!allow_partial_url && (!proto_end || proto_end == url)) {
 		if (!quiet)
 			warning(_("url has no scheme: %s"), url);
 		return -1;
 	}
-	cp = proto_end + 3;
+	cp = proto_end ? proto_end + 3 : url;
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');

@@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
 		host = at + 1;
 	}
 
-	c->protocol = xmemdupz(url, proto_end - url);
-	c->host = url_decode_mem(host, slash - host);
+	if (proto_end && proto_end - url > 0)
+		c->protocol = xmemdupz(url, proto_end - url);
+	if (!allow_partial_url || slash - host > 0)
+		c->host = url_decode_mem(host, slash - host);
 	/* Trim leading and trailing slashes from path */
 	while (*slash == '/')
 		slash++;

@@ -407,6 +409,6 @@ int credential_from_url_gently(struct credential *c, const char *url,
 
 void credential_from_url(struct credential *c, const char *url)
 {
-	if (credential_from_url_gently(c, url, 0) < 0)
+	if (credential_from_url_gently(c, url, 0, 0) < 0)
 		die(_("credential url cannot be parsed: %s"), url);
 }

index 5a86502d95c..5abc865b95d 100644
@@ -41,9 +41,33 @@ void credential_write(const struct credential *, FILE *);
  * an error but leave the broken state in the credential object for further
  * examination.  The non-gentle form will issue a warning to stderr and return
  * an empty credential.
+ *
+ * If allow_partial_url is non-zero, partial URLs are allowed, i.e. it can, but
+ * does not have to, contain
+ *
+ * - a protocol (or scheme) of the form "<protocol>://"
+ *
+ * - a host name (the part after the protocol and before the first slash after
+ *   that, if any)
+ *
+ * - a user name and potentially a password (as "<user>[:<password>]@" part of
+ *   the host name)
+ *
+ * - a path (the part after the host name, if any, starting with the slash)
+ *
+ * Missing parts will be left unset in `struct credential`. Thus, `https://`
+ * will have only the `protocol` set, `example.com` only the host name, and
+ * `/git` only the path.
+ *
+ * Note that an empty host name in an otherwise fully-qualified URL will be
+ * treated as unset when allow_partial_url is non-zero (and only then,
+ * otherwise it is treated as the empty string).
+ *
+ * The credential_from_url() function does not allow partial URLs.
  */
 void credential_from_url(struct credential *, const char *url);
-int credential_from_url_gently(struct credential *, const char *url, int quiet);
+int credential_from_url_gently(struct credential *, const char *url,
+			       int allow_partial_url, int quiet);
 
 int credential_match(const struct credential *have,
 		     const struct credential *want);

index 31b5be05f54..cec97b63901 100644
@@ -1076,7 +1076,7 @@ static int check_submodule_url(const char *url)
 	else if (url_to_curl_url(url, &curl_url)) {
 		struct credential c = CREDENTIAL_INIT;
 		int ret = 0;
-		if (credential_from_url_gently(&c, curl_url, 1) ||
+		if (credential_from_url_gently(&c, curl_url, 0, 1) ||
 		    !*c.host)
 			ret = -1;
 		credential_clear(&c);

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the patches for CVE-2020-11008, the ability to specify credential settings in the config for partial URLs got lost. For example, it used to be possible to specify a credential helper for a specific protocol:

	[credential "https://"]
		helper = my-https-helper

Likewise, it used to be possible to configure settings for a specific host, e.g.:

[credential "dev.azure.com"]
useHTTPPath = true

Let's reinstate this behavior.

While at it, increase the test coverage to document and verify the behavior with a couple other categories of partial URLs.

---
 credential.c           |  8 +++++++-
 t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

index 52965a5122c..3505f6356d8 100644
@@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,
 		char *url = xmemdupz(key, dot - key);
 		int matched;
 
-		credential_from_url(&want, url);
+		if (credential_from_url_gently(&want, url, 1, 0) < 0) {
+			warning(_("skipping credential lookup for key: %s"),
+				var);
+			credential_clear(&want);
+			free(url);
+			return 0;
+		}
 		matched = credential_match(&want, c);
 
 		credential_clear(&want);

index efed3ea2955..6fff76cb932 100755
@@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' '
 	test_i18ncmp expect stderr
 '
 
+test_expect_success 'credential config with partial URLs' '
+	echo "echo password=yep" | write_script git-credential-yep &&
+	test_write_lines url=https://[email protected]/repo.git >input &&
+	for partial in \
+		example.com \
+		[email protected] \
+		https:// \
+		https://example.com \
+		https://example.com/ \
+		https://[email protected] \
+		https://[email protected]/ \
+		https://example.com/repo.git \
+		https://[email protected]/repo.git \
+		/repo.git
+	do
+		git -c credential.$partial.helper=yep \
+			credential fill <input >output &&
+		grep yep output ||
+		return 1
+	done &&
+
+	for partial in \
+		dont.use.this \
+		http:// \
+		/repo
+	do
+		git -c credential.$partial.helper=yep \
+			credential fill <input >output &&
+		! grep yep output ||
+		return 1
+	done &&
+
+	git -c credential.$partial.helper=yep \
+		-c credential.with%0anewline.username=uh-oh \
+		credential fill <input >output 2>error &&
+	test_i18ngrep "skipping credential lookup for key" error
+
+'
+
 test_done

gitgitgadget

Hi Carlo,

We don't have any quiet variable here.

I am sure I don't follow. What non-essential changes are you assuming will follow up?

Are the tests I added to t0300 not enough? Or do you think it will need a native test helper that is included in t/helper/test-tool and exercised in the test suite somehow?

Ciao,
Dscho

my bad, should had been more explicit as it is confusing with all those booleans at the end without a flags parameter.

I mean the call should be instead :

  if (credential_from_url_gently(&want, url, 1, 1) < 0) {

since we want to warn if the configuration is not supported just like is done after this check.

the ones that were discussed with Jonathan in a differen thread :

* using a flags parameter instead of two ints
* whatever will be needed so it also works in master and maint

I think they are enough, it is only that is easier to quickly iterate with possible bad input with a helper. which is what I was offering for the next thread since its need is orthogonal to what you are trying to accomplish.

Carlo

Hi Carlo,

Since Junio expressed support for Jonathan's idea of using separate functions that wrap one helper function, I went with that instead.

Oy, I am still under water just trying to get the MinGit for Windows backports updated so that users can upgrade instead of complaining on bug trackers and on Twitter...

But yes, that will be the next step. Now that I have a comprehensive test case, it should be relatively easy.

Usually I would agree with you. It's quicker to iterate, and a ton faster to run (especially on Windows).

In this instance, I am going for a regression test case rather than a unit test, though, because I really need to ensure that what end users are trying to do on their machines will work (and continue to work).

Ciao,
Dscho

Speaking of which, it is not clear which one of "...url, 1, 0)" is the "quiet" bit. I somehow thought that somebody suggested to roll these two into a flags word and give quiet and the other bit a name, and after seeing this line, I tend to agree that would be great for readability.

Thanks, everybody, for help polishing this topic.

Ah, I should have checked before opening my mouth. It was this message <[email protected]> from Jonathan Nieder.

I also am OK with his "two thin wrappers around the underlying helper that takes two separate arguments", if that makes the resulting code easier to follow. I have a feeling that the caller knows (from the context, or the reason why it calls the credential-from-url code) if it wants strict or nonstrict variant and that is not something the caller is told from its caller. And if that is the case, "two thin wrappers" approach does make a lot of sense.

Thanks.

Hi Junio,

All right, two wrapper functions it is.

Ciao,
Dscho

for the whole series

This fixes the problem illustrated by Peff's example
[https://lore.kernel.org/git/[email protected]/]
, in maint-2.17:

  $ echo url=https://example.com |
    git -c credential.example.com.username=foo credential fill
  warning: url has no scheme: example.com
  fatal: credential url cannot be parsed: example.com

The fix is necessarily different than what was proposed by brian
[https://lore.kernel.org/git/[email protected]/]
because that fix targets v2.26.x which has 46fd7b390034 (credential: allow wildcard patterns when matching config, 2020-02-20).

This patch series targets maint-2.17 instead (and might actually not be able to fix maint due to that wildcard pattern patch; I haven't had the time to check yet).

Please note that Git v2.17.4 will not do what we would expect here: if any host name (without protocol) is specified, e.g. -c credential.golli.wog.username = boo, it will actually ignore the host name. That is, this will populate the username:

  $ echo url=https://example.com |
    git -c credential.totally.bog.us.username=foo credential fill

Obviously, this is unexpected, as a Git config like this would leave the last specified user name as "winner":

[credential "first.com"]
username = Whos On
[credential "second.com"]
username = Who

This patch series fixes this. The quoted part of [credential "<value>"] will be interpreted as a partial URL:

* It can start with a protocol followed by ://, but does not have to.
* If it starts with a protocol, the host name will always be set (if the :// is followed immediately by yet another slash, then it will be set to the empty string).
* If it starts without a protocol, it is treated as a path if the value starts with a slash (and the host will be left unset).
* If it starts without a protocol and the first character is not a slash, it will be treated as a host name, optionally followed by a slash and the path.

Changes since v2:

* Refactored out the credential_from_potentially_partial_url() function so that existing callers (except for the one we want to change) stay untouched.
* When encountering an invalid URL while parsing the config, we no longer suppress the parser's warning.
* The test now uses the file name convention stdin/stdout/stderr of t/lib-credential.sh.

Changes since v1:

* The condition when the c->host field is set was made more intuitive.
* The "fix grammar" commit now has Jonathan's "reviewed-by" footer.
* Inverted the meaning of the parameter strict and renamed it to allow_partial_urls, to clarify its role.
* Enhanced the commit message of the second patch to illustrate the motivation for the lenient mode a bit better.
* [Not a change] I did leave the second and the third patch separate from one another. This makes it a lot easier to follow the iteration and to keep the reviews straight: it separates the "how do we make URL parts optional?" from the "where do we need URL parts to be optional?"
* A partial URL https:// is now correctly interpreted as having only the protocol set, but not host name nor path.
* The lenient mode is now explained a lot more verbosely in the code comment in credential.h.
* When skipping a config setting, we now show the config key, not just the URL (which might be incomplete, i.e. not actually a URL).
* When skipping a config setting, the correct struct credential is cleared (i.e. the just-parsed one, not the one against which we wanted to match the just-parsed one).
* Added a ton more partial URLs to the test, and the test now also verifies that the warning comes out all right.

Johannes Schindelin (3):
credential: fix grammar
credential: optionally allow partial URLs in
credential_from_url_gently()
credential: handle credential.<partial-URL>.<key> again

 credential.c           | 60 +++++++++++++++++++++++++++++++++++++-----
 credential.h           |  2 +-
 t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 8 deletions(-)

base-commit: df5be6dc3fd18c294ec93a9af0321334e3f92c9c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-615%2Fdscho%2Fcredential-config-partial-url-maint-2.17-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-615/dscho/credential-config-partial-url-maint-2.17-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/615

Range-diff vs v2:

 1:  2c1c0ae91eb = 1:  2c1c0ae91eb credential: fix grammar
 2:  fc772d21b74 ! 2:  e92f139dfc7 credential: optionally allow partial URLs in credential_from_url_gently()

@@ Commit message
Settings like this might be desired when users want to use, say, a given user name on a given host, regardless of the protocol to be used.

- In preparation for fixing that bug, let's add a parameter called
- allow_partial_url to the credential_from_url_gently() function and
- convert the existing callers to set that parameter to 0, i.e. do not
- change the existing behavior, just add the option to be more lenient.
+ In preparation for fixing that bug, let's refactor the code to
+ optionally allow for partial URLs. For the moment, this functionality is
+ only exposed via the now-renamed function credential_from_url_1(), but
+ it is not used. The intention is to make it easier to verify that this
+ commit does not change the existing behavior unless explicitly allowing
+ for partial URLs.

Please note that this patch does more than just reinstating a way to imitate the behavior before those CVE-2020-11008 fixes: Before that, we
@@ Commit message

## credential.c ##
@@ credential.c: static int check_url_component(const char *url, int quiet,
+ return -1;
}

- int credential_from_url_gently(struct credential *c, const char *url, +-int credential_from_url_gently(struct credential *c, const char *url,
- int quiet)
-+ int allow_partial_url, int quiet)
++/*
++ * Potentially-partial URLs can, but do not have to, contain
++ *
++ * - a protocol (or scheme) of the form "<protocol>://"
++ *
++ * - a host name (the part after the protocol and before the first slash after
++ * that, if any)
++ *
++ * - a user name and potentially a password (as "<user>[:<password>]@" part of
++ * the host name)
++ *
++ * - a path (the part after the host name, if any, starting with the slash) ++ *
++ * Missing parts will be left unset in struct credential. Thus, https:// ++ * will have only the protocol set, example.com only the host name, and ++ * /git only the path.
++ *
++ * Note that an empty host name in an otherwise fully-qualified URL (e.g. ++ * cert:///path/to/cert.pem) will be treated as unset if we expect the URL to ++ * be potentially partial, and only then (otherwise, the empty string is used). ++ *
++ * The credential_from_url() function does not allow partial URLs. ++ */
++static int credential_from_url_1(struct credential *c, const char *url,
++ int allow_partial_url, int quiet)
{
const char *at, *colon, *cp, *slash, *host, *proto_end;

@@ credential.c: int credential_from_url_gently(struct credential *c, const char *u
while (*slash == '/')
slash++;
@@ credential.c: int credential_from_url_gently(struct credential *c, const char *url,
+ return 0;
+ }

++int credential_from_url_gently(struct credential *c, const char *url, int quiet) ++{
++ return credential_from_url_1(c, url, 0, quiet);
++}
++
void credential_from_url(struct credential *c, const char *url)
{
-- if (credential_from_url_gently(c, url, 0) < 0)
-+ if (credential_from_url_gently(c, url, 0, 0) < 0)
- die(_("credential url cannot be parsed: %s"), url);
- }
-
- ## credential.h ##
-@@ credential.h: void credential_write(const struct credential *, FILE *);
- * an error but leave the broken state in the credential object for further
- * examination. The non-gentle form will issue a warning to stderr and return
- * an empty credential.
-+ *
-+ * If allow_partial_url is non-zero, partial URLs are allowed, i.e. it can, but -+ * does not have to, contain
-+ *
-+ * - a protocol (or scheme) of the form "<protocol>://"
-+ *
-+ * - a host name (the part after the protocol and before the first slash after
-+ * that, if any)
-+ *
-+ * - a user name and potentially a password (as "<user>[:<password>]@" part of
-+ * the host name)
-+ *
-+ * - a path (the part after the host name, if any, starting with the slash) -+ *
-+ * Missing parts will be left unset in struct credential. Thus, https:// -+ * will have only the protocol set, example.com only the host name, and -+ * /git only the path.
-+ *
-+ * Note that an empty host name in an otherwise fully-qualified URL will be -+ * treated as unset when allow_partial_url is non-zero (and only then, -+ * otherwise it is treated as the empty string).
-+ *
-+ * The credential_from_url() function does not allow partial URLs.
- */
- void credential_from_url(struct credential *, const char *url);
--int credential_from_url_gently(struct credential *, const char *url, int quiet); -+int credential_from_url_gently(struct credential *, const char *url,
-+ int allow_partial_url, int quiet);
-
- int credential_match(const struct credential *have,
- const struct credential *want);
-
- ## fsck.c ##
-@@ fsck.c: static int check_submodule_url(const char *url)
- else if (url_to_curl_url(url, &curl_url)) {
- struct credential c = CREDENTIAL_INIT;
- int ret = 0;
-- if (credential_from_url_gently(&c, curl_url, 1) ||
-+ if (credential_from_url_gently(&c, curl_url, 0, 1) ||
- !*c.host)
- ret = -1;
- credential_clear(&c);
+ if (credential_from_url_gently(c, url, 0) < 0)
3: daedaffe960 ! 3: 0535908dd7e credential: handle credential.<partial-URL>.<key> again

     @@ Commit message
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## credential.c ##
     +@@ credential.c: int credential_match(const struct credential *want,
     + #undef CHECK
     + }
     + 
     ++
     ++static int credential_from_potentially_partial_url(struct credential *c,
     ++						   const char *url);
     ++
     + static int credential_config_callback(const char *var, const char *value,
     + 				      void *data)
     + {
      @@ credential.c: static int credential_config_callback(const char *var, const char *value,
       		char *url = xmemdupz(key, dot - key);
       		int matched;
       
      -		credential_from_url(&want, url);
     -+		if (credential_from_url_gently(&want, url, 1, 0) < 0) {
     ++		if (credential_from_potentially_partial_url(&want, url) < 0) {
      +			warning(_("skipping credential lookup for key: %s"),
      +				var);
      +			credential_clear(&want);
     @@ credential.c: static int credential_config_callback(const char *var, const char
       		matched = credential_match(&want, c);
       
       		credential_clear(&want);
     +@@ credential.c: static int credential_from_url_1(struct credential *c, const char *url,
     + 	return 0;
     + }
     + 
     ++static int credential_from_potentially_partial_url(struct credential *c,
     ++						   const char *url)
     ++{
     ++	return credential_from_url_1(c, url, 1, 0);
     ++}
     ++
     + int credential_from_url_gently(struct credential *c, const char *url, int quiet)
     + {
     + 	return credential_from_url_1(c, url, 0, quiet);
      
       ## t/t0300-credentials.sh ##
      @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work with missing protocol' '
     @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w
       
      +test_expect_success 'credential config with partial URLs' '
      +	echo "echo password=yep" | write_script git-credential-yep &&
     -+	test_write_lines url=https://[email protected]/repo.git >input &&
     ++	test_write_lines url=https://[email protected]/repo.git >stdin &&
      +	for partial in \
      +		example.com \
      +		[email protected] \
     @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w
      +		/repo.git
      +	do
      +		git -c credential.$partial.helper=yep \
     -+			credential fill <input >output &&
     -+		grep yep output ||
     ++			credential fill <stdin >stdout &&
     ++		grep yep stdout ||
      +		return 1
      +	done &&
      +
     @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w
      +		/repo
      +	do
      +		git -c credential.$partial.helper=yep \
     -+			credential fill <input >output &&
     -+		! grep yep output ||
     ++			credential fill <stdin >stdout &&
     ++		! grep yep stdout ||
      +		return 1
      +	done &&
      +
      +	git -c credential.$partial.helper=yep \
      +		-c credential.with%0anewline.username=uh-oh \
     -+		credential fill <input >output 2>error &&
     -+	test_i18ngrep "skipping credential lookup for key" error
     ++		credential fill <stdin >stdout 2>stderr &&
     ++	test_i18ngrep "skipping credential lookup for key" stderr
      +
      +'
      +

gitgitgadget

From: Johannes Schindelin <[email protected]>

There was a lot going on behind the scenes when the vulnerability and possible solutions were discussed. Grammar was not a primary focus, that's why this slipped in.

---
 credential.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 122a23cd2f1..5a86502d95c 100644
@@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *);
 /*
  * Parse a url into a credential struct, replacing any existing contents.
  *
- * Ifthe url can't be parsed (e.g., a missing "proto://" component), the
+ * If the url can't be parsed (e.g., a missing "proto://" component), the
  * resulting credential will be empty but we'll still return success from the
  * "gently" form.
  *

gitgitgadget

From: Johannes Schindelin <[email protected]>

Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we required from a URL in order to parse it into a struct credential. That led to serious vulnerabilities.

There was one call site, though, that really needed that leniency: when parsing config settings a la credential.dev.azure.com.useHTTPPath. Settings like this might be desired when users want to use, say, a given user name on a given host, regardless of the protocol to be used.

In preparation for fixing that bug, let's refactor the code to optionally allow for partial URLs. For the moment, this functionality is only exposed via the now-renamed function credential_from_url_1(), but it is not used. The intention is to make it easier to verify that this commit does not change the existing behavior unless explicitly allowing for partial URLs.

Please note that this patch does more than just reinstating a way to imitate the behavior before those CVE-2020-11008 fixes: Before that, we would simply ignore URLs without a protocol. In other words, misleadingly, the following setting would be applied to _all_ URLs:

[credential "example.com"]
username = that-me

The obvious intention is to match the host name only. With this patch, we allow precisely that: when parsing the URL with non-zero allow_partial_url, we do not simply return success if there was no protocol, but we simply leave the protocol unset and continue parsing the URL.

---
 credential.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

index 64a841eddca..7dbbf26f174 100644
@@ -343,8 +343,31 @@ static int check_url_component(const char *url, int quiet,
 	return -1;
 }
 
-int credential_from_url_gently(struct credential *c, const char *url,
-			       int quiet)
+/*
+ * Potentially-partial URLs can, but do not have to, contain
+ *
+ * - a protocol (or scheme) of the form "<protocol>://"
+ *
+ * - a host name (the part after the protocol and before the first slash after
+ *   that, if any)
+ *
+ * - a user name and potentially a password (as "<user>[:<password>]@" part of
+ *   the host name)
+ *
+ * - a path (the part after the host name, if any, starting with the slash)
+ *
+ * Missing parts will be left unset in `struct credential`. Thus, `https://`
+ * will have only the `protocol` set, `example.com` only the host name, and
+ * `/git` only the path.
+ *
+ * Note that an empty host name in an otherwise fully-qualified URL (e.g.
+ * `cert:///path/to/cert.pem`) will be treated as unset if we expect the URL to
+ * be potentially partial, and only then (otherwise, the empty string is used).
+ *
+ * The credential_from_url() function does not allow partial URLs.
+ */
+static int credential_from_url_1(struct credential *c, const char *url,
+				 int allow_partial_url, int quiet)
 {
 	const char *at, *colon, *cp, *slash, *host, *proto_end;
 

@@ -357,12 +380,12 @@ int credential_from_url_gently(struct credential *c, const char *url,
 	 *   (3) proto://<user>:<pass>@<host>/...
 	 */
 	proto_end = strstr(url, "://");
-	if (!proto_end || proto_end == url) {
+	if (!allow_partial_url && (!proto_end || proto_end == url)) {
 		if (!quiet)
 			warning(_("url has no scheme: %s"), url);
 		return -1;
 	}
-	cp = proto_end + 3;
+	cp = proto_end ? proto_end + 3 : url;
 	at = strchr(cp, '@');
 	colon = strchr(cp, ':');
 	slash = strchrnul(cp, '/');

@@ -382,8 +405,10 @@ int credential_from_url_gently(struct credential *c, const char *url,
 		host = at + 1;
 	}
 
-	c->protocol = xmemdupz(url, proto_end - url);
-	c->host = url_decode_mem(host, slash - host);
+	if (proto_end && proto_end - url > 0)
+		c->protocol = xmemdupz(url, proto_end - url);
+	if (!allow_partial_url || slash - host > 0)
+		c->host = url_decode_mem(host, slash - host);
 	/* Trim leading and trailing slashes from path */
 	while (*slash == '/')
 		slash++;

@@ -405,6 +430,11 @@ int credential_from_url_gently(struct credential *c, const char *url,
 	return 0;
 }
 
+int credential_from_url_gently(struct credential *c, const char *url, int quiet)
+{
+	return credential_from_url_1(c, url, 0, quiet);
+}
+
 void credential_from_url(struct credential *c, const char *url)
 {
 	if (credential_from_url_gently(c, url, 0) < 0)

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the patches for CVE-2020-11008, the ability to specify credential settings in the config for partial URLs got lost. For example, it used to be possible to specify a credential helper for a specific protocol:

	[credential "https://"]
		helper = my-https-helper

Likewise, it used to be possible to configure settings for a specific host, e.g.:

[credential "dev.azure.com"]
useHTTPPath = true

Let's reinstate this behavior.

While at it, increase the test coverage to document and verify the behavior with a couple other categories of partial URLs.

---
 credential.c           | 18 +++++++++++++++++-
 t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

index 7dbbf26f174..c1a9ca4e485 100644
@@ -35,6 +35,10 @@ int credential_match(const struct credential *want,
 #undef CHECK
 }
 
+
+static int credential_from_potentially_partial_url(struct credential *c,
+						   const char *url);
+
 static int credential_config_callback(const char *var, const char *value,
 				      void *data)
 {

@@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,
 		char *url = xmemdupz(key, dot - key);
 		int matched;
 
-		credential_from_url(&want, url);
+		if (credential_from_potentially_partial_url(&want, url) < 0) {
+			warning(_("skipping credential lookup for key: %s"),
+				var);
+			credential_clear(&want);
+			free(url);
+			return 0;
+		}
 		matched = credential_match(&want, c);
 
 		credential_clear(&want);

@@ -430,6 +440,12 @@ static int credential_from_url_1(struct credential *c, const char *url,
 	return 0;
 }
 
+static int credential_from_potentially_partial_url(struct credential *c,
+						   const char *url)
+{
+	return credential_from_url_1(c, url, 1, 0);
+}
+
 int credential_from_url_gently(struct credential *c, const char *url, int quiet)
 {
 	return credential_from_url_1(c, url, 0, quiet);

index efed3ea2955..f796bbfd48b 100755
@@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' '
 	test_i18ncmp expect stderr
 '
 
+test_expect_success 'credential config with partial URLs' '
+	echo "echo password=yep" | write_script git-credential-yep &&
+	test_write_lines url=https://[email protected]/repo.git >stdin &&
+	for partial in \
+		example.com \
+		[email protected] \
+		https:// \
+		https://example.com \
+		https://example.com/ \
+		https://[email protected] \
+		https://[email protected]/ \
+		https://example.com/repo.git \
+		https://[email protected]/repo.git \
+		/repo.git
+	do
+		git -c credential.$partial.helper=yep \
+			credential fill <stdin >stdout &&
+		grep yep stdout ||
+		return 1
+	done &&
+
+	for partial in \
+		dont.use.this \
+		http:// \
+		/repo
+	do
+		git -c credential.$partial.helper=yep \
+			credential fill <stdin >stdout &&
+		! grep yep stdout ||
+		return 1
+	done &&
+
+	git -c credential.$partial.helper=yep \
+		-c credential.with%0anewline.username=uh-oh \
+		credential fill <stdin >stdout 2>stderr &&
+	test_i18ngrep "skipping credential lookup for key" stderr
+
+'
+
 test_done

gitgitgadget

Hi Carlo,

It might be more grep'able, but it sounds really awkward to me, that's why I did not use that name (it was my first candidate).

Let me counter this:

- It would take extra code _to prevent_ the username from being used, and

- There is precedent where the user name _does_ matter: it is relatively normal to access different orgs' repositories at
https://dev.azure.com/<org>/<repo>/_git via different accounts.

Together, those points convince me that special-casing the username _and explicitly ignoring it_ would not make sense.

Thank you for reviewing it!

Ciao,
Dscho

"Johannes Schindelin via GitGitGadget" <[email protected]>

Hi Junio,

I prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not necessarily feel 100% comfortable with that approach yet, but at least it lets the new test case of t0300 pass.

Ciao,
Dscho

The -2.17 patch series (your [v3 3/3]) ends t0300 like so

    +	done &&
    +
    +	git -c credential.$partial.helper=yep \
    +		-c credential.with%0anewline.username=uh-oh \
    +		credential fill <stdin >stdout 2>stderr &&
    +	test_i18ngrep "skipping credential lookup for key" stderr
    +
    +'
    +
     test_done

while the other branch lacks the extra blank line just before the single quote is closed. I queued 850ef627 (SQUASH??? lose excess blank line to match the other side of the eventual merge, 2020-04-24) on top so that the "-s ours" merge would be without unnecessary evil-merge noise. You probably would want to squash it in.

Thanks.

Hi Junio,

Thank you for pointing that out. I already did that:
https://github.com/gitgitgadget/git/compare/0535908dd7ea4487b342c0f86182579279c57b34..d59738ecf741a5fddd70f133eaaf69768e245bcc

Do you want me to send another iteration, for completeness' sake?

Ciao,
Dscho

Then if I squash 850ef627 on my end, we'd be in sync, so unless there is any other change, no need to resend it. Have we got enough eyeballs that looked at the patches already?

Thanks.

Hi Junio,

Perfect, thanks!

I would obviously prefer more reviews.

Having said that, GitHub Desktop rolled out a new release this past Monday with the MinGit backport I prepared with these here patches, and the only additional report at https://github.com/desktop/desktop/issues/9597 talks about macOS (and the MinGit backport is Windows-only). Which I take as good news for the robustness and correctness of the fixes.

Ciao,
Dscho

Good. My preference actually is to fast-track this down to 'master' without the usual 'at least about a week in next' gestation period.