git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 6/7] contrib/credential: avoid fixed-size buffer in libsecret

From
Taylor Blau <me@ttaylorr.com>
Date
May 1, 2023, 15:54 UTC
Message-ID
<9b9e2c0dd170eee96aa2c39eae061f8c92732bc4.1682956419.git.me@ttaylorr.com>
In-Reply-To
<cover.1682956419.git.me@ttaylorr.com>

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
Previous: Taylor BlauNext: Taylor Blau
Message 7 of 10 in “contrib/credential: avoid protocol injection attacks”
  1. 0/7 contrib/credential: avoid protocol injection attacksTaylor Blau, May 1, 2023
  2. 1/7 credential.c: store "wwwauth[]" values in `credential_read()`Taylor Blau, May 1, 2023
  3. 2/7 t/lib-credential.sh: ensure credential helpers handle long headersTaylor Blau, May 1, 2023
  4. 3/7 contrib/credential: avoid fixed-size buffer in osxkeychainTaylor Blau, May 1, 2023
  5. 4/7 contrib/credential: remove 'gnome-keyring' credential helperTaylor Blau, May 1, 2023
  6. 5/7 contrib/credential: .gitignore libsecret build artifactsTaylor Blau, May 1, 2023
  7. 6/7 contrib/credential: avoid fixed-size buffer in libsecretTaylor Blau, May 1, 2023
  8. 7/7 contrib/credential: embiggen fixed-size buffer in wincredTaylor Blau, May 1, 2023
  9. Derrick StoleeMay 5, 2023
  10. Taylor BlauMay 5, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.