Re: [PATCH 1/4] osxkeychain: replace deprecated SecKeychain API

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 18, 2024, 06:08 UTC
Message-ID: <CAPig+cR_XYjArdYpU-qm+Wont=yEEXe5hANRyz+YRdhv=UZf=Q@mail.gmail.com>
In-Reply-To: <f7031316a043b36fac10ecf784d2294894967e7b.1708212896.git.gitgitgadget@gmail.com>

On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 19 quoted lines

> The SecKeychain API was deprecated in macOS 10.10, nearly 10 years ago.
> The replacement SecItem API however is available as far back as macOS
> 10.6.
>
> While supporting older macOS was perhaps prevously a concern,
> git-credential-osxkeychain already requires a minimum of macOS 10.7
> since 5747c8072b (contrib/credential: avoid fixed-size buffer in
> osxkeychain, 2023-05-01) so using the newer API should not regress the
> range of macOS versions supported.
>
> Adapting to use the newer SecItem API also happens to fix two test
> failures in osxkeychain:
>
>     8 - helper (osxkeychain) overwrites on store
>     9 - helper (osxkeychain) can forget host
>
> The new API is compatible with credentials saved with the older API.
>
> Signed-off-by: Bo Anderson <mail@boanderson.me>

I haven't studied the SecItem API, so I can't comment on the meat of the patch, but I can make a few generic observations...

Show 18 quoted lines

> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -3,14 +3,39 @@
> -__attribute__((format (printf, 1, 2)))
> +#define ENCODING kCFStringEncodingUTF8
> +static CFStringRef protocol; /* Stores constant strings - not memory managed */
> +static CFStringRef host;
> [...]
> +
> +static void clear_credential(void)
> +{
> +       if (host) {
> +               CFRelease(host);
> +               host = NULL;
> +       }
> +       [...]
> +}
> +
> +__attribute__((format (printf, 1, 2), __noreturn__))

The addition of `__noreturn__` to the `__attribute__` seems unrelated to the stated purpose of this patch. As such, it typically would be placed in its own patch. If it really is too minor for a separate patch, mentioning it in the commit message as a "While at it..." would be helpful.

Show 13 quoted lines

> @@ -19,70 +44,135 @@ static void die(const char *err, ...)
> +static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
> +{
> +       va_list args;
> +       const void *key;
> +       CFMutableDictionaryRef result;
> +
> +       result = CFDictionaryCreateMutable(allocator,
> +                                          0,
> +                                          &kCFTypeDictionaryKeyCallBacks,
> +                                          &kCFTypeDictionaryValueCallBacks);
> +
> +

Style: one blank line is preferred over two

Show 8 quoted lines

> +       va_start(args, allocator);
> +       while ((key = va_arg(args, const void *)) != NULL) {
> +               const void *value;
> +               value = va_arg(args, const void *);
> +               if (value)
> +                       CFDictionarySetValue(result, key, value);
> +       }
> +       va_end(args);

A couple related comments...

If va_arg() ever returns NULL for `value`, the next iteration of the loop will call va_arg() again, but calling va_arg() again after it has already returned NULL is likely undefined behavior. At minimum, I would have expected this to be written as:

    while (...) {
        ...
        if (!value)
            break;
        CFDictionarySetValue(...);
    }

However, isn't it a programmer error if va_arg() returns NULL for `value`? If so, I'd think we'd want to scream loudly about that rather than silently ignoring it. So, perhaps something like this:

    while (...) {
        ...
        if (!value) {
            fprintf(stderr, "BUG: ...");
            abort();
        }
        CFDictionarySetValue(...);
   }

Or, perhaps just call the existing die() function in this file with a suitable "BUG ..." message.

Show 12 quoted lines

> +static void find_username_in_item(CFDictionaryRef item)
>  {
> +       CFStringRef account_ref;
> +       char *username_buf;
> +       CFIndex buffer_len;
>
> +       account_ref = CFDictionaryGetValue(item, kSecAttrAccount);
> +       if (!account_ref)
> +       {
> +               write_item("username", "", 0);
> +               return;
> +       }

Style: opening brace sticks to the `if` line:

    if !(account_ref) {
        ...
    }

Same comment applies to the `if` below.

Show 6 quoted lines

> +       username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
> +       if (username_buf)
> +       {
> +               write_item("username", username_buf, strlen(username_buf));
>                 return;
> +       }

According to the documentation for CFStringGetCStringPtr(), the returned C-string is not newly-allocated, so the caller does not have to free it. Therefore, can `username_buf` be declared `const char *` rather than `char *` to make it clear to readers that nothing is being leaked here? Same comment about the `(char *)` cast.

> +       /* If we can't get a CString pointer then
> +        * we need to allocate our own buffer */

Style:

    /*
     * Multi-line comments
     * are formatted like this.
     */

Show 10 quoted lines

> +       buffer_len = CFStringGetMaximumSizeForEncoding(
> +                       CFStringGetLength(account_ref), ENCODING) + 1;
> +       username_buf = xmalloc(buffer_len);
> +       if (CFStringGetCString(account_ref,
> +                               username_buf,
> +                               buffer_len,
> +                               ENCODING)) {
> +               write_item("username", username_buf, buffer_len - 1);
> +       }
> +       free(username_buf);

Okay, this explains why `username_buf` is declared `char *` rather than `const char *`. Typically, when we have a situation in which a value may or may not need freeing, we use a `to_free` variable like this:

    const char *username_buf;
    char *to_free = NULL;
    ...
    username_buf = (const char *)CFStringGetCStringPtr(...);
    if (username_buf) {
        ...
        return;
    }
    ...
    username_buf = to_free = xmalloc(buffer_len);
    if (CFStringGetCString(...))
        ...
    free(to_free);

But that may be overkill for this simple case, and what you have here may be "good enough" for anyone already familiar with the API and who knows that the `return` after CFStringGetCStringPtr() isn't leaking.

Show 13 quoted lines

> +static OSStatus find_internet_password(void)
>  {
> +       CFDictionaryRef attrs;
> +       [...]
>
> +       attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne,
> +                                     kSecReturnAttributes, kCFBooleanTrue,
> +                                     kSecReturnData, kCFBooleanTrue,
> +                                     NULL);
> +       result = SecItemCopyMatching(attrs, (CFTypeRef *)&item);
> +       if (result) {
> +               goto out;
> +       }

We omit braces when the body is a single statement:

    if (result)
        goto out;

(Same comment applies to other code in this patch.)

Show 5 quoted lines

> +       data = CFDictionaryGetValue(item, kSecValueData);
> +       [...]
> +
> +out:
> +       CFRelease(attrs);

Good, `attrs` is released in all cases.

Show 15 quoted lines

> +static OSStatus add_internet_password(void)
>  {
> +       [...]
> +       attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,
> +                                     NULL);
> +       result = SecItemAdd(attrs, NULL);
> +       if (result == errSecDuplicateItem) {
> +               CFDictionaryRef query;
> +               query = CREATE_SEC_ATTRIBUTES(NULL);
> +               result = SecItemUpdate(query, attrs);
> +               CFRelease(query);
> +       }
> +       CFRelease(attrs);
> +       return result;
>  }

Good, `attrs` and `query` are released in all cases.