patchcredential-osxkeychain: Clear username_buffer before getting the converted C string.

> So it looks like that git_credential_out has invalid UTF-8 byte
> sequence. I print it after the system_command "git":
> 
> password=gho_SHADOWED
> username=jdp1024��`
> F�
> capability[]=state
> state[]=osxkeychain:seen=1
> 
> and
> 
> echo "protocol=https\nhost=github.com\n" | git credential-osxkeychain get
> 
> reproduced the problem.

> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index 6ce22a28ed..89cd575bd5 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -137,6 +137,7 @@ static void find_username_in_item(CFDictionaryRef item)
>   buffer_len = CFStringGetMaximumSizeForEncoding(
>   CFStringGetLength(account_ref), ENCODING) + 1;
>   username_buf = xmalloc(buffer_len);
> + memset(username_buf, 0, buffer_len);
>   if (CFStringGetCString(account_ref,
>   username_buf,
>   buffer_len,

> On 31 Jul 2024, at 08:42, Jeff King <peff@peff.net> wrote:
> 
> Hrm. Just looking at the code, here's a wild hypothesis: the problem
> could be not that the buffer is not NUL-terminated, but that after the
> NUL it contains junk, and we print that junk. That is, the code looks
> like this:
> 
>          /* If we can't get a CString pointer then
>           * we need to allocate our own buffer */
>          buffer_len = CFStringGetMaximumSizeForEncoding(
>                          CFStringGetLength(account_ref), ENCODING) + 1;
>          username_buf = xmalloc(buffer_len);
>          if (CFStringGetCString(account_ref,
>                                  username_buf,
>                                  buffer_len,
>                                  ENCODING)) {
>                  write_item("username", username_buf, buffer_len - 1);
>          }
> 
> So we asked the system for the _maximum_ size that the string could be
> (and added one for the NUL). Then we got the string, and we printed out
> the _whole_ buffer, not just the string up to the NUL. And your fix
> "works" because NULs end up getting ignored on the read side (or at
> least cause ruby not to complain about bogus utf8).
> 
> If that hypothesis is true, then the fix is more like:
> 
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index 6ce22a28ed..1c8310d7fe 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -141,7 +141,7 @@ static void find_username_in_item(CFDictionaryRef item)
> 				username_buf,
> 				buffer_len,
> 				ENCODING)) {
> -		write_item("username", username_buf, buffer_len - 1);
> +		write_item("username", username_buf, strlen(username_buf));
> 	}
> 	free(username_buf);
> }

> This is correct.
> 
> The reason I couldn’t reproduce the problem and how few will have noticed up to
> now is that for most users the CFStringGetCStringPtr call, which correctly uses
> strlen, does what is necessary and we return early. I don't entirely know the
> precise criteria where the fallback is used but I imagine it depends on certain
> system encodings/locales.
> 
> The patch changing this to strlen looks good to me to apply to master & maint.

>
> On Wed, Jul 31, 2024 at 02:07:32PM +0100, Bo Anderson wrote:
>
> > This is correct.
> >
> > The reason I couldn’t reproduce the problem and how few will have noticed up to
> > now is that for most users the CFStringGetCStringPtr call, which correctly uses
> > strlen, does what is necessary and we return early. I don't entirely know the
> > precise criteria where the fallback is used but I imagine it depends on certain
> > system encodings/locales.
> >
> > The patch changing this to strlen looks good to me to apply to master & maint.
>
> Thanks. Here it is with a commit message. Hopefully Hong Jiang can
> confirm that this fixes the problem, and we can added a "Tested-by"
> trailer.
>
> -- >8 --
> Subject: [PATCH] credential/osxkeychain: respect NUL terminator in username
>
> This patch fixes a case where git-credential-osxkeychain might output
> uninitialized bytes to stdout.
>
> We need to get the username string from a system API using
> CFStringGetCString(). To do that, we get the max size for the string
> from CFStringGetMaximumSizeForEncoding(), allocate a buffer based on
> that, and then read into it. But then we print the entire buffer to
> stdout, including the trailing NUL and any extra bytes which were not
> needed. Instead, we should stop at the NUL.
>
> This code comes from 9abe31f5f1 (osxkeychain: replace deprecated
> SecKeychain API, 2024-02-17). The bug was probably overlooked back then
> because this code is only used as a fallback when we can't get the
> string via CFStringGetCStringPtr(). According to Apple's documentation:
>
>   Whether or not this function returns a valid pointer or NULL depends
>   on many factors, all of which depend on how the string was created and
>   its properties.
>
> So it's not clear how we could make a test for this, and we'll have to
> rely on manually testing on a system that triggered the bug in the first
> place.
>
> Reported-by: Hong Jiang <ilford@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is not even compile tested by me! It looks like an obvious enough
> fix, and I wanted to make sure we don't forget about it. But anybody who
> can reproduce or test would be greatly appreciated.
>
>  contrib/credential/osxkeychain/git-credential-osxkeychain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index 6ce22a28ed..1c8310d7fe 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -141,7 +141,7 @@ static void find_username_in_item(CFDictionaryRef item)
>                                 username_buf,
>                                 buffer_len,
>                                 ENCODING)) {
> -               write_item("username", username_buf, buffer_len - 1);
> +               write_item("username", username_buf, strlen(username_buf));
>         }
>         free(username_buf);
>  }
> --
> 2.46.0.452.g3bd18f5164
>

> On Thu, Aug 1, 2024 at 4:25 PM Jeff King <peff@peff.net> wrote:
> ...
>> ---
>> This is not even compile tested by me! It looks like an obvious enough
>> fix, and I wanted to make sure we don't forget about it. But anybody who
>> can reproduce or test would be greatly appreciated.