From b0d7580f9bf27d787c36bb9e04d5756413f655f7 Mon Sep 17 00:00:00 2001 From: Tobias Hagemann Date: Tue, 20 Oct 2026 14:17:56 +0200 Subject: [PATCH 1/4] Report keychain load errors instead of treating them as missing passwords --- CHANGELOG.md | 6 +- ...omator_macos_keychain_MacKeychain_Native.h | 4 +- .../macos/keychain/MacKeychain.java | 63 ++++----- .../keychain/MacSystemKeychainAccess.java | 2 +- .../macos/keychain/TouchIdKeychainAccess.java | 2 +- .../Integrations.xcodeproj/project.pbxproj | 4 +- ...omator_macos_keychain_MacKeychain_Native.m | 126 +++++++++++------- ..._macos_uiappearance_AppAppearance_Native.m | 8 +- .../macos/keychain/MacKeychainTest.java | 32 ++++- .../keychain/MacSystemKeychainAccessTest.java | 16 ++- 10 files changed, 153 insertions(+), 110 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2a8e47..0a16fc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,12 @@ The changelog starts with version 1.4.1. Changes to prior versions can be found on the [Github release page](https://github.com/cryptomator/integrations-mac/releases). ## [Unreleased](https://github.com/cryptomator/integrations-mac/compare/1.5.1...HEAD) +### Changed +* Loading a stored password reports errors, e.g. cancelled authentication, instead of reporting no stored password +* Require macOS 11 -No changes yet. +### Fixed +* Possible out-of-bounds read when converting keychain service and account names ## [1.5.1](https://github.com/cryptomator/integrations-mac/releases/tag/1.5.1) - 2026-10-06 diff --git a/src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h b/src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h index 177ca4e..3d82cd8 100644 --- a/src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h +++ b/src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h @@ -18,9 +18,9 @@ JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Nati /* * Class: org_cryptomator_macos_keychain_MacKeychain_Native * Method: loadPassword - * Signature: ([B[B)[B + * Signature: ([B[B)Lorg/cryptomator/macos/keychain/MacKeychain/LoadResult; */ -JNIEXPORT jbyteArray JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_loadPassword +JNIEXPORT jobject JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_loadPassword (JNIEnv *, jobject, jbyteArray, jbyteArray); /* diff --git a/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java b/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java index ce9a63f..f110214 100644 --- a/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java +++ b/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java @@ -40,49 +40,28 @@ public void storePassword(String serviceName, String account, CharSequence passw * * @param serviceName Service name * @param account Unique account identifier - * @return password or null if no such keychain entry could be loaded from the keychain. - * @see SecKeychainFindGenericPassword + * @return password or null if no such keychain entry exists. + * @throws KeychainAccessException If loading fails for any reason other than a missing entry, e.g. because authentication was cancelled + * @see SecItemCopyMatching */ - public char[] loadPassword(String serviceName, String account) { - byte[] pwBytes = Native.INSTANCE.loadPassword(serviceName.getBytes(UTF_8), account.getBytes(UTF_8)); - if (pwBytes == null) { - // this if-statement is a workaround for https://github.com/cryptomator/integrations-mac/issues/13: - if ("Cryptomator".equals(serviceName) && tryMigratePassword(account)) { - // on success: retry - return loadPassword(serviceName, account); - } - return null; - } else { - CharBuffer pwBuf = UTF_8.decode(ByteBuffer.wrap(pwBytes)); - char[] pw = new char[pwBuf.remaining()]; - pwBuf.get(pw); - Arrays.fill(pwBytes, (byte) 0x00); - Arrays.fill(pwBuf.array(), (char) 0x00); - return pw; - } + public char[] loadPassword(String serviceName, String account) throws KeychainAccessException { + return toPassword(Native.INSTANCE.loadPassword(serviceName.getBytes(UTF_8), account.getBytes(UTF_8))); } - /** - * Fix for Issue 13. - * Attempts to find the account with the old hard-coded service name 'Cryptomator\0' - * - * @param account Unique account identifier - * @return true on success - */ - private boolean tryMigratePassword(String account) { - byte[] oldServiceName = {'C', 'r', 'y', 'p', 't', 'o', 'm', 'a', 't', 'o', 'r', '\0'}; - byte[] newServiceName = {'C', 'r', 'y', 'p', 't', 'o', 'm', 'a', 't', 'o', 'r'}; - byte[] pwBytes = Native.INSTANCE.loadPassword(oldServiceName, account.getBytes(UTF_8)); - if (pwBytes == null) { - return false; + // visible for testing + static char[] toPassword(LoadResult result) throws KeychainAccessException { + if (result.status() == OSSTATUS_NOT_FOUND) { + return null; + } else if (result.status() != OSSTATUS_SUCCESS || result.password() == null) { + throw new KeychainAccessException("Failed to load password. Error code " + result.status()); } - int errorCode = Native.INSTANCE.storePassword(newServiceName, account.getBytes(UTF_8), pwBytes, false); + byte[] pwBytes = result.password(); + CharBuffer pwBuf = UTF_8.decode(ByteBuffer.wrap(pwBytes)); + char[] pw = new char[pwBuf.remaining()]; + pwBuf.get(pw); Arrays.fill(pwBytes, (byte) 0x00); - if (errorCode != OSSTATUS_SUCCESS) { - return false; - } - Native.INSTANCE.deletePassword(oldServiceName, account.getBytes(UTF_8)); - return true; + Arrays.fill(pwBuf.array(), (char) 0x00); + return pw; } /** @@ -91,7 +70,7 @@ private boolean tryMigratePassword(String account) { * @param serviceName Service name * @param account Unique account identifier * @return true if the passwords has been deleted, false if no entry for the given key exists. - * @see SecKeychainItemDelete + * @see SecItemDelete */ public boolean deletePassword(String serviceName, String account) throws KeychainAccessException { int errorCode = Native.INSTANCE.deletePassword(serviceName.getBytes(UTF_8), account.getBytes(UTF_8)); @@ -113,6 +92,10 @@ public boolean isTouchIDavailable() { return Native.INSTANCE.isTouchIDavailable(); } + // constructed via JNI by class name and constructor signature, so renaming or reordering its components breaks the native loadPassword + record LoadResult(int status, byte[] password) { + } + // initialization-on-demand pattern, as loading the .dylib is an expensive operation private static class Native { static final Native INSTANCE = new Native(); @@ -123,7 +106,7 @@ private Native() { public native int storePassword(byte[] service, byte[] account, byte[] value, boolean requireOsAuthentication); - public native byte[] loadPassword(byte[] service, byte[] account); + public native LoadResult loadPassword(byte[] service, byte[] account); public native int deletePassword(byte[] service, byte[] account); diff --git a/src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java b/src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java index a1926aa..85fa7e3 100644 --- a/src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java +++ b/src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java @@ -36,7 +36,7 @@ public void storePassphrase(String key, String displayName, CharSequence passphr } @Override - public char[] loadPassphrase(String key) { + public char[] loadPassphrase(String key) throws KeychainAccessException { return keychain.loadPassword(SERVICE_NAME, key); } diff --git a/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java b/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java index 74c701f..ffe0313 100644 --- a/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java +++ b/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java @@ -37,7 +37,7 @@ public void storePassphrase(String key, String displayName, CharSequence passphr } @Override - public char[] loadPassphrase(String key) { + public char[] loadPassphrase(String key) throws KeychainAccessException { return keychain.loadPassword(SERVICE_NAME, key); } diff --git a/src/main/native/Integrations.xcodeproj/project.pbxproj b/src/main/native/Integrations.xcodeproj/project.pbxproj index a1c46cb..a114563 100644 --- a/src/main/native/Integrations.xcodeproj/project.pbxproj +++ b/src/main/native/Integrations.xcodeproj/project.pbxproj @@ -207,7 +207,7 @@ GCC_WARN_UNUSED_VARIABLE = YES; HEADER_SEARCH_PATHS = "$(JAVA_HOME)/include/**"; JAVA_HOME = "$(HOME)/.sdkman/candidates/java/current"; - MACOSX_DEPLOYMENT_TARGET = 10.13; + MACOSX_DEPLOYMENT_TARGET = 11.0; MTL_ENABLE_DEBUG_INFO = YES; ONLY_ACTIVE_ARCH = YES; SDKROOT = macosx; @@ -260,7 +260,7 @@ GCC_WARN_UNUSED_VARIABLE = YES; HEADER_SEARCH_PATHS = "$(JAVA_HOME)/include/**"; JAVA_HOME = "$(HOME)/.sdkman/candidates/java/current"; - MACOSX_DEPLOYMENT_TARGET = 10.13; + MACOSX_DEPLOYMENT_TARGET = 11.0; MTL_ENABLE_DEBUG_INFO = NO; SDKROOT = macosx; USER_HEADER_SEARCH_PATHS = ../headers; diff --git a/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m b/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m index 2741bc7..ddd5962 100644 --- a/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m +++ b/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m @@ -20,18 +20,40 @@ return sharedContext; } -JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key, jbyteArray password, jboolean requireOsAuthentication) { - jbyte *serviceStr = (*env)->GetByteArrayElements(env, service, NULL); - jbyte *keyStr = (*env)->GetByteArrayElements(env, key, NULL); - jbyte *pwStr = (*env)->GetByteArrayElements(env, password, NULL); - jsize length = (*env)->GetArrayLength(env, password); +// returns nil without further JNI calls, if a previous call left an exception pending +static NSString* stringFromByteArray(JNIEnv *env, jbyteArray array) { + if ((*env)->ExceptionCheck(env)) { + return nil; + } + jbyte *bytes = (*env)->GetByteArrayElements(env, array, NULL); + if (bytes == NULL) { + return nil; + } + NSString *string = [[NSString alloc] initWithBytes:bytes length:(*env)->GetArrayLength(env, array) encoding:NSUTF8StringEncoding]; + (*env)->ReleaseByteArrayElements(env, array, bytes, JNI_ABORT); + return string; +} - NSString *serviceString = [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding]; - NSString *keyString = [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding]; +// Items requiring user presence live in the data protection keychain, all others in the file-based keychain. Loading targets one of them explicitly, +// because a query spanning both reports a failed authentication in the data protection keychain as errSecItemNotFound. +static NSDictionary* itemQuery(NSString *service, NSString *account, BOOL dataProtectionKeychain) { + return @{ + (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, + (__bridge id)kSecAttrService: service, + (__bridge id)kSecAttrAccount: account, + (__bridge id)kSecUseDataProtectionKeychain: @(dataProtectionKeychain) + }; +} + +JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key, jbyteArray password, jboolean requireOsAuthentication) { + NSString *serviceString = stringFromByteArray(env, service); + NSString *keyString = stringFromByteArray(env, key); + jbyte *pwStr = (*env)->ExceptionCheck(env) ? NULL : (*env)->GetByteArrayElements(env, password, NULL); OSStatus status = errSecParam; - if (serviceString && keyString) { // guard both NSStrings not to be NIL + if (serviceString && keyString && pwStr) { + jsize length = (*env)->GetArrayLength(env, password); // find existing: NSMutableDictionary *query = [@{ (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, @@ -69,67 +91,73 @@ JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Nati NSLog(@"Error storing item in keychain. Status code: %d", (int)status); } } else { - NSLog(@"Invalid UTF-8 input: service or key string conversion failed"); + NSLog(@"Invalid input: service or key string conversion failed or password unavailable"); } - (*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT); - (*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT); - (*env)->ReleaseByteArrayElements(env, password, pwStr, JNI_ABORT); + if (pwStr) { + (*env)->ReleaseByteArrayElements(env, password, pwStr, JNI_ABORT); + } return status; } -JNIEXPORT jbyteArray JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_loadPassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key) { - jbyte *serviceStr = (*env)->GetByteArrayElements(env, service, NULL); - jbyte *keyStr = (*env)->GetByteArrayElements(env, key, NULL); - - NSString *serviceString = [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding]; - NSString *keyString = [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding]; +JNIEXPORT jobject JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_loadPassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key) { + NSString *serviceString = stringFromByteArray(env, service); + NSString *keyString = stringFromByteArray(env, key); - jbyteArray password = NULL; + OSStatus status = errSecParam; + NSData *passwordData = nil; if (serviceString && keyString) { // guard both NSStrings not to be NIL - // find existing: - LAContext *context = getSharedLAContext(); - NSDictionary *query = @{ - (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, - (__bridge id)kSecAttrService: serviceString, - (__bridge id)kSecAttrAccount: keyString, - (__bridge id)kSecReturnAttributes: @YES, - (__bridge id)kSecReturnData: @YES, - (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne, - (__bridge id)kSecUseAuthenticationContext: context - }; - CFDictionaryRef result = NULL; - OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); + // find existing, preferring the protected item: + NSMutableDictionary *query = [itemQuery(serviceString, keyString, YES) mutableCopy]; + query[(__bridge id)kSecReturnData] = @YES; + query[(__bridge id)kSecMatchLimit] = (__bridge id)kSecMatchLimitOne; + query[(__bridge id)kSecUseAuthenticationContext] = getSharedLAContext(); + CFTypeRef result = NULL; + status = SecItemCopyMatching((__bridge CFDictionaryRef)query, &result); + if (status == errSecItemNotFound || status == errSecMissingEntitlement) { + query[(__bridge id)kSecUseDataProtectionKeychain] = @NO; + status = SecItemCopyMatching((__bridge CFDictionaryRef)query, &result); + } if (status == errSecSuccess && result != NULL) { - // retrieve password: - NSDictionary *attributes = (__bridge_transfer NSDictionary *)result; - NSData *passwordData = attributes[(__bridge id)kSecValueData]; - password = (*env)->NewByteArray(env, (int)passwordData.length); - (*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes); + passwordData = (__bridge_transfer NSData *)result; } else if (status != errSecItemNotFound) { NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status); } } else { - NSLog(@"Invalid UTF-8 input: service or key string conversion failed"); + NSLog(@"Invalid input: service or key string conversion failed"); } - (*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT); - (*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT); - return password; + if ((*env)->ExceptionCheck(env)) { + return NULL; + } + jbyteArray password = NULL; + if (passwordData) { + password = (*env)->NewByteArray(env, (int)passwordData.length); + if (password == NULL) { + return NULL; + } + (*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes); + } + jclass resultClass = (*env)->FindClass(env, "org/cryptomator/macos/keychain/MacKeychain$LoadResult"); + if (resultClass == NULL) { + return NULL; + } + jmethodID resultConstructor = (*env)->GetMethodID(env, resultClass, "", "(I[B)V"); + if (resultConstructor == NULL) { + return NULL; + } + return (*env)->NewObject(env, resultClass, resultConstructor, (jint)status, password); } JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_deletePassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key) { - jbyte *serviceStr = (*env)->GetByteArrayElements(env, service, NULL); - jbyte *keyStr = (*env)->GetByteArrayElements(env, key, NULL); - - NSString *serviceString = [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding]; - NSString *keyString = [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding]; + NSString *serviceString = stringFromByteArray(env, service); + NSString *keyString = stringFromByteArray(env, key); OSStatus status = errSecParam; if (serviceString && keyString) { // guard both NSStrings not to be NIL - // find existing: + // spans both keychains on purpose, removing protected and unprotected items alike: LAContext *context = getSharedLAContext(); NSDictionary *query = @{ (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, @@ -142,11 +170,9 @@ JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Nati NSLog(@"Error deleting item from keychain. Status code: %d", (int)status); } } else { - NSLog(@"Invalid UTF-8 input: service or key string conversion failed"); + NSLog(@"Invalid input: service or key string conversion failed"); } - (*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT); - (*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT); return status; } diff --git a/src/main/native/org_cryptomator_macos_uiappearance_AppAppearance_Native.m b/src/main/native/org_cryptomator_macos_uiappearance_AppAppearance_Native.m index 9b17d0b..2461444 100644 --- a/src/main/native/org_cryptomator_macos_uiappearance_AppAppearance_Native.m +++ b/src/main/native/org_cryptomator_macos_uiappearance_AppAppearance_Native.m @@ -10,13 +10,9 @@ #import JNIEXPORT void JNICALL Java_org_cryptomator_macos_uiappearance_AppAppearance_00024Native_setToAqua(JNIEnv *env, jobject thisObj) { - if (@available(macOS 10.14, *)) { - NSApp.appearance = [NSAppearance appearanceNamed:NSAppearanceNameAqua]; - } + NSApp.appearance = [NSAppearance appearanceNamed:NSAppearanceNameAqua]; } JNIEXPORT void JNICALL Java_org_cryptomator_macos_uiappearance_AppAppearance_00024Native_setToDarkAqua(JNIEnv *env, jobject thisObj) { - if (@available(macOS 10.14, *)) { - NSApp.appearance = [NSAppearance appearanceNamed:NSAppearanceNameDarkAqua]; - } + NSApp.appearance = [NSAppearance appearanceNamed:NSAppearanceNameDarkAqua]; } diff --git a/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java b/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java index da6daee..2c722d9 100644 --- a/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java +++ b/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java @@ -10,6 +10,21 @@ public class MacKeychainTest { private final MacKeychain keychain = new MacKeychain(); + @Test + public void testLoadResultNotFound() throws KeychainAccessException { + Assertions.assertNull(MacKeychain.toPassword(new MacKeychain.LoadResult(-25300, null))); + } + + @Test + public void testLoadResultError() { + Assertions.assertThrows(KeychainAccessException.class, () -> MacKeychain.toPassword(new MacKeychain.LoadResult(-128, null))); + } + + @Test + public void testLoadResultWithoutPassword() { + Assertions.assertThrows(KeychainAccessException.class, () -> MacKeychain.toPassword(new MacKeychain.LoadResult(0, null))); + } + @Nested public class WithStoredPassword { @@ -21,29 +36,36 @@ public void setup() throws KeychainAccessException { } @Test - public void testLoadWithInvalidAccount() { + public void testLoadWithInvalidAccount() throws KeychainAccessException { char[] loadedPw = keychain.loadPassword("service", "wrong"); Assertions.assertNull(loadedPw); } @Test - public void testLoadWithInvalidService() { + public void testLoadWithInvalidService() throws KeychainAccessException { char[] loadedPw = keychain.loadPassword("wrong", "account"); Assertions.assertNull(loadedPw); } @Test - public void testLoad() { + public void testLoad() throws KeychainAccessException { char[] loadedPw = keychain.loadPassword("service", "account"); Assertions.assertArrayEquals(storedPw.toCharArray(), loadedPw); } @Test - public void testDelete() { + public void testOverwrite() throws KeychainAccessException { + keychain.storePassword("service", "account", "0ther pw", false); + char[] loadedPw = keychain.loadPassword("service", "account"); + Assertions.assertArrayEquals("0ther pw".toCharArray(), loadedPw); + } + + @Test + public void testDelete() throws KeychainAccessException { Assertions.assertDoesNotThrow(() -> keychain.deletePassword("service", "account")); char[] deletedPw = keychain.loadPassword("service", "account"); Assertions.assertNull(deletedPw); } } -} \ No newline at end of file +} diff --git a/src/test/java/org/cryptomator/macos/keychain/MacSystemKeychainAccessTest.java b/src/test/java/org/cryptomator/macos/keychain/MacSystemKeychainAccessTest.java index 728a4d7..ab3ac54 100644 --- a/src/test/java/org/cryptomator/macos/keychain/MacSystemKeychainAccessTest.java +++ b/src/test/java/org/cryptomator/macos/keychain/MacSystemKeychainAccessTest.java @@ -48,7 +48,7 @@ public void testStoreError() throws KeychainAccessException { @Test @DisplayName("loadPassphrase() succeeds") - public void testLoadSuccess() { + public void testLoadSuccess() throws KeychainAccessException { Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn("pass".toCharArray()); char[] result = keychainAccess.loadPassphrase("key"); @@ -58,7 +58,7 @@ public void testLoadSuccess() { @Test @DisplayName("loadPassphrase() doesn't find pw") - public void testLoadNotFound() { + public void testLoadNotFound() throws KeychainAccessException { Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn(null); char[] result = keychainAccess.loadPassphrase("key"); @@ -66,6 +66,18 @@ public void testLoadNotFound() { Assertions.assertNull(result); } + @Test + @DisplayName("loadPassphrase() fails") + public void testLoadError() throws KeychainAccessException { + KeychainAccessException e = new KeychainAccessException("fail."); + Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenThrow(e); + + KeychainAccessException thrown = Assertions.assertThrows(KeychainAccessException.class, () -> { + keychainAccess.loadPassphrase("key"); + }); + Assertions.assertSame(thrown, e); + } + @Test @DisplayName("deletePassphrase() succeeds") public void testDeleteSuccess() throws KeychainAccessException { From 9fd5ac4547a99efb24d1fec1b12ec451390d4b78 Mon Sep 17 00:00:00 2001 From: Tobias Hagemann Date: Tue, 20 Oct 2026 14:18:48 +0200 Subject: [PATCH 2/4] Store Touch ID keychain entries with user presence protection, also when replacing existing ones --- CHANGELOG.md | 1 + .../macos/keychain/MacKeychain.java | 20 ++-- .../keychain/MacSystemKeychainAccess.java | 3 +- .../macos/keychain/TouchIdKeychainAccess.java | 28 ++++- ...omator_macos_keychain_MacKeychain_Native.m | 70 ++++++------ .../macos/keychain/MacKeychainTest.java | 42 +++++-- .../keychain/MacSystemKeychainAccessTest.java | 2 +- .../keychain/TouchIdKeychainAccessTest.java | 104 ++++++++++++++++++ 8 files changed, 211 insertions(+), 59 deletions(-) create mode 100644 src/test/java/org/cryptomator/macos/keychain/TouchIdKeychainAccessTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a16fc4..bf06ae1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Changes to prior versions can be found on the [Github release page](https://gith * Require macOS 11 ### Fixed +* Touch ID keychain entries that were stored without authentication are now protected when loaded * Possible out-of-bounds read when converting keychain service and account names diff --git a/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java b/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java index f110214..6441429 100644 --- a/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java +++ b/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java @@ -20,8 +20,8 @@ class MacKeychain { * @param serviceName Service name * @param account Unique account identifier * @param password Passphrase to store - * @param requireOsAuthentication Defines, whether the user needs to authenticate to store a passphrase - * @see SecKeychainAddGenericPassword + * @param requireOsAuthentication Defines, whether the user needs to authenticate to load the stored passphrase. If false, an existing entry requiring authentication stays in place and still takes precedence when loading. + * @see SecItemAdd */ public void storePassword(String serviceName, String account, CharSequence password, boolean requireOsAuthentication) throws KeychainAccessException { ByteBuffer pwBuf = UTF_8.encode(CharBuffer.wrap(password)); @@ -44,12 +44,12 @@ public void storePassword(String serviceName, String account, CharSequence passw * @throws KeychainAccessException If loading fails for any reason other than a missing entry, e.g. because authentication was cancelled * @see SecItemCopyMatching */ - public char[] loadPassword(String serviceName, String account) throws KeychainAccessException { - return toPassword(Native.INSTANCE.loadPassword(serviceName.getBytes(UTF_8), account.getBytes(UTF_8))); + public StoredPassword loadPassword(String serviceName, String account) throws KeychainAccessException { + return toStoredPassword(Native.INSTANCE.loadPassword(serviceName.getBytes(UTF_8), account.getBytes(UTF_8))); } // visible for testing - static char[] toPassword(LoadResult result) throws KeychainAccessException { + static StoredPassword toStoredPassword(LoadResult result) throws KeychainAccessException { if (result.status() == OSSTATUS_NOT_FOUND) { return null; } else if (result.status() != OSSTATUS_SUCCESS || result.password() == null) { @@ -61,7 +61,7 @@ static char[] toPassword(LoadResult result) throws KeychainAccessException { pwBuf.get(pw); Arrays.fill(pwBytes, (byte) 0x00); Arrays.fill(pwBuf.array(), (char) 0x00); - return pw; + return new StoredPassword(pw, result.dataProtectionKeychain()); } /** @@ -92,8 +92,14 @@ public boolean isTouchIDavailable() { return Native.INSTANCE.isTouchIDavailable(); } + /** + * @param requiresOsAuthentication Whether the keychain entry is stored in the data protection keychain, where {@link #storePassword(String, String, CharSequence, boolean)} only stores entries requiring user presence + */ + record StoredPassword(char[] value, boolean requiresOsAuthentication) { + } + // constructed via JNI by class name and constructor signature, so renaming or reordering its components breaks the native loadPassword - record LoadResult(int status, byte[] password) { + record LoadResult(int status, byte[] password, boolean dataProtectionKeychain) { } // initialization-on-demand pattern, as loading the .dylib is an expensive operation diff --git a/src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java b/src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java index 85fa7e3..3810386 100644 --- a/src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java +++ b/src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java @@ -37,7 +37,8 @@ public void storePassphrase(String key, String displayName, CharSequence passphr @Override public char[] loadPassphrase(String key) throws KeychainAccessException { - return keychain.loadPassword(SERVICE_NAME, key); + var password = keychain.loadPassword(SERVICE_NAME, key); + return password != null ? password.value() : null; } @Override diff --git a/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java b/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java index ffe0313..1826000 100644 --- a/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java +++ b/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java @@ -6,11 +6,14 @@ import org.cryptomator.integrations.keychain.KeychainAccessException; import org.cryptomator.integrations.keychain.KeychainAccessProvider; +import java.nio.CharBuffer; +import java.util.Arrays; + /** - * Stores passwords in the macOS system keychain. Requires an authenticated user to do so. + * Stores passwords in the macOS system keychain, requiring an authenticated user to load them. * Authentication is done via TouchID or password as a fallback, when TouchID is not available. *

- * Items are stored in the default keychain with the service name Cryptomator, unless configured otherwise + * Items are stored in the data protection keychain with the service name Cryptomator, unless configured otherwise * using the system property cryptomator.integrationsMac.keychainServiceName. */ @Priority(1010) @@ -38,7 +41,26 @@ public void storePassphrase(String key, String displayName, CharSequence passphr @Override public char[] loadPassphrase(String key) throws KeychainAccessException { - return keychain.loadPassword(SERVICE_NAME, key); + var password = keychain.loadPassword(SERVICE_NAME, key); + if (password == null) { + return null; + } else if (password.requiresOsAuthentication()) { + return password.value(); + } + // the entry was stored without user presence (e.g. by MacSystemKeychainAccess): protect it, then load it again through the authenticated path + try { + keychain.storePassword(SERVICE_NAME, key, CharBuffer.wrap(password.value()), true); + } finally { + Arrays.fill(password.value(), '\0'); + } + var protectedPassword = keychain.loadPassword(SERVICE_NAME, key); + if (protectedPassword == null || !protectedPassword.requiresOsAuthentication()) { + if (protectedPassword != null) { + Arrays.fill(protectedPassword.value(), '\0'); + } + throw new KeychainAccessException("Failed to protect stored passphrase with user presence"); + } + return protectedPassword.value(); } @Override diff --git a/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m b/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m index ddd5962..f91f65b 100644 --- a/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m +++ b/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m @@ -34,7 +34,7 @@ return string; } -// Items requiring user presence live in the data protection keychain, all others in the file-based keychain. Loading targets one of them explicitly, +// Items requiring user presence live in the data protection keychain, all others in the file-based keychain. Storing and loading target one of them explicitly, // because a query spanning both reports a failed authentication in the data protection keychain as errSecItemNotFound. static NSDictionary* itemQuery(NSString *service, NSString *account, BOOL dataProtectionKeychain) { return @{ @@ -53,41 +53,36 @@ JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Nati OSStatus status = errSecParam; if (serviceString && keyString && pwStr) { - jsize length = (*env)->GetArrayLength(env, password); - // find existing: - NSMutableDictionary *query = [@{ - (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, - (__bridge id)kSecAttrService: serviceString, - (__bridge id)kSecAttrAccount: keyString, - (__bridge id)kSecReturnAttributes: @YES, - (__bridge id)kSecReturnData: @YES, - (__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne - } mutableCopy]; + NSData *passwordData = [NSData dataWithBytes:pwStr length:(*env)->GetArrayLength(env, password)]; if (requireOsAuthentication) { - LAContext *context = getSharedLAContext(); - query[(__bridge id)kSecUseAuthenticationContext] = context; - } - CFDictionaryRef result = NULL; - status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result); - if (status == errSecSuccess && result != NULL) { - // update existing: - NSDictionary *attributesToUpdate = @{ - (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length] - }; - status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate); - } else if (status == errSecItemNotFound) { - // add new: - NSMutableDictionary *attributes = [@{ - (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, - (__bridge id)kSecAttrService: serviceString, - (__bridge id)kSecAttrAccount: keyString, - (__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length] - } mutableCopy]; - if (requireOsAuthentication) { - attributes[(__bridge id)kSecAttrAccessControl] = (__bridge_transfer id)SecAccessControlCreateWithFlags(kCFAllocatorDefault, kSecAttrAccessibleWhenUnlocked, kSecAccessControlUserPresence, NULL); + id accessControl = (__bridge_transfer id)SecAccessControlCreateWithFlags(kCFAllocatorDefault, kSecAttrAccessibleWhenUnlocked, kSecAccessControlUserPresence, NULL); + if (accessControl) { + // replace a protected item, but remove an unprotected one only after adding succeeded, so that a failed add never loses it: + NSDictionary *protectedQuery = itemQuery(serviceString, keyString, YES); + status = SecItemDelete((__bridge CFDictionaryRef)protectedQuery); + if (status == errSecSuccess || status == errSecItemNotFound) { + NSMutableDictionary *attributes = [protectedQuery mutableCopy]; + attributes[(__bridge id)kSecValueData] = passwordData; + attributes[(__bridge id)kSecAttrAccessControl] = accessControl; + status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL); + } + if (status == errSecSuccess) { + OSStatus deleteStatus = SecItemDelete((__bridge CFDictionaryRef)itemQuery(serviceString, keyString, NO)); + if (deleteStatus != errSecItemNotFound) { + status = deleteStatus; + } + } } - status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL); } else { + NSDictionary *query = itemQuery(serviceString, keyString, NO); + status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)@{(__bridge id)kSecValueData: passwordData}); + if (status == errSecItemNotFound) { + NSMutableDictionary *attributes = [query mutableCopy]; + attributes[(__bridge id)kSecValueData] = passwordData; + status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL); + } + } + if (status != errSecSuccess) { NSLog(@"Error storing item in keychain. Status code: %d", (int)status); } } else { @@ -106,6 +101,7 @@ JNIEXPORT jobject JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024N OSStatus status = errSecParam; NSData *passwordData = nil; + jboolean dataProtectionKeychain = JNI_FALSE; if (serviceString && keyString) { // guard both NSStrings not to be NIL // find existing, preferring the protected item: @@ -115,7 +111,9 @@ JNIEXPORT jobject JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024N query[(__bridge id)kSecUseAuthenticationContext] = getSharedLAContext(); CFTypeRef result = NULL; status = SecItemCopyMatching((__bridge CFDictionaryRef)query, &result); - if (status == errSecItemNotFound || status == errSecMissingEntitlement) { + if (status == errSecSuccess) { + dataProtectionKeychain = JNI_TRUE; + } else if (status == errSecItemNotFound || status == errSecMissingEntitlement) { query[(__bridge id)kSecUseDataProtectionKeychain] = @NO; status = SecItemCopyMatching((__bridge CFDictionaryRef)query, &result); } @@ -143,11 +141,11 @@ JNIEXPORT jobject JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024N if (resultClass == NULL) { return NULL; } - jmethodID resultConstructor = (*env)->GetMethodID(env, resultClass, "", "(I[B)V"); + jmethodID resultConstructor = (*env)->GetMethodID(env, resultClass, "", "(I[BZ)V"); if (resultConstructor == NULL) { return NULL; } - return (*env)->NewObject(env, resultClass, resultConstructor, (jint)status, password); + return (*env)->NewObject(env, resultClass, resultConstructor, (jint)status, password, dataProtectionKeychain); } JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_deletePassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key) { diff --git a/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java b/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java index 2c722d9..b03e382 100644 --- a/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java +++ b/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java @@ -3,26 +3,36 @@ import org.cryptomator.integrations.keychain.KeychainAccessException; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import java.nio.charset.StandardCharsets; + public class MacKeychainTest { private final MacKeychain keychain = new MacKeychain(); @Test public void testLoadResultNotFound() throws KeychainAccessException { - Assertions.assertNull(MacKeychain.toPassword(new MacKeychain.LoadResult(-25300, null))); + Assertions.assertNull(MacKeychain.toStoredPassword(new MacKeychain.LoadResult(-25300, null, false))); } @Test public void testLoadResultError() { - Assertions.assertThrows(KeychainAccessException.class, () -> MacKeychain.toPassword(new MacKeychain.LoadResult(-128, null))); + Assertions.assertThrows(KeychainAccessException.class, () -> MacKeychain.toStoredPassword(new MacKeychain.LoadResult(-128, null, false))); } @Test public void testLoadResultWithoutPassword() { - Assertions.assertThrows(KeychainAccessException.class, () -> MacKeychain.toPassword(new MacKeychain.LoadResult(0, null))); + Assertions.assertThrows(KeychainAccessException.class, () -> MacKeychain.toStoredPassword(new MacKeychain.LoadResult(0, null, false))); + } + + @Test + public void testLoadResultProtected() throws KeychainAccessException { + var loadedPw = MacKeychain.toStoredPassword(new MacKeychain.LoadResult(0, "pw".getBytes(StandardCharsets.UTF_8), true)); + Assertions.assertArrayEquals("pw".toCharArray(), loadedPw.value()); + Assertions.assertTrue(loadedPw.requiresOsAuthentication()); } @Nested @@ -37,35 +47,45 @@ public void setup() throws KeychainAccessException { @Test public void testLoadWithInvalidAccount() throws KeychainAccessException { - char[] loadedPw = keychain.loadPassword("service", "wrong"); + var loadedPw = keychain.loadPassword("service", "wrong"); Assertions.assertNull(loadedPw); } @Test public void testLoadWithInvalidService() throws KeychainAccessException { - char[] loadedPw = keychain.loadPassword("wrong", "account"); + var loadedPw = keychain.loadPassword("wrong", "account"); Assertions.assertNull(loadedPw); } @Test public void testLoad() throws KeychainAccessException { - char[] loadedPw = keychain.loadPassword("service", "account"); - Assertions.assertArrayEquals(storedPw.toCharArray(), loadedPw); + var loadedPw = keychain.loadPassword("service", "account"); + Assertions.assertArrayEquals(storedPw.toCharArray(), loadedPw.value()); + Assertions.assertFalse(loadedPw.requiresOsAuthentication()); } @Test public void testOverwrite() throws KeychainAccessException { keychain.storePassword("service", "account", "0ther pw", false); - char[] loadedPw = keychain.loadPassword("service", "account"); - Assertions.assertArrayEquals("0ther pw".toCharArray(), loadedPw); + var loadedPw = keychain.loadPassword("service", "account"); + Assertions.assertArrayEquals("0ther pw".toCharArray(), loadedPw.value()); + } + + @Test + @DisplayName("protecting fails without keychain-access-groups entitlement, as in this test JVM, but keeps the stored password") + public void testProtectWithoutEntitlement() throws KeychainAccessException { + Assertions.assertThrows(KeychainAccessException.class, () -> keychain.storePassword("service", "account", "0ther pw", true)); + var loadedPw = keychain.loadPassword("service", "account"); + Assertions.assertArrayEquals(storedPw.toCharArray(), loadedPw.value()); + Assertions.assertFalse(loadedPw.requiresOsAuthentication()); } @Test public void testDelete() throws KeychainAccessException { Assertions.assertDoesNotThrow(() -> keychain.deletePassword("service", "account")); - char[] deletedPw = keychain.loadPassword("service", "account"); + var deletedPw = keychain.loadPassword("service", "account"); Assertions.assertNull(deletedPw); } } -} +} \ No newline at end of file diff --git a/src/test/java/org/cryptomator/macos/keychain/MacSystemKeychainAccessTest.java b/src/test/java/org/cryptomator/macos/keychain/MacSystemKeychainAccessTest.java index ab3ac54..b19735f 100644 --- a/src/test/java/org/cryptomator/macos/keychain/MacSystemKeychainAccessTest.java +++ b/src/test/java/org/cryptomator/macos/keychain/MacSystemKeychainAccessTest.java @@ -49,7 +49,7 @@ public void testStoreError() throws KeychainAccessException { @Test @DisplayName("loadPassphrase() succeeds") public void testLoadSuccess() throws KeychainAccessException { - Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn("pass".toCharArray()); + Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn(new MacKeychain.StoredPassword("pass".toCharArray(), false)); char[] result = keychainAccess.loadPassphrase("key"); diff --git a/src/test/java/org/cryptomator/macos/keychain/TouchIdKeychainAccessTest.java b/src/test/java/org/cryptomator/macos/keychain/TouchIdKeychainAccessTest.java new file mode 100644 index 0000000..53613c7 --- /dev/null +++ b/src/test/java/org/cryptomator/macos/keychain/TouchIdKeychainAccessTest.java @@ -0,0 +1,104 @@ +package org.cryptomator.macos.keychain; + +import org.cryptomator.integrations.keychain.KeychainAccessException; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +public class TouchIdKeychainAccessTest { + + private MacKeychain keychain; + private TouchIdKeychainAccess keychainAccess; + + @BeforeEach + public void setup() { + keychain = Mockito.mock(MacKeychain.class); + keychainAccess = new TouchIdKeychainAccess(keychain); + } + + @Test + @DisplayName("loadPassphrase() returns protected pw") + public void testLoadProtected() throws KeychainAccessException { + Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn(new MacKeychain.StoredPassword("pass".toCharArray(), true)); + + char[] result = keychainAccess.loadPassphrase("key"); + + Assertions.assertArrayEquals("pass".toCharArray(), result); + Mockito.verify(keychain, Mockito.never()).storePassword(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.anyBoolean()); + } + + @Test + @DisplayName("loadPassphrase() protects unprotected pw before returning it") + public void testLoadUnprotected() throws KeychainAccessException { + char[] unprotectedPw = "pass".toCharArray(); + Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn( // + new MacKeychain.StoredPassword(unprotectedPw, false), // + new MacKeychain.StoredPassword("pass".toCharArray(), true)); + var storedPw = new StringBuilder(); + Mockito.doAnswer(invocation -> storedPw.append(invocation.getArgument(2, CharSequence.class))).when(keychain).storePassword(Mockito.eq("Cryptomator"), Mockito.eq("key"), Mockito.any(), Mockito.eq(true)); + + char[] result = keychainAccess.loadPassphrase("key"); + + Assertions.assertArrayEquals("pass".toCharArray(), result); + Assertions.assertEquals("pass", storedPw.toString()); + Assertions.assertArrayEquals(new char[4], unprotectedPw); + var inOrder = Mockito.inOrder(keychain); + inOrder.verify(keychain).loadPassword("Cryptomator", "key"); + inOrder.verify(keychain).storePassword(Mockito.eq("Cryptomator"), Mockito.eq("key"), Mockito.any(), Mockito.eq(true)); + inOrder.verify(keychain).loadPassword("Cryptomator", "key"); + } + + @Test + @DisplayName("loadPassphrase() fails if pw stays unprotected") + public void testLoadStaysUnprotected() throws KeychainAccessException { + char[] reloadedPw = "pass".toCharArray(); + Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn( // + new MacKeychain.StoredPassword("pass".toCharArray(), false), // + new MacKeychain.StoredPassword(reloadedPw, false)); + + Assertions.assertThrows(KeychainAccessException.class, () -> { + keychainAccess.loadPassphrase("key"); + }); + Assertions.assertArrayEquals(new char[4], reloadedPw); + } + + @Test + @DisplayName("loadPassphrase() fails if protected pw vanished") + public void testLoadVanished() throws KeychainAccessException { + Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn( // + new MacKeychain.StoredPassword("pass".toCharArray(), false), // + null); + + Assertions.assertThrows(KeychainAccessException.class, () -> { + keychainAccess.loadPassphrase("key"); + }); + } + + @Test + @DisplayName("loadPassphrase() fails if unprotected pw cannot be protected") + public void testLoadProtectError() throws KeychainAccessException { + KeychainAccessException e = new KeychainAccessException("fail."); + char[] unprotectedPw = "pass".toCharArray(); + Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn(new MacKeychain.StoredPassword(unprotectedPw, false)); + Mockito.doThrow(e).when(keychain).storePassword(Mockito.eq("Cryptomator"), Mockito.eq("key"), Mockito.any(), Mockito.eq(true)); + + KeychainAccessException thrown = Assertions.assertThrows(KeychainAccessException.class, () -> { + keychainAccess.loadPassphrase("key"); + }); + Assertions.assertSame(thrown, e); + Assertions.assertArrayEquals(new char[4], unprotectedPw); + } + + @Test + @DisplayName("loadPassphrase() doesn't find pw") + public void testLoadNotFound() throws KeychainAccessException { + Mockito.when(keychain.loadPassword("Cryptomator", "key")).thenReturn(null); + + char[] result = keychainAccess.loadPassphrase("key"); + + Assertions.assertNull(result); + } + +} From 3bfe908ddb0392eda28d33eb4896b4783c3fc638 Mon Sep 17 00:00:00 2001 From: Tobias Hagemann Date: Tue, 20 Oct 2026 14:19:30 +0200 Subject: [PATCH 3/4] Offer Touch ID keychain with password fallback when biometrics are unavailable --- CHANGELOG.md | 1 + ...omator_macos_keychain_MacKeychain_Native.h | 4 ++-- .../macos/keychain/MacKeychain.java | 10 ++++----- .../macos/keychain/TouchIdKeychainAccess.java | 2 +- ...omator_macos_keychain_MacKeychain_Native.m | 21 ++++++++++++++----- .../macos/keychain/MacKeychainTest.java | 6 ++++++ 6 files changed, 31 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf06ae1..078b115 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Changes to prior versions can be found on the [Github release page](https://gith ## [Unreleased](https://github.com/cryptomator/integrations-mac/compare/1.5.1...HEAD) ### Changed +* Touch ID keychain falls back to the device password when biometrics are unavailable * Loading a stored password reports errors, e.g. cancelled authentication, instead of reporting no stored password * Require macOS 11 diff --git a/src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h b/src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h index 3d82cd8..f85b828 100644 --- a/src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h +++ b/src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h @@ -33,10 +33,10 @@ JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Nati /* * Class: org_cryptomator_macos_keychain_MacKeychain_Native - * Method: isTouchIDavailable + * Method: isOsAuthenticationAvailable * Signature: ()Z */ -JNIEXPORT jboolean JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_isTouchIDavailable +JNIEXPORT jboolean JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_isOsAuthenticationAvailable (JNIEnv *, jobject); #ifdef __cplusplus diff --git a/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java b/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java index 6441429..ae7db7d 100644 --- a/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java +++ b/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java @@ -84,12 +84,12 @@ public boolean deletePassword(String serviceName, String account) throws Keychai } /** - * Tests whether biometric authentication via Touch ID is supported and allowed on the device + * Tests whether the user can authenticate via Touch ID or, as a fallback, their password. Also tests whether passwords requiring such authentication can be stored. * - * @return true if biometric authentication is available, false otherwise + * @return true if passwords requiring user authentication are supported, false otherwise */ - public boolean isTouchIDavailable() { - return Native.INSTANCE.isTouchIDavailable(); + public boolean isOsAuthenticationAvailable() { + return Native.INSTANCE.isOsAuthenticationAvailable(); } /** @@ -116,7 +116,7 @@ private Native() { public native int deletePassword(byte[] service, byte[] account); - public native boolean isTouchIDavailable(); + public native boolean isOsAuthenticationAvailable(); } } diff --git a/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java b/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java index 1826000..4cd96e4 100644 --- a/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java +++ b/src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java @@ -65,7 +65,7 @@ public char[] loadPassphrase(String key) throws KeychainAccessException { @Override public boolean isSupported() { - return keychain.isTouchIDavailable(); + return keychain.isOsAuthenticationAvailable(); } @Override diff --git a/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m b/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m index f91f65b..feb5459 100644 --- a/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m +++ b/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m @@ -174,13 +174,24 @@ JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Nati return status; } -JNIEXPORT jboolean JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_isTouchIDavailable(JNIEnv *env, jobject thisObj) { +JNIEXPORT jboolean JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_isOsAuthenticationAvailable(JNIEnv *env, jobject thisObj) { NSError *error = nil; LAContext *context = getSharedLAContext(); - if ([context canEvaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics error:&error]) { - return JNI_TRUE; - } else { - NSLog(@"Touch ID is not available: %@", error.localizedDescription); + // matches kSecAccessControlUserPresence, which falls back to the user's password when biometrics are unavailable: + if (![context canEvaluatePolicy:LAPolicyDeviceOwnerAuthentication error:&error]) { + NSLog(@"Device owner authentication is not available: %@", error.localizedDescription); + return JNI_FALSE; + } + // Protected items require the data protection keychain, which is only accessible with an entitlement granting a keychain access group. + // Without it, lookups merely find nothing, while deleting a never-existing item reports the missing entitlement: + NSDictionary *probe = @{ + (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, + (__bridge id)kSecAttrService: [NSUUID UUID].UUIDString, + (__bridge id)kSecUseDataProtectionKeychain: @YES + }; + if (SecItemDelete((__bridge CFDictionaryRef)probe) == errSecMissingEntitlement) { + NSLog(@"Data protection keychain is not available: missing entitlement"); return JNI_FALSE; } + return JNI_TRUE; } diff --git a/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java b/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java index b03e382..98b0649 100644 --- a/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java +++ b/src/test/java/org/cryptomator/macos/keychain/MacKeychainTest.java @@ -13,6 +13,12 @@ public class MacKeychainTest { private final MacKeychain keychain = new MacKeychain(); + @Test + @DisplayName("OS authentication is unsupported without keychain-access-groups entitlement, as in this test JVM") + public void testOsAuthenticationWithoutEntitlement() { + Assertions.assertFalse(keychain.isOsAuthenticationAvailable()); + } + @Test public void testLoadResultNotFound() throws KeychainAccessException { Assertions.assertNull(MacKeychain.toStoredPassword(new MacKeychain.LoadResult(-25300, null, false))); From 2e578f100f847afb354d87e8817c4a7dd73eb50e Mon Sep 17 00:00:00 2001 From: Tobias Hagemann Date: Tue, 20 Oct 2026 17:19:10 +0200 Subject: [PATCH 4/4] Keep an existing protected keychain entry protected when storing without authentication --- .../macos/keychain/MacKeychain.java | 2 +- ...omator_macos_keychain_MacKeychain_Native.m | 43 +++++++++++++------ 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java b/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java index ae7db7d..6f40c4a 100644 --- a/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java +++ b/src/main/java/org/cryptomator/macos/keychain/MacKeychain.java @@ -20,7 +20,7 @@ class MacKeychain { * @param serviceName Service name * @param account Unique account identifier * @param password Passphrase to store - * @param requireOsAuthentication Defines, whether the user needs to authenticate to load the stored passphrase. If false, an existing entry requiring authentication stays in place and still takes precedence when loading. + * @param requireOsAuthentication Defines, whether the user needs to authenticate to load the stored passphrase. If false, an existing entry requiring authentication keeps its protection, and updating it requires the user to authenticate. * @see SecItemAdd */ public void storePassword(String serviceName, String account, CharSequence password, boolean requireOsAuthentication) throws KeychainAccessException { diff --git a/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m b/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m index feb5459..157c2b0 100644 --- a/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m +++ b/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m @@ -45,6 +45,11 @@ }; } +// without an entitlement granting a keychain access group, the data protection keychain is unavailable, so depending on the operation, it reports either status: +static BOOL isAbsentFromDataProtectionKeychain(OSStatus status) { + return status == errSecItemNotFound || status == errSecMissingEntitlement; +} + JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key, jbyteArray password, jboolean requireOsAuthentication) { NSString *serviceString = stringFromByteArray(env, service); NSString *keyString = stringFromByteArray(env, key); @@ -54,10 +59,10 @@ JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Nati if (serviceString && keyString && pwStr) { NSData *passwordData = [NSData dataWithBytes:pwStr length:(*env)->GetArrayLength(env, password)]; + BOOL protectedItemStored = NO; if (requireOsAuthentication) { id accessControl = (__bridge_transfer id)SecAccessControlCreateWithFlags(kCFAllocatorDefault, kSecAttrAccessibleWhenUnlocked, kSecAccessControlUserPresence, NULL); if (accessControl) { - // replace a protected item, but remove an unprotected one only after adding succeeded, so that a failed add never loses it: NSDictionary *protectedQuery = itemQuery(serviceString, keyString, YES); status = SecItemDelete((__bridge CFDictionaryRef)protectedQuery); if (status == errSecSuccess || status == errSecItemNotFound) { @@ -66,20 +71,30 @@ JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Nati attributes[(__bridge id)kSecAttrAccessControl] = accessControl; status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL); } - if (status == errSecSuccess) { - OSStatus deleteStatus = SecItemDelete((__bridge CFDictionaryRef)itemQuery(serviceString, keyString, NO)); - if (deleteStatus != errSecItemNotFound) { - status = deleteStatus; - } - } + protectedItemStored = status == errSecSuccess; } } else { - NSDictionary *query = itemQuery(serviceString, keyString, NO); - status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)@{(__bridge id)kSecValueData: passwordData}); - if (status == errSecItemNotFound) { - NSMutableDictionary *attributes = [query mutableCopy]; - attributes[(__bridge id)kSecValueData] = passwordData; - status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL); + // an existing protected item takes precedence when loading, so update it in place. The update keeps its protection and requires the user to authenticate: + NSDictionary *valueAttributes = @{(__bridge id)kSecValueData: passwordData}; + NSMutableDictionary *protectedQuery = [itemQuery(serviceString, keyString, YES) mutableCopy]; + protectedQuery[(__bridge id)kSecUseAuthenticationContext] = getSharedLAContext(); + status = SecItemUpdate((__bridge CFDictionaryRef)protectedQuery, (__bridge CFDictionaryRef)valueAttributes); + protectedItemStored = status == errSecSuccess; + if (isAbsentFromDataProtectionKeychain(status)) { + NSDictionary *query = itemQuery(serviceString, keyString, NO); + status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)valueAttributes); + if (status == errSecItemNotFound) { + NSMutableDictionary *attributes = [query mutableCopy]; + attributes[(__bridge id)kSecValueData] = passwordData; + status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL); + } + } + } + if (protectedItemStored) { + // delete the unprotected item only once the protected one is stored, so that a failed store never loses the password: + OSStatus deleteStatus = SecItemDelete((__bridge CFDictionaryRef)itemQuery(serviceString, keyString, NO)); + if (deleteStatus != errSecItemNotFound) { + status = deleteStatus; } } if (status != errSecSuccess) { @@ -113,7 +128,7 @@ JNIEXPORT jobject JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024N status = SecItemCopyMatching((__bridge CFDictionaryRef)query, &result); if (status == errSecSuccess) { dataProtectionKeychain = JNI_TRUE; - } else if (status == errSecItemNotFound || status == errSecMissingEntitlement) { + } else if (isAbsentFromDataProtectionKeychain(status)) { query[(__bridge id)kSecUseDataProtectionKeychain] = @NO; status = SecItemCopyMatching((__bridge CFDictionaryRef)query, &result); }