{"thread":{"id":"59677","subject":"[PATCH 0/7] contrib/credential: avoid protocol injection attacks","startedAt":"2023-05-01T15:53:52Z","lastAt":"2023-05-05T17:48:10Z","messageCount":10,"participants":["Taylor Blau","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"476339","messageId":"cover.1682956419.git.me@ttaylorr.com","threadId":"59677","inReplyTo":null,"subject":"[PATCH 0/7] contrib/credential: avoid protocol injection attacks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-01T15:53:44Z","receivedAt":"2023-05-01T15:53:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series addresses a handful of potential protocol injection attacks\npossible via some of the credential helpers in contrib/credential, and\nthe new \"wwwauth[]\" directive.\n\nThe attack is described in complete detail in 2/7, but roughly boils\ndown to using a long line to incur multiple fgets() calls which can\ntreat data in the middle of the line as if it appeared at the beginning.\n\nLuckily, all protocol fields part of tagged versions of Git are immune\nfrom this attack. Briefly:\n\n - \"protocol\" is restricted to known values\n - \"host\" is immune because curl will reject hostnames that have a '='\n character in them, which would be required to carry out this attack.\n - \"username\", and \"path\" are immune, because the buffer characters to\n fill out the first `fgets()` call would pollute the\n `username`/`path` field, causing the credential helper to return\n nothing\n - \"password\" is immune because providing a password instructs\n credential helpers to avoid filling credentials in the first place.\n\nBut the new \"wwwauth[]\" field does allow this attack to take place.\n\nSince these credential helpers are tested via t0303 (which requires some\nextensive set-up), we opted not to make these fixes during the last\nembargo period, and instead do them before the \"wwwauth[]\" feature\nbecomes part of a tagged version.\n\nWith the additional time, we have been able to verify that the affected\ncredential helpers which are modified in this series all fail the new\ntest before their patches, and pass afterwords. Thanks to Peff for\nlooking at libsecret, Matthew Cheetham for looking at wincred. I looked\nat osxkeychain.\n\nTaylor Blau (7):\n credential.c: store \"wwwauth[]\" values in `credential_read()`\n t/lib-credential.sh: ensure credential helpers handle long headers\n contrib/credential: avoid fixed-size buffer in osxkeychain\n contrib/credential: remove 'gnome-keyring' credential helper\n contrib/credential: .gitignore libsecret build artifacts\n contrib/credential: avoid fixed-size buffer in libsecret\n contrib/credential: embiggen fixed-size buffer in wincred\n\n contrib/credential/gnome-keyring/.gitignore | 1 -\n contrib/credential/gnome-keyring/Makefile | 25 -\n .../git-credential-gnome-keyring.c | 470 ------------------\n contrib/credential/libsecret/.gitignore | 1 +\n .../libsecret/git-credential-libsecret.c | 15 +-\n .../osxkeychain/git-credential-osxkeychain.c | 10 +-\n .../wincred/git-credential-wincred.c | 21 +-\n credential.c | 2 +\n t/lib-credential.sh | 29 ++\n 9 files changed, 63 insertions(+), 511 deletions(-)\n delete mode 100644 contrib/credential/gnome-keyring/.gitignore\n delete mode 100644 contrib/credential/gnome-keyring/Makefile\n delete mode 100644 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c\n create mode 100644 contrib/credential/libsecret/.gitignore\n\n-- \n2.40.1.452.gb3cd41c833\n"},{"id":"476340","messageId":"a104afd5e1de792fadd51bbd6887725880dd23f9.1682956419.git.me@ttaylorr.com","threadId":"59677","inReplyTo":"cover.1682956419.git.me@ttaylorr.com","subject":"[PATCH 1/7] credential.c: store \"wwwauth[]\" values in `credential_read()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-01T15:53:48Z","receivedAt":"2023-05-01T15:53:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Teach git-credential to read \"wwwauth[]\" value(s) when parsing the\noutput of a credential helper.\n\nThese extra headers are not needed for Git's own HTTP support to use the\nfeature internally, but the feature would not be available for a\nscripted caller (say, git-remote-mediawiki providing the header in the\nsame way).\n\nAs a bonus, this also makes it easier to use wwwauth[] in synthetic\ncredential inputs in our test suite.\n\nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n credential.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/credential.c b/credential.c\nindex e6417bf880..96fc0daa35 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -238,6 +238,8 @@ int credential_read(struct credential *c, FILE *fp)\n \t\t} else if (!strcmp(key, \"path\")) {\n \t\t\tfree(c->path);\n \t\t\tc->path = xstrdup(value);\n+\t\t} else if (!strcmp(key, \"wwwauth[]\")) {\n+\t\t\tstrvec_push(&c->wwwauth_headers, value);\n \t\t} else if (!strcmp(key, \"password_expiry_utc\")) {\n \t\t\terrno = 0;\n \t\t\tc->password_expiry_utc = parse_timestamp(value, NULL, 10);\n-- \n2.40.1.452.gb3cd41c833\n\n"},{"id":"476341","messageId":"73800b548a39fb657a41bf5d7061ce5406f09efd.1682956419.git.me@ttaylorr.com","threadId":"59677","inReplyTo":"cover.1682956419.git.me@ttaylorr.com","subject":"[PATCH 2/7] t/lib-credential.sh: ensure credential helpers handle long headers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-01T15:53:51Z","receivedAt":"2023-05-01T15:54:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Add a test ensuring that the \"wwwauth[]\" field cannot be used to\ninject malicious data into the credential helper stream.\n\nMany of the credential helpers in contrib/credential read the\nnewline-delimited protocol stream one line at a time by repeatedly\ncalling fgets() into a fixed-size buffer.\n\nThis assumes that each line is no more than 1024 characters long, since\neach iteration of the loop assumes that it is parsing starting at the\nbeginning of a new line in the stream. However, similar to a5bb10fd5e\n(config: avoid fixed-sized buffer when renaming/deleting a section,\n2023-04-06), if a line is longer than 1024 characters, a malicious actor\ncan embed another command within an existing line, bypassing the usual\nchecks introduced in 9a6bbee800 (credential: avoid writing values with\nnewlines, 2020-03-11).\n\nAs with the problem fixed in that commit, specially crafted input can\ncause the helper to return the credential for the wrong host, letting an\nattacker trick the victim into sending credentials for one host to\nanother.\n\nLuckily, all parts of the credential helper protocol that are available\nin a tagged release of Git are immune to this attack:\n\n - \"protocol\" is restricted to known values, and is thus immune.\n\n - \"host\" is immune because curl will reject hostnames that have a '='\n character in them, which would be required to carry out this attack.\n\n - \"username\" is immune, because the buffer characters to fill out the\n first `fgets()` call would pollute the `username` field, causing the\n credential helper to return nothing (because it would match a\n username if present, and the username of the credential to be stolen\n is likely not 1024 characters).\n\n - \"password\" is immune because providing a password instructs\n credential helpers to avoid filling credentials in the first place.\n\n - \"path\" is similar to username; if present, it is not likely to match\n any credential the victim is storing. It's also not enabled by\n default; the victim would have to set credential.useHTTPPath\n explicitly.\n\nHowever, the new \"wwwauth[]\" field introduced via 5f2117b24f\n(credential: add WWW-Authenticate header to cred requests, 2023-02-27)\ncan be used to inject data into the credential helper stream. For\nexample, running:\n\n {\n printf 'HTTP/1.1 401\\r\\n'\n printf 'WWW-Authenticate: basic realm='\n perl -e 'print \"a\" x 1024'\n printf 'host=victim.com\\r\\n'\n } | nc -Nlp 8080\n\nin one terminal, and then:\n\n git clone http://localhost:8080\n\nin another would result in a line like:\n\n wwwauth[]=basic realm=aaa[...]aaahost=victim.com\n\nbeing sent to the credential helper. If we tweak that \"1024\" to align\nour output with the helper's buffer size and the rest of the data on the\nline, it can cause the helper to see \"host=victim.com\" on its own line,\nallowing motivated attackers to exfiltrate credentials belonging to\n\"victim.com\".\n\nThe below test demonstrates these failures and provides us with a test\nto ensure that our fix is correct. That said, it has a couple of\nshortcomings:\n\n - it's in t0303, since that's the only mechanism we have for testing\n random helpers. But that means nobody is going to run it under\n normal circumstances.\n\n - to get the attack right, it has to line up the stuffed name with the\n buffer size, so we depend on the exact buffer size. I parameterized\n it so it could be used to test other helpers, but in practice it's\n not likely for anybody to do that.\n\nStill, it's the best we can do, and will help us confirm the presence of\nthe problem (and our fixes) in the new few patches.\n\nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n t/lib-credential.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex 5ea8bc9f1d..d7d03c3cd9 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -270,6 +270,35 @@ helper_test() {\n \t\tpassword=\n \t\tEOF\n \t'\n+\n+\t: ${GIT_TEST_LONG_CRED_BUFFER:=1024}\n+\t# 23 bytes accounts for \"wwwauth[]=basic realm=\" plus NUL\n+\tLONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))\n+\tLONG_VALUE=$(perl -e 'print \"a\" x shift' $LONG_VALUE_LEN)\n+\n+\ttest_expect_success \"helper ($HELPER) not confused by long header\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=victim.example.com\n+\t\tusername=user\n+\t\tpassword=to-be-stolen\n+\t\tEOF\n+\n+\t\tcheck fill $HELPER <<-EOF\n+\t\tprotocol=https\n+\t\thost=badguy.example.com\n+\t\twwwauth[]=basic realm=${LONG_VALUE}host=victim.example.com\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=badguy.example.com\n+\t\tusername=askpass-username\n+\t\tpassword=askpass-password\n+\t\twwwauth[]=basic realm=${LONG_VALUE}host=victim.example.com\n+\t\t--\n+\t\taskpass: Username for '\\''https://badguy.example.com'\\'':\n+\t\taskpass: Password for '\\''https://askpass-username@badguy.example.com'\\'':\n+\t\tEOF\n+\t'\n }\n \n helper_test_timeout() {\n-- \n2.40.1.452.gb3cd41c833\n\n"},{"id":"476342","messageId":"301bc27a5b4500ff6681bbf5a113d763b8f0b4c9.1682956419.git.me@ttaylorr.com","threadId":"59677","inReplyTo":"cover.1682956419.git.me@ttaylorr.com","subject":"[PATCH 3/7] contrib/credential: avoid fixed-size buffer in osxkeychain","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-01T15:53:54Z","receivedAt":"2023-05-01T15:54:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The macOS Keychain-based credential helper reads the newline-delimited\nprotocol stream one line at a time by repeatedly calling fgets() into a\nfixed-size buffer, and is thus affected by the vulnerability described\nin the previous commit.\n\nTo mitigate this attack, avoid using a fixed-size buffer, and instead\nrely on getline() to allocate a buffer as large as necessary to fit the\nentire content of the line, preventing any protocol injection.\n\nWe solved a similar problem in a5bb10fd5e (config: avoid fixed-sized\nbuffer when renaming/deleting a section, 2023-04-06) by switching to\nstrbuf_getline(). We can't do that here because the contrib helpers do\nnot link with the rest of Git, and so can't use a strbuf. But we can use\nthe system getline() directly, which works similarly.\n\nIn most parts of Git we don't assume that every platform has getline().\nBut this helper is run only on OS X, and that platform added support in\n10.7 (\"Lion\") which was released in 2011.\n\nTested-by: Taylor Blau \nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n .../osxkeychain/git-credential-osxkeychain.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex e29cc28779..5f2e5f16c8 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -113,14 +113,16 @@ static void add_internet_password(void)\n \n static void read_credential(void)\n {\n-\tchar buf[1024];\n+\tchar *buf = NULL;\n+\tsize_t alloc;\n+\tssize_t line_len;\n \n-\twhile (fgets(buf, sizeof(buf), stdin)) {\n+\twhile ((line_len = getline(&buf, &alloc, stdin)) > 0) {\n \t\tchar *v;\n \n \t\tif (!strcmp(buf, \"\\n\"))\n \t\t\tbreak;\n-\t\tbuf[strlen(buf)-1] = '\\0';\n+\t\tbuf[line_len-1] = '\\0';\n \n \t\tv = strchr(buf, '=');\n \t\tif (!v)\n@@ -165,6 +167,8 @@ static void read_credential(void)\n \t\t * learn new lines, and the helpers are updated to match.\n \t\t */\n \t}\n+\n+\tfree(buf);\n }\n \n int main(int argc, const char **argv)\n-- \n2.40.1.452.gb3cd41c833\n\n"},{"id":"476343","messageId":"aebc4f7794a7b423ebcf22389530b3de1adfd1fe.1682956419.git.me@ttaylorr.com","threadId":"59677","inReplyTo":"cover.1682956419.git.me@ttaylorr.com","subject":"[PATCH 4/7] contrib/credential: remove 'gnome-keyring' credential helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-01T15:53:57Z","receivedAt":"2023-05-01T15:54:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"libgnome-keyring was deprecated in 2014 (in favor of libsecret), more\nthan nine years ago [1].\n\nThe credential helper implemented using libgnome-keyring has had a small\nhandful of commits since 2013, none of which implemented or changed any\nfunctionality. The last commit to do substantial work in this area was\n15f7221686 (contrib/git-credential-gnome-keyring.c: support really\nancient gnome-keyring, 2013-09-23), just shy of nine years ago.\n\nThis credential helper suffers from the same `fgets()`-related injection\nattack (using the new \"wwwauth[]\" feature) as in the previous commit.\nInstead of patching it, let's remove this helper as deprecated.\n\n[1]: https://mail.gnome.org/archives/commits-list/2014-January/msg01585.html\n\nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n contrib/credential/gnome-keyring/.gitignore | 1 -\n contrib/credential/gnome-keyring/Makefile | 25 -\n .../git-credential-gnome-keyring.c | 470 ------------------\n 3 files changed, 496 deletions(-)\n delete mode 100644 contrib/credential/gnome-keyring/.gitignore\n delete mode 100644 contrib/credential/gnome-keyring/Makefile\n delete mode 100644 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c\n\ndiff --git a/contrib/credential/gnome-keyring/.gitignore b/contrib/credential/gnome-keyring/.gitignore\ndeleted file mode 100644\nindex 88d8fcdbce..0000000000\n--- a/contrib/credential/gnome-keyring/.gitignore\n+++ /dev/null\n@@ -1 +0,0 @@\n-git-credential-gnome-keyring\ndiff --git a/contrib/credential/gnome-keyring/Makefile b/contrib/credential/gnome-keyring/Makefile\ndeleted file mode 100644\nindex 22c19df94b..0000000000\n--- a/contrib/credential/gnome-keyring/Makefile\n+++ /dev/null\n@@ -1,25 +0,0 @@\n-MAIN:=git-credential-gnome-keyring\n-all:: $(MAIN)\n-\n-CC = gcc\n-RM = rm -f\n-CFLAGS = -g -O2 -Wall\n-PKG_CONFIG = pkg-config\n-\n--include ../../../config.mak.autogen\n--include ../../../config.mak\n-\n-INCS:=$(shell $(PKG_CONFIG) --cflags gnome-keyring-1 glib-2.0)\n-LIBS:=$(shell $(PKG_CONFIG) --libs gnome-keyring-1 glib-2.0)\n-\n-SRCS:=$(MAIN).c\n-OBJS:=$(SRCS:.c=.o)\n-\n-%.o: %.c\n-\t$(CC) $(CFLAGS) $(CPPFLAGS) $(INCS) -o $@ -c $<\n-\n-$(MAIN): $(OBJS)\n-\t$(CC) -o $@ $(LDFLAGS) $^ $(LIBS)\n-\n-clean:\n-\t@$(RM) $(MAIN) $(OBJS)\ndiff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c\ndeleted file mode 100644\nindex 5927e27ae6..0000000000\n--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c\n+++ /dev/null\n@@ -1,470 +0,0 @@\n-/*\n- * Copyright (C) 2011 John Szakmeister \n- * 2012 Philipp A. Hartmann \n- *\n- * This program is free software; you can redistribute it and/or modify\n- * it under the terms of the GNU General Public License as published by\n- * the Free Software Foundation; either version 2 of the License, or\n- * (at your option) any later version.\n- *\n- * This program is distributed in the hope that it will be useful,\n- * but WITHOUT ANY WARRANTY; without even the implied warranty of\n- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n- * GNU General Public License for more details.\n- *\n- * You should have received a copy of the GNU General Public License\n- * along with this program; if not, see .\n- */\n-\n-/*\n- * Credits:\n- * - GNOME Keyring API handling originally written by John Szakmeister\n- * - ported to credential helper API by Philipp A. Hartmann\n- */\n-\n-#include \n-#include \n-#include \n-#include \n-#include \n-\n-#ifdef GNOME_KEYRING_DEFAULT\n-\n- /* Modern gnome-keyring */\n-\n-#include \n-\n-#else\n-\n- /*\n- * Support ancient gnome-keyring, circ. RHEL 5.X.\n- * GNOME_KEYRING_DEFAULT seems to have been introduced with Gnome 2.22,\n- * and the other features roughly around Gnome 2.20, 6 months before.\n- * Ubuntu 8.04 used Gnome 2.22 (I think). Not sure any distro used 2.20.\n- * So the existence/non-existence of GNOME_KEYRING_DEFAULT seems like\n- * a decent thing to use as an indicator.\n- */\n-\n-#define GNOME_KEYRING_DEFAULT NULL\n-\n-/*\n- * ancient gnome-keyring returns DENIED when an entry is not found.\n- * Setting NO_MATCH to DENIED will prevent us from reporting DENIED\n- * errors during get and erase operations, but we will still report\n- * DENIED errors during a store.\n- */\n-#define GNOME_KEYRING_RESULT_NO_MATCH GNOME_KEYRING_RESULT_DENIED\n-\n-#define gnome_keyring_memory_alloc g_malloc\n-#define gnome_keyring_memory_free gnome_keyring_free_password\n-#define gnome_keyring_memory_strdup g_strdup\n-\n-static const char *gnome_keyring_result_to_message(GnomeKeyringResult result)\n-{\n-\tswitch (result) {\n-\tcase GNOME_KEYRING_RESULT_OK:\n-\t\treturn \"OK\";\n-\tcase GNOME_KEYRING_RESULT_DENIED:\n-\t\treturn \"Denied\";\n-\tcase GNOME_KEYRING_RESULT_NO_KEYRING_DAEMON:\n-\t\treturn \"No Keyring Daemon\";\n-\tcase GNOME_KEYRING_RESULT_ALREADY_UNLOCKED:\n-\t\treturn \"Already UnLocked\";\n-\tcase GNOME_KEYRING_RESULT_NO_SUCH_KEYRING:\n-\t\treturn \"No Such Keyring\";\n-\tcase GNOME_KEYRING_RESULT_BAD_ARGUMENTS:\n-\t\treturn \"Bad Arguments\";\n-\tcase GNOME_KEYRING_RESULT_IO_ERROR:\n-\t\treturn \"IO Error\";\n-\tcase GNOME_KEYRING_RESULT_CANCELLED:\n-\t\treturn \"Cancelled\";\n-\tcase GNOME_KEYRING_RESULT_ALREADY_EXISTS:\n-\t\treturn \"Already Exists\";\n-\tdefault:\n-\t\treturn \"Unknown Error\";\n-\t}\n-}\n-\n-/*\n- * Support really ancient gnome-keyring, circ. RHEL 4.X.\n- * Just a guess for the Glib version. Glib 2.8 was roughly Gnome 2.12 ?\n- * Which was released with gnome-keyring 0.4.3 ??\n- */\n-#if GLIB_MAJOR_VERSION == 2 && GLIB_MINOR_VERSION < 8\n-\n-static void gnome_keyring_done_cb(GnomeKeyringResult result, gpointer user_data)\n-{\n-\tgpointer *data = (gpointer *)user_data;\n-\tint *done = (int *)data[0];\n-\tGnomeKeyringResult *r = (GnomeKeyringResult *)data[1];\n-\n-\t*r = result;\n-\t*done = 1;\n-}\n-\n-static void wait_for_request_completion(int *done)\n-{\n-\tGMainContext *mc = g_main_context_default();\n-\twhile (!*done)\n-\t\tg_main_context_iteration(mc, TRUE);\n-}\n-\n-static GnomeKeyringResult gnome_keyring_item_delete_sync(const char *keyring, guint32 id)\n-{\n-\tint done = 0;\n-\tGnomeKeyringResult result;\n-\tgpointer data[] = { &done, &result };\n-\n-\tgnome_keyring_item_delete(keyring, id, gnome_keyring_done_cb, data,\n-\t\tNULL);\n-\n-\twait_for_request_completion(&done);\n-\n-\treturn result;\n-}\n-\n-#endif\n-#endif\n-\n-/*\n- * This credential struct and API is simplified from git's credential.{h,c}\n- */\n-struct credential {\n-\tchar *protocol;\n-\tchar *host;\n-\tunsigned short port;\n-\tchar *path;\n-\tchar *username;\n-\tchar *password;\n-};\n-\n-#define CREDENTIAL_INIT { 0 }\n-\n-typedef int (*credential_op_cb)(struct credential *);\n-\n-struct credential_operation {\n-\tchar *name;\n-\tcredential_op_cb op;\n-};\n-\n-#define CREDENTIAL_OP_END { NULL, NULL }\n-\n-/* ----------------- GNOME Keyring functions ----------------- */\n-\n-/* create a special keyring option string, if path is given */\n-static char *keyring_object(struct credential *c)\n-{\n-\tif (!c->path)\n-\t\treturn NULL;\n-\n-\tif (c->port)\n-\t\treturn g_strdup_printf(\"%s:%hd/%s\", c->host, c->port, c->path);\n-\n-\treturn g_strdup_printf(\"%s/%s\", c->host, c->path);\n-}\n-\n-static int keyring_get(struct credential *c)\n-{\n-\tchar *object = NULL;\n-\tGList *entries;\n-\tGnomeKeyringNetworkPasswordData *password_data;\n-\tGnomeKeyringResult result;\n-\n-\tif (!c->protocol || !(c->host || c->path))\n-\t\treturn EXIT_FAILURE;\n-\n-\tobject = keyring_object(c);\n-\n-\tresult = gnome_keyring_find_network_password_sync(\n-\t\t\t\tc->username,\n-\t\t\t\tNULL /* domain */,\n-\t\t\t\tc->host,\n-\t\t\t\tobject,\n-\t\t\t\tc->protocol,\n-\t\t\t\tNULL /* authtype */,\n-\t\t\t\tc->port,\n-\t\t\t\t&entries);\n-\n-\tg_free(object);\n-\n-\tif (result == GNOME_KEYRING_RESULT_NO_MATCH)\n-\t\treturn EXIT_SUCCESS;\n-\n-\tif (result == GNOME_KEYRING_RESULT_CANCELLED)\n-\t\treturn EXIT_SUCCESS;\n-\n-\tif (result != GNOME_KEYRING_RESULT_OK) {\n-\t\tg_critical(\"%s\", gnome_keyring_result_to_message(result));\n-\t\treturn EXIT_FAILURE;\n-\t}\n-\n-\t/* pick the first one from the list */\n-\tpassword_data = (GnomeKeyringNetworkPasswordData *)entries->data;\n-\n-\tgnome_keyring_memory_free(c->password);\n-\tc->password = gnome_keyring_memory_strdup(password_data->password);\n-\n-\tif (!c->username)\n-\t\tc->username = g_strdup(password_data->user);\n-\n-\tgnome_keyring_network_password_list_free(entries);\n-\n-\treturn EXIT_SUCCESS;\n-}\n-\n-\n-static int keyring_store(struct credential *c)\n-{\n-\tguint32 item_id;\n-\tchar *object = NULL;\n-\tGnomeKeyringResult result;\n-\n-\t/*\n-\t * Sanity check that what we are storing is actually sensible.\n-\t * In particular, we can't make a URL without a protocol field.\n-\t * Without either a host or pathname (depending on the scheme),\n-\t * we have no primary key. And without a username and password,\n-\t * we are not actually storing a credential.\n-\t */\n-\tif (!c->protocol || !(c->host || c->path) ||\n-\t !c->username || !c->password)\n-\t\treturn EXIT_FAILURE;\n-\n-\tobject = keyring_object(c);\n-\n-\tresult = gnome_keyring_set_network_password_sync(\n-\t\t\t\tGNOME_KEYRING_DEFAULT,\n-\t\t\t\tc->username,\n-\t\t\t\tNULL /* domain */,\n-\t\t\t\tc->host,\n-\t\t\t\tobject,\n-\t\t\t\tc->protocol,\n-\t\t\t\tNULL /* authtype */,\n-\t\t\t\tc->port,\n-\t\t\t\tc->password,\n-\t\t\t\t&item_id);\n-\n-\tg_free(object);\n-\n-\tif (result != GNOME_KEYRING_RESULT_OK &&\n-\t result != GNOME_KEYRING_RESULT_CANCELLED) {\n-\t\tg_critical(\"%s\", gnome_keyring_result_to_message(result));\n-\t\treturn EXIT_FAILURE;\n-\t}\n-\n-\treturn EXIT_SUCCESS;\n-}\n-\n-static int keyring_erase(struct credential *c)\n-{\n-\tchar *object = NULL;\n-\tGList *entries;\n-\tGnomeKeyringNetworkPasswordData *password_data;\n-\tGnomeKeyringResult result;\n-\n-\t/*\n-\t * Sanity check that we actually have something to match\n-\t * against. The input we get is a restrictive pattern,\n-\t * so technically a blank credential means \"erase everything\".\n-\t * But it is too easy to accidentally send this, since it is equivalent\n-\t * to empty input. So explicitly disallow it, and require that the\n-\t * pattern have some actual content to match.\n-\t */\n-\tif (!c->protocol && !c->host && !c->path && !c->username)\n-\t\treturn EXIT_FAILURE;\n-\n-\tobject = keyring_object(c);\n-\n-\tresult = gnome_keyring_find_network_password_sync(\n-\t\t\t\tc->username,\n-\t\t\t\tNULL /* domain */,\n-\t\t\t\tc->host,\n-\t\t\t\tobject,\n-\t\t\t\tc->protocol,\n-\t\t\t\tNULL /* authtype */,\n-\t\t\t\tc->port,\n-\t\t\t\t&entries);\n-\n-\tg_free(object);\n-\n-\tif (result == GNOME_KEYRING_RESULT_NO_MATCH)\n-\t\treturn EXIT_SUCCESS;\n-\n-\tif (result == GNOME_KEYRING_RESULT_CANCELLED)\n-\t\treturn EXIT_SUCCESS;\n-\n-\tif (result != GNOME_KEYRING_RESULT_OK) {\n-\t\tg_critical(\"%s\", gnome_keyring_result_to_message(result));\n-\t\treturn EXIT_FAILURE;\n-\t}\n-\n-\t/* pick the first one from the list (delete all matches?) */\n-\tpassword_data = (GnomeKeyringNetworkPasswordData *)entries->data;\n-\n-\tresult = gnome_keyring_item_delete_sync(\n-\t\tpassword_data->keyring, password_data->item_id);\n-\n-\tgnome_keyring_network_password_list_free(entries);\n-\n-\tif (result != GNOME_KEYRING_RESULT_OK) {\n-\t\tg_critical(\"%s\", gnome_keyring_result_to_message(result));\n-\t\treturn EXIT_FAILURE;\n-\t}\n-\n-\treturn EXIT_SUCCESS;\n-}\n-\n-/*\n- * Table with helper operation callbacks, used by generic\n- * credential helper main function.\n- */\n-static struct credential_operation const credential_helper_ops[] = {\n-\t{ \"get\", keyring_get },\n-\t{ \"store\", keyring_store },\n-\t{ \"erase\", keyring_erase },\n-\tCREDENTIAL_OP_END\n-};\n-\n-/* ------------------ credential functions ------------------ */\n-\n-static void credential_init(struct credential *c)\n-{\n-\tmemset(c, 0, sizeof(*c));\n-}\n-\n-static void credential_clear(struct credential *c)\n-{\n-\tg_free(c->protocol);\n-\tg_free(c->host);\n-\tg_free(c->path);\n-\tg_free(c->username);\n-\tgnome_keyring_memory_free(c->password);\n-\n-\tcredential_init(c);\n-}\n-\n-static int credential_read(struct credential *c)\n-{\n-\tchar *buf;\n-\tsize_t line_len;\n-\tchar *key;\n-\tchar *value;\n-\n-\tkey = buf = gnome_keyring_memory_alloc(1024);\n-\n-\twhile (fgets(buf, 1024, stdin)) {\n-\t\tline_len = strlen(buf);\n-\n-\t\tif (line_len && buf[line_len-1] == '\\n')\n-\t\t\tbuf[--line_len] = '\\0';\n-\n-\t\tif (!line_len)\n-\t\t\tbreak;\n-\n-\t\tvalue = strchr(buf, '=');\n-\t\tif (!value) {\n-\t\t\tg_warning(\"invalid credential line: %s\", key);\n-\t\t\tgnome_keyring_memory_free(buf);\n-\t\t\treturn -1;\n-\t\t}\n-\t\t*value++ = '\\0';\n-\n-\t\tif (!strcmp(key, \"protocol\")) {\n-\t\t\tg_free(c->protocol);\n-\t\t\tc->protocol = g_strdup(value);\n-\t\t} else if (!strcmp(key, \"host\")) {\n-\t\t\tg_free(c->host);\n-\t\t\tc->host = g_strdup(value);\n-\t\t\tvalue = strrchr(c->host, ':');\n-\t\t\tif (value) {\n-\t\t\t\t*value++ = '\\0';\n-\t\t\t\tc->port = atoi(value);\n-\t\t\t}\n-\t\t} else if (!strcmp(key, \"path\")) {\n-\t\t\tg_free(c->path);\n-\t\t\tc->path = g_strdup(value);\n-\t\t} else if (!strcmp(key, \"username\")) {\n-\t\t\tg_free(c->username);\n-\t\t\tc->username = g_strdup(value);\n-\t\t} else if (!strcmp(key, \"password\")) {\n-\t\t\tgnome_keyring_memory_free(c->password);\n-\t\t\tc->password = gnome_keyring_memory_strdup(value);\n-\t\t\twhile (*value)\n-\t\t\t\t*value++ = '\\0';\n-\t\t}\n-\t\t/*\n-\t\t * Ignore other lines; we don't know what they mean, but\n-\t\t * this future-proofs us when later versions of git do\n-\t\t * learn new lines, and the helpers are updated to match.\n-\t\t */\n-\t}\n-\n-\tgnome_keyring_memory_free(buf);\n-\n-\treturn 0;\n-}\n-\n-static void credential_write_item(FILE *fp, const char *key, const char *value)\n-{\n-\tif (!value)\n-\t\treturn;\n-\tfprintf(fp, \"%s=%s\\n\", key, value);\n-}\n-\n-static void credential_write(const struct credential *c)\n-{\n-\t/* only write username/password, if set */\n-\tcredential_write_item(stdout, \"username\", c->username);\n-\tcredential_write_item(stdout, \"password\", c->password);\n-}\n-\n-static void usage(const char *name)\n-{\n-\tstruct credential_operation const *try_op = credential_helper_ops;\n-\tconst char *basename = strrchr(name, '/');\n-\n-\tbasename = (basename) ? basename + 1 : name;\n-\tfprintf(stderr, \"usage: %s <\", basename);\n-\twhile (try_op->name) {\n-\t\tfprintf(stderr, \"%s\", (try_op++)->name);\n-\t\tif (try_op->name)\n-\t\t\tfprintf(stderr, \"%s\", \"|\");\n-\t}\n-\tfprintf(stderr, \"%s\", \">\\n\");\n-}\n-\n-int main(int argc, char *argv[])\n-{\n-\tint ret = EXIT_SUCCESS;\n-\n-\tstruct credential_operation const *try_op = credential_helper_ops;\n-\tstruct credential cred = CREDENTIAL_INIT;\n-\n-\tif (!argv[1]) {\n-\t\tusage(argv[0]);\n-\t\texit(EXIT_FAILURE);\n-\t}\n-\n-\tg_set_application_name(\"Git Credential Helper\");\n-\n-\t/* lookup operation callback */\n-\twhile (try_op->name && strcmp(argv[1], try_op->name))\n-\t\ttry_op++;\n-\n-\t/* unsupported operation given -- ignore silently */\n-\tif (!try_op->name || !try_op->op)\n-\t\tgoto out;\n-\n-\tret = credential_read(&cred);\n-\tif (ret)\n-\t\tgoto out;\n-\n-\t/* perform credential operation */\n-\tret = (*try_op->op)(&cred);\n-\n-\tcredential_write(&cred);\n-\n-out:\n-\tcredential_clear(&cred);\n-\treturn ret;\n-}\n-- \n2.40.1.452.gb3cd41c833\n\n"},{"id":"476344","messageId":"60bab84359fd453c231adaae9a11b20bb19173db.1682956419.git.me@ttaylorr.com","threadId":"59677","inReplyTo":"cover.1682956419.git.me@ttaylorr.com","subject":"[PATCH 5/7] contrib/credential: .gitignore libsecret build artifacts","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-01T15:54:00Z","receivedAt":"2023-05-01T15:54:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The libsecret credential helper does not mark its build artifact as\nignored, so running \"make\" results in a dirty working tree.\n\nMark the \"git-credential-libsecret\" binary as ignored to avoid the above.\n\nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n contrib/credential/libsecret/.gitignore | 1 +\n 1 file changed, 1 insertion(+)\n create mode 100644 contrib/credential/libsecret/.gitignore\n\ndiff --git a/contrib/credential/libsecret/.gitignore b/contrib/credential/libsecret/.gitignore\nnew file mode 100644\nindex 0000000000..4fa22359e2\n--- /dev/null\n+++ b/contrib/credential/libsecret/.gitignore\n@@ -0,0 +1 @@\n+git-credential-libsecret\n-- \n2.40.1.452.gb3cd41c833\n\n"},{"id":"476345","messageId":"9b9e2c0dd170eee96aa2c39eae061f8c92732bc4.1682956419.git.me@ttaylorr.com","threadId":"59677","inReplyTo":"cover.1682956419.git.me@ttaylorr.com","subject":"[PATCH 6/7] contrib/credential: avoid fixed-size buffer in libsecret","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-01T15:54:03Z","receivedAt":"2023-05-01T15:54:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The libsecret credential helper reads the newline-delimited\nprotocol stream one line at a time by repeatedly calling fgets() into a\nfixed-size buffer, and is thus affected by the vulnerability described\nin the previous commit.\n\nTo mitigate this attack, avoid using a fixed-size buffer, and instead\nrely on getline() to allocate a buffer as large as necessary to fit the\nentire content of the line, preventing any protocol injection.\n\nIn most parts of Git we don't assume that every platform has getline().\nBut libsecret is primarily used on Linux, where we do already assume it\n(using a knob in config.mak.uname). POSIX also added getline() in 2008,\nso we'd expect other recent Unix-like operating systems to have it\n(e.g., FreeBSD also does).\n\nNote that the buffer was already allocated on the heap in this case, but\nwe'll swap `g_free()` for `free()`, since it will now be allocated by\nthe system `getline()`, rather than glib's `g_malloc()`.\n\nTested-by: Jeff King \nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n .../libsecret/git-credential-libsecret.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\nindex 2c5d76d789..ef681f29d5 100644\n--- a/contrib/credential/libsecret/git-credential-libsecret.c\n+++ b/contrib/credential/libsecret/git-credential-libsecret.c\n@@ -244,17 +244,16 @@ static void credential_clear(struct credential *c)\n \n static int credential_read(struct credential *c)\n {\n-\tchar *buf;\n-\tsize_t line_len;\n+\tchar *buf = NULL;\n+\tsize_t alloc;\n+\tssize_t line_len;\n \tchar *key;\n \tchar *value;\n \n-\tkey = buf = g_malloc(1024);\n+\twhile ((line_len = getline(&buf, &alloc, stdin)) > 0) {\n+\t\tkey = buf;\n \n-\twhile (fgets(buf, 1024, stdin)) {\n-\t\tline_len = strlen(buf);\n-\n-\t\tif (line_len && buf[line_len-1] == '\\n')\n+\t\tif (buf[line_len-1] == '\\n')\n \t\t\tbuf[--line_len] = '\\0';\n \n \t\tif (!line_len)\n@@ -298,7 +297,7 @@ static int credential_read(struct credential *c)\n \t\t */\n \t}\n \n-\tg_free(buf);\n+\tfree(buf);\n \n \treturn 0;\n }\n-- \n2.40.1.452.gb3cd41c833\n\n"},{"id":"476346","messageId":"59a934a256a177e1d47f05a772f53f40ff015ced.1682956419.git.me@ttaylorr.com","threadId":"59677","inReplyTo":"cover.1682956419.git.me@ttaylorr.com","subject":"[PATCH 7/7] contrib/credential: embiggen fixed-size buffer in wincred","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-01T15:54:06Z","receivedAt":"2023-05-01T15:54:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As in previous commits, harden the wincred credential helper against the\naforementioned protocol injection attack.\n\nUnlike the approached used for osxkeychain and libsecret, where a\nfixed-size buffer was replaced with `getline()`, we must take a\ndifferent approach here. There is no `getline()` equivalent in Windows,\nand the function is not available to us with ordinary compiler settings.\n\nInstead, allocate a larger (still fixed-size) buffer in which to process\neach line. The value of 100 KiB is chosen to match the maximum-length\nheader that curl will allow, CURL_MAX_HTTP_HEADER.\n\nTo ensure that we are reading complete lines at a time, and that we\naren't susceptible to a similar injection attack (albeit with more\npadding), ensure that each read terminates at a newline (i.e., that no\nline is more than 100 KiB long).\n\nNote that it isn't sufficient to turn the old loop into something like:\n\n while (len && strchr(\"\\r\\n\", buf[len - 1])) {\n buf[--len] = 0;\n ends_in_newline = 1;\n }\n\nbecause if an attacker sends something like:\n\n [aaaaa.....]\\r\n host=example.com\\r\\n\n\nthe credential helper would fill its buffer after reading up through the\nfirst '\\r', call fgets() again, and then see \"host=example.com\\r\\n\" on\nits line.\n\nNote that the original code was written in a way that would trim an\narbitrary number of \"\\r\" and \"\\n\" from the end of the string. We should\nget only a single \"\\n\" (since the point of `fgets()` is to return the\nbuffer to us when it sees one), and likewise would not expect to see\nmore than one associated \"\\r\". The new code trims a single \"\\r\\n\", which\nmatches the original intent.\n\n[1]: https://curl.se/libcurl/c/CURLOPT_HEADERFUNCTION.html\n\nTested-by: Matthew John Cheetham \nHelped-by: Matthew John Cheetham \nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n .../wincred/git-credential-wincred.c | 21 +++++++++++++++----\n 1 file changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c\nindex ead6e267c7..32ebef7f65 100644\n--- a/contrib/credential/wincred/git-credential-wincred.c\n+++ b/contrib/credential/wincred/git-credential-wincred.c\n@@ -249,16 +249,27 @@ static WCHAR *utf8_to_utf16_dup(const char *str)\n \treturn wstr;\n }\n \n+#define KB (1024)\n+\n static void read_credential(void)\n {\n-\tchar buf[1024];\n+\tsize_t alloc = 100 * KB;\n+\tchar *buf = calloc(alloc, sizeof(*buf));\n \n-\twhile (fgets(buf, sizeof(buf), stdin)) {\n+\twhile (fgets(buf, alloc, stdin)) {\n \t\tchar *v;\n-\t\tint len = strlen(buf);\n+\t\tsize_t len = strlen(buf);\n+\t\tint ends_in_newline = 0;\n \t\t/* strip trailing CR / LF */\n-\t\twhile (len && strchr(\"\\r\\n\", buf[len - 1]))\n+\t\tif (len && buf[len - 1] == '\\n') {\n \t\t\tbuf[--len] = 0;\n+\t\t\tends_in_newline = 1;\n+\t\t}\n+\t\tif (len && buf[len - 1] == '\\r')\n+\t\t\tbuf[--len] = 0;\n+\n+\t\tif (!ends_in_newline)\n+\t\t\tdie(\"bad input: %s\", buf);\n \n \t\tif (!*buf)\n \t\t\tbreak;\n@@ -284,6 +295,8 @@ static void read_credential(void)\n \t\t * learn new lines, and the helpers are updated to match.\n \t\t */\n \t}\n+\n+\tfree(buf);\n }\n \n int main(int argc, char *argv[])\n-- \n2.40.1.452.gb3cd41c833\n"},{"id":"476624","messageId":"bd745f48-038f-f4f2-2041-f4b2c7bd0d20@github.com","threadId":"59677","inReplyTo":"cover.1682956419.git.me@ttaylorr.com","subject":"Re: [PATCH 0/7] contrib/credential: avoid protocol injection attacks","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-05-05T15:24:44Z","receivedAt":"2023-05-05T15:26:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/1/2023 11:53 AM, Taylor Blau wrote:\n> This series addresses a handful of potential protocol injection attacks\n> possible via some of the credential helpers in contrib/credential, and\n> the new \"wwwauth[]\" directive.\n\nSorry for being late to review this. I was not one of the three\ndevelopers involved in writing and/or testing these changes, but I\nam motivated to see these fixes land.\n\n> But the new \"wwwauth[]\" field does allow this attack to take place.\n\nIn particular, because this should be resolved before 2.41.0-rc0.\n\nEach patch was simple to read and well-motivated. I was particularly\nhappy with this diffstat:\n\n> contrib/credential/gnome-keyring/.gitignore | 1 -\n> contrib/credential/gnome-keyring/Makefile | 25 -\n> .../git-credential-gnome-keyring.c | 470 ------------------\n\nThe rest of the changes looked to be obvious improvements, so this\nv1 LGTM.\n\nThanks,\n-Stolee\n"},{"id":"476637","messageId":"ZFVA6MyjUE+L+x9P@nand.local","threadId":"59677","inReplyTo":"bd745f48-038f-f4f2-2041-f4b2c7bd0d20@github.com","subject":"Re: [PATCH 0/7] contrib/credential: avoid protocol injection attacks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-05-05T17:46:16Z","receivedAt":"2023-05-05T17:48:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 05, 2023 at 11:24:44AM -0400, Derrick Stolee wrote:\n> > But the new \"wwwauth[]\" field does allow this attack to take place.\n>\n> In particular, because this should be resolved before 2.41.0-rc0.\n\nYes, definitely.\n\n> Each patch was simple to read and well-motivated. I was particularly\n> happy with this diffstat:\n>\n> > contrib/credential/gnome-keyring/.gitignore | 1 -\n> > contrib/credential/gnome-keyring/Makefile | 25 -\n> > .../git-credential-gnome-keyring.c | 470 ------------------\n>\n> The rest of the changes looked to be obvious improvements, so this\n> v1 LGTM.\n\nThanks. Much credit is owed to Peff, who worked on these patches with\nme. And FWIW, dropping support for the gnome-keyring helper was his\nidea.\n\nThanks for the review :-).\n\nThanks,\nTaylor\n"}]}