From 692455ed04743fbf2c102ae4bcaa0ac7bcc433a0 Mon Sep 17 00:00:00 2001

From: Christian Couder <chriscool@tuxfamily.org>

Date: Sun, 1 Dec 2013 08:49:16 +0100

Subject: [PATCH 01/11] strbuf: introduce starts_with() and ends_with()

prefixcmp() and suffixcmp() share the common "cmp" suffix that

typically are used to name functions that can be used for ordering,

but they can't, because they are not antisymmetric:

        prefixcmp("foo", "foobar") < 0

        prefixcmp("foobar", "foo") == 0

We in fact do not use these functions for ordering.  Replace them

with functions that just check for equality.

Add starts_with() and end_with() that will be used to replace

prefixcmp() and suffixcmp(), respectively, as the first step.  These

are named after corresponding functions/methods in programming

languages, like Java, Python and Ruby.

In vcs-svn/fast_export.c, there was already an ends_with() function

that did the same thing. Let's use the new one instead while at it.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

Signed-off-by: Junio C Hamano <gitster@pobox.com>

---

 git-compat-util.h     |  2 ++

 strbuf.c              | 18 ++++++++++++++++++

 vcs-svn/fast_export.c | 11 +----------

 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h

index bd62ab07a8..c4f0afc104 100644

--- a/git-compat-util.h

+++ b/git-compat-util.h

@@ -341,7 +341,9 @@ extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_lis

 extern void set_error_routine(void (*routine)(const char *err, va_list params));

 extern void set_die_is_recursing_routine(int (*routine)(void));

+extern int starts_with(const char *str, const char *prefix);

 extern int prefixcmp(const char *str, const char *prefix);

+extern int ends_with(const char *str, const char *suffix);

 extern int suffixcmp(const char *str, const char *suffix);

 static inline const char *skip_prefix(const char *str, const char *prefix)

diff --git a/strbuf.c b/strbuf.c

index 1170d01c43..83caf4a914 100644

--- a/strbuf.c

+++ b/strbuf.c

@@ -1,6 +1,15 @@

 #include "cache.h"

 #include "refs.h"

+int starts_with(const char *str, const char *prefix)

+{

+	for (; ; str++, prefix++)

+		if (!*prefix)

+			return 1;

+		else if (*str != *prefix)

+			return 0;

+}

+

 int prefixcmp(const char *str, const char *prefix)

 {

 	for (; ; str++, prefix++)

@@ -10,6 +19,15 @@ int prefixcmp(const char *str, const char *prefix)

 			return (unsigned char)*prefix - (unsigned char)*str;

 }

+int ends_with(const char *str, const char *suffix)

+{

+	int len = strlen(str), suflen = strlen(suffix);

+	if (len < suflen)

+		return 0;

+	else

+		return !strcmp(str + len - suflen, suffix);

+}

+

 int suffixcmp(const char *str, const char *suffix)

 {

 	int len = strlen(str), suflen = strlen(suffix);

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c

index f2b23c81de..bd0f2c2b86 100644

--- a/vcs-svn/fast_export.c

+++ b/vcs-svn/fast_export.c

@@ -162,22 +162,13 @@ static void die_short_read(struct line_buffer *input)

 	die("invalid dump: unexpected end of file");

 }

-static int ends_with(const char *s, size_t len, const char *suffix)

-{

-	const size_t suffixlen = strlen(suffix);

-	if (len < suffixlen)

-		return 0;

-	return !memcmp(s + len - suffixlen, suffix, suffixlen);

-}

-

 static int parse_cat_response_line(const char *header, off_t *len)

 {

-	size_t headerlen = strlen(header);

 	uintmax_t n;

 	const char *type;

 	const char *end;

-	if (ends_with(header, headerlen, " missing"))

+	if (ends_with(header, " missing"))

 		return error("cat-blob reports missing blob: %s", header);

 	type = strstr(header, " blob ");

 	if (!type)

-- 

2.26.2

From 6a8afa912b4d2fe5169fbb904537d1a50a3eb66f Mon Sep 17 00:00:00 2001

From: Jeff King <peff@peff.net>

Date: Thu, 12 Mar 2020 01:31:11 -0400

Subject: [PATCH 02/11] credential: detect unrepresentable values when parsing

 urls

The credential protocol can't represent newlines in values, but URLs can

embed percent-encoded newlines in various components. A previous commit

taught the low-level writing routines to die() when encountering this,

but we can be a little friendlier to the user by detecting them earlier

and handling them gracefully.

This patch teaches credential_from_url() to notice such components,

issue a warning, and blank the credential (which will generally result

in prompting the user for a username and password). We blank the whole

credential in this case. Another option would be to blank only the

invalid component. However, we're probably better off not feeding a

partially-parsed URL result to a credential helper. We don't know how a

given helper would handle it, so we're better off to err on the side of

matching nothing rather than something unexpected.

The die() call in credential_write() is _probably_ impossible to reach

after this patch. Values should end up in credential structs only by URL

parsing (which is covered here), or by reading credential protocol input

(which by definition cannot read a newline into a value). But we should

definitely keep the low-level check, as it's our final and most accurate

line of defense against protocol injection attacks. Arguably it could

become a BUG(), but it probably doesn't matter much either way.

Note that the public interface of credential_from_url() grows a little

more than we need here. We'll use the extra flexibility in a future

patch to help fsck catch these cases.

---

 credential.c           | 36 ++++++++++++++++++++++++++++++++++--

 credential.h           | 16 ++++++++++++++++

 t/t0300-credentials.sh | 12 ++++++++++--

 3 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/credential.c b/credential.c

index 096cc5efe9..f56a0a2c02 100644

--- a/credential.c

+++ b/credential.c

@@ -317,7 +317,22 @@ void credential_reject(struct credential *c)

 	c->approved = 0;

 }

-void credential_from_url(struct credential *c, const char *url)

+static int check_url_component(const char *url, int quiet,

+			       const char *name, const char *value)

+{

+	if (!value)

+		return 0;

+	if (!strchr(value, '\n'))

+		return 0;

+

+	if (!quiet)

+		warning(_("url contains a newline in its %s component: %s"),

+			name, url);

+	return -1;

+}

+

+int credential_from_url_gently(struct credential *c, const char *url,

+			       int quiet)

 {

 	const char *at, *colon, *cp, *slash, *host, *proto_end;

@@ -331,7 +346,7 @@ void credential_from_url(struct credential *c, const char *url)

 	 */

 	proto_end = strstr(url, "://");

 	if (!proto_end)

-		return;

+		return 0;

 	cp = proto_end + 3;

 	at = strchr(cp, '@');

 	colon = strchr(cp, ':');

@@ -366,4 +381,21 @@ void credential_from_url(struct credential *c, const char *url)

 		while (p > c->path && *p == '/')

 			*p-- = '\0';

 	}

+

+	if (check_url_component(url, quiet, "username", c->username) < 0 ||

+	    check_url_component(url, quiet, "password", c->password) < 0 ||

+	    check_url_component(url, quiet, "protocol", c->protocol) < 0 ||

+	    check_url_component(url, quiet, "host", c->host) < 0 ||

+	    check_url_component(url, quiet, "path", c->path) < 0)

+		return -1;

+

+	return 0;

+}

+

+void credential_from_url(struct credential *c, const char *url)

+{

+	if (credential_from_url_gently(c, url, 0) < 0) {

+		warning(_("skipping credential lookup for url: %s"), url);

+		credential_clear(c);

+	}

 }

diff --git a/credential.h b/credential.h

index 0c3e85e8e4..2846e47cc8 100644

--- a/credential.h

+++ b/credential.h

@@ -27,7 +27,23 @@ void credential_reject(struct credential *);

 int credential_read(struct credential *, FILE *);

 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

+ * resulting credential will be empty but we'll still return success from the

+ * "gently" form.

+ *

+ * If we encounter a component which cannot be represented as a credential

+ * value (e.g., because it contains a newline), the "gently" form will return

+ * 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.

+ */

 void credential_from_url(struct credential *, const char *url);

+int credential_from_url_gently(struct credential *, const char *url, int quiet);

+

 int credential_match(const struct credential *have,

 		     const struct credential *want);

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh

index acc167c774..88bcf6534b 100755

--- a/t/t0300-credentials.sh

+++ b/t/t0300-credentials.sh

@@ -289,9 +289,17 @@ test_expect_success 'http paths can be part of context' '

 	EOF

 '

-test_expect_success 'url parser rejects embedded newlines' '

-	test_must_fail git credential fill <<-\EOF

+test_expect_success 'url parser ignores embedded newlines' '

+	check fill <<-EOF

 	url=https://one.example.com?%0ahost=two.example.com/

+	--

+	username=askpass-username

+	password=askpass-password

+	--

+	warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/

+	warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/

+	askpass: Username:

+	askpass: Password:

 	EOF

 '

-- 

2.26.2

From 3b80da45e70d60a4abc91acc45d23d18092708a4 Mon Sep 17 00:00:00 2001

From: Jeff King <peff@peff.net>

Date: Wed, 11 Mar 2020 18:48:24 -0400

Subject: [PATCH 03/11] fsck: detect gitmodules URLs with embedded newlines

The credential protocol can't handle values with newlines. We already

detect and block any such URLs from being used with credential helpers,

but let's also add an fsck check to detect and block gitmodules files

with such URLs. That will let us notice the problem earlier when

transfer.fsckObjects is turned on. And in particular it will prevent bad

objects from spreading, which may protect downstream users running older

versions of Git.

We'll file this under the existing gitmodulesUrl flag, which covers URLs

with option injection. There's really no need to distinguish the exact

flaw in the URL in this context. Likewise, I've expanded the description

of t7416 to cover all types of bogus URLs.

---

 fsck.c                        | 16 +++++++++++++++-

 t/t7416-submodule-dash-url.sh | 18 +++++++++++++++++-

 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/fsck.c b/fsck.c

index 1ed7172ee6..b72faea0c2 100644

--- a/fsck.c

+++ b/fsck.c

@@ -8,6 +8,7 @@

 #include "fsck.h"

 #include "hashmap.h"

 #include "submodule.h"

+#include "credential.h"

 struct oidhash_entry {

 	struct hashmap_entry ent;

@@ -419,6 +420,19 @@ static int fsck_tag(struct tag *tag, const char *data,

 	return 0;

 }

+static int check_submodule_url(const char *url)

+{

+	struct credential c = CREDENTIAL_INIT;

+	int ret;

+

+	if (looks_like_command_line_option(url))

+		return -1;

+

+	ret = credential_from_url_gently(&c, url, 1);

+	credential_clear(&c);

+	return ret;

+}

+

 struct fsck_gitmodules_data {

 	struct object *obj;

 	fsck_error error_func;

@@ -442,7 +456,7 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata)

 					      "disallowed submodule name: %s",

 					      name);

 	if (!strcmp(key, "url") && value &&

-	    looks_like_command_line_option(value))

+	    check_submodule_url(value) < 0)

 		data->ret += data->error_func(data->obj, FSCK_ERROR,

 					      "disallowed submodule url: %s",

 					      value);

diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh

index e85f2e9d29..5356d8766a 100755

--- a/t/t7416-submodule-dash-url.sh

+++ b/t/t7416-submodule-dash-url.sh

@@ -1,6 +1,6 @@

 #!/bin/sh

-test_description='check handling of .gitmodule url with dash'

+test_description='check handling of disallowed .gitmodule urls'

 . ./test-lib.sh

 test_expect_success 'create submodule with protected dash in url' '

@@ -46,4 +46,20 @@ test_expect_success 'fsck rejects unprotected dash' '

 	test_i18ngrep "disallowed submodule url" err

 '

+test_expect_success 'fsck rejects embedded newline in url' '

+	# create an orphan branch to avoid existing .gitmodules objects

+	git checkout --orphan newline &&

+	cat >.gitmodules <<-\EOF &&

+	[submodule "foo"]

+	url = "https://one.example.com?%0ahost=two.example.com/foo.git"

+	EOF

+	git add .gitmodules &&

+	git commit -m "gitmodules with newline" &&

+	test_when_finished "rm -rf dst" &&

+	git init --bare dst &&

+	( cd dst && git config transfer.fsckObjects true ) &&

+	test_must_fail git push dst HEAD 2>err &&

+	grep "disallowed submodule url" err

+'

+

 test_done

-- 

2.26.2

From a44bd3335d9dd715e2d141a593704aababcc83e4 Mon Sep 17 00:00:00 2001

From: Jeff King <peff@peff.net>

Date: Sat, 18 Apr 2020 20:47:30 -0700

Subject: [PATCH 04/11] t0300: use more realistic inputs

Many of the tests in t0300 give partial inputs to git-credential,

omitting a protocol or hostname. We're checking only high-level things

like whether and how helpers are invoked at all, and we don't care about

specific hosts. However, in preparation for tightening up the rules

about when we're willing to run a helper, let's start using input that's

a bit more realistic: pretend as if http://example.com is being

examined.

This shouldn't change the point of any of the tests, but do note we have

to adjust the expected output to accommodate this (filling a credential

will repeat back the protocol/host fields to stdout, and the helper

debug messages and askpass prompt will change on stderr).

Signed-off-by: Jeff King <peff@peff.net>

Reviewed-by: Taylor Blau <me@ttaylorr.com>

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

---

 t/t0300-credentials.sh | 76 ++++++++++++++++++++++++++++++++++++++++--

 1 file changed, 73 insertions(+), 3 deletions(-)

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh

index 88bcf6534b..2f0ff6bcae 100755

--- a/t/t0300-credentials.sh

+++ b/t/t0300-credentials.sh

@@ -35,43 +35,71 @@ test_expect_success 'setup helper scripts' '

 test_expect_success 'credential_fill invokes helper' '

 	check fill "verbatim foo bar" <<-\EOF

+	protocol=http

+	host=example.com

 	--

+	protocol=http

+	host=example.com

 	username=foo

 	password=bar

 	--

 	verbatim: get

+	verbatim: protocol=http

+	verbatim: host=example.com

 	EOF

 '

 test_expect_success 'credential_fill invokes multiple helpers' '

 	check fill useless "verbatim foo bar" <<-\EOF

+	protocol=http

+	host=example.com

 	--

+	protocol=http

+	host=example.com

 	username=foo

 	password=bar

 	--

 	useless: get

+	useless: protocol=http

+	useless: host=example.com

 	verbatim: get

+	verbatim: protocol=http

+	verbatim: host=example.com

 	EOF

 '

 test_expect_success 'credential_fill stops when we get a full response' '

 	check fill "verbatim one two" "verbatim three four" <<-\EOF

+	protocol=http

+	host=example.com

 	--

+	protocol=http

+	host=example.com

 	username=one

 	password=two

 	--

 	verbatim: get

+	verbatim: protocol=http

+	verbatim: host=example.com

 	EOF

 '

 test_expect_success 'credential_fill continues through partial response' '

 	check fill "verbatim one \"\"" "verbatim two three" <<-\EOF

+	protocol=http

+	host=example.com

 	--

+	protocol=http

+	host=example.com

 	username=two

 	password=three

 	--

 	verbatim: get

+	verbatim: protocol=http

+	verbatim: host=example.com

 	verbatim: get

+	verbatim: protocol=http

+	verbatim: host=example.com

 	verbatim: username=one

 	EOF

 '

@@ -97,14 +125,20 @@ test_expect_success 'credential_fill passes along metadata' '

 test_expect_success 'credential_approve calls all helpers' '

 	check approve useless "verbatim one two" <<-\EOF

+	protocol=http

+	host=example.com

 	username=foo

 	password=bar

 	--

 	--

 	useless: store

+	useless: protocol=http

+	useless: host=example.com

 	useless: username=foo

 	useless: password=bar

 	verbatim: store

+	verbatim: protocol=http

+	verbatim: host=example.com

 	verbatim: username=foo

 	verbatim: password=bar

 	EOF

@@ -112,6 +146,8 @@ test_expect_success 'credential_approve calls all helpers' '

 test_expect_success 'do not bother storing password-less credential' '

 	check approve useless <<-\EOF

+	protocol=http

+	host=example.com

 	username=foo

 	--

 	--

@@ -121,14 +157,20 @@ test_expect_success 'do not bother storing password-less credential' '

 test_expect_success 'credential_reject calls all helpers' '

 	check reject useless "verbatim one two" <<-\EOF

+	protocol=http

+	host=example.com

 	username=foo

 	password=bar

 	--

 	--

 	useless: erase

+	useless: protocol=http

+	useless: host=example.com

 	useless: username=foo

 	useless: password=bar

 	verbatim: erase

+	verbatim: protocol=http

+	verbatim: host=example.com

 	verbatim: username=foo

 	verbatim: password=bar

 	EOF

@@ -136,33 +178,49 @@ test_expect_success 'credential_reject calls all helpers' '

 test_expect_success 'usernames can be preserved' '

 	check fill "verbatim \"\" three" <<-\EOF

+	protocol=http

+	host=example.com

 	username=one

 	--

+	protocol=http

+	host=example.com

 	username=one

 	password=three

 	--

 	verbatim: get

+	verbatim: protocol=http

+	verbatim: host=example.com

 	verbatim: username=one

 	EOF

 '

 test_expect_success 'usernames can be overridden' '

 	check fill "verbatim two three" <<-\EOF

+	protocol=http

+	host=example.com

 	username=one

 	--

+	protocol=http

+	host=example.com

 	username=two

 	password=three

 	--

 	verbatim: get

+	verbatim: protocol=http

+	verbatim: host=example.com

 	verbatim: username=one

 	EOF

 '

 test_expect_success 'do not bother completing already-full credential' '

 	check fill "verbatim three four" <<-\EOF

+	protocol=http

+	host=example.com

 	username=one

 	password=two

 	--

+	protocol=http

+	host=example.com

 	username=one

 	password=two

 	--

@@ -174,23 +232,31 @@ test_expect_success 'do not bother completing already-full credential' '

 # askpass helper is run, we know the internal getpass is working.

 test_expect_success 'empty helper list falls back to internal getpass' '

 	check fill <<-\EOF

+	protocol=http

+	host=example.com

 	--

+	protocol=http

+	host=example.com

 	username=askpass-username

 	password=askpass-password

 	--

-	askpass: Username:

-	askpass: Password:

+	askpass: Username for '\''http://example.com'\'':

+	askpass: Password for '\''http://askpass-username@example.com'\'':

 	EOF

 '

 test_expect_success 'internal getpass does not ask for known username' '

 	check fill <<-\EOF

+	protocol=http

+	host=example.com

 	username=foo

 	--

+	protocol=http

+	host=example.com

 	username=foo

 	password=askpass-password

 	--

-	askpass: Password:

+	askpass: Password for '\''http://foo@example.com'\'':

 	EOF

 '

@@ -202,7 +268,11 @@ HELPER="!f() {

 test_expect_success 'respect configured credentials' '

 	test_config credential.helper "$HELPER" &&

 	check fill <<-\EOF

+	protocol=http

+	host=example.com

 	--

+	protocol=http

+	host=example.com

 	username=foo

 	password=bar

 	--

-- 

2.26.2

From 6eae9cd97a8f481c04dd4f8ad507eeff69a335da Mon Sep 17 00:00:00 2001

From: Jeff King <peff@peff.net>

Date: Sat, 18 Apr 2020 20:48:05 -0700

Subject: [PATCH 05/11] credential: parse URL without host as empty host, not

 unset

We may feed a URL like "cert:///path/to/cert.pem" into the credential

machinery to get the key for a client-side certificate. That

credential has no hostname field, which is about to be disallowed (to

avoid confusion with protocols where a helper _would_ expect a

hostname).

This means as of the next patch, credential helpers won't work for

unlocking certs. Let's fix that by doing two things:

  - when we parse a url with an empty host, set the host field to the

    empty string (asking only to match stored entries with an empty

    host) rather than NULL (asking to match _any_ host).

  - when we build a cert:// credential by hand, similarly assign an

    empty string

It's the latter that is more likely to impact real users in practice,

since it's what's used for http connections. But we don't have good

infrastructure to test it.

The url-parsing version will help anybody using git-credential in a

script, and is easy to test.

Signed-off-by: Jeff King <peff@peff.net>

Reviewed-by: Taylor Blau <me@ttaylorr.com>

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

---

 credential.c           |  3 +--

 http.c                 |  1 +

 t/t0300-credentials.sh | 17 +++++++++++++++++

 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/credential.c b/credential.c

index f56a0a2c02..745b0e6a6d 100644

--- a/credential.c

+++ b/credential.c

@@ -369,8 +369,7 @@ int credential_from_url_gently(struct credential *c, const char *url,

 	if (proto_end - url > 0)

 		c->protocol = xmemdupz(url, proto_end - url);

-	if (slash - host > 0)

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

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

 	/* Trim leading and trailing slashes from path */

 	while (*slash == '/')

 		slash++;

diff --git a/http.c b/http.c

index 3320590e4b..1533c9338c 100644

--- a/http.c

+++ b/http.c

@@ -269,6 +269,7 @@ static int has_cert_password(void)

 		return 0;

 	if (!cert_auth.password) {

 		cert_auth.protocol = xstrdup("cert");

+		cert_auth.host = xstrdup("");

 		cert_auth.username = xstrdup("");

 		cert_auth.path = xstrdup(ssl_cert);

 		credential_fill(&cert_auth);

diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh

index 2f0ff6bcae..0bce92af98 100755

--- a/t/t0300-credentials.sh

+++ b/t/t0300-credentials.sh

@@ -373,4 +373,21 @@ test_expect_success 'url parser ignores embedded newlines' '

 	EOF

 '

+test_expect_success 'host-less URLs are parsed as empty host' '

+	check fill "verbatim foo bar" <<-\EOF

+	url=cert:///path/to/cert.pem

+	--

+	protocol=cert

+	host=

+	path=path/to/cert.pem

+	username=foo

+	password=bar

+	--

+	verbatim: get

+	verbatim: protocol=cert

+	verbatim: host=

+	verbatim: path=path/to/cert.pem

+	EOF

+'

+

 test_done

-- 

2.26.2

From 215df2b679d0f0d3458fafe27f4e3a3f05544400 Mon Sep 17 00:00:00 2001

From: Jeff King <peff@peff.net>

Date: Sat, 18 Apr 2020 20:50:48 -0700

Subject: [PATCH 06/11] credential: refuse to operate when missing host or

 protocol