From: Jeff King <peff@peff.net>
To: M Hickford via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, M Hickford <mirth.hickford@gmail.com>
Subject: Re: [PATCH] credential/osxkeychain: store new attributes
Date: Wed, 14 Feb 2024 23:59:38 -0500 [thread overview]
Message-ID: <20240215045938.GB2821179@coredump.intra.peff.net> (raw)
In-Reply-To: <pull.1663.git.1707860618119.gitgitgadget@gmail.com>
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
prev parent reply other threads:[~2024-02-15 4:59 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-13 21:43 [PATCH] credential/osxkeychain: store new attributes M Hickford via GitGitGadget
2024-02-14 18:25 ` Junio C Hamano
2024-02-14 22:35 ` M Hickford
2024-02-27 20:00 ` M Hickford
2024-02-27 20:13 ` Junio C Hamano
2024-02-15 4:59 ` Jeff King [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240215045938.GB2821179@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=mirth.hickford@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).