@matheusbsbs / matheus-bagliano / commits / 9de345cb27

wincred: avoid buffer overflow in wcsncat()

The wincred credential helper uses a static buffer ("target") as a unique key for storing and comparing against internal storage. It does this by building up a string is supposed to look like: git:$PROTOCOL://$USERNAME@$HOST/@PATH However, the static "target" buffer is declared as a wide string with no more than 1,024 wide characters. The first call to wcsncat() is almost correct (it copies no more than ARRAY_SIZE(target) wchar_t's), but does not account for the trailing NUL, introducing an off-by-one error. But subsequent calls to wcsncat() have an additional problem on top of the off-by-one. They do not account for the length of the existing wide string being built up in 'target'. So the following: $ perl -e ' my $x = "x" x 1_000; print "protocol=$x\nhost=$x\nusername=$x\npath=$x\n" ' | C\:/Program\ Files/Git/mingw64/libexec/git-core/git-credential-wincred.exe get will result in a segmentation fault from over-filling buffer. This bug is as old as the wincred helper itself, dating back to a6253da0f3 (contrib: add win32 credential-helper, 2012-07-27). Commit 8b2d219a3d (wincred: improve compatibility with windows versions, 2013-01-10) replaced the use of strncat() with wcsncat(), but retained the buggy behavior. Fix this by using a "target_append()" helper which accounts for both the length of the existing string within the buffer, as well as the trailing NUL character. Reported-by: David Leadbeater <dgl@dgl.cx> Helped-by: David Leadbeater <dgl@dgl.cx> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Taylor Blau committed May 19, 2025 at 18:30 UTC 9de345cb273cc7faaeda279c7e07149d8a15a319

1 file changed +15 -7

contrib/credential/wincred/git-credential-wincred.c

+15 -7

@@ -37,6 +37,14 @@ static void *xmalloc(size_t size)

37 static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],

38 *password_expiry_utc;

40 +static void target_append(const WCHAR *src)

41 +{

42 + size_t avail = ARRAY_SIZE(target) - wcslen(target) - 1; /* -1 for NUL */

43 + if (avail < wcslen(src))

44 + die("target buffer overflow");

45 + wcsncat(target, src, avail);

46 +}

47 +

48 static void write_item(const char *what, LPCWSTR wbuf, int wlen)

49 {

50 char *buf;

@@ -294,17 +302,17 @@ int main(int argc, char *argv[])

302

303 /* prepare 'target', the unique key for the credential */

304 wcscpy(target, L"git:");

297 - wcsncat(target, protocol, ARRAY_SIZE(target));

298 - wcsncat(target, L"://", ARRAY_SIZE(target));

305 + target_append(protocol);

306 + target_append(L"://");

307 if (wusername) {

300 - wcsncat(target, wusername, ARRAY_SIZE(target));

301 - wcsncat(target, L"@", ARRAY_SIZE(target));

308 + target_append(wusername);

309 + target_append(L"@");

310 }

311 if (host)

304 - wcsncat(target, host, ARRAY_SIZE(target));

312 + target_append(host);

313 if (path) {

306 - wcsncat(target, L"/", ARRAY_SIZE(target));

307 - wcsncat(target, path, ARRAY_SIZE(target));

314 + target_append(L"/");

315 + target_append(path);

316 }

317

318 if (!strcmp(argv[1], "get"))