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
