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
a6253da (contrib: add win32 credential-helper, 2012-07-27). Commit
8b2d219 (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 <[email protected]>
Helped-by: David Leadbeater <[email protected]>
Helped-by: Jeff King <[email protected]>
Signed-off-by: Taylor Blau <[email protected]>

Author: ttaylorr

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

@@ -37,6 +37,14 @@ static void *xmalloc(size_t size)
 static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
 	*password_expiry_utc;
 
+static void target_append(const WCHAR *src)
+{
+	size_t avail = ARRAY_SIZE(target) - wcslen(target) - 1; /* -1 for NUL */
+	if (avail < wcslen(src))
+		die("target buffer overflow");
+	wcsncat(target, src, avail);
+}
+
 static void write_item(const char *what, LPCWSTR wbuf, int wlen)
 {
 	char *buf;
@@ -294,17 +302,17 @@ int main(int argc, char *argv[])
 
 	/* prepare 'target', the unique key for the credential */
 	wcscpy(target, L"git:");
-	wcsncat(target, protocol, ARRAY_SIZE(target));
-	wcsncat(target, L"://", ARRAY_SIZE(target));
+	target_append(protocol);
+	target_append(L"://");
 	if (wusername) {
-		wcsncat(target, wusername, ARRAY_SIZE(target));
-		wcsncat(target, L"@", ARRAY_SIZE(target));
+		target_append(wusername);
+		target_append(L"@");
 	}
 	if (host)
-		wcsncat(target, host, ARRAY_SIZE(target));
+		target_append(host);
 	if (path) {
-		wcsncat(target, L"/", ARRAY_SIZE(target));
-		wcsncat(target, path, ARRAY_SIZE(target));
+		target_append(L"/");
+		target_append(path);
 	}
 
 	if (!strcmp(argv[1], "get"))