From 9abe31f5f161be4d69118bdfae00103cd6efa510 Mon Sep 17 00:00:00 2001 From: Bo Anderson Date: Sat, 17 Feb 2024 23:34:53 +0000 Subject: osxkeychain: replace deprecated SecKeychain API 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 Signed-off-by: Junio C Hamano --- contrib/credential/osxkeychain/Makefile | 3 +- .../osxkeychain/git-credential-osxkeychain.c | 265 +++++++++++++++------ 2 files changed, 199 insertions(+), 69 deletions(-) diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile index 4b3a08a2ba..238f5f8c36 100644 --- a/contrib/credential/osxkeychain/Makefile +++ b/contrib/credential/osxkeychain/Makefile @@ -8,7 +8,8 @@ CFLAGS = -g -O2 -Wall -include ../../../config.mak git-credential-osxkeychain: git-credential-osxkeychain.o - $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) -Wl,-framework -Wl,Security + $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \ + -framework Security -framework CoreFoundation git-credential-osxkeychain.o: git-credential-osxkeychain.c $(CC) -c $(CFLAGS) $< diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c index 5f2e5f16c8..dc294ae944 100644 --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c @@ -3,14 +3,39 @@ #include #include -static SecProtocolType protocol; -static char *host; -static char *path; -static char *username; -static char *password; -static UInt16 port; - -__attribute__((format (printf, 1, 2))) +#define ENCODING kCFStringEncodingUTF8 +static CFStringRef protocol; /* Stores constant strings - not memory managed */ +static CFStringRef host; +static CFStringRef path; +static CFStringRef username; +static CFDataRef password; +static CFNumberRef port; + +static void clear_credential(void) +{ + if (host) { + CFRelease(host); + host = NULL; + } + if (path) { + CFRelease(path); + path = NULL; + } + if (username) { + CFRelease(username); + username = NULL; + } + if (password) { + CFRelease(password); + password = NULL; + } + if (port) { + CFRelease(port); + port = NULL; + } +} + +__attribute__((format (printf, 1, 2), __noreturn__)) static void die(const char *err, ...) { char msg[4096]; @@ -19,70 +44,135 @@ static void die(const char *err, ...) vsnprintf(msg, sizeof(msg), err, params); fprintf(stderr, "%s\n", msg); va_end(params); + clear_credential(); exit(1); } -static void *xstrdup(const char *s1) +static void *xmalloc(size_t len) { - void *ret = strdup(s1); + void *ret = malloc(len); if (!ret) die("Out of memory"); return ret; } -#define KEYCHAIN_ITEM(x) (x ? strlen(x) : 0), x -#define KEYCHAIN_ARGS \ - NULL, /* default keychain */ \ - KEYCHAIN_ITEM(host), \ - 0, NULL, /* account domain */ \ - KEYCHAIN_ITEM(username), \ - KEYCHAIN_ITEM(path), \ - port, \ - protocol, \ - kSecAuthenticationTypeDefault - -static void write_item(const char *what, const char *buf, int len) +static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...) +{ + va_list args; + const void *key; + CFMutableDictionaryRef result; + + result = CFDictionaryCreateMutable(allocator, + 0, + &kCFTypeDictionaryKeyCallBacks, + &kCFTypeDictionaryValueCallBacks); + + + 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); + + return result; +} + +#define CREATE_SEC_ATTRIBUTES(...) \ + create_dictionary(kCFAllocatorDefault, \ + kSecClass, kSecClassInternetPassword, \ + kSecAttrServer, host, \ + kSecAttrAccount, username, \ + kSecAttrPath, path, \ + kSecAttrPort, port, \ + kSecAttrProtocol, protocol, \ + kSecAttrAuthenticationType, \ + kSecAttrAuthenticationTypeDefault, \ + __VA_ARGS__); + +static void write_item(const char *what, const char *buf, size_t len) { printf("%s=", what); fwrite(buf, 1, len, stdout); putchar('\n'); } -static void find_username_in_item(SecKeychainItemRef item) +static void find_username_in_item(CFDictionaryRef item) { - SecKeychainAttributeList list; - SecKeychainAttribute attr; + CFStringRef account_ref; + char *username_buf; + CFIndex buffer_len; - list.count = 1; - list.attr = &attr; - attr.tag = kSecAccountItemAttr; + account_ref = CFDictionaryGetValue(item, kSecAttrAccount); + if (!account_ref) + { + write_item("username", "", 0); + return; + } - if (SecKeychainItemCopyContent(item, NULL, &list, NULL, NULL)) + username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING); + if (username_buf) + { + write_item("username", username_buf, strlen(username_buf)); return; + } - write_item("username", attr.data, attr.length); - SecKeychainItemFreeContent(&list, NULL); + /* If we can't get a CString pointer then + * we need to allocate our own buffer */ + 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); } -static void find_internet_password(void) +static OSStatus find_internet_password(void) { - void *buf; - UInt32 len; - SecKeychainItemRef item; + CFDictionaryRef attrs; + CFDictionaryRef item; + CFDataRef data; + OSStatus result; - if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item)) - return; + attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne, + kSecReturnAttributes, kCFBooleanTrue, + kSecReturnData, kCFBooleanTrue, + NULL); + result = SecItemCopyMatching(attrs, (CFTypeRef *)&item); + if (result) { + goto out; + } - write_item("password", buf, len); + data = CFDictionaryGetValue(item, kSecValueData); + + write_item("password", + (const char *)CFDataGetBytePtr(data), + CFDataGetLength(data)); if (!username) find_username_in_item(item); - SecKeychainItemFreeContent(NULL, buf); + CFRelease(item); + +out: + CFRelease(attrs); + + /* We consider not found to not be an error */ + if (result == errSecItemNotFound) + result = errSecSuccess; + + return result; } -static void delete_internet_password(void) +static OSStatus delete_internet_password(void) { - SecKeychainItemRef item; + CFDictionaryRef attrs; + OSStatus result; /* * Require at least a protocol and host for removal, which is what git @@ -90,25 +180,42 @@ static void delete_internet_password(void) * Keychain manager. */ if (!protocol || !host) - return; + return -1; - if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, 0, NULL, &item)) - return; + attrs = CREATE_SEC_ATTRIBUTES(NULL); + result = SecItemDelete(attrs); + CFRelease(attrs); + + /* We consider not found to not be an error */ + if (result == errSecItemNotFound) + result = errSecSuccess; - SecKeychainItemDelete(item); + return result; } -static void add_internet_password(void) +static OSStatus add_internet_password(void) { + CFDictionaryRef attrs; + OSStatus result; + /* Only store complete credentials */ if (!protocol || !host || !username || !password) - return; + return -1; - if (SecKeychainAddInternetPassword( - KEYCHAIN_ARGS, - KEYCHAIN_ITEM(password), - NULL)) - return; + 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; } static void read_credential(void) @@ -131,36 +238,52 @@ static void read_credential(void) if (!strcmp(buf, "protocol")) { if (!strcmp(v, "imap")) - protocol = kSecProtocolTypeIMAP; + protocol = kSecAttrProtocolIMAP; else if (!strcmp(v, "imaps")) - protocol = kSecProtocolTypeIMAPS; + protocol = kSecAttrProtocolIMAPS; else if (!strcmp(v, "ftp")) - protocol = kSecProtocolTypeFTP; + protocol = kSecAttrProtocolFTP; else if (!strcmp(v, "ftps")) - protocol = kSecProtocolTypeFTPS; + protocol = kSecAttrProtocolFTPS; else if (!strcmp(v, "https")) - protocol = kSecProtocolTypeHTTPS; + protocol = kSecAttrProtocolHTTPS; else if (!strcmp(v, "http")) - protocol = kSecProtocolTypeHTTP; + protocol = kSecAttrProtocolHTTP; else if (!strcmp(v, "smtp")) - protocol = kSecProtocolTypeSMTP; - else /* we don't yet handle other protocols */ + protocol = kSecAttrProtocolSMTP; + else { + /* we don't yet handle other protocols */ + clear_credential(); exit(0); + } } else if (!strcmp(buf, "host")) { char *colon = strchr(v, ':'); if (colon) { + UInt16 port_i; *colon++ = '\0'; - port = atoi(colon); + port_i = atoi(colon); + port = CFNumberCreate(kCFAllocatorDefault, + kCFNumberShortType, + &port_i); } - host = xstrdup(v); + host = CFStringCreateWithCString(kCFAllocatorDefault, + v, + ENCODING); } else if (!strcmp(buf, "path")) - path = xstrdup(v); + path = CFStringCreateWithCString(kCFAllocatorDefault, + v, + ENCODING); else if (!strcmp(buf, "username")) - username = xstrdup(v); + username = CFStringCreateWithCString( + kCFAllocatorDefault, + v, + ENCODING); else if (!strcmp(buf, "password")) - password = xstrdup(v); + password = CFDataCreate(kCFAllocatorDefault, + (UInt8 *)v, + strlen(v)); /* * Ignore other lines; we don't know what they mean, but * this future-proofs us when later versions of git do @@ -173,6 +296,7 @@ static void read_credential(void) int main(int argc, const char **argv) { + OSStatus result = 0; const char *usage = "usage: git credential-osxkeychain "; @@ -182,12 +306,17 @@ int main(int argc, const char **argv) read_credential(); if (!strcmp(argv[1], "get")) - find_internet_password(); + result = find_internet_password(); else if (!strcmp(argv[1], "store")) - add_internet_password(); + result = add_internet_password(); else if (!strcmp(argv[1], "erase")) - delete_internet_password(); + result = delete_internet_password(); /* otherwise, ignore unknown action */ + if (result) + die("failed to %s: %d", argv[1], (int)result); + + clear_credential(); + return 0; } -- cgit 1.2.3-korg From 9032bcad82f45a403e4a8de86e1fcb4bfd1ab282 Mon Sep 17 00:00:00 2001 From: Bo Anderson Date: Sat, 17 Feb 2024 23:34:54 +0000 Subject: osxkeychain: erase all matching credentials Other credential managers erased all matching credentials, as indicated by a test case that osxkeychain failed: 15 - helper (osxkeychain) erases all matching credentials Signed-off-by: Bo Anderson Signed-off-by: Junio C Hamano --- contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c index dc294ae944..e9cee3aed4 100644 --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c @@ -182,7 +182,8 @@ static OSStatus delete_internet_password(void) if (!protocol || !host) return -1; - attrs = CREATE_SEC_ATTRIBUTES(NULL); + attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitAll, + NULL); result = SecItemDelete(attrs); CFRelease(attrs); -- cgit 1.2.3-korg From e3cef40db89f5a7c91f4e9d6c4959ca1e41f4647 Mon Sep 17 00:00:00 2001 From: Bo Anderson Date: Sat, 17 Feb 2024 23:34:55 +0000 Subject: osxkeychain: erase matching passwords only Other credential helpers support deleting credentials that match a specified password. See 7144dee3ec (credential/libsecret: erase matching creds only, 2023-07-26) and cb626f8e5c (credential/wincred: erase matching creds only, 2023-07-26). Support this in osxkeychain too by extracting, decrypting and comparing the stored password before deleting. Fixes the following test failure with osxkeychain: 11 - helper (osxkeychain) does not erase a password distinct from input Signed-off-by: Bo Anderson Signed-off-by: Junio C Hamano --- .../osxkeychain/git-credential-osxkeychain.c | 56 +++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c index e9cee3aed4..9e74279633 100644 --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c @@ -169,9 +169,55 @@ out: return result; } +static OSStatus delete_ref(const void *itemRef) +{ + CFArrayRef item_ref_list; + CFDictionaryRef delete_query; + OSStatus result; + + item_ref_list = CFArrayCreate(kCFAllocatorDefault, + &itemRef, + 1, + &kCFTypeArrayCallBacks); + delete_query = create_dictionary(kCFAllocatorDefault, + kSecClass, kSecClassInternetPassword, + kSecMatchItemList, item_ref_list, + NULL); + + if (password) { + /* We only want to delete items with a matching password */ + CFIndex capacity; + CFMutableDictionaryRef query; + CFDataRef data; + + capacity = CFDictionaryGetCount(delete_query) + 1; + query = CFDictionaryCreateMutableCopy(kCFAllocatorDefault, + capacity, + delete_query); + CFDictionarySetValue(query, kSecReturnData, kCFBooleanTrue); + result = SecItemCopyMatching(query, (CFTypeRef *)&data); + if (!result) { + if (CFEqual(data, password)) + result = SecItemDelete(delete_query); + + CFRelease(data); + } + + CFRelease(query); + } else { + result = SecItemDelete(delete_query); + } + + CFRelease(delete_query); + CFRelease(item_ref_list); + + return result; +} + static OSStatus delete_internet_password(void) { CFDictionaryRef attrs; + CFArrayRef refs; OSStatus result; /* @@ -183,10 +229,18 @@ static OSStatus delete_internet_password(void) return -1; attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitAll, + kSecReturnRef, kCFBooleanTrue, NULL); - result = SecItemDelete(attrs); + result = SecItemCopyMatching(attrs, (CFTypeRef *)&refs); CFRelease(attrs); + if (!result) { + for (CFIndex i = 0; !result && i < CFArrayGetCount(refs); i++) + result = delete_ref(CFArrayGetValueAtIndex(refs, i)); + + CFRelease(refs); + } + /* We consider not found to not be an error */ if (result == errSecItemNotFound) result = errSecSuccess; -- cgit 1.2.3-korg From d5b35bba86e6fdf0484ea71bf5b8ef1167f14015 Mon Sep 17 00:00:00 2001 From: Bo Anderson Date: Sat, 17 Feb 2024 23:34:56 +0000 Subject: osxkeychain: store new attributes d208bfdfef (credential: new attribute password_expiry_utc, 2023-02-18) and a5c76569e7 (credential: new attribute oauth_refresh_token, 2023-04-21) introduced new credential attributes but support was missing from git-credential-osxkeychain. Support these attributes by appending the data to the password in the keychain, separated by line breaks. Line breaks cannot appear in a git credential password so it is an appropriate separator. Fixes the remaining test failures with osxkeychain: 18 - helper (osxkeychain) gets password_expiry_utc 19 - helper (osxkeychain) overwrites when password_expiry_utc changes 21 - helper (osxkeychain) gets oauth_refresh_token Signed-off-by: Bo Anderson Signed-off-by: Junio C Hamano --- .../osxkeychain/git-credential-osxkeychain.c | 68 ++++++++++++++++++++-- 1 file changed, 62 insertions(+), 6 deletions(-) diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c index 9e74279633..6a40917b1e 100644 --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c @@ -6,10 +6,12 @@ #define ENCODING kCFStringEncodingUTF8 static CFStringRef protocol; /* Stores constant strings - not memory managed */ static CFStringRef host; +static CFNumberRef port; static CFStringRef path; static CFStringRef username; static CFDataRef password; -static CFNumberRef port; +static CFDataRef password_expiry_utc; +static CFDataRef oauth_refresh_token; static void clear_credential(void) { @@ -17,6 +19,10 @@ static void clear_credential(void) CFRelease(host); host = NULL; } + if (port) { + CFRelease(port); + port = NULL; + } if (path) { CFRelease(path); path = NULL; @@ -29,12 +35,18 @@ static void clear_credential(void) CFRelease(password); password = NULL; } - if (port) { - CFRelease(port); - port = NULL; + if (password_expiry_utc) { + CFRelease(password_expiry_utc); + password_expiry_utc = NULL; + } + if (oauth_refresh_token) { + CFRelease(oauth_refresh_token); + oauth_refresh_token = NULL; } } +#define STRING_WITH_LENGTH(s) s, sizeof(s) - 1 + __attribute__((format (printf, 1, 2), __noreturn__)) static void die(const char *err, ...) { @@ -197,9 +209,27 @@ static OSStatus delete_ref(const void *itemRef) CFDictionarySetValue(query, kSecReturnData, kCFBooleanTrue); result = SecItemCopyMatching(query, (CFTypeRef *)&data); if (!result) { - if (CFEqual(data, password)) + CFDataRef kc_password; + const UInt8 *raw_data; + const UInt8 *line; + + /* Don't match appended metadata */ + raw_data = CFDataGetBytePtr(data); + line = memchr(raw_data, '\n', CFDataGetLength(data)); + if (line) + kc_password = CFDataCreateWithBytesNoCopy( + kCFAllocatorDefault, + raw_data, + line - raw_data, + kCFAllocatorNull); + else + kc_password = data; + + if (CFEqual(kc_password, password)) result = SecItemDelete(delete_query); + if (line) + CFRelease(kc_password); CFRelease(data); } @@ -250,6 +280,7 @@ static OSStatus delete_internet_password(void) static OSStatus add_internet_password(void) { + CFMutableDataRef data; CFDictionaryRef attrs; OSStatus result; @@ -257,7 +288,23 @@ static OSStatus add_internet_password(void) if (!protocol || !host || !username || !password) return -1; - attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password, + data = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password); + if (password_expiry_utc) { + CFDataAppendBytes(data, + (const UInt8 *)STRING_WITH_LENGTH("\npassword_expiry_utc=")); + CFDataAppendBytes(data, + CFDataGetBytePtr(password_expiry_utc), + CFDataGetLength(password_expiry_utc)); + } + if (oauth_refresh_token) { + CFDataAppendBytes(data, + (const UInt8 *)STRING_WITH_LENGTH("\noauth_refresh_token=")); + CFDataAppendBytes(data, + CFDataGetBytePtr(oauth_refresh_token), + CFDataGetLength(oauth_refresh_token)); + } + + attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, data, NULL); result = SecItemAdd(attrs, NULL); @@ -268,6 +315,7 @@ static OSStatus add_internet_password(void) CFRelease(query); } + CFRelease(data); CFRelease(attrs); return result; @@ -339,6 +387,14 @@ static void read_credential(void) password = CFDataCreate(kCFAllocatorDefault, (UInt8 *)v, strlen(v)); + else if (!strcmp(buf, "password_expiry_utc")) + password_expiry_utc = CFDataCreate(kCFAllocatorDefault, + (UInt8 *)v, + strlen(v)); + else if (!strcmp(buf, "oauth_refresh_token")) + oauth_refresh_token = CFDataCreate(kCFAllocatorDefault, + (UInt8 *)v, + strlen(v)); /* * Ignore other lines; we don't know what they mean, but * this future-proofs us when later versions of git do -- cgit 1.2.3-korg