From: Junio C Hamano Date: Fri, 09 Oct 2026 21:16:12 GMT Subject: Re: [PATCH] wincred: fix line split of secret blob content Message-ID: In-Reply-To: "Marc Becker via GitGitGadget" writes: > From: Marc Becker > > 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 > --- > 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 > #include > #include > +#include > #include > > /* 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