Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KeychainSettings crash in getStringOrNull #144

Open
sebj opened this issue Feb 27, 2023 · 14 comments
Open

KeychainSettings crash in getStringOrNull #144

sebj opened this issue Feb 27, 2023 · 14 comments
Milestone

Comments

@sebj
Copy link

sebj commented Feb 27, 2023

We're seeing a crash (EXC_CRASH (SIGABRT)) on iOS 16 devices when calling getStringOrNull on a KeychainSettings instance, in a KMM framework included statically in my iOS app. This occurs across iOS 16 versions (16.1, 16.2, 16.3, 16.4) and device types (iPhone and iPad). Unfortunately it's not something I've been able to recreate yet, nor do I have too much information as we're having some crash report symbolication issues right now.

This function is being indirectly called via an Apollo HTTP Interceptor in our app.

We're providing two default properties when creating our singleton KeychainSettings instance:

KeychainSettings(
    kSecAttrAccessible to kSecAttrAccessibleWhenUnlocked,
    kSecAttrAccessGroup to CFBridgingRetain("<redacted access group>")
)
@russhwolf
Copy link
Owner

How frequently is it happening? Not sure what I can do without more information, but definitely update here if you figure out more.

@jsm174
Copy link

jsm174 commented Mar 8, 2023

We are using an access group as well like this:

return KeychainSettings(
    kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock,
    kSecAttrService to CFBridgingRetain(Constants.IOS_KEYCHAIN_SERVICE),
    kSecAttrAccessGroup to CFBridgingRetain(Constants.IOS_KEYCHAIN_GROUP),
)

I'm seeing issues where I get:

Uncaught Kotlin exception: kotlin.IllegalStateException: Keychain error -25299: The specified item already exists in the keychain.
    at 0   shared                              0x1054bfe2f        kfun:kotlin.Throwable#<init>(kotlin.String?){} + 123 
    at 1   shared                              0x1054b7efb        kfun:kotlin.Exception#<init>(kotlin.String?){} + 119 
    at 2   shared                              0x1054b81c7        kfun:kotlin.RuntimeException#<init>(kotlin.String?){} + 119 
    at 3   shared                              0x1054b8a4f        kfun:kotlin.IllegalStateException#<init>(kotlin.String?){} + 119 
    at 4   shared                              0x105f32da7        kfun:com.russhwolf.settings.KeychainSettings#putString(kotlin.String;kotlin.String){} + 41279 

This happens on the device and not the simulator. What's strange is, I've even tried to clear with .clear() and even iterating over the keys doesn't show the key it is complaining about

        val iterator = secure.keys.iterator()
        while(iterator.hasNext()) {
            val element = iterator.next()
            println("---- KEY FOUND: " + element + "\n")
        }

@russhwolf
Copy link
Owner

Can you try calling SecItemCopyMatching directly and see what status code is returned? KeychainSettings assumes that either the key is present or SecItemCopyMatching will return errSecItemNotFound. But maybe there are other returns that need to be considered when you're using other access-control flags. I don't know the best-practice around this stuff very well and I've never tested with kSecAttrAccessible or kSecAttrAccessGroup.

@0x6368656174
Copy link

If someone faces with such issue. You should know, that when you stored values without kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock, then set kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock and try to call clear, nothing will be cleared, because the library will try to find and clear keys with kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock, but them was stored with kSecAttrAccessible to kSecAttrAccessibleWhenUnlocked.

You should run KeychainSettings().clear() with your previous parameters to clear keys.

@russhwolf . I think that keys and hasKeychainItem should try to find key without any passed defaultProperties, to be sure that keys will be found when kSecAttrAccessible is changed.

@russhwolf
Copy link
Owner

Oh that's interesting. I haven't considered the case where you change what parameters you pass. Can you describe your use-case more, just to make sure I understand?

@0x6368656174
Copy link

There are no cases=)

We just started using this library without any extra configuration by let settings = KeychaingSettings(). Then we started to get the app crashes when the app was in background with Keychain error -25308: User interaction is not allowed. error. After invistigating we understood that we should use the kSecAttrAccessibleAfterFirstUnlock. We set it and our app started crashing with Keychain error -25299: The specified item already exists in the keychain. error, because we tried to rewrite existed keys that was stored with the default kSecAttrAccessibleWhenUnlocked param. Then we tried to call clear and didn't see any result, because the clear method tried to clear keys with kSecAttrAccessibleAfterFirstUnlock param, but they was stored with the default kSecAttrAccessibleWhenUnlocked param. After changing clear to the KeychainSettings(Pair(kSecAttrAccessible, kSecAttrAccessibleWhenUnlocked)).clear() everything worked fine.

Right now our code looks like this:

// Clear previous keys with wrong kSecAttrAccessible
KeychainSettings(Pair(kSecAttrAccessible, kSecAttrAccessibleWhenUnlocked)).clear()

let settings = KeychainSettings(Pair(kSecAttrAccessible, kSecAttrAccessibleAfterFirstUnlock))

// Continue to use settiongs

@tbgradinariu
Copy link

I also encountered a crash in getStringOrNull, but I cannot extract too much from the strack trace other than:
#0 0x1052d9d50 in inlined-out:error ()
#1 0x1052d9d50 in inlined-out:checkError ()
#2 0x1052d9d50 in <inlined-out:> ()
#3 0x1052d9d50 in <inlined-out:> ()
#4 0x1052d9d50 in inlined-out:memScoped ()
#5 0x1052d9d50 in inlined-out:cfRetain ()
#6 0x1052d9d50 in inlined-out:getKeychainItem ()
#7 0x1052d9d50 in kfun:com.russhwolf.settings.KeychainSettings#getStringOrNull(kotlin.String){}kotlin.String? ()

As I understand it, the error() call at

will throw an IllegalStateException. From reading the API it is not obvious that an exception could be thrown. Maybe annotating getStringOrNull with @throws would make it clearer?

@russhwolf
Copy link
Owner

@tbgradinariu How are you initializing KeychainSettings? The other reports in this ticket are specifically around passing certain access control flags.

@tbgradinariu
Copy link

sorry, forgot to mention.
private val keychainSettings = KeychainSettings(serviceName)
I'm using the default constructor, without any other properties.

@mirabo-trunghoang
Copy link

mirabo-trunghoang commented Apr 1, 2024

Hi, I ran into the same crash log. And I inject the KeychainSettings by construction KeychainSettings.Factory()
Is there any update on this issue?

Last Exception Backtrace:
0   shared                        	0x10a52ab70 <inlined-out:error> + 52 (Preconditions.kt:143)
1   shared                        	0x10a52ab70 <inlined-out:checkError> + 52 (KeychainSettings.kt:249)
2   shared                        	0x10a52ab70 <inlined-out:<anonymous>> + 560 (KeychainSettings.kt:220)
3   shared                        	0x10a52ab70 <inlined-out:<anonymous>> + 560 (KeychainSettings.kt:282)
4   shared                        	0x10a52ab70 <inlined-out:memScoped> + 560 (Utils.kt:638)
5   shared                        	0x10a52ab70 <inlined-out:cfRetain> + 560 (KeychainSettings.kt:279)
6   shared                        	0x10a52ab70 <inlined-out:getKeychainItem> + 560 (KeychainSettings.kt:213)
7   shared                        	0x10a52ab70 kfun:com.russhwolf.settings.KeychainSettings#getStringOrNull(kotlin.String){}kotlin.String? + 9016 (KeychainSettings.kt:156)
8   shared                        	0x10a561a30 kfun:com.russhwolf.settings.serialization.SettingsDecoder.decodeString#internal + 352 (SettingsSerialization.kt:386)
9   shared                        	0x10a4078b4 kfun:kotlinx.serialization.encoding.AbstractDecoder#decodeStringElement(kotlinx.serialization.descriptors.SerialDescriptor;kotlin.Int){}kotlin.String + 68 (AbstractDecoder.kt:58)
10  shared                        	0x10a58bf80 
kfun:jp.jcl.cyclecanvas.core.datastore.dto.LocalToken.$serializer#deserialize(kotlinx.serialization.encoding.Decoder){}

@lammertw
Copy link

I'm also receiving many crash reports for iOS devices such as these:

Last Exception Backtrace:
0   shared                        	0x103bdf710 kfun:com.russhwolf.settings.KeychainSettings#getStringOrNull(kotlin.String){}kotlin.String? + 5516 (KeychainSettings.kt:170)
1   shared                        	0x103dcd694 kfun:com.russhwolf.settings.Settings#getStringOrNull(kotlin.String){}kotlin.String?-trampoline + 192 (Settings.kt:119)
2   shared                        	0x103cd8740 kfun:com.forbes.forbesreader.auth.SettingsTokenStore#loadToken(com.paoapps.fifi.model.ModelEnvironment){}com.forbes.forbesreader.api.domain.Token? + 280 (SettingsTokenStore.kt:12)
3   shared                        	0x103dd6ddc kfun:com.forbes.forbesreader.auth.TokenStore#loadToken(com.paoapps.fifi.model.ModelEnvironment){}com.forbes.forbesreader.api.domain.Token?-trampoline + 196 (TokenStore.kt:10)
4   shared                        	0x103cfbb18 kfun:com.forbes.forbesreader.model.AuthModelImpl.$<init>$lambda$7$lambda$6$FUNCTION_REFERENCE$16.emit#internal + 132 (AuthModelImpl.kt:68)
5   shared                        	0x103db8f10 kfun:kotlinx.coroutines.flow.FlowCollector#emit#suspend(1:0;kotlin.coroutines.Continuation<kotlin.Unit>){}kotlin.Any-trampoline + 196 (FlowCollector.kt:31)
6   shared                        	0x103ad86c8 kfun:kotlinx.coroutines.flow.StateFlowImpl.$collectCOROUTINE$0.invokeSuspend#internal + 908 (StateFlow.kt:396)
7   shared                        	0x103ad8a34 kfun:kotlinx.coroutines.flow.StateFlowImpl.collect#internal + 156 (StateFlow.kt:407)
8   shared                        	0x103db8fe4 kfun:kotlinx.coroutines.flow.Flow#collect#suspend(kotlinx.coroutines.flow.FlowCollector<1:0>;kotlin.coroutines.Continuation<kotlin.Unit>){}kotlin.Any-trampoline + 196 (Flow.kt:194)
9   shared                        	0x103cfb888 kfun:com.forbes.forbesreader.model.AuthModelImpl.$<init>$lambda$7$FUNCTION_REFERENCE$12.invoke#internal + 188 (AuthModelImpl.kt:67)
10  shared                        	0x103dac464 kfun:kotlin.Function2#invoke(1:0;1:1){}1:2-trampoline + 196 ([K][Suspend]Functions:1)
11  shared                        	0x103a2c9fc kfun:kotlin.coroutines.intrinsics.object-3.invokeSuspend#internal + 444 (IntrinsicsNative.kt:254)
12  shared                        	0x103a2bba0 kfun:kotlin.coroutines.native.internal.BaseContinuationImpl#resumeWith(kotlin.Result<kotlin.Any?>){} + 184 (ContinuationImpl.kt:26)
13  shared                        	0x103ae9584 kfun:kotlinx.coroutines.DispatchedTask#run(){} + 472 (DispatchedTask.kt:0)
14  shared                        	0x103afb1f4 kfun:kotlinx.coroutines.DarwinMainDispatcher.$dispatch$lambda$0$FUNCTION_REFERENCE$1.$<bridge-UNN>invoke(){}#internal + 40 (Dispatchers.kt:41)
15  shared                        	0x103daa394 kfun:kotlin.Function0#invoke(){}1:0-trampoline + 196 ([K][Suspend]Functions:1)
16  shared                        	0x1041c7bb8 ___6f72672e6a6574627261696e732e6b6f746c696e783a6b6f746c696e782d636f726f7574696e65732d636f72652f6f70742f6275696c644167656e742f776f726b2f343465633665383530643563363366302f6b6f746c696e782d636f726f7574696e65732d636f72652f6e617469766544617277696e2f7372632f44697370617463686572732e6b74_knbridge7_block_invoke + 232
17  libdispatch.dylib             	0x198d5c370 _dispatch_call_block_and_release + 32 (init.c:1549)
18  libdispatch.dylib             	0x198d5e0d0 _dispatch_client_callout + 20 (object.m:576)
19  libdispatch.dylib             	0x198d6c9e0 _dispatch_main_queue_drain + 980 (queue.c:8093)
20  libdispatch.dylib             	0x198d6c5fc _dispatch_main_queue_callback_4CF + 44 (queue.c:8253)
21  CoreFoundation                	0x1910b7f64 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 16 (CFRunLoop.c:1773)
22  CoreFoundation                	0x1910b5188 __CFRunLoopRun + 1996 (CFRunLoop.c:3143)
23  CoreFoundation                	0x1910b45b8 CFRunLoopRunSpecific + 572 (CFRunLoop.c:3414)
24  GraphicsServices              	0x1dcb4a1c4 GSEventRunModal + 164 (GSEvent.c:2196)
25  UIKitCore                     	0x193c0a2c0 -[UIApplication _run] + 816 (UIApplication.m:3825)
26  UIKitCore                     	0x193cb8ddc UIApplicationMain + 340 (UIApplication.m:5477)
27  SwiftUI                       	0x195813804 closure #1 in KitRendererCommon(_:) + 168 (UIKitApp.swift:68)
28  SwiftUI                       	0x1957f0e68 runApp<A>(_:) + 100 (UIKitApp.swift:16)
29  SwiftUI                       	0x1957f3d68 static App.main() + 180 (App.swift:121)
30  iosApp                        	0x102501d80 static ForbesApp.$main() + 52 (ForbesApp.swift:0)
31  iosApp                        	0x102501d80 main + 64
32  dyld                          	0x1b6888d34 start + 2724 (dyldMain.cpp:1334)

Here is how I initialize it:

return KeychainSettings(
        kSecAttrService to CFBridgingRetain(serviceName),
        kSecAttrAccessible to kSecAttrAccessibleWhenUnlockedThisDeviceOnly
    )

@russhwolf
Copy link
Owner

@lammertw Do you know whether or not the crashes are happening when the device is locked?

@lammertw
Copy link

Unfortunately I don't know, but that was also my suspicion since I'm not able to reproduce it when running the app myself.

@russhwolf
Copy link
Owner

I was trying to keep them separate for a while, but I think ultimately this is going to come down to the same redesign of KeychainSettings as I'm planning in #171

@russhwolf russhwolf added this to the 1.4 milestone Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants