Skip to content
This repository has been archived by the owner on Aug 24, 2019. It is now read-only.

Make the default security stronger #42

Closed
wants to merge 3 commits into from

Conversation

hashier
Copy link
Contributor

@hashier hashier commented Aug 9, 2013

In SSKeychain's doc it says for the AccessibilityType: If the value is NULL (the default), the Keychain default will be used.
But I couldn't find in the apple docs what the default is and since this is a wrapper to make it easy for the user/developer I strongly think we should make it as save as possible and Apple recommends as well to always set kSecAttrAccessibleWhenUnlocked if possible.

@soffes
Copy link
Owner

soffes commented Aug 9, 2013

Love it. Could you update the documentation in the header?

@hashier
Copy link
Contributor Author

hashier commented Aug 9, 2013

Can I somehow change the Commits and Files Changed? Or just create a new push request?

@soffes
Copy link
Owner

soffes commented Aug 9, 2013

Just push to that same branch on your fork and it will update

@calebd
Copy link
Collaborator

calebd commented Aug 9, 2013

Setting the default to kSecAttrAccessibleWhenUnlocked is a breaking change that might make apps stop working. Basically the entire keychain would be inaccessible to an app that runs while the device is locked. I propose setting the default to kSecAttrAccessibleAfterFirstUnlock which will leave access open to the app as long as the user has unlocked the device at least once.

@hashier
Copy link
Contributor Author

hashier commented Aug 9, 2013

Yes you're right but it will only break on password updates and I don't think there are so many apps that actually need access to the keychain while the device is locked (or at least not yet with iOS6, so maybe it's a good point to change it before the iOS7 release).

Maybe add an #error or #warning? So if a developer updates he/she will definitely see that the default behaviour has changed and he/she has to comment it out? (I know kind of an ugly way)

But I don't think it's a good idea to have any other value as the default value. If you do not want the safest option you should knowingly set it so.

@calebd
Copy link
Collaborator

calebd commented Aug 9, 2013

It will only break on updates

What do you mean? AFAIK you wouldn't be able to read from or write to the keychain if the device is locked.

And plenty of apps make use of backgrounding, even if it's just a long task handler. If the device locked and the app needs keychain data still it would be unreadable.

@hashier
Copy link
Contributor Author

hashier commented Aug 9, 2013

Only if you write a new password to the keychain since the accessibility is saved with the password in the keychain so this only effects newly written passwords therefore my ("ugly") idea with maybe set a #warning to make sure ever developer sees this new behaviour.

And with iOS6 only few apps (like SIP, audio,..) are even allowed to run in background so everything else needs to unlock the phone anyway before the app can do anything. (Or am I missing anything obvious).

I definitely see your point but as said before I think that the default behaviour should always be as safe as possible even though it might mean some inconvenience.
Further I think that people who need background keychain access should have set it before anyway since I couldn't find any default value/behaviour so it was never certain what it was. Maybe it is kSecAttrAccessibleAfterFirstUnlock and just imagine Apple set's it to kSecAttrAccessibleWhenUnlocked then it's the same problem for all developers.

I think not having the most secure option as a default and put everyone (old and new) at risk is worse than maybe break some old apps that never should have relied on certain unsafe default options anyway. If you want to loosen up security you should do it knowingly.

@hashier
Copy link
Contributor Author

hashier commented Aug 29, 2013

poke 3 weeks and we are getting closer to iOS7 release.

I want to bring this up once again and say that I think it's the right think to do it now, because iOS6 hasn't had many apps anyway that could run in the background while the device was locked. This will change with iOS7.

And since we don't even know what the default behaviour was all along we should settle on one, preferable the most secure one (:

@thesleeper13
Copy link

What frequency use? Or is that National Security Crap.

@soffes
Copy link
Owner

soffes commented Jun 19, 2015

After thinking on this, I think it's best for everyone to set their own since it varies a lot from app to app. Sorry.

@soffes soffes closed this Jun 19, 2015
soffes pushed a commit that referenced this pull request Mar 11, 2016
Based on @hashier's pull request in
#42 this better documents that
`AccessibilityType` really should be set. It does not change any
defaults but only updates documentation accordingly. So it is 100%
backwards compatible but should help developers to make an informed
decosion about how to store items in the keychain.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants