I confirm the patch works on my system.
On Thu, Aug 1, 2024 at 4:25 PM Jeff King <peff@xxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> 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
>
