Re: [PATCH] wincred: fix line split of secret blob content

[Junio C Hamano <gitster@pobox.com>]

"Marc Becker via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Marc Becker <becm@gmx.de>
>
> operate on immutable blob data (wcsncpy_s still had invalid target size)
> split on newline character to avoid bleed-over on multi-line content

This needs a bit more work to make it more readable than a bulleted
list of lowercase fragments.

When in doubt, keep in mind that the usual way to compose a log
message of this project is to:

 - Give an observation on how the current system works in the
   present tense (so no need to say "Currently X is Y", or
   "Previously X was Y" to describe the state before your change;
   just "X is Y" is enough), and discuss what you perceive as a
   problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to somebody editing the codebase to "make it so",
   instead of saying "This commit does X".

in this order.

 - It mentions wcsncpy_s having an invalid target size, but does not
   explain why it was invalid or the consequences. Is the issue that
   wcsncpy_s expects the buffer size in wide characters, but was
   being passed a size in bytes, which obviously cannot always
   agree?

 - It mentions "bleed-over on multi-line content", but does not
   describe the observable symptoms.  Is the issue that when the
   password is empty, the skipping by wcstok_s delimiter would cause
   the oauth_refresh_token line to be erroneously parsed as the
   password?

 - The final sentence should be an imperative command to the
   codebase, e.g., "Parse the blob in-place without copying and
   split lines manually using wmemchr()."

>
> Signed-off-by: Marc Becker <becm@gmx.de>
> ---
>     wincred: fix line split of secret blob content
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2251%2Fbecm%2Ffix-wincred-secret-linesplit-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2251/becm/fix-wincred-secret-linesplit-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2251
>
>  .../wincred/git-credential-wincred.c          | 86 ++++++++++++-------
>  1 file changed, 55 insertions(+), 31 deletions(-)
>
> diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
> index 22eb27ca31..584f457774 100644
> --- a/contrib/credential/wincred/git-credential-wincred.c
> +++ b/contrib/credential/wincred/git-credential-wincred.c
> @@ -6,6 +6,7 @@
>  #include <stdio.h>
>  #include <io.h>
>  #include <fcntl.h>
> +#include <wchar.h>
>  #include <wincred.h>
>  
>  /* common helpers */
> @@ -148,51 +149,74 @@ static void get_credential(void)
>  {
>  	CREDENTIALW **creds;
>  	DWORD num_creds;
> -	int i;
> -	CREDENTIAL_ATTRIBUTEW *attr;
> -	WCHAR *secret;
> -	WCHAR *line;
> -	WCHAR *remaining_lines;
> -	WCHAR *part;
> -	WCHAR *remaining_parts;
>  
>  	if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
>  		return;
>  
> -	/* search for the first credential that matches username */
> -	for (i = 0; i < num_creds; ++i)
> +	/* search for the first credential that matches target and username */
> +	for (int i = 0; i < num_creds; ++i) {
>  		if (match_cred(creds[i], 0)) {
> -			write_item("username", creds[i]->UserName,
> -				creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
> -			if (creds[i]->CredentialBlobSize > 0) {
> -				secret = xmalloc(creds[i]->CredentialBlobSize + sizeof(WCHAR));
> -				wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
> -				line = wcstok_s(secret, L"\r\n", &remaining_lines);
> -				write_item("password", line, line ? wcslen(line) : 0);
> -				while(line != NULL) {
> -					part = wcstok_s(line, L"=", &remaining_parts);
> -					if (!wcscmp(part, L"oauth_refresh_token")) {
> -						write_item("oauth_refresh_token", remaining_parts, remaining_parts ? wcslen(remaining_parts) : 0);
> -					}
> -					line = wcstok_s(NULL, L"\r\n", &remaining_lines);
> -				}
> -				free(secret);

The original was already bad, but this makes it even worse to have
the code nested too deeply.  Would separating out the body of the
for loop into a separate helper function, or perhaps standard tricks
like this

	for (...) {
		if (!match_cred(...))
			continue;
		... rest of the loop dedented by one tab stop ...
	}

make it readable?

> +			LPCWSTR username = creds[i]->UserName;
> +			LPCWSTR blob = (LPCWSTR)creds[i]->CredentialBlob;
> +			LPCWSTR end;
> +			DWORD wlen;
> +
> +			write_item("username", username, username ? wcslen(username) : 0);
> +
> +			wlen = creds[i]->CredentialBlobSize / sizeof(WCHAR);
> +
> +			// check if content is single line

			/* our single line comment should look like this */

> +			if ((end = wmemchr(blob, '\n', wlen)) == NULL) {

I do not do Windows and I do not often deal with wchar_t, so I do
not know how much practitioners of code like this one cares, but
would it be better to make the fact clear that we are not dealing
with a regular 'char' by writing a wchar_t literal like this as
L'\n'?  This is not a correctness suggestion, but a readability one.
Having a function prototype would coerse the parameter types, so
you may end up passing L'\n' either way.

> +				write_item("password", blob, wlen);
>  			} else {
> -				write_item("password",
> -						(LPCWSTR)creds[i]->CredentialBlob,
> -						creds[i]->CredentialBlobSize / sizeof(WCHAR));
> +				DWORD length = end++ - blob;

Here, "end" is of LPCWSTR type, aka "wchar_t *".  So is "blob".  The
difference would give us how many wide characters are in there.
That is not necessarily number of bytes starting at &blob[0].

> +				// correct remaining size and drop carriage return at line end
> +				wlen -= length + 1;
> +				if (length && blob[length - 1] == '\r') {

This CR is also side, right?

> +					--length;
> +				}
> +				write_item("password", blob, length);
> +
> +				// key/value content starting on next line
> +				blob = end;
> +				do {
> +					LPCWSTR value;
> +
> +					// find line end
> +					if ((end = wmemchr(blob, '\n', wlen)) == NULL) {
> +						length = wlen;
> +					} else {
> +						length = end++ - blob;
> +						// correct remaining size and drop carriage return at line end
> +						wlen -= length + 1;
> +						if (length && blob[length - 1] == '\r') {
> +							--length;
> +						}
> +					}
> +					// find key/value separator for extended credential info
> +					if ((value = wmemchr(blob, '=', length)) != NULL) {
> +						static const LPCWSTR refresh = L"oauth_refresh_token";
> +						DWORD klen = value - blob;

Value is also "wchar_t *", so klen counts the length in wchar_t,
which may be wider than a byte.  So is

> +						// write entries known to git credential protocol
> +						if (klen == wcslen(refresh) && memcmp(blob, refresh, klen) == 0) {

klen that counts number of wchar_t letters in refresh[] string.

So, is the memcmp() used to check if early part of blob[] match the
refresh[] as a whole correct, or is it only checking an early half
(or one fourth, depending on how much wider your wchar_t is compared
to char) of the string?

> +							write_item("oauth_refresh_token", value + 1, length - klen - 1);
> +						}
> +					}
> +				} while ((blob = end));
>  			}
>  			for (int j = 0; j < creds[i]->AttributeCount; j++) {
> -				attr = creds[i]->Attributes + j;
> +				CREDENTIAL_ATTRIBUTEW *attr = creds[i]->Attributes + j;
> +
>  				if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
> -					write_item("password_expiry_utc", (LPCWSTR)attr->Value,
> -					attr->ValueSize / sizeof(WCHAR));
> +					write_item("password_expiry_utc", (LPCWSTR)attr->Value, attr->ValueSize / sizeof(WCHAR));
>  					break;
>  				}
>  			}
>  			break;
>  		}
> -
> +	}
>  	CredFree(creds);
>  }
>  
>
> base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd