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 <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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index ead6e267c78120f06bfd2e74def8bbe2f4b66e50..32ebef7f6556614cee1438edf95caeaf4245ef4d 100644 (file)
--- 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[])