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

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

Open
cohen72 opened this issue Oct 17, 2023 · 30 comments
Milestone

Comments

@cohen72
Copy link

cohen72 commented Oct 17, 2023

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
Copy link
Owner

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
Copy link

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
Copy link

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

@kirillzh
Copy link

Also looks similar to #144 (comment)?

@russhwolf
Copy link
Owner

russhwolf commented Oct 27, 2023

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
Copy link

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

@alexwhb
Copy link

alexwhb commented Nov 21, 2023

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
Copy link

plindberg commented Jan 17, 2024

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
Copy link
Owner

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

vinodiOS commented Nov 6, 2024

@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
Copy link

vinodiOS commented Nov 6, 2024

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

@alessiotoma8
Copy link

@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
Copy link

vinodiOS commented Nov 6, 2024

Thanks for valuable insights @alessiotoma8 👍

@russhwolf
Copy link
Owner

@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
Copy link

@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.

  • In this way the keychain remain accessible after screen lock.
  • Differently from putting only into addKeychainItem func, the previously value saved without initializing keychain in this way, is correctly accessible.

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
Copy link

vinodiOS commented Nov 18, 2024

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
Copy link

alessiotoma8 commented Nov 18, 2024

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
#171 (comment)

@russhwolf
Copy link
Owner

@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.

@alessiotoma8
Copy link

alessiotoma8 commented Nov 25, 2024

I tried to insert this thing in the real application and I got an unwanted behavior. The value that was previously saved without kSecAttrAccessibleAfterFirstUnlock was returned with default values.
Using the save function instead it returned an error telling me that the value with that key already existed.
This happend cause the kSecAttrAccesible is part of identifier of the key and putting it into defaultProprieties tell all the queries of keychainOperation to put it as part of query.

I tried everything again in the test application and it actually doesn't seem to work as I expect. Probably I didn't test thoroughly before and when I changed the initialization I didn't reset the values correctly.

The most correct solution also from what I think I've read around would be to force the user to specify this key and insert it only in the addKeychainItem function and in the updateKeychainItem if needed.
In this way there are no problems in reading or writing even of previous values. The only thing is that as previously said the values not saved with this key will respect the default configuration kSecAttrAccessibleWhenUnlocked.
A new call to update or add can save/ update the new kSecAttrAccessibleAfterFirstUnlock key.
It could eventually implement a migration mechanism for which all the values of all the keys are migrated by saving the value with the new "accesibility", which could become mandatory in the constructor. So in this way change from the defualt one to the specified one.
For me is revelant to not lose the values
This is more or less what I thought. Let me know if you found better solutions.
I think I will implement this solution now and keep you updated on crashes in firebase

Here's code snippest to test:

Button("Get Value") {
  // Unlock the screen after 8 sec after clicked this button to ensure the task is not stopped by the os
  DispatchQueue.main.asyncAfter(deadline :.now() + 30) {
    print("KO") outputText = selectedItem().get()
  }
}

Useful links:
KVault
Medium

@russhwolf
Copy link
Owner

Alright, I'm clear at this point that there's some changes to make to KeychainSettings in order to address this.

An ideal API would set an explicit default for kSecAttrAccessible but I want to make sure to do so in a way that doesn't break existing users, to the extent that's possible. (To the extent that ends up not being possible, I'll remind you that that's why you opted into @ExperimentalSettingsImplementation.) I also want to maintain at least some of the flexibility that was intended with the current implementation with regard to being able to pass custom flags if the default of setting kSecClass to kSecClassGenericPassword and kSecAttrService to a custom name don't work for you.

I'll likely come back to this ticket looking for people to help test once I have a prototype ready.

@rs-georg
Copy link

We had the same problem in our project. We ended up passing kSecAttrAccessibleAfterFirstUnlock as constructor parameter to KeychainSettings. This caused an issue where values stored before the change could not be read anymore. To fix it, we created a migration. First we read the values from an instance without the property set. Then we cleared the entire store. After that we created a new instance with kSecAttrAccessibleAfterFirstUnlock set and wrote all values to this new instance.

@vinodiOS
Copy link

@rs-georg This approach appears straightforward, but could you elaborate on how we can transition to a new instance at the implementation level?

@rs-georg
Copy link

Hi @vinodiOS
we did it kinda like this

fun migrate() {
  val oldStore = KeychainSettings(service = "myStore")
  val newStore = KeychainSettings(
    kSecAttrService to CFBridgingRetain("myStore"),
    kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlock,
  )

  if (newStore.hasKey("_version")) return

  val oldValue = oldStore.getStringOrNull("myKey")

  oldStore.clear()

  oldValue?.let { newStore.putString("myKey", it) }
  newStore.putString("_version", "1")
}

@vinodiOS
Copy link

@rs-georg That’s helpful 😊

@alessiotoma8
Copy link

alessiotoma8 commented Dec 13, 2024

Just to inform and keep my promise about my future updates:
I tried to implement something like @rs-georg suggests and checking the Firebase crash analysis, the problem seems to be solved.
Previously I had 20/25 errors a week and for this week I didn't notice them even once!

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

9 participants