[PATCH 0/7] contrib/credential: avoid protocol injection attacks

git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 0/7] contrib/credential: avoid protocol injection attacks
@ 2023-05-01 15:53 Taylor Blau
  2023-05-01 15:53 ` [PATCH 1/7] credential.c: store "wwwauth[]" values in `credential_read()` Taylor Blau
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Taylor Blau @ 2023-05-01 15:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/7] credential.c: store "wwwauth[]" values in `credential_read()`
  2023-05-01 15:53 [PATCH 0/7] contrib/credential: avoid protocol injection attacks Taylor Blau
@ 2023-05-01 15:53 ` Taylor Blau
  2023-05-01 15:53 ` [PATCH 2/7] t/lib-credential.sh: ensure credential helpers handle long headers Taylor Blau
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2023-05-01 15:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin, Derrick Stolee

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.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 credential.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/credential.c b/credential.c
index e6417bf880..96fc0daa35 100644
--- a/credential.c
+++ b/credential.c
@@ -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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/7] t/lib-credential.sh: ensure credential helpers handle long headers
  2023-05-01 15:53 [PATCH 0/7] contrib/credential: avoid protocol injection attacks Taylor Blau
  2023-05-01 15:53 ` [PATCH 1/7] credential.c: store "wwwauth[]" values in `credential_read()` Taylor Blau
@ 2023-05-01 15:53 ` Taylor Blau
  2023-05-01 15:53 ` [PATCH 3/7] contrib/credential: avoid fixed-size buffer in osxkeychain Taylor Blau
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2023-05-01 15:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin, Derrick Stolee

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.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/lib-credential.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 5ea8bc9f1d..d7d03c3cd9 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -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://askpass-username@badguy.example.com'\'':
+		EOF
+	'
 }

 helper_test_timeout() {
-- 
2.40.1.452.gb3cd41c833

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/7] contrib/credential: avoid fixed-size buffer in osxkeychain
  2023-05-01 15:53 [PATCH 0/7] contrib/credential: avoid protocol injection attacks Taylor Blau
  2023-05-01 15:53 ` [PATCH 1/7] credential.c: store "wwwauth[]" values in `credential_read()` Taylor Blau
  2023-05-01 15:53 ` [PATCH 2/7] t/lib-credential.sh: ensure credential helpers handle long headers Taylor Blau
@ 2023-05-01 15:53 ` Taylor Blau
  2023-05-01 15:53 ` [PATCH 4/7] contrib/credential: remove 'gnome-keyring' credential helper Taylor Blau
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2023-05-01 15:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin, Derrick Stolee

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.

Tested-by: Taylor Blau <me@ttaylorr.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 .../osxkeychain/git-credential-osxkeychain.c           | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index e29cc28779..5f2e5f16c8 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/7] contrib/credential: remove 'gnome-keyring' credential helper
  2023-05-01 15:53 [PATCH 0/7] contrib/credential: avoid protocol injection attacks Taylor Blau
                   ` (2 preceding siblings ...)
  2023-05-01 15:53 ` [PATCH 3/7] contrib/credential: avoid fixed-size buffer in osxkeychain Taylor Blau
@ 2023-05-01 15:53 ` Taylor Blau
  2023-05-01 15:54 ` [PATCH 5/7] contrib/credential: .gitignore libsecret build artifacts Taylor Blau
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2023-05-01 15:53 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin, Derrick Stolee

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

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 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

diff --git a/contrib/credential/gnome-keyring/.gitignore b/contrib/credential/gnome-keyring/.gitignore
deleted file mode 100644
index 88d8fcdbce..0000000000
--- a/contrib/credential/gnome-keyring/.gitignore
+++ /dev/null
@@ -1 +0,0 @@
-git-credential-gnome-keyring
diff --git a/contrib/credential/gnome-keyring/Makefile b/contrib/credential/gnome-keyring/Makefile
deleted file mode 100644
index 22c19df94b..0000000000
--- a/contrib/credential/gnome-keyring/Makefile
+++ /dev/null
@@ -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)
diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
deleted file mode 100644
index 5927e27ae6..0000000000
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ /dev/null
@@ -1,470 +0,0 @@
-/*
- * Copyright (C) 2011 John Szakmeister <john@szakmeister.net>
- *               2012 Philipp A. Hartmann <pah@qo.cx>
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, see <http://www.gnu.org/licenses/>.
- */
-
-/*
- * Credits:
- * - GNOME Keyring API handling originally written by John Szakmeister
- * - ported to credential helper API by Philipp A. Hartmann
- */
-
-#include <stdio.h>
-#include <string.h>
-#include <stdlib.h>
-#include <glib.h>
-#include <gnome-keyring.h>
-
-#ifdef GNOME_KEYRING_DEFAULT
-
-   /* Modern gnome-keyring */
-
-#include <gnome-keyring-memory.h>
-
-#else
-
-   /*
-    * Support ancient gnome-keyring, circ. RHEL 5.X.
-    * GNOME_KEYRING_DEFAULT seems to have been introduced with Gnome 2.22,
-    * and the other features roughly around Gnome 2.20, 6 months before.
-    * Ubuntu 8.04 used Gnome 2.22 (I think).  Not sure any distro used 2.20.
-    * So the existence/non-existence of GNOME_KEYRING_DEFAULT seems like
-    * a decent thing to use as an indicator.
-    */
-
-#define GNOME_KEYRING_DEFAULT NULL
-
-/*
- * ancient gnome-keyring returns DENIED when an entry is not found.
- * Setting NO_MATCH to DENIED will prevent us from reporting DENIED
- * errors during get and erase operations, but we will still report
- * DENIED errors during a store.
- */
-#define GNOME_KEYRING_RESULT_NO_MATCH GNOME_KEYRING_RESULT_DENIED
-
-#define gnome_keyring_memory_alloc g_malloc
-#define gnome_keyring_memory_free gnome_keyring_free_password
-#define gnome_keyring_memory_strdup g_strdup
-
-static const char *gnome_keyring_result_to_message(GnomeKeyringResult result)
-{
-	switch (result) {
-	case GNOME_KEYRING_RESULT_OK:
-		return "OK";
-	case GNOME_KEYRING_RESULT_DENIED:
-		return "Denied";
-	case GNOME_KEYRING_RESULT_NO_KEYRING_DAEMON:
-		return "No Keyring Daemon";
-	case GNOME_KEYRING_RESULT_ALREADY_UNLOCKED:
-		return "Already UnLocked";
-	case GNOME_KEYRING_RESULT_NO_SUCH_KEYRING:
-		return "No Such Keyring";
-	case GNOME_KEYRING_RESULT_BAD_ARGUMENTS:
-		return "Bad Arguments";
-	case GNOME_KEYRING_RESULT_IO_ERROR:
-		return "IO Error";
-	case GNOME_KEYRING_RESULT_CANCELLED:
-		return "Cancelled";
-	case GNOME_KEYRING_RESULT_ALREADY_EXISTS:
-		return "Already Exists";
-	default:
-		return "Unknown Error";
-	}
-}
-
-/*
- * Support really ancient gnome-keyring, circ. RHEL 4.X.
- * Just a guess for the Glib version.  Glib 2.8 was roughly Gnome 2.12 ?
- * Which was released with gnome-keyring 0.4.3 ??
- */
-#if GLIB_MAJOR_VERSION == 2 && GLIB_MINOR_VERSION < 8
-
-static void gnome_keyring_done_cb(GnomeKeyringResult result, gpointer user_data)
-{
-	gpointer *data = (gpointer *)user_data;
-	int *done = (int *)data[0];
-	GnomeKeyringResult *r = (GnomeKeyringResult *)data[1];
-
-	*r = result;
-	*done = 1;
-}
-
-static void wait_for_request_completion(int *done)
-{
-	GMainContext *mc = g_main_context_default();
-	while (!*done)
-		g_main_context_iteration(mc, TRUE);
-}
-
-static GnomeKeyringResult gnome_keyring_item_delete_sync(const char *keyring, guint32 id)
-{
-	int done = 0;
-	GnomeKeyringResult result;
-	gpointer data[] = { &done, &result };
-
-	gnome_keyring_item_delete(keyring, id, gnome_keyring_done_cb, data,
-		NULL);
-
-	wait_for_request_completion(&done);
-
-	return result;
-}
-
-#endif
-#endif
-
-/*
- * This credential struct and API is simplified from git's credential.{h,c}
- */
-struct credential {
-	char *protocol;
-	char *host;
-	unsigned short port;
-	char *path;
-	char *username;
-	char *password;
-};
-
-#define CREDENTIAL_INIT { 0 }
-
-typedef int (*credential_op_cb)(struct credential *);
-
-struct credential_operation {
-	char *name;
-	credential_op_cb op;
-};
-
-#define CREDENTIAL_OP_END { NULL, NULL }
-
-/* ----------------- GNOME Keyring functions ----------------- */
-
-/* create a special keyring option string, if path is given */
-static char *keyring_object(struct credential *c)
-{
-	if (!c->path)
-		return NULL;
-
-	if (c->port)
-		return g_strdup_printf("%s:%hd/%s", c->host, c->port, c->path);
-
-	return g_strdup_printf("%s/%s", c->host, c->path);
-}
-
-static int keyring_get(struct credential *c)
-{
-	char *object = NULL;
-	GList *entries;
-	GnomeKeyringNetworkPasswordData *password_data;
-	GnomeKeyringResult result;
-
-	if (!c->protocol || !(c->host || c->path))
-		return EXIT_FAILURE;
-
-	object = keyring_object(c);
-
-	result = gnome_keyring_find_network_password_sync(
-				c->username,
-				NULL /* domain */,
-				c->host,
-				object,
-				c->protocol,
-				NULL /* authtype */,
-				c->port,
-				&entries);
-
-	g_free(object);
-
-	if (result == GNOME_KEYRING_RESULT_NO_MATCH)
-		return EXIT_SUCCESS;
-
-	if (result == GNOME_KEYRING_RESULT_CANCELLED)
-		return EXIT_SUCCESS;
-
-	if (result != GNOME_KEYRING_RESULT_OK) {
-		g_critical("%s", gnome_keyring_result_to_message(result));
-		return EXIT_FAILURE;
-	}
-
-	/* pick the first one from the list */
-	password_data = (GnomeKeyringNetworkPasswordData *)entries->data;
-
-	gnome_keyring_memory_free(c->password);
-	c->password = gnome_keyring_memory_strdup(password_data->password);
-
-	if (!c->username)
-		c->username = g_strdup(password_data->user);
-
-	gnome_keyring_network_password_list_free(entries);
-
-	return EXIT_SUCCESS;
-}
-
-
-static int keyring_store(struct credential *c)
-{
-	guint32 item_id;
-	char *object = NULL;
-	GnomeKeyringResult result;
-
-	/*
-	 * Sanity check that what we are storing is actually sensible.
-	 * In particular, we can't make a URL without a protocol field.
-	 * Without either a host or pathname (depending on the scheme),
-	 * we have no primary key. And without a username and password,
-	 * we are not actually storing a credential.
-	 */
-	if (!c->protocol || !(c->host || c->path) ||
-	    !c->username || !c->password)
-		return EXIT_FAILURE;
-
-	object = keyring_object(c);
-
-	result = gnome_keyring_set_network_password_sync(
-				GNOME_KEYRING_DEFAULT,
-				c->username,
-				NULL /* domain */,
-				c->host,
-				object,
-				c->protocol,
-				NULL /* authtype */,
-				c->port,
-				c->password,
-				&item_id);
-
-	g_free(object);
-
-	if (result != GNOME_KEYRING_RESULT_OK &&
-	    result != GNOME_KEYRING_RESULT_CANCELLED) {
-		g_critical("%s", gnome_keyring_result_to_message(result));
-		return EXIT_FAILURE;
-	}
-
-	return EXIT_SUCCESS;
-}
-
-static int keyring_erase(struct credential *c)
-{
-	char *object = NULL;
-	GList *entries;
-	GnomeKeyringNetworkPasswordData *password_data;
-	GnomeKeyringResult result;
-
-	/*
-	 * Sanity check that we actually have something to match
-	 * against. The input we get is a restrictive pattern,
-	 * so technically a blank credential means "erase everything".
-	 * But it is too easy to accidentally send this, since it is equivalent
-	 * to empty input. So explicitly disallow it, and require that the
-	 * pattern have some actual content to match.
-	 */
-	if (!c->protocol && !c->host && !c->path && !c->username)
-		return EXIT_FAILURE;
-
-	object = keyring_object(c);
-
-	result = gnome_keyring_find_network_password_sync(
-				c->username,
-				NULL /* domain */,
-				c->host,
-				object,
-				c->protocol,
-				NULL /* authtype */,
-				c->port,
-				&entries);
-
-	g_free(object);
-
-	if (result == GNOME_KEYRING_RESULT_NO_MATCH)
-		return EXIT_SUCCESS;
-
-	if (result == GNOME_KEYRING_RESULT_CANCELLED)
-		return EXIT_SUCCESS;
-
-	if (result != GNOME_KEYRING_RESULT_OK) {
-		g_critical("%s", gnome_keyring_result_to_message(result));
-		return EXIT_FAILURE;
-	}
-
-	/* pick the first one from the list (delete all matches?) */
-	password_data = (GnomeKeyringNetworkPasswordData *)entries->data;
-
-	result = gnome_keyring_item_delete_sync(
-		password_data->keyring, password_data->item_id);
-
-	gnome_keyring_network_password_list_free(entries);
-
-	if (result != GNOME_KEYRING_RESULT_OK) {
-		g_critical("%s", gnome_keyring_result_to_message(result));
-		return EXIT_FAILURE;
-	}
-
-	return EXIT_SUCCESS;
-}
-
-/*
- * Table with helper operation callbacks, used by generic
- * credential helper main function.
- */
-static struct credential_operation const credential_helper_ops[] = {
-	{ "get",   keyring_get },
-	{ "store", keyring_store },
-	{ "erase", keyring_erase },
-	CREDENTIAL_OP_END
-};
-
-/* ------------------ credential functions ------------------ */
-
-static void credential_init(struct credential *c)
-{
-	memset(c, 0, sizeof(*c));
-}
-
-static void credential_clear(struct credential *c)
-{
-	g_free(c->protocol);
-	g_free(c->host);
-	g_free(c->path);
-	g_free(c->username);
-	gnome_keyring_memory_free(c->password);
-
-	credential_init(c);
-}
-
-static int credential_read(struct credential *c)
-{
-	char *buf;
-	size_t line_len;
-	char *key;
-	char *value;
-
-	key = buf = gnome_keyring_memory_alloc(1024);
-
-	while (fgets(buf, 1024, stdin)) {
-		line_len = strlen(buf);
-
-		if (line_len && buf[line_len-1] == '\n')
-			buf[--line_len] = '\0';
-
-		if (!line_len)
-			break;
-
-		value = strchr(buf, '=');
-		if (!value) {
-			g_warning("invalid credential line: %s", key);
-			gnome_keyring_memory_free(buf);
-			return -1;
-		}
-		*value++ = '\0';
-
-		if (!strcmp(key, "protocol")) {
-			g_free(c->protocol);
-			c->protocol = g_strdup(value);
-		} else if (!strcmp(key, "host")) {
-			g_free(c->host);
-			c->host = g_strdup(value);
-			value = strrchr(c->host, ':');
-			if (value) {
-				*value++ = '\0';
-				c->port = atoi(value);
-			}
-		} else if (!strcmp(key, "path")) {
-			g_free(c->path);
-			c->path = g_strdup(value);
-		} else if (!strcmp(key, "username")) {
-			g_free(c->username);
-			c->username = g_strdup(value);
-		} else if (!strcmp(key, "password")) {
-			gnome_keyring_memory_free(c->password);
-			c->password = gnome_keyring_memory_strdup(value);
-			while (*value)
-				*value++ = '\0';
-		}
-		/*
-		 * Ignore other lines; we don't know what they mean, but
-		 * this future-proofs us when later versions of git do
-		 * learn new lines, and the helpers are updated to match.
-		 */
-	}
-
-	gnome_keyring_memory_free(buf);
-
-	return 0;
-}
-
-static void credential_write_item(FILE *fp, const char *key, const char *value)
-{
-	if (!value)
-		return;
-	fprintf(fp, "%s=%s\n", key, value);
-}
-
-static void credential_write(const struct credential *c)
-{
-	/* only write username/password, if set */
-	credential_write_item(stdout, "username", c->username);
-	credential_write_item(stdout, "password", c->password);
-}
-
-static void usage(const char *name)
-{
-	struct credential_operation const *try_op = credential_helper_ops;
-	const char *basename = strrchr(name, '/');
-
-	basename = (basename) ? basename + 1 : name;
-	fprintf(stderr, "usage: %s <", basename);
-	while (try_op->name) {
-		fprintf(stderr, "%s", (try_op++)->name);
-		if (try_op->name)
-			fprintf(stderr, "%s", "|");
-	}
-	fprintf(stderr, "%s", ">\n");
-}
-
-int main(int argc, char *argv[])
-{
-	int ret = EXIT_SUCCESS;
-
-	struct credential_operation const *try_op = credential_helper_ops;
-	struct credential cred = CREDENTIAL_INIT;
-
-	if (!argv[1]) {
-		usage(argv[0]);
-		exit(EXIT_FAILURE);
-	}
-
-	g_set_application_name("Git Credential Helper");
-
-	/* lookup operation callback */
-	while (try_op->name && strcmp(argv[1], try_op->name))
-		try_op++;
-
-	/* unsupported operation given -- ignore silently */
-	if (!try_op->name || !try_op->op)
-		goto out;
-
-	ret = credential_read(&cred);
-	if (ret)
-		goto out;
-
-	/* perform credential operation */
-	ret = (*try_op->op)(&cred);
-
-	credential_write(&cred);
-
-out:
-	credential_clear(&cred);
-	return ret;
-}
-- 
2.40.1.452.gb3cd41c833


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 5/7] contrib/credential: .gitignore libsecret build artifacts
  2023-05-01 15:53 [PATCH 0/7] contrib/credential: avoid protocol injection attacks Taylor Blau
                   ` (3 preceding siblings ...)
  2023-05-01 15:53 ` [PATCH 4/7] contrib/credential: remove 'gnome-keyring' credential helper Taylor Blau
@ 2023-05-01 15:54 ` Taylor Blau
  2023-05-01 15:54 ` [PATCH 6/7] contrib/credential: avoid fixed-size buffer in libsecret Taylor Blau
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2023-05-01 15:54 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin, Derrick Stolee

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.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 contrib/credential/libsecret/.gitignore | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 contrib/credential/libsecret/.gitignore

diff --git a/contrib/credential/libsecret/.gitignore b/contrib/credential/libsecret/.gitignore
new file mode 100644
index 0000000000..4fa22359e2
--- /dev/null
+++ b/contrib/credential/libsecret/.gitignore
@@ -0,0 +1 @@
+git-credential-libsecret
-- 
2.40.1.452.gb3cd41c833


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 6/7] contrib/credential: avoid fixed-size buffer in libsecret
  2023-05-01 15:53 [PATCH 0/7] contrib/credential: avoid protocol injection attacks Taylor Blau
                   ` (4 preceding siblings ...)
  2023-05-01 15:54 ` [PATCH 5/7] contrib/credential: .gitignore libsecret build artifacts Taylor Blau
@ 2023-05-01 15:54 ` Taylor Blau
  2023-05-01 15:54 ` [PATCH 7/7] contrib/credential: embiggen fixed-size buffer in wincred Taylor Blau
  2023-05-05 15:24 ` [PATCH 0/7] contrib/credential: avoid protocol injection attacks Derrick Stolee
  7 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2023-05-01 15:54 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin, Derrick Stolee

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()`.

Tested-by: Jeff King <peff@peff.net>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 .../libsecret/git-credential-libsecret.c          | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index 2c5d76d789..ef681f29d5 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 7/7] contrib/credential: embiggen fixed-size buffer in wincred
  2023-05-01 15:53 [PATCH 0/7] contrib/credential: avoid protocol injection attacks Taylor Blau
                   ` (5 preceding siblings ...)
  2023-05-01 15:54 ` [PATCH 6/7] contrib/credential: avoid fixed-size buffer in libsecret Taylor Blau
@ 2023-05-01 15:54 ` Taylor Blau
  2023-05-05 15:24 ` [PATCH 0/7] contrib/credential: avoid protocol injection attacks Derrick Stolee
  7 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2023-05-01 15:54 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin, Derrick Stolee

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

Tested-by: Matthew John Cheetham <mjcheetham@outlook.com>
Helped-by: Matthew John Cheetham <mjcheetham@outlook.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 .../wincred/git-credential-wincred.c          | 21 +++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index ead6e267c7..32ebef7f65 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/7] contrib/credential: avoid protocol injection attacks
  2023-05-01 15:53 [PATCH 0/7] contrib/credential: avoid protocol injection attacks Taylor Blau
                   ` (6 preceding siblings ...)
  2023-05-01 15:54 ` [PATCH 7/7] contrib/credential: embiggen fixed-size buffer in wincred Taylor Blau
@ 2023-05-05 15:24 ` Derrick Stolee
  2023-05-05 17:46   ` Taylor Blau
  7 siblings, 1 reply; 10+ messages in thread
From: Derrick Stolee @ 2023-05-05 15:24 UTC (permalink / raw)
  To: Taylor Blau, git
  Cc: Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin

On 5/1/2023 11:53 AM, Taylor Blau wrote:
> 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.

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.

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

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:

>  contrib/credential/gnome-keyring/.gitignore   |   1 -
>  contrib/credential/gnome-keyring/Makefile     |  25 -
>  .../git-credential-gnome-keyring.c            | 470 ------------------

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

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/7] contrib/credential: avoid protocol injection attacks
  2023-05-05 15:24 ` [PATCH 0/7] contrib/credential: avoid protocol injection attacks Derrick Stolee
@ 2023-05-05 17:46   ` Taylor Blau
  0 siblings, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2023-05-05 17:46 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: git, Jeff King, Junio C Hamano, Matthew John Cheetham,
	Johannes Schindelin

On Fri, May 05, 2023 at 11:24:44AM -0400, Derrick Stolee wrote:
> > But the new "wwwauth[]" field does allow this attack to take place.
>
> In particular, because this should be resolved before 2.41.0-rc0.

Yes, definitely.

> Each patch was simple to read and well-motivated. I was particularly
> happy with this diffstat:
>
> >  contrib/credential/gnome-keyring/.gitignore   |   1 -
> >  contrib/credential/gnome-keyring/Makefile     |  25 -
> >  .../git-credential-gnome-keyring.c            | 470 ------------------
>
> The rest of the changes looked to be obvious improvements, so this
> v1 LGTM.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-05-05 17:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-01 15:53 [PATCH 0/7] contrib/credential: avoid protocol injection attacks Taylor Blau
2023-05-01 15:53 ` [PATCH 1/7] credential.c: store "wwwauth[]" values in `credential_read()` Taylor Blau
2023-05-01 15:53 ` [PATCH 2/7] t/lib-credential.sh: ensure credential helpers handle long headers Taylor Blau
2023-05-01 15:53 ` [PATCH 3/7] contrib/credential: avoid fixed-size buffer in osxkeychain Taylor Blau
2023-05-01 15:53 ` [PATCH 4/7] contrib/credential: remove 'gnome-keyring' credential helper Taylor Blau
2023-05-01 15:54 ` [PATCH 5/7] contrib/credential: .gitignore libsecret build artifacts Taylor Blau
2023-05-01 15:54 ` [PATCH 6/7] contrib/credential: avoid fixed-size buffer in libsecret Taylor Blau
2023-05-01 15:54 ` [PATCH 7/7] contrib/credential: embiggen fixed-size buffer in wincred Taylor Blau
2023-05-05 15:24 ` [PATCH 0/7] contrib/credential: avoid protocol injection attacks Derrick Stolee
2023-05-05 17:46   ` Taylor Blau

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