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

[PATCH 7/7] contrib/credential: embiggen fixed-size buffer in wincred

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

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
Previous: Taylor BlauNext: Derrick Stolee
Message 8 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.