Re: [PATCH] credential/wincred: store password_expiry_utc
Hi M,
On Sat, 25 Mar 2023, M Hickford via GitGitGadget wrote:
Show 17 quoted lines
> From: M Hickford <mirth.hickford@gmail.com>
>
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
> credential/wincred: store password_expiry_utc
>
> Help wanted from a Windows user to test. I tried testing on Linux with
> Wine after cross-compiling [1] but Wine has incomplete support for
> wincred.h [2]. To test:
>
> cd contrib/credential/wincred/
> make
> echo host=example.com\nprotocol=https\nusername=tim\npassword=xyzzy\npassword_expiry_utc=2000 | ./git-credential-wincred.exe store
> echo host=example.com\nprotocol=https | ./git-credential-wincred.exe get
>
>
> Output of second command should include line password_expiry_utc=2000
Sadly, no, it's empty:
$ cd contrib/credential/wincred/
$ make
cc git-credential-wincred.c -o git-credential-wincred.exe
$ echo host=example.com\nprotocol=https\nusername=tim\npassword=xyzzy\npassword_expiry_utc=2000 | ./git-credential-wincred.exe store
$ echo host=example.com\nprotocol=https | ./git-credential-wincred.exe get
The reason is that `echo` does not interpret the escaped `n`, it does not even get the backslash because Bash eats it first.
But even with `printf` it does not work:
$ printf 'host=example.com\nprotocol=https\nusername=tim\npassword=xyzzy\npassword_expiry_utc=2000\n' | ./git-credential-wincred.exe store
$ printf 'host=example.com\nprotocol=https\n' | ./git-credential-wincred.exe get username=tim
password=xyzzy
And the reason is...
Show 7 quoted lines
> @@ -195,6 +197,15 @@ static void get_credential(void)
> write_item("password",
> (LPCWSTR)creds[i]->CredentialBlob,
> creds[i]->CredentialBlobSize / sizeof(WCHAR));
> + attr = creds[i]->Attributes;
> + for (int j = 0; j < creds[i]->AttributeCount; j++) {
> + if (wcscmp(attr->Keyword, L"git_password_expiry_utc")) { ^^^^^^
... here. Note how the return value of `wcscmp()` needs to be non-zero to enter the conditional block? But `wcscmp()` returns 0 if there are no differences between the two provided strings.
That's not the only bug, though. While the loop iterates over all of the attributes, the `attr` variable is unchanged, and always points to the first attribute. You have to access it as `creds[i]->Attributes[j]`, though, see e.g. https://github.com/sandboxie-plus/Sandboxie/blob/f2a357f9222b81e7bdc994e5d9824790a1b5d826/Sandboxie/core/dll/cred.c#L324
So with this patch on top of your patch, it works for me:
-- snip --
Show changes to contrib/credential/wincred/git-credential-wincred.c +2 −3
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 9be892610d0..1aa840e31a0 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -197,9 +197,9 @@ static void get_credential(void)
write_item("password",
(LPCWSTR)creds[i]->CredentialBlob,
creds[i]->CredentialBlobSize / sizeof(WCHAR));
- attr = creds[i]->Attributes;
for (int j = 0; j < creds[i]->AttributeCount; j++) {
- if (wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
+ 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));
break;
-- snap --
But I have to wonder: why even bother with `git-wincred`? This credential
helper is so ridiculously limited in its capabilities, it does not even
support any host that is remotely close to safe (no 2FA, no OAuth, just
passwords). So I would be just as happy if I weren't asked to spend my
time to review changes to a credential helper I'd much rather see retired
than actively worked on.
Ciao,
Johannes
> + write_item("password_expiry_utc", (LPCWSTR)attr->Value,
> + attr->ValueSize / sizeof(WCHAR));
> + break;
> + }
> + attr++;
> + }
> break;
> }
>
> @@ -204,6 +215,7 @@ static void get_credential(void)
> static void store_credential(void)
> {
> CREDENTIALW cred;
> + CREDENTIAL_ATTRIBUTEW expiry_attr;
>
> if (!wusername || !password)
> return;
> @@ -217,6 +229,14 @@ static void store_credential(void)
> cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
> cred.AttributeCount = 0;
> cred.Attributes = NULL;
> + if (password_expiry_utc != NULL) {
> + expiry_attr.Keyword = L"git_password_expiry_utc";
> + expiry_attr.Value = (LPVOID)password_expiry_utc;
> + expiry_attr.ValueSize = (wcslen(password_expiry_utc)) * sizeof(WCHAR);
> + expiry_attr.Flags = 0;
> + cred.Attributes = &expiry_attr;
> + cred.AttributeCount = 1;
> + }
> cred.TargetAlias = NULL;
> cred.UserName = wusername;
>
> @@ -278,6 +298,8 @@ static void read_credential(void)
> wusername = utf8_to_utf16_dup(v);
> } else if (!strcmp(buf, "password"))
> password = utf8_to_utf16_dup(v);
> + else if (!strcmp(buf, "password_expiry_utc"))
> + password_expiry_utc = utf8_to_utf16_dup(v);
> /*
> * Ignore other lines; we don't know what they mean, but
> * this future-proofs us when later versions of git do
> @@ -292,7 +314,7 @@ int main(int argc, char *argv[])
> "usage: git credential-wincred <get|store|erase>\n";
>
> if (!argv[1])
> - die(usage);
> + die("%s", usage);
>
> /* git use binary pipes to avoid CRLF-issues */
> _setmode(_fileno(stdin), _O_BINARY);
>
> base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e
> --
> gitgitgadget
>
M Hickford via GitGitGadget
