| Message ID | CAEcKSiyo3dyNpGkE_FWE-Y710RV0H3EytM2psC=+by=4wP5qpg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | credential-osxkeychain: Clear username_buffer before getting the converted C string. | expand |
On Wed, Jul 31, 2024 at 11:41:23AM +0800, Hong Jiang wrote: > 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. Hmm. That does look like it could be uninitialized memory (assuming you don't have those garbage characters in the keychain storage). > So I made the patch, which zeros the username_buf before retrieving > the converted C string. If that helps, then that implies that the string we are getting is not NUL-terminated. But... > 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, ...we are getting it by calling CFStringGetCString(). I don't know anything about the OS API here, and I don't have a system to test on. But according to the documentation at: https://developer.apple.com/documentation/corefoundation/1542721-cfstringgetcstring it should return a NUL-terminated string. 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); } But somebody with a functioning macOS system would need to check whether any of what I just said is true. This code comes from 9abe31f5f1 (osxkeychain: replace deprecated SecKeychain API, 2024-02-17). Adding the author to the CC. -Peff
> 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. Bo
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,
I encountered this problem with homebrew after I upgraded to macOS 12.7.5, but I am not sure the OS upgrade is the only reason. After `brew upgrade`, I received the following message: Error: invalid byte sequence in UTF-8 /usr/local/Homebrew/Library/Homebrew/utils/github/api.rb:182:in `[]' /usr/local/Homebrew/Library/Homebrew/utils/github/api.rb:182:in `block in keychain_username_password' The related lines in api.rb are: git_credential_out, _, result = system_command "git", args: ["credential-osxkeychain", "get"], input: ["protocol=https\n", "host=github.com\n"], env: { "HOME" => uid_home }.compact, print_stderr: false return unless result.success? github_username = git_credential_out[/username=(.+)/, 1] github_password = git_credential_out[/password=(.+)/, 1] return unless github_username 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. So I made the patch, which zeros the username_buf before retrieving the converted C string. From: Jiang Hong <ilford@gmail.com> Date: Wed, 31 Jul 2024 11:05:44 +0800 Subject: [PATCH] Zeroing username_buffer before retrieving the converted C string. In macOS 12.7.5 and 12.7.6, the uninitialized username_buffer receives a non-NULL-terminated C string. --- contrib/credential/osxkeychain/git-credential-osxkeychain.c | 1 + 1 file changed, 1 insertion(+)