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>

diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 4cd56c42e24469a48a40e7b02de4b4921ff8662c..ceff44207ad8c03abf6db3861cc5d22742f0d64e 100644 (file)
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/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"))