contrib/credential: avoid protocol injection attacks

This series addresses a handful of potential protocol injection attacks possible via some of the credential helpers in contrib/credential, and the new "wwwauth[]" directive.

The attack is described in complete detail in 2/7, but roughly boils down to using a long line to incur multiple fgets() calls which can treat data in the middle of the line as if it appeared at the beginning.

Luckily, all protocol fields part of tagged versions of Git are immune from this attack. Briefly:

- "protocol" is restricted to known values
- "host" is immune because curl will reject hostnames that have a '=' character in them, which would be required to carry out this attack.
- "username", and "path" are immune, because the buffer characters to fill out the first fgets() call would pollute the username/path field, causing the credential helper to return nothing
- "password" is immune because providing a password instructs credential helpers to avoid filling credentials in the first place.

But the new "wwwauth[]" field does allow this attack to take place.

Since these credential helpers are tested via t0303 (which requires some extensive set-up), we opted not to make these fixes during the last embargo period, and instead do them before the "wwwauth[]" feature becomes part of a tagged version.

With the additional time, we have been able to verify that the affected credential helpers which are modified in this series all fail the new test before their patches, and pass afterwords. Thanks to Peff for looking at libsecret, Matthew Cheetham for looking at wincred. I looked at osxkeychain.

Taylor Blau (7):
credential.c: store "wwwauth[]" values in credential_read() t/lib-credential.sh: ensure credential helpers handle long headers contrib/credential: avoid fixed-size buffer in osxkeychain contrib/credential: remove 'gnome-keyring' credential helper contrib/credential: .gitignore libsecret build artifacts contrib/credential: avoid fixed-size buffer in libsecret contrib/credential: embiggen fixed-size buffer in wincred

 contrib/credential/gnome-keyring/.gitignore   |   1 -
 contrib/credential/gnome-keyring/Makefile     |  25 -
 .../git-credential-gnome-keyring.c            | 470 ------------------
 contrib/credential/libsecret/.gitignore       |   1 +
 .../libsecret/git-credential-libsecret.c      |  15 +-
 .../osxkeychain/git-credential-osxkeychain.c  |  10 +-
 .../wincred/git-credential-wincred.c          |  21 +-
 credential.c                                  |   2 +
 t/lib-credential.sh                           |  29 ++
 9 files changed, 63 insertions(+), 511 deletions(-)
 delete mode 100644 contrib/credential/gnome-keyring/.gitignore
 delete mode 100644 contrib/credential/gnome-keyring/Makefile
 delete mode 100644 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
 create mode 100644 contrib/credential/libsecret/.gitignore

2.40.1.452.gb3cd41c833

Teach git-credential to read "wwwauth[]" value(s) when parsing the output of a credential helper.

These extra headers are not needed for Git's own HTTP support to use the feature internally, but the feature would not be available for a scripted caller (say, git-remote-mediawiki providing the header in the same way).

As a bonus, this also makes it easier to use wwwauth[] in synthetic credential inputs in our test suite.

---
 credential.c | 2 ++
 1 file changed, 2 insertions(+)

index e6417bf880..96fc0daa35 100644
@@ -238,6 +238,8 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "path")) {
 			free(c->path);
 			c->path = xstrdup(value);
+		} else if (!strcmp(key, "wwwauth[]")) {
+			strvec_push(&c->wwwauth_headers, value);
 		} else if (!strcmp(key, "password_expiry_utc")) {
 			errno = 0;
 			c->password_expiry_utc = parse_timestamp(value, NULL, 10);

2.40.1.452.gb3cd41c833

Add a test ensuring that the "wwwauth[]" field cannot be used to inject malicious data into the credential helper stream.

Many of the credential helpers in contrib/credential read the newline-delimited protocol stream one line at a time by repeatedly calling fgets() into a fixed-size buffer.

This assumes that each line is no more than 1024 characters long, since each iteration of the loop assumes that it is parsing starting at the beginning of a new line in the stream. However, similar to a5bb10fd5e (config: avoid fixed-sized buffer when renaming/deleting a section, 2023-04-06), if a line is longer than 1024 characters, a malicious actor can embed another command within an existing line, bypassing the usual checks introduced in 9a6bbee800 (credential: avoid writing values with newlines, 2020-03-11).

As with the problem fixed in that commit, specially crafted input can cause the helper to return the credential for the wrong host, letting an attacker trick the victim into sending credentials for one host to another.

Luckily, all parts of the credential helper protocol that are available in a tagged release of Git are immune to this attack:

- "protocol" is restricted to known values, and is thus immune.

- "host" is immune because curl will reject hostnames that have a '=' character in them, which would be required to carry out this attack.

- "username" is immune, because the buffer characters to fill out the first fgets() call would pollute the username field, causing the credential helper to return nothing (because it would match a username if present, and the username of the credential to be stolen is likely not 1024 characters).

- "password" is immune because providing a password instructs credential helpers to avoid filling credentials in the first place.

- "path" is similar to username; if present, it is not likely to match any credential the victim is storing. It's also not enabled by default; the victim would have to set credential.useHTTPPath explicitly.

However, the new "wwwauth[]" field introduced via 5f2117b24f (credential: add WWW-Authenticate header to cred requests, 2023-02-27) can be used to inject data into the credential helper stream. For example, running:

    {
      printf 'HTTP/1.1 401\r\n'
      printf 'WWW-Authenticate: basic realm='
      perl -e 'print "a" x 1024'
      printf 'host=victim.com\r\n'
    } | nc -Nlp 8080

in one terminal, and then:

    git clone http://localhost:8080

in another would result in a line like:

    wwwauth[]=basic realm=aaa[...]aaahost=victim.com

being sent to the credential helper. If we tweak that "1024" to align our output with the helper's buffer size and the rest of the data on the line, it can cause the helper to see "host=victim.com" on its own line, allowing motivated attackers to exfiltrate credentials belonging to "victim.com".

The below test demonstrates these failures and provides us with a test to ensure that our fix is correct. That said, it has a couple of shortcomings:

- it's in t0303, since that's the only mechanism we have for testing random helpers. But that means nobody is going to run it under normal circumstances.

- to get the attack right, it has to line up the stuffed name with the buffer size, so we depend on the exact buffer size. I parameterized it so it could be used to test other helpers, but in practice it's not likely for anybody to do that.

Still, it's the best we can do, and will help us confirm the presence of the problem (and our fixes) in the new few patches.

---
 t/lib-credential.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

index 5ea8bc9f1d..d7d03c3cd9 100644
@@ -270,6 +270,35 @@ helper_test() {
 		password=
 		EOF
 	'
+
+	: ${GIT_TEST_LONG_CRED_BUFFER:=1024}
+	# 23 bytes accounts for "wwwauth[]=basic realm=" plus NUL
+	LONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))
+	LONG_VALUE=$(perl -e 'print "a" x shift' $LONG_VALUE_LEN)
+
+	test_expect_success "helper ($HELPER) not confused by long header" '
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=victim.example.com
+		username=user
+		password=to-be-stolen
+		EOF
+
+		check fill $HELPER <<-EOF
+		protocol=https
+		host=badguy.example.com
+		wwwauth[]=basic realm=${LONG_VALUE}host=victim.example.com
+		--
+		protocol=https
+		host=badguy.example.com
+		username=askpass-username
+		password=askpass-password
+		wwwauth[]=basic realm=${LONG_VALUE}host=victim.example.com
+		--
+		askpass: Username for '\''https://badguy.example.com'\'':
+		askpass: Password for '\''https://[email protected]'\'':
+		EOF
+	'
 }
 
 helper_test_timeout() {

2.40.1.452.gb3cd41c833

The macOS Keychain-based credential helper reads the newline-delimited protocol stream one line at a time by repeatedly calling fgets() into a fixed-size buffer, and is thus affected by the vulnerability described in the previous commit.

To mitigate this attack, avoid using a fixed-size buffer, and instead rely on getline() to allocate a buffer as large as necessary to fit the entire content of the line, preventing any protocol injection.

We solved a similar problem in a5bb10fd5e (config: avoid fixed-sized buffer when renaming/deleting a section, 2023-04-06) by switching to strbuf_getline(). We can't do that here because the contrib helpers do not link with the rest of Git, and so can't use a strbuf. But we can use the system getline() directly, which works similarly.

In most parts of Git we don't assume that every platform has getline(). But this helper is run only on OS X, and that platform added support in 10.7 ("Lion") which was released in 2011.

---
 .../osxkeychain/git-credential-osxkeychain.c           | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

index e29cc28779..5f2e5f16c8 100644
@@ -113,14 +113,16 @@ static void add_internet_password(void)
 
 static void read_credential(void)
 {
-	char buf[1024];
+	char *buf = NULL;
+	size_t alloc;
+	ssize_t line_len;
 
-	while (fgets(buf, sizeof(buf), stdin)) {
+	while ((line_len = getline(&buf, &alloc, stdin)) > 0) {
 		char *v;
 
 		if (!strcmp(buf, "\n"))
 			break;
-		buf[strlen(buf)-1] = '\0';
+		buf[line_len-1] = '\0';
 
 		v = strchr(buf, '=');
 		if (!v)

@@ -165,6 +167,8 @@ static void read_credential(void)
 		 * learn new lines, and the helpers are updated to match.
 		 */
 	}
+
+	free(buf);
 }
 
 int main(int argc, const char **argv)

2.40.1.452.gb3cd41c833

libgnome-keyring was deprecated in 2014 (in favor of libsecret), more than nine years ago [1].

The credential helper implemented using libgnome-keyring has had a small handful of commits since 2013, none of which implemented or changed any functionality. The last commit to do substantial work in this area was 15f7221686 (contrib/git-credential-gnome-keyring.c: support really ancient gnome-keyring, 2013-09-23), just shy of nine years ago.

This credential helper suffers from the same fgets()-related injection attack (using the new "wwwauth[]" feature) as in the previous commit. Instead of patching it, let's remove this helper as deprecated.

[1]: https://mail.gnome.org/archives/commits-list/2014-January/msg01585.html

---
 contrib/credential/gnome-keyring/.gitignore   |   1 -
 contrib/credential/gnome-keyring/Makefile     |  25 -
 .../git-credential-gnome-keyring.c            | 470 ------------------
 3 files changed, 496 deletions(-)
 delete mode 100644 contrib/credential/gnome-keyring/.gitignore
 delete mode 100644 contrib/credential/gnome-keyring/Makefile
 delete mode 100644 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c

deleted file mode 100644
index 88d8fcdbce..0000000000
@@ -1 +0,0 @@
-git-credential-gnome-keyring

deleted file mode 100644
index 22c19df94b..0000000000
@@ -1,25 +0,0 @@
-MAIN:=git-credential-gnome-keyring
-all:: $(MAIN)
-
-CC = gcc
-RM = rm -f
-CFLAGS = -g -O2 -Wall
-PKG_CONFIG = pkg-config
-
--include ../../../config.mak.autogen
--include ../../../config.mak
-
-INCS:=$(shell $(PKG_CONFIG) --cflags gnome-keyring-1 glib-2.0)
-LIBS:=$(shell $(PKG_CONFIG) --libs gnome-keyring-1 glib-2.0)
-
-SRCS:=$(MAIN).c
-OBJS:=$(SRCS:.c=.o)
-
-%.o: %.c
-	$(CC) $(CFLAGS) $(CPPFLAGS) $(INCS) -o $@ -c $<
-
-$(MAIN): $(OBJS)
-	$(CC) -o $@ $(LDFLAGS) $^ $(LIBS)
-
-clean:
-	@$(RM) $(MAIN) $(OBJS)

2.40.1.452.gb3cd41c833

The libsecret credential helper does not mark its build artifact as ignored, so running "make" results in a dirty working tree.

Mark the "git-credential-libsecret" binary as ignored to avoid the above.

---
 contrib/credential/libsecret/.gitignore | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 contrib/credential/libsecret/.gitignore

new file mode 100644
index 0000000000..4fa22359e2
@@ -0,0 +1 @@
+git-credential-libsecret

2.40.1.452.gb3cd41c833

The libsecret credential helper reads the newline-delimited protocol stream one line at a time by repeatedly calling fgets() into a fixed-size buffer, and is thus affected by the vulnerability described in the previous commit.

To mitigate this attack, avoid using a fixed-size buffer, and instead rely on getline() to allocate a buffer as large as necessary to fit the entire content of the line, preventing any protocol injection.

In most parts of Git we don't assume that every platform has getline(). But libsecret is primarily used on Linux, where we do already assume it (using a knob in config.mak.uname). POSIX also added getline() in 2008, so we'd expect other recent Unix-like operating systems to have it (e.g., FreeBSD also does).

Note that the buffer was already allocated on the heap in this case, but we'll swap g_free() for free(), since it will now be allocated by the system getline(), rather than glib's g_malloc().

---
 .../libsecret/git-credential-libsecret.c          | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

index 2c5d76d789..ef681f29d5 100644
@@ -244,17 +244,16 @@ static void credential_clear(struct credential *c)
 
 static int credential_read(struct credential *c)
 {
-	char *buf;
-	size_t line_len;
+	char *buf = NULL;
+	size_t alloc;
+	ssize_t line_len;
 	char *key;
 	char *value;
 
-	key = buf = g_malloc(1024);
+	while ((line_len = getline(&buf, &alloc, stdin)) > 0) {
+		key = buf;
 
-	while (fgets(buf, 1024, stdin)) {
-		line_len = strlen(buf);
-
-		if (line_len && buf[line_len-1] == '\n')
+		if (buf[line_len-1] == '\n')
 			buf[--line_len] = '\0';
 
 		if (!line_len)

@@ -298,7 +297,7 @@ static int credential_read(struct credential *c)
 		 */
 	}
 
-	g_free(buf);
+	free(buf);
 
 	return 0;
 }

2.40.1.452.gb3cd41c833

As in previous commits, harden the wincred credential helper against the aforementioned protocol injection attack.

Unlike the approached used for osxkeychain and libsecret, where a fixed-size buffer was replaced with getline(), we must take a different approach here. There is no getline() equivalent in Windows, and the function is not available to us with ordinary compiler settings.

Instead, allocate a larger (still fixed-size) buffer in which to process each line. The value of 100 KiB is chosen to match the maximum-length header that curl will allow, CURL_MAX_HTTP_HEADER.

To ensure that we are reading complete lines at a time, and that we aren't susceptible to a similar injection attack (albeit with more padding), ensure that each read terminates at a newline (i.e., that no line is more than 100 KiB long).

Note that it isn't sufficient to turn the old loop into something like:

    while (len && strchr("\r\n", buf[len - 1])) {
      buf[--len] = 0;
      ends_in_newline = 1;
    }

because if an attacker sends something like:

    [aaaaa.....]\r
    host=example.com\r\n

the credential helper would fill its buffer after reading up through the first '\r', call fgets() again, and then see "host=example.com\r\n" on its line.

Note that the original code was written in a way that would trim an arbitrary number of "\r" and "\n" from the end of the string. We should get only a single "\n" (since the point of fgets() is to return the buffer to us when it sees one), and likewise would not expect to see more than one associated "\r". The new code trims a single "\r\n", which matches the original intent.

[1]: https://curl.se/libcurl/c/CURLOPT_HEADERFUNCTION.html

---
 .../wincred/git-credential-wincred.c          | 21 +++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

index ead6e267c7..32ebef7f65 100644
@@ -249,16 +249,27 @@ static WCHAR *utf8_to_utf16_dup(const char *str)
 	return wstr;
 }
 
+#define KB (1024)
+
 static void read_credential(void)
 {
-	char buf[1024];
+	size_t alloc = 100 * KB;
+	char *buf = calloc(alloc, sizeof(*buf));
 
-	while (fgets(buf, sizeof(buf), stdin)) {
+	while (fgets(buf, alloc, stdin)) {
 		char *v;
-		int len = strlen(buf);
+		size_t len = strlen(buf);
+		int ends_in_newline = 0;
 		/* strip trailing CR / LF */
-		while (len && strchr("\r\n", buf[len - 1]))
+		if (len && buf[len - 1] == '\n') {
 			buf[--len] = 0;
+			ends_in_newline = 1;
+		}
+		if (len && buf[len - 1] == '\r')
+			buf[--len] = 0;
+
+		if (!ends_in_newline)
+			die("bad input: %s", buf);
 
 		if (!*buf)
 			break;

@@ -284,6 +295,8 @@ static void read_credential(void)
 		 * learn new lines, and the helpers are updated to match.
 		 */
 	}
+
+	free(buf);
 }
 
 int main(int argc, char *argv[])

2.40.1.452.gb3cd41c833

Sorry for being late to review this. I was not one of the three developers involved in writing and/or testing these changes, but I am motivated to see these fixes land.

In particular, because this should be resolved before 2.41.0-rc0.

Each patch was simple to read and well-motivated. I was particularly happy with this diffstat:

The rest of the changes looked to be obvious improvements, so this v1 LGTM.

Thanks,
-Stolee

Yes, definitely.

Thanks. Much credit is owed to Peff, who worked on these patches with me. And FWIW, dropping support for the gnome-keyring helper was his idea.

Thanks for the review :-).

Thanks,
Taylor