{"thread":{"id":"61872","subject":"[patch] credential-osxkeychain: Clear username_buffer before getting the converted C string.","startedAt":"2024-07-31T03:41:36Z","lastAt":"2024-08-01T15:54:38Z","messageCount":7,"participants":["Hong Jiang","Jeff King","Bo Anderson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"499661","messageId":"CAEcKSiyo3dyNpGkE_FWE-Y710RV0H3EytM2psC=+by=4wP5qpg@mail.gmail.com","threadId":"61872","inReplyTo":null,"subject":"[patch] credential-osxkeychain: Clear username_buffer before getting the converted C string.","fromName":"Hong Jiang","fromEmail":"ilford@gmail.com","sentAt":"2024-07-31T03:41:23Z","receivedAt":"2024-07-31T03:41:36Z","isPatch":true,"sender":{"key":"ilford@gmail.com","avatar":null},"body":"I encountered this problem with homebrew after I upgraded to macOS\n12.7.5, but I am not sure the OS upgrade is the only reason.\n\nAfter `brew upgrade`, I received the following message:\n\nError: invalid byte sequence in UTF-8\n/usr/local/Homebrew/Library/Homebrew/utils/github/api.rb:182:in `[]'\n/usr/local/Homebrew/Library/Homebrew/utils/github/api.rb:182:in `block\nin keychain_username_password'\n\nThe related lines in api.rb are:\n\n git_credential_out, _, result = system_command \"git\",\n args:\n[\"credential-osxkeychain\", \"get\"],\n input:\n[\"protocol=https\\n\", \"host=github.com\\n\"],\n env: {\n\"HOME\" => uid_home }.compact,\n print_stderr: false\n return unless result.success?\n\n github_username = git_credential_out[/username=(.+)/, 1]\n github_password = git_credential_out[/password=(.+)/, 1]\n return unless github_username\n\nSo it looks like that git_credential_out has invalid UTF-8 byte\nsequence. I print it after the system_command \"git\":\n\npassword=gho_SHADOWED\nusername=jdp1024��`\nF�\ncapability[]=state\nstate[]=osxkeychain:seen=1\n\nand\n\necho \"protocol=https\\nhost=github.com\\n\" | git credential-osxkeychain get\n\nreproduced the problem.\n\nSo I made the patch, which zeros the username_buf before retrieving\nthe converted C string.\n\nFrom: Jiang Hong \nDate: Wed, 31 Jul 2024 11:05:44 +0800\nSubject: [PATCH] Zeroing username_buffer before retrieving the\nconverted C string.\n\nIn macOS 12.7.5 and 12.7.6, the uninitialized username_buffer receives\na non-NULL-terminated C string.\n---\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nb/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 6ce22a28ed..89cd575bd5 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -137,6 +137,7 @@ static void find_username_in_item(CFDictionaryRef item)\n buffer_len = CFStringGetMaximumSizeForEncoding(\n CFStringGetLength(account_ref), ENCODING) + 1;\n username_buf = xmalloc(buffer_len);\n+ memset(username_buf, 0, buffer_len);\n if (CFStringGetCString(account_ref,\n username_buf,\n buffer_len,\n-- \n2.37.1 (Apple Git-137.1)\n"},{"id":"499669","messageId":"20240731074228.GC595974@coredump.intra.peff.net","threadId":"61872","inReplyTo":"CAEcKSiyo3dyNpGkE_FWE-Y710RV0H3EytM2psC=+by=4wP5qpg@mail.gmail.com","subject":"Re: [patch] credential-osxkeychain: Clear username_buffer before getting the converted C string.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-31T07:42:28Z","receivedAt":"2024-07-31T07:42:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2024 at 11:41:23AM +0800, Hong Jiang wrote:\n\n> So it looks like that git_credential_out has invalid UTF-8 byte\n> sequence. I print it after the system_command \"git\":\n> \n> password=gho_SHADOWED\n> username=jdp1024��`\n> F�\n> capability[]=state\n> state[]=osxkeychain:seen=1\n> \n> and\n> \n> echo \"protocol=https\\nhost=github.com\\n\" | git credential-osxkeychain get\n> \n> reproduced the problem.\n\nHmm. That does look like it could be uninitialized memory (assuming you\ndon't have those garbage characters in the keychain storage).\n\n> So I made the patch, which zeros the username_buf before retrieving\n> the converted C string.\n\nIf that helps, then that implies that the string we are getting is not\nNUL-terminated. But...\n\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index 6ce22a28ed..89cd575bd5 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -137,6 +137,7 @@ static void find_username_in_item(CFDictionaryRef item)\n> buffer_len = CFStringGetMaximumSizeForEncoding(\n> CFStringGetLength(account_ref), ENCODING) + 1;\n> username_buf = xmalloc(buffer_len);\n> + memset(username_buf, 0, buffer_len);\n> if (CFStringGetCString(account_ref,\n> username_buf,\n> buffer_len,\n\n...we are getting it by calling CFStringGetCString(). I don't know\nanything about the OS API here, and I don't have a system to test on.\nBut according to the documentation at:\n\n https://developer.apple.com/documentation/corefoundation/1542721-cfstringgetcstring\n\nit should return a NUL-terminated string.\n\nHrm. Just looking at the code, here's a wild hypothesis: the problem\ncould be not that the buffer is not NUL-terminated, but that after the\nNUL it contains junk, and we print that junk. That is, the code looks\nlike this:\n\n /* If we can't get a CString pointer then\n * we need to allocate our own buffer */\n buffer_len = CFStringGetMaximumSizeForEncoding(\n CFStringGetLength(account_ref), ENCODING) + 1;\n username_buf = xmalloc(buffer_len);\n if (CFStringGetCString(account_ref,\n username_buf,\n buffer_len,\n ENCODING)) {\n write_item(\"username\", username_buf, buffer_len - 1);\n }\n\nSo we asked the system for the _maximum_ size that the string could be\n(and added one for the NUL). Then we got the string, and we printed out\nthe _whole_ buffer, not just the string up to the NUL. And your fix\n\"works\" because NULs end up getting ignored on the read side (or at\nleast cause ruby not to complain about bogus utf8).\n\nIf that hypothesis is true, then the fix is more like:\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 6ce22a28ed..1c8310d7fe 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -141,7 +141,7 @@ static void find_username_in_item(CFDictionaryRef item)\n \t\t\t\tusername_buf,\n \t\t\t\tbuffer_len,\n \t\t\t\tENCODING)) {\n-\t\twrite_item(\"username\", username_buf, buffer_len - 1);\n+\t\twrite_item(\"username\", username_buf, strlen(username_buf));\n \t}\n \tfree(username_buf);\n }\n\nBut somebody with a functioning macOS system would need to check whether\nany of what I just said is true. This code comes from 9abe31f5f1\n(osxkeychain: replace deprecated SecKeychain API, 2024-02-17). Adding\nthe author to the CC.\n\n-Peff\n"},{"id":"499688","messageId":"9AA59434-916C-4978-B3A1-33FD70619BFC@boanderson.me","threadId":"61872","inReplyTo":"20240731074228.GC595974@coredump.intra.peff.net","subject":"Re: [patch] credential-osxkeychain: Clear username_buffer before getting the converted C string.","fromName":"Bo Anderson","fromEmail":"mail@boanderson.me","sentAt":"2024-07-31T13:07:32Z","receivedAt":"2024-07-31T13:07:56Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"\n> On 31 Jul 2024, at 08:42, Jeff King wrote:\n> \n> Hrm. Just looking at the code, here's a wild hypothesis: the problem\n> could be not that the buffer is not NUL-terminated, but that after the\n> NUL it contains junk, and we print that junk. That is, the code looks\n> like this:\n> \n> /* If we can't get a CString pointer then\n> * we need to allocate our own buffer */\n> buffer_len = CFStringGetMaximumSizeForEncoding(\n> CFStringGetLength(account_ref), ENCODING) + 1;\n> username_buf = xmalloc(buffer_len);\n> if (CFStringGetCString(account_ref,\n> username_buf,\n> buffer_len,\n> ENCODING)) {\n> write_item(\"username\", username_buf, buffer_len - 1);\n> }\n> \n> So we asked the system for the _maximum_ size that the string could be\n> (and added one for the NUL). Then we got the string, and we printed out\n> the _whole_ buffer, not just the string up to the NUL. And your fix\n> \"works\" because NULs end up getting ignored on the read side (or at\n> least cause ruby not to complain about bogus utf8).\n> \n> If that hypothesis is true, then the fix is more like:\n> \n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index 6ce22a28ed..1c8310d7fe 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -141,7 +141,7 @@ static void find_username_in_item(CFDictionaryRef item)\n> \t\t\t\tusername_buf,\n> \t\t\t\tbuffer_len,\n> \t\t\t\tENCODING)) {\n> -\t\twrite_item(\"username\", username_buf, buffer_len - 1);\n> +\t\twrite_item(\"username\", username_buf, strlen(username_buf));\n> \t}\n> \tfree(username_buf);\n> }\n\nThis is correct.\n\nThe reason I couldn’t reproduce the problem and how few will have noticed up to\nnow is that for most users the CFStringGetCStringPtr call, which correctly uses\nstrlen, does what is necessary and we return early. I don't entirely know the\nprecise criteria where the fallback is used but I imagine it depends on certain\nsystem encodings/locales.\n\nThe patch changing this to strlen looks good to me to apply to master & maint.\n\nBo\n\n"},{"id":"499774","messageId":"20240801082556.GA640360@coredump.intra.peff.net","threadId":"61872","inReplyTo":"9AA59434-916C-4978-B3A1-33FD70619BFC@boanderson.me","subject":"[PATCH] credential/osxkeychain: respect NUL terminator in username","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-01T08:25:56Z","receivedAt":"2024-08-01T08:26:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2024 at 02:07:32PM +0100, Bo Anderson wrote:\n\n> This is correct.\n> \n> The reason I couldn’t reproduce the problem and how few will have noticed up to\n> now is that for most users the CFStringGetCStringPtr call, which correctly uses\n> strlen, does what is necessary and we return early. I don't entirely know the\n> precise criteria where the fallback is used but I imagine it depends on certain\n> system encodings/locales.\n> \n> The patch changing this to strlen looks good to me to apply to master & maint.\n\nThanks. Here it is with a commit message. Hopefully Hong Jiang can\nconfirm that this fixes the problem, and we can added a \"Tested-by\"\ntrailer.\n\n-- >8 --\nSubject: [PATCH] credential/osxkeychain: respect NUL terminator in username\n\nThis patch fixes a case where git-credential-osxkeychain might output\nuninitialized bytes to stdout.\n\nWe need to get the username string from a system API using\nCFStringGetCString(). To do that, we get the max size for the string\nfrom CFStringGetMaximumSizeForEncoding(), allocate a buffer based on\nthat, and then read into it. But then we print the entire buffer to\nstdout, including the trailing NUL and any extra bytes which were not\nneeded. Instead, we should stop at the NUL.\n\nThis code comes from 9abe31f5f1 (osxkeychain: replace deprecated\nSecKeychain API, 2024-02-17). The bug was probably overlooked back then\nbecause this code is only used as a fallback when we can't get the\nstring via CFStringGetCStringPtr(). According to Apple's documentation:\n\n Whether or not this function returns a valid pointer or NULL depends\n on many factors, all of which depend on how the string was created and\n its properties.\n\nSo it's not clear how we could make a test for this, and we'll have to\nrely on manually testing on a system that triggered the bug in the first\nplace.\n\nReported-by: Hong Jiang \nSigned-off-by: Jeff King \n---\nThis is not even compile tested by me! It looks like an obvious enough\nfix, and I wanted to make sure we don't forget about it. But anybody who\ncan reproduce or test would be greatly appreciated.\n\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 6ce22a28ed..1c8310d7fe 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -141,7 +141,7 @@ static void find_username_in_item(CFDictionaryRef item)\n \t\t\t\tusername_buf,\n \t\t\t\tbuffer_len,\n \t\t\t\tENCODING)) {\n-\t\twrite_item(\"username\", username_buf, buffer_len - 1);\n+\t\twrite_item(\"username\", username_buf, strlen(username_buf));\n \t}\n \tfree(username_buf);\n }\n-- \n2.46.0.452.g3bd18f5164\n\n"},{"id":"499826","messageId":"CAEcKSiyf7JPypM93XLFJLjC1T-k9h6kushM7GqTyCBe8rico=g@mail.gmail.com","threadId":"61872","inReplyTo":"20240801082556.GA640360@coredump.intra.peff.net","subject":"Re: [PATCH] credential/osxkeychain: respect NUL terminator in username","fromName":"Hong Jiang","fromEmail":"ilford@gmail.com","sentAt":"2024-08-01T10:57:31Z","receivedAt":"2024-08-01T10:57:48Z","isPatch":true,"sender":{"key":"ilford@gmail.com","avatar":null},"body":"I confirm the patch works on my system.\n\nOn Thu, Aug 1, 2024 at 4:25 PM Jeff King wrote:\n>\n> On Wed, Jul 31, 2024 at 02:07:32PM +0100, Bo Anderson wrote:\n>\n> > This is correct.\n> >\n> > The reason I couldn’t reproduce the problem and how few will have noticed up to\n> > now is that for most users the CFStringGetCStringPtr call, which correctly uses\n> > strlen, does what is necessary and we return early. I don't entirely know the\n> > precise criteria where the fallback is used but I imagine it depends on certain\n> > system encodings/locales.\n> >\n> > The patch changing this to strlen looks good to me to apply to master & maint.\n>\n> Thanks. Here it is with a commit message. Hopefully Hong Jiang can\n> confirm that this fixes the problem, and we can added a \"Tested-by\"\n> trailer.\n>\n> -- >8 --\n> Subject: [PATCH] credential/osxkeychain: respect NUL terminator in username\n>\n> This patch fixes a case where git-credential-osxkeychain might output\n> uninitialized bytes to stdout.\n>\n> We need to get the username string from a system API using\n> CFStringGetCString(). To do that, we get the max size for the string\n> from CFStringGetMaximumSizeForEncoding(), allocate a buffer based on\n> that, and then read into it. But then we print the entire buffer to\n> stdout, including the trailing NUL and any extra bytes which were not\n> needed. Instead, we should stop at the NUL.\n>\n> This code comes from 9abe31f5f1 (osxkeychain: replace deprecated\n> SecKeychain API, 2024-02-17). The bug was probably overlooked back then\n> because this code is only used as a fallback when we can't get the\n> string via CFStringGetCStringPtr(). According to Apple's documentation:\n>\n> Whether or not this function returns a valid pointer or NULL depends\n> on many factors, all of which depend on how the string was created and\n> its properties.\n>\n> So it's not clear how we could make a test for this, and we'll have to\n> rely on manually testing on a system that triggered the bug in the first\n> place.\n>\n> Reported-by: Hong Jiang \n> Signed-off-by: Jeff King \n> ---\n> This is not even compile tested by me! It looks like an obvious enough\n> fix, and I wanted to make sure we don't forget about it. But anybody who\n> can reproduce or test would be greatly appreciated.\n>\n> contrib/credential/osxkeychain/git-credential-osxkeychain.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index 6ce22a28ed..1c8310d7fe 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -141,7 +141,7 @@ static void find_username_in_item(CFDictionaryRef item)\n> username_buf,\n> buffer_len,\n> ENCODING)) {\n> - write_item(\"username\", username_buf, buffer_len - 1);\n> + write_item(\"username\", username_buf, strlen(username_buf));\n> }\n> free(username_buf);\n> }\n> --\n> 2.46.0.452.g3bd18f5164\n>\n"},{"id":"499837","messageId":"20240801111426.GT1159276@coredump.intra.peff.net","threadId":"61872","inReplyTo":"CAEcKSiyf7JPypM93XLFJLjC1T-k9h6kushM7GqTyCBe8rico=g@mail.gmail.com","subject":"Re: [PATCH] credential/osxkeychain: respect NUL terminator in username","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-01T11:14:26Z","receivedAt":"2024-08-01T11:14:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2024 at 06:57:31PM +0800, Hong Jiang wrote:\n\n> I confirm the patch works on my system.\n\nThank you!\n\n-Peff\n"},{"id":"499869","messageId":"xmqq7cd0xmq0.fsf@gitster.g","threadId":"61872","inReplyTo":"CAEcKSiyf7JPypM93XLFJLjC1T-k9h6kushM7GqTyCBe8rico=g@mail.gmail.com","subject":"Re: [PATCH] credential/osxkeychain: respect NUL terminator in username","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-01T15:54:31Z","receivedAt":"2024-08-01T15:54:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hong Jiang writes:\n\n> On Thu, Aug 1, 2024 at 4:25 PM Jeff King wrote:\n> ...\n>> ---\n>> This is not even compile tested by me! It looks like an obvious enough\n>> fix, and I wanted to make sure we don't forget about it. But anybody who\n>> can reproduce or test would be greatly appreciated.\n\n> I confirm the patch works on my system.\n\nThanks, both. Will queue.\n\n"}]}