Error: cannotAccessKeychain despite canAccessKeychain() returning true immediately before calling, caused by saving largeish data

[christianselig]

I've ran into a bug where saving to the Keychain with Valet will fail with a "cannotAccessKeychain" error if you're saving larger amounts of data (say, several megabytes) despite everything else seeming proper and canAccessKeychain() being called immediately before returning true.

I ran into this because I foolishly was testing encrypting more of the user's data (performance was fine on modern devices) than necessary, and for some users with a large amount of data combined with older iOS devices they were running into issues with data saving. Sure enough I was able to replicate this within a few tries with an iPhone 6 running iOS 12.4.3.

I was admittedly lulled to a false sense of security a little (totally my fault, should have done my due diligence) by this phrase in the README that maybe should be updated to reflect issues around larger data saving:

Valet guarantees it will never fail to write to or read from the keychain unless canAccessKeychain() returns false. There are only a few cases that can lead to the keychain being inaccessible:

In this can canAccessKeychain indeed returns true, but the call immediately following it will fail. Here's an exaggerated test case showing it with a 10MB data set (excuse the forced unwrapping, just for illustrative purposes):

var testDict: [String: String] = [:]
        
for i in 0 ..< 100_000 {
    testDict["\(i)"] = "once upon a time there was a ghost named earl the ghost"
 }

let largeData = try! NSKeyedArchiver.archivedData(withRootObject: testDict, requiringSecureCoding: true)
print(largeData.count) // 10457973 bytes, or 10.4MB

let valet = Valet.sharedGroupValet(with: SharedGroupIdentifier(appIDPrefix: "---", nonEmptyGroup: "---")!, accessibility: .afterFirstUnlock)

// Just to show that we're in pretty much the optimal situation
if valet.canAccessKeychain() && UIApplication.shared.applicationState == .active && UIApplication.shared.isProtectedDataAvailable {
    let smallData = "foo".data(using: .utf8)!
    try! valet.setObject(smallData, forKey: "test123") // Success!
    try! valet.setObject(largeData, forKey: "test456") // Error: cannot access keychain
    print("Complete! But we often don't reach this point.")
}

(It may take a few tries to reproduce, but I see the same error that users are reporting. On main thread in this example if relevant, but occurs on background threads as well. Also my logs show the error occurring on the most up to date version of iOS 13 (13.7) as well as the most recent iOS 14 beta, so it doesn't seem to be limited to iOS 12. Also worth mentioning it happens in other libraries like Lockbox so it appears to be an internal Keychain issue, not specific to Valet, but still might be worth mentioning in that exceptions section.)

Again, 100% my dumb fault for saving things larger than I likely should be and not testing thoroughly enough on older devices, but thought it might be helpful to mention in case anyone else runs into it.

And thank you for the library!

[dfed]

Go figure! Thank you for the detailed report with sample code. I agree that the README language needs updating (or we need to surface better errors). I've tried saving a couple hundred kb to the keychain before, but admittedly I never tried saving something so large. Excited to play with this and see what I find 🙂

[christianselig]

Go figure! Thank you for the detailed report with sample code. I agree that the README language needs updating (or we need to surface better errors). I've tried saving a couple hundred kb to the keychain before, but admittedly I never tried saving something so large. Excited to play with this and see what I find 🙂

You sound like you have more reasonable expectations of the Keychain than me. 😄 Happy to help, thank you again for the library!

[dfed]

So I pulled this up in the debugger. Looks like we're getting errSecNotAvailable (or -25291) from the Keychain. The comment on this error is No keychain is available. You may need to restart your computer.

We've managed to break the keychain!

Looking into my console logs, I see the following:

ReportCrash Formulating fatal report for corpse[20895] securityd

Seems like we're literally crashing securityd, which is the keychain daemon. Still playing a bit. Will report back.

[dfed]

I was able to get crashes inserting a Data(count: 7_000_000) into the keychain on my iPhone 11 Pro Max. On an iPhone simulator, crashes started occurring when inserting a Data(count: 16_000_000). The maximum size of a keychain object may be hardware-dependent.

My sample code:

let valet = Valet.valet(with: Identifier(nonEmpty: "Test")!, accessibility: .afterFirstUnlock)

try! valet.removeAllObjects() // Remove values we may have previously put into the keychain
try! valet.setObject(Data(count: 7_000_000), forKey: "test")

Since we don't know the upper size limit of a keychain item, I think we should likely update our in-code documentation here. Throwing couldNotAccessKeychain in this case isn't fantastic, but it's also reasonable since we could not access the keychain? I'm thinking we'll want to update our setObject(_:forKey:) and setString(_:forKey:) documentation to add a line akin to: /// - Important: Inserting data larger than 5mb in size may sporadically fail. I'm not sure the README needs updating here, since the keychain is available... you're just crashing it 😂

Open to other approaches as well. How does the above sound to you?

[christianselig]

That sounds phenomenal to me! I would have been happy with even a "Hey doofus, don't try to store a ton of info in the Keychain, it's probably not going to work out" in the README, so this is even better. :)

I think just communicating in the README to any library consumers that canAccessKeychain might not be accurate/no guarantee if the data you're storage is large might be wise. Any specifics beyond that is just cake!

I'd lower than 5MB recommendation to even 1MB, I was able to make it crash on an iPhone 6 (not as often, but still sometimes) with a single megabyte. Might just be wise to mention the 1MB as a guideline and to inspect the size of data you're storing (as well as the device) if you're running into weird behavior.

[NickEntin]

The other option I see here is to set some lower bound where we know there are issues (maybe 1 MB or so) and call canAccessKeychain() when we get errSecNotAvailable back to see whether our "normal" access works. We can then throw .couldNotAccessKeychain or (a newly added) .dataExceedsSizeLimits depending on the result. This assumes that after we've crashed the keychain daemon it starts back up immediately.

That said, I think it's reasonable to update the documentation and say that large data blobs are not supported.

[dfed]

While we could add an error case, given how little we know about this error I worry about creating an explicit contract. I think it's best to add some "here be dragons" warnings and call it a day.

[KhaosT]

Dug a little bit around this and here are some findings:

As of iOS 13.7, there is no hard limit defined by Keychain Services around the size of individual item being saved in the Keychain. The experiment in #246 (comment) was caused by securityd (the daemon powering the Keychain Services) getting killed by system due to exceeding the memory limit (48MB) under memory pressure. (If you check console, I believe you can find log around securityd crossing the limit). It happens less on newer devices since they are equipped with larger RAMs.

Things get a little more interest when apps try to put data > 16MB into keychain. In that case, you'll get error -50, for invalid query.

KeychinSize[557]/1#4 LF=0 add Error Domain=NSOSStatusErrorDomain Code=-50 "trailing garbage after der decoded object for key query" UserInfo={NSDescription=trailing garbage after der decoded object for key query, NSUnderlyingError=0x100ea22b0 {Error Domain=com.apple.security.cfder.error Code=-1 "Unknown data encoding, expected CCDER_CONSTRUCTED_SET" UserInfo={NSDescription=Unknown data encoding, expected CCDER_CONSTRUCTED_SET}}}

It's pretty nice that Apple open sourced most of Keychain's implementation. After looking around for this error, it turned out that the Keychain APIs encodes client queries into DER format before sending it to the daemon, and on the daemon side, the decoding logic from Apple's corecrypto refuses to decode (in function ccder_decode_len_internal) payload larger than 16MB (length longer than 3 bytes) 😅

For the soft limit, the implementation considered a reasonable size for data to be around 4KB.

[dfed]

Great digging! Sounds like we should update our recommendation to 4K, and get less explicit about what'll happen if you exceed 🙂