From 16b305cd2ba1747bcc6d160665eccdbbf7f2ea10 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Mon, 1 May 2023 11:53:48 -0400 Subject: credential.c: store "wwwauth[]" values in `credential_read()` 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 Signed-off-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- 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); -- cgit v1.2.1 From 71201ab0e53d61f3908a63078265e4e0742ef10d Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Mon, 1 May 2023 11:53:51 -0400 Subject: t/lib-credential.sh: ensure credential helpers handle long headers 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 Signed-off-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- 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() { -- cgit v1.2.1 From 5747c8072b74d26a179267acc09da1e8c5becf64 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Mon, 1 May 2023 11:53:54 -0400 Subject: contrib/credential: avoid fixed-size buffer in osxkeychain 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 Co-authored-by: Jeff King Signed-off-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- contrib/credential/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) -- cgit v1.2.1 From 048b673d7279d71dfdda486cf97893a3ec082dfe Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Mon, 1 May 2023 11:53:57 -0400 Subject: contrib/credential: remove 'gnome-keyring' credential helper 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 Signed-off-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- contrib/credential/gnome-keyring/.gitignore | 1 - contrib/credential/gnome-keyring/Makefile | 25 -- .../gnome-keyring/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 - * 2012 Philipp A. Hartmann - * - * 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 . - */ - -/* - * Credits: - * - GNOME Keyring API handling originally written by John Szakmeister - * - ported to credential helper API by Philipp A. Hartmann - */ - -#include -#include -#include -#include -#include - -#ifdef GNOME_KEYRING_DEFAULT - - /* Modern gnome-keyring */ - -#include - -#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; -} -- cgit v1.2.1 From de2fb99006575d55f878f9a4d02726cb598c361d Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Mon, 1 May 2023 11:54:00 -0400 Subject: contrib/credential: .gitignore libsecret build artifacts 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 Signed-off-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- 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 -- cgit v1.2.1 From 64f1e658e935bea6c9afdc4fa8be1d3ad6740355 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Mon, 1 May 2023 11:54:03 -0400 Subject: contrib/credential: avoid fixed-size buffer in libsecret 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 Co-authored-by: Jeff King Signed-off-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- contrib/credential/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; } -- cgit v1.2.1 From 0a3a972c163b2c5ed81a8e2c12fbf0c53eeb210c Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Mon, 1 May 2023 11:54:06 -0400 Subject: contrib/credential: embiggen fixed-size buffer in wincred 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 Helped-by: Matthew John Cheetham Co-authored-by: Jeff King Signed-off-by: Jeff King Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- contrib/credential/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,17 +249,28 @@ 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[]) -- cgit v1.2.1