| Message ID | 20240801082556.GA640360@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b201316835bbf2c49c2780f23cfd6146f6b8d1a2 |
| Headers | show |
| Series | credential/osxkeychain: respect NUL terminator in username | expand |
I confirm the patch works on my system. On Thu, Aug 1, 2024 at 4:25 PM Jeff King <peff@peff.net> 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@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 01, 2024 at 06:57:31PM +0800, Hong Jiang wrote:
> I confirm the patch works on my system.
Thank you!
-Peff
Hong Jiang <ilford@gmail.com> writes: > 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. > I confirm the patch works on my system. Thanks, both. Will queue.
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); }