On Tue, Feb 13, 2024 at 09:43:38PM +0000, M Hickford via GitGitGadget wrote:
> Is any keen MacOS user interested in building and testing this RFC
> patch? I personally don't have a MacOS machine, so haven't tried
> building it. Fixes are surely necessary. Once it builds, you can test
> the feature with:
>
> GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh
You might also need:
GIT_TEST_CREDENTIAL_HELPER_SETUP="export HOME=$HOME"
according to 34961d30da (contrib: add credential helper for OS X
Keychain, 2011-12-10). IIRC I also ran into problems trying to test over
ssh, as those sessions did not have access to the keychain.
(Sorry, I haven't touched a mac since adding the helper back then, but
maybe those hints will help somebody else).
> static void add_internet_password(void)
> {
> + int len;
> +
This should probably be a size_t to avoid integer overflow for malicious
inputs. I suspect it's hard to get a super-long string into the system.
We do use the dynamic getline(), but stuff like host, user, etc, almost
certainly comes from the user or from a URL that was passed over a
command-line. Maybe oauth_refresh_token() could be long, though?
Anyway, probably better safe than sorry (though see below).
> /* Only store complete credentials */
> if (!protocol || !host || !username || !password)
> return;
>
> + char *secret;
This is a decl-after-statement, which our style forbids (though I am
happy to defer on style issues to anybody who volunteers to maintain
a slice of contrib/, and I don't think we need to worry about pre-c99
compilers here).
> + if (password_expiry_utc && oauth_refresh_token) {
> + len = strlen(password) + strlen(password_expiry_utc) + strlen(oauth_refresh_token) + strlen("\npassword_expiry_utc=\noauth_refresh_token=");
> + secret = xmalloc(len);
> + snprintf(secret, len, len, "%s\npassword_expiry_utc=%s\noauth_refresh_token=%s", password, oauth_refresh_token);
Do you need to add one more byte to "len" for the NUL terminator?
I think there is also a mismatch in your snprintf call, which has three
%s placeholders and only two var-args.
Since we added xmalloc() as a helper, I wonder if we could go just a
little further with (totally untested):
__attribute__((format (printf, 1, 2)))
char *xstrfmt(const char *fmt, ...)
{
va_list ap, cp;
char *ret;
int len;
va_start(ap, fmt);
va_copy(cp, ap);
len = vsnprintf(NULL, 0, fmt, cp);
va_end(cp);
/*
* sadly we must use int for the length, since that's what the
* standard specifies. But good implementations will return a
* negative value if the resulting length would overflow.
*/
if (len < 0)
die("xstrfmt string too long");
ret = xmalloc(len + 1);
vsnprintf(ret, len, fmt, ap);
va_end(ap);
return ret;
}
Then you can just write:
secret = xstrfmt("%s\npassword_expiry_utc=%s\noauth_refresh_token=%s",
password, password_expiry_utc, oauth_refresh_token);
Even across the three instances, I doubt it is saving any lines, but it
is much easier to verify that we sized the buffer correctly and did not
introduce an overflow.
-Peff
