Re: [PATCH] credential/wincred: store password_expiry_utc

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi M,

On Sat, 25 Mar 2023, M Hickford via GitGitGadget wrote:

> 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...

> @@ -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 --
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
>