SwiftKeychainWrapper is reporting that a key always exists when it does not

NEW
Unassigned

Status

()

Firefox for iOS
Data Storage
2 years ago
2 years ago

People

(Reporter: rnewman, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [simulator])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
* Reset the contents of your simulator.
* Check out

  rnewman/debug-log-key

* Run.
* Open bugzilla.mozilla.org.

Expected:
* A key is logged.

Actual:
* nil is logged:

2015-11-29 21:16:14.003 [Debug] [BrowserDB.swift:37] init(filename:secretKey:files:) > Initializing BrowserDB: logins.db.
2015-11-29 21:16:14.004 [Debug] [BrowserDB.swift:46] init(filename:secretKey:files:) > Key is nil



* Now check out

  rnewman/ensure-key-exists

You'll see different behavior; we find no key in the keychain, create one, and then fail to open the existing -- unencrypted! -- database.

[BrowserDB.swift:37] init(filename:secretKey:files:) > Initializing BrowserDB: logins.db.
[BrowserDB.swift:46] init(filename:secretKey:files:) > Key is Optional("4IFLvfEOK4baFItOsFk8joIeZHBCfZw2w4gKf7BuIkGwd2iInmUn9JCtsNt+6jg980z+zls2zSLoAL/clalrmb1cN/ku0FMyqzr9VBECWGlOR7Udx5hfiVhZoCE6nBSMfQs4uBaUj7cqyaYOYlUpHAZAL0T77Cy2BaM0rgx5UjCYIqQOZ58d7bosICPSOvz9S9RCvWfUawimhJnK/cUsK8PdND3mcmAQQlGIUTTV/++1J86IKmbaNulVZpS4GwPGKvnZvbxkQUpt1Ad4KgwbJIP98dIIpXry3ozJC8JjOtF8BmtVGhWozOjGQS7cQEQgm9WhCvnEH9Q0JrZ2wFlRrQ==")
[BrowserDB.swift:48] init(filename:secretKey:files:) > Creating db: /Users/rnewman/Library/Developer/CoreSimulator/Devices/522A062C-9D9F-4011-BC6C-972FA475A849/data/Containers/Shared/AppGroup/A94A16A8-DB93-49AB-A2A3-DB0D1E9A5475/profile.profile/logins.db with secret = Optional("4IFLvfEOK4baFItOsFk8joIeZHBCfZw2w4gKf7BuIkGwd2iInmUn9JCtsNt+6jg980z+zls2zSLoAL/clalrmb1cN/ku0FMyqzr9VBECWGlOR7Udx5hfiVhZoCE6nBSMfQs4uBaUj7cqyaYOYlUpHAZAL0T77Cy2BaM0rgx5UjCYIqQOZ58d7bosICPSOvz9S9RCvWfUawimhJnK/cUsK8PdND3mcmAQQlGIUTTV/++1J86IKmbaNulVZpS4GwPGKvnZvbxkQUpt1Ad4KgwbJIP98dIIpXry3ozJC8JjOtF8BmtVGhWozOjGQS7cQEQgm9WhCvnEH9Q0JrZ2wFlRrQ==")
[SwiftData.swift:83] getSharedConnection() > >>> Creating shared SQLiteDBConnection for /Users/rnewman/Library/Developer/CoreSimulator/Devices/522A062C-9D9F-4011-BC6C-972FA475A849/data/Containers/Shared/AppGroup/A94A16A8-DB93-49AB-A2A3-DB0D1E9A5475/profile.profile/logins.db on thread <NSThread: 0x79e590d0>{number = 1, name = main}.
[SwiftData.swift:318] init(filename:flags:key:prevKey:) > Opening connection to /Users/rnewman/Library/Developer/CoreSimulator/Devices/522A062C-9D9F-4011-BC6C-972FA475A849/data/Containers/Shared/AppGroup/A94A16A8-DB93-49AB-A2A3-DB0D1E9A5475/profile.profile/logins.db.
[SwiftData.swift:513] executeQuery(_:factory:withArgs:) > SQL error: File opened that is not a database file file is encrypted or is not a database.
[SwiftData.swift:445] closeCustomConnection() > Closing custom connection for /Users/rnewman/Library/Developer/CoreSimulator/Devices/522A062C-9D9F-4011-BC6C-972FA475A849/data/Containers/Shared/AppGroup/A94A16A8-DB93-49AB-A2A3-DB0D1E9A5475/profile.profile/logins.db on <NSThread: 0x79e590d0>{number = 1, name = main}.
[SwiftData.swift:454] closeCustomConnection() > Closed /Users/rnewman/Library/Developer/CoreSimulator/Devices/522A062C-9D9F-4011-BC6C-972FA475A849/data/Containers/Shared/AppGroup/A94A16A8-DB93-49AB-A2A3-DB0D1E9A5475/profile.profile/logins.db.
[SwiftData.swift:513] executeQuery(_:factory:withArgs:) > SQL error: File opened that is not a database file file is encrypted or is not a database.




The issue appears to be in SwiftKeychainWrapper, which is reporting that it has a value when it does not. We can work around it, granted.


I am concerned that this affects existing builds, and that logins.db in the wild are in cleartext.

It's hard to tell, because logins.db lives in the shared app container, and thus isn't copied when extracting the container in Xcode.

I have not yet verified whether this behavior occurs on device.
(Reporter)

Comment 1

2 years ago
See also

https://github.com/jrendel/SwiftKeychainWrapper/issues/21

And this is fixed in the maintained version:

-    /// :param: keyName The key to check for.
-    /// :returns: True if a value exists for the key. False otherwise.
+    /// - parameter keyName: The key to check for.
+    /// - returns: True if a value exists for the key. False otherwise.
     public class func hasValueForKey(keyName: String) -> Bool {
-        var keychainData: NSData? = self.dataForKey(keyName)
-        if let data = keychainData {
+        let keychainData: NSData? = self.dataForKey(keyName)
+        if keychainData != nil {

so this will be part-fixed by Bug 1223400. Not a complete fix, because we need to address migration.
Depends on: 1223400
Flags: needinfo?(etoop)
PRAGMA rekey changes the encryption key for a SQLCipher database (https://www.zetetic.net/sqlcipher/sqlcipher-api/). From reading the SQLite docs, it looks like if you provide a nil for the existing key then that will encrypt an unencrypted DB (https://www.sqlite.org/see/doc/trunk/www/readme.wiki)

Hold that. 

SQLCipher have provided a migration guide - https://discuss.zetetic.net/t/how-to-encrypt-a-plaintext-sqlite-database-to-use-sqlcipher-and-avoid-file-is-encrypted-or-is-not-a-database-errors/868
Flags: needinfo?(etoop)
Created attachment 8693508 [details]
Output from Testing of issue

:rnewman - I tried to replicate the issue on my 6S and wasn't able to reproduce the nil key issue at all. I was able to reproduce the corrupt DB though.
Attachment #8693508 - Flags: feedback+
Able to repro nil key on simulator though (also 6S)
The simulator behaves in a different way. Specifically, if the SecItem contains an access group attribute (which we use) the Add operation fails on the Simulator with errSecNoAccessForItem.

If you look at Apple's GenericKeyChain sample code, they special case running on the simulator and remove the kSecAttrAccessGroup attribute for every operation. Otherwise they fail. SwiftKeyChainWrapper does no such check.

Is that happening?
(Reporter)

Comment 6

2 years ago
Will be solved by Bug 1223400.
tracking-fxios: ? → 2.0+
(Reporter)

Updated

2 years ago
tracking-fxios: 2.0+ → ---
Whiteboard: [simulator]
Richard, should this get closed now? Was it ever a real security issue or just a sec-audit sort of thing?
Flags: needinfo?(rnewman)
(Reporter)

Comment 8

2 years ago
Yeah, not a sec bug, but it's still a valid bug, so let's leave it open.
Flags: needinfo?(rnewman)
Group: firefox-core-security
You need to log in before you can comment on or make changes to this bug.