Re: [PATCH] credential/osxkeychain: store new attributes

From: Jeff King <peff@peff.net>
Date: Feb 15, 2024, 04:59 UTC
Message-ID: <20240215045938.GB2821179@coredump.intra.peff.net>
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:

Show 6 quoted lines

>     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).

Show 5 quoted lines

>  	/* 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