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>

Taylor Blau committed May 1, 2023 at 11:54 UTC 0a3a972c163b2c5ed81a8e2c12fbf0c53eeb210c
1 file changed +17 -4
contrib/credential/wincred/git-credential-wincred.c
+17 -4
@@ -249,17 +249,28 @@ static WCHAR *utf8_to_utf16_dup(const char *str)
249 return wstr;
250 }
251
252 +#define KB (1024)
253 +
254 static void read_credential(void)
255 {
254 - char buf[1024];
256 + size_t alloc = 100 * KB;
257 + char *buf = calloc(alloc, sizeof(*buf));
258
256 - while (fgets(buf, sizeof(buf), stdin)) {
259 + while (fgets(buf, alloc, stdin)) {
260 char *v;
258 - int len = strlen(buf);
261 + size_t len = strlen(buf);
262 + int ends_in_newline = 0;
263 /* strip trailing CR / LF */
260 - while (len && strchr("\r\n", buf[len - 1]))
264 + if (len && buf[len - 1] == '\n') {
265 + buf[--len] = 0;
266 + ends_in_newline = 1;
267 + }
268 + if (len && buf[len - 1] == '\r')
269 buf[--len] = 0;
270
271 + if (!ends_in_newline)
272 + die("bad input: %s", buf);
273 +
274 if (!*buf)
275 break;
276
@@ -284,6 +295,8 @@ static void read_credential(void)
295 * learn new lines, and the helpers are updated to match.
296 */
297 }
298 +
299 + free(buf);
300 }
301
302 int main(int argc, char *argv[])