Richard Newman [:rnewman] Reporter
	Description • 9 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.

	Richard Newman [:rnewman] Reporter
	Comment 1 • 9 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.

	Emily Toop (:fluffyemily)
	Comment 2 • 9 years ago

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

	Emily Toop (:fluffyemily)
	Comment 3 • 9 years ago

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

	Emily Toop (:fluffyemily)
	Comment 4 • 9 years ago

Able to repro nil key on simulator though (also 6S)

	Stefan Arentz | :st3fan | ⏰ EST | he/him
	Comment 5 • 9 years ago

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?

	Richard Newman [:rnewman] Reporter
	Comment 6 • 9 years ago

Will be solved by Bug 1223400.

	Andrew McCreight [:mccr8]
	Comment 7 • 9 years ago

Richard, should this get closed now? Was it ever a real security issue or just a sec-audit sort of thing?

	Richard Newman [:rnewman] Reporter
	Comment 8 • 9 years ago

Yeah, not a sec bug, but it's still a valid bug, so let's leave it open.