russhwolf / multiplatform-settings

A Kotlin Multiplatform library for saving simple key-value data
Apache License 2.0
1.69k stars 67 forks source link

kotlin.IllegalStateException: Keychain error -25308: User interaction is not allowed. #171

Open cohen72 opened 1 year ago

cohen72 commented 1 year ago

We are seeing random users crashing with this error on app launch. Is there any additional info that I can provide that will help this thread to evolve and resolve the issue?

russhwolf commented 1 year ago

Can you show how you're initializing KeychainSettings? Do you have a stack trace of the crash? What are you doing at launch that interacts with Settings? What platform are you running on?

The error suggests that you're trying to access a key that requires some form of interaction, which I think means it's something that requires a password input or face/fingerprint auth or the like. KeychainSettings won't easily support this I don't think, though you might be able to hack your way through passing custom kSec properties.

kirillzh commented 1 year ago

We are seeing similar issue occasionally, we don't do anything too special, just reading/writing a value into settings. Settings are initialized like so: KeychainSettings(service = storeName).toSuspendSettings()

kirillzh commented 1 year ago

This thread looks relevant, though not seeing any obvious explanation nor solution for this error: https://developer.apple.com/forums/thread/97091.

kirillzh commented 1 year ago

Also looks similar to https://github.com/russhwolf/multiplatform-settings/issues/144#issuecomment-1562813538?

russhwolf commented 1 year ago

Interesting. Good to have confirmation you can get this error even with the default kSec args. To be clear, you're seeing the same 25308 error code?

I suspect the similarity to #144 is a red herring. My sense of that issue is that there are cases when you pass certain access control flags that break the assumptions that KeychainSettings makes about status codes. If you're seeing that error without passing any non-default flags I think it's coming from a different cause. But I haven't dug deep enough into #144 to be certain, so maybe they are related.

kirillzh commented 1 year ago

That's right, we are seeing the exact same error code -25308.

alexwhb commented 1 year ago

I'm seeing this error in my app as well

Other than this... the library is awesome! Thanks @russhwolf for the work you put into this.

I suspect it happens when a user uninstalls the app then installs the app and that key may still be in keychain, but is maybe not accessible to this new app instance so it throws that exception. Just a hunch. Here's a stack trace from my app:

Screenshot 2023-11-21 at 9 15 46 AM
plindberg commented 10 months ago

This Apple Dev Forums thread was helpful for us: https://forums.developer.apple.com/forums/thread/114159. For our app, these errors seem to coincide with the user locking their device shortly before we attempt to fetch a Keychain item for which we specified kSecAttrAccessible to kSecAttrAccessibleWhenUnlocked.

russhwolf commented 9 months ago

Has anyone with these errors ever seen them in a dev build, or does it only for apps released through the store? I've done some testing but haven't been able to reproduce it when uninstalling/reinstalling through XCode.

plindberg commented 9 months ago

Has anyone with these errors ever seen them in a dev build, or does it only for apps released through the store? I've done some testing but haven't been able to reproduce it when uninstalling/reinstalling through XCode.

I have only been able to reproduce his when reading a Keychain item from an iOS widget that is on the active Home Screen page. An item written with kSecAttrAccessible to kSecAttrAccessibleWhenUnlocked (or similar) is then quite likely to be read before the device is fully unlocked.

alessiotoma8 commented 8 months ago

I finally found the issue talking for my case. There was a kotlin flow still active with screen locked that try to read from KeyChain something. This caused the error described so making sure to not trying access keychain from lock screen, the issue seems to be not present anymore. Following @plindberg it could be potentially fixed adding the key (verify it)

Has anyone with these errors ever seen them in a dev build, or does it only for apps released through the store? I've done some testing but haven't been able to reproduce it when uninstalling/reinstalling through XCode.

Talking for me, the app is published on app store, but the issue seems to be present on trying access keychain from/ after lock screen, with the app in foreground

alessiotoma8 commented 8 months ago

I'm be able to reproduce it in the test application you provide, setting a value in KeyChain and then adding a delay of 30 seconds (10 or 20 isn't enough) before reading this. Meanwhile of this delay i lock the screen. Every time throws the error above. This Is caused by the default kSecAttrAccessibleWhenUnlocked key when reading values

Keychain access on lock

I try to set the kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock on the saving function (addKeychainItem of KeyChainSettings class) and it works pretty good, so the key is access even with screen locked, only need to be unlocked once before the restart of device and then remain available; And is the suitable way apple provide to access it for background task.

Block Keychain access on lock

Another solution is to put an observer on UIApplicationProtectedDataWillBecomeUnavailable that is a notification that posts before protected files are locked down and become inaccessible, so block the coroutine and then resume if it return accessibile with ApplicationProtectedDataDidBecomeAvailable

It would bee nice that those behaviours Is implemented in lib and customizable on the implementation

Useful links:

Apple Dev - restricting keychain item

Apple Dev Forum

Apple security guidelines

trunghvbk commented 7 months ago

Hi @alessiotoma8 Thanks for showing the solutions. For this, could you please give some codes or make a PR to be applied to the library?

I try to set the kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock on the saving function (addKeychainItem of KeyChainSettings class) and it works pretty good, so the key is access even with screen locked, only need to be unlocked once before the restart of device and then remain available;
And is the suitable way apple provide to access it for background task.
alessiotoma8 commented 7 months ago

Hi @alessiotoma8 Thanks for showing the solutions. For this, could you please give some codes or make a PR to be applied to the library?

I try to set the kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock on the saving function (addKeychainItem of KeyChainSettings class) and it works pretty good, so the key is access even with screen locked, only need to be unlocked once before the restart of device and then remain available;
And is the suitable way apple provide to access it for background task.

Keychain access on lock

This is the code i try to add in my copy paste implementation of KeychainSettings

private inline fun addKeychainItem(key: String, value: NSData?): Unit = cfRetain(key, value) { cfKey, cfValue ->
        val status = keyChainOperation(
            kSecAttrAccount to cfKey,
            kSecValueData to cfValue,
           kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock
        ) { SecItemAdd(it, null) }
        status.checkError()
    }

Please note that the previous value saved without this key is still not accessible with locked screen

Alternately the second approach i mention

Block Keychain access on lock

class BlockingKeyChain {
    private val notificationCenter = CFNotificationCenterGetLocalCenter()
    private val dataUnavailable = UIApplicationProtectedDataWillBecomeUnavailable?.toCFString()
    private val dataAvailable = UIApplicationProtectedDataDidBecomeAvailable?.toCFString()
    override fun getFlowProtectedDataAvailable(): Flow<Boolean> {
        return NotificationActions.sharedFlowProtectedDataAvailable
    }
    private object NotificationActions{
        private val isDataAvailable: Boolean
            get() = UIApplication.sharedApplication.protectedDataAvailable
        val sharedFlowProtectedDataAvailable = MutableStateFlow(isDataAvailable)

        val callbackUnavailable: CPointer<CFunction<(CFNotificationCenterRef?, COpaquePointer?, CFNotificationName?, COpaquePointer?, CFDictionaryRef?) -> Unit>> =
            staticCFunction { _, _, _, _, _ ->
                coroutineScope.launch {
                    //Emitting keychain unavailable, screen is locked
                    sharedFlowProtectedDataAvailable.tryEmit(false)
                }
            }

        val callbackAvailable: CPointer<CFunction<(CFNotificationCenterRef?, COpaquePointer?, CFNotificationName?, COpaquePointer?, CFDictionaryRef?) -> Unit>> =
            staticCFunction { _, _, _, _, _ ->
                coroutineScope.launch {
                    //Emitting keychain available, screen is unlocked
                    sharedFlowProtectedDataAvailable.tryEmit(true)
                }
            }
    }

    override fun registerProtectedDataObservers() {
        CFNotificationCenterAddObserver(
            callBack = NotificationActions.callbackUnavailable,
        )

        CFNotificationCenterAddObserver(
            callBack = NotificationActions.callbackAvailable,
        )
        dataUnavailable?.release()
        dataAvailable?.release()
    }

    override fun unregisterProtectedDataObservers() {
        CFNotificationCenterRemoveObserver(
            name = dataUnavailable,
        )
        CFNotificationCenterRemoveObserver(
            name = dataAvailable,
        )
        dataUnavailable?.release()
        dataAvailable?.release()
    }
}

suspend fun <T> performOnKeyChainAvailable(action: () -> T): T {
        return getFlowProtectedDataAvailable()
            .filter { it }
            .distinctUntilChanged()
            .map {
                action()
            }.first()
    }

 performOnKeyChainAvailable{
     settings.putString(KEY, value)
}
vinodiOS commented 2 weeks ago

@russhwolf I have integrated this library into my app, and it’s functioning smoothly. However, there are numerous crashes occurring on iOS. Is there a reliable solution to address this issue?

vinodiOS commented 2 weeks ago

@alessiotoma8 I’m initializing KeychainSettings(service = "someString"). Is there a way to set kSecAttrAccessibleAfterFirstUnlock during initialization?

alessiotoma8 commented 2 weeks ago

@alessiotoma8 I’m initializing KeychainSettings(service = "someString"). Is there a way to set kSecAttrAccessibleAfterFirstUnlock during initialization?

I didn't find a way to do it, the only one think to do is to set kSecAttrAccessibleAfterFirstUnlock into the addKeychainItem as i shown and this have a limitation: that the previous value saved without this key is still not accessible with locked screen

I hope to a new integrated solution to manage the customization of the 2 ways to access protected data. So who implement can decide if keychain should be available or not with locked screen and the libs manage the decision without throwing errors.

PS: Please note that my BlockingKeyChain is not stable and production ready. I've integrated it and reduced significantly the crash occurred but still happening.

vinodiOS commented 2 weeks ago

Thanks for valuable insights @alessiotoma8 👍

russhwolf commented 2 weeks ago

@alessiotoma8 Does it work for you if you initialize your settings as KeychainSettings(kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock, kSecAttrService to CFBridgingRetain("service name"))? That's the intended way to pass custom properties, but the keychain has lots of undocumented weird edge-cases so maybe it doesn't work for some reason.

alessiotoma8 commented 1 week ago

@alessiotoma8 Does it work for you if you initialize your settings as KeychainSettings(kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock, kSecAttrService to CFBridgingRetain("service name"))? That's the intended way to pass custom properties, but the keychain has lots of undocumented weird edge-cases so maybe it doesn't work for some reason.

I try to do this and this seems to resolve the issue.

The first local test I do seems to work into the "multiplatform settings sample app". I’ll try to implement in this way into the real app, and I'll let you know here if this solution actually solves the problem. In this case I think it makes sense, in the default constructor, to insert this property and document it. Thank you very much

vinodiOS commented 4 days ago
public class Factory : Settings.Factory {
    override fun create(name: String?): KeychainSettings {
        return if (name != null) {
            KeychainSettings(
                kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock,
                kSecAttrService to CFBridgingRetain("service name")
            )
        } else {
            KeychainSettings()
        }
    }
}

@alessiotoma8 @russhwolf Is this what you were referring to? Could you please confirm?

alessiotoma8 commented 4 days ago

Yes it's depends on your implementation I'm previously using directly KeychainSettings(name)

That how @russhwolf say it can be KeychainSettings(kSecAttrService to CFBridgingRetain(name), kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock)

If you are using the Settings Factory to create an instance instead, you need to do it as you shown, but it's more correct to specify the kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlockalso into the else block

If you are copy pasting the implementation of KeychainSettings into yout projject you can directly do:

class TestKeyChain(vararg defaultProperties: Pair<CFStringRef?, CFTypeRef?>) : Settings {
public constructor(service: String) : this(kSecAttrService to CFBridgingRetain(service), kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock)

So you don't need to specify it also into Factory create function

As i said before, I'm note sure that's working on 100%, I'll do some test and release an application version, checking to firebase if the crash is less or is disappeard. And i let known the result of this

Please upvote https://github.com/russhwolf/multiplatform-settings/issues/171#issuecomment-2478461148

russhwolf commented 4 days ago

@alessiotoma8 Thanks for investigating. I'll await your results.

As you say, a solution here is to default to passing kSecAttrAccessibleAfterFirstUnlock. I'm also considering deprecating the existing constructors and forcing the user to pass a kSecAttrAccessible value themselves. Either way. this has some implications for macOS that I want to make sure I understand before making such a change, as described here.