[PATCH 7/7] contrib/credential: embiggen fixed-size buffer in wincred
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 1, 2023, 15:54 UTC
- Message-ID
- <59a934a256a177e1d47f05a772f53f40ff015ced.1682956419.git.me@ttaylorr.com>
- In-Reply-To
- <cover.1682956419.git.me@ttaylorr.com>
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\nthe 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> --- .../wincred/git-credential-wincred.c | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c index ead6e267c7..32ebef7f65 100644 --- a/contrib/credential/wincred/git-credential-wincred.c +++ b/contrib/credential/wincred/git-credential-wincred.c @@ -249,16 +249,27 @@ 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[])
-- 2.40.1.452.gb3cd41c833