Closed Bug 1147071 Opened 9 years ago Closed 9 years ago

Use encrypted database storage for passwords

Categories

(Firefox for iOS :: Data Storage, defect)

Product:
Firefox for iOS
Component:
Data Storage
Platform:
All
iOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
fxios + ---

People

(Reporter: st3fan, Assigned: wesj)

References

Details

(Whiteboard: noteworthy)

Attachments

(1 file, 1 obsolete file)

PR https://github.com/mozilla/firefox-ios/pull/470
47 bytes, text/x-github-pull-request
rnewman
: review+
Details | Review 
We currently store plain text unencrypted passwords in our browser.db file.

We should encrypt these instead.

This means plaintext passwords possibly travel to iCloud and most certainly to local backups to Mac or PC that people do with iTunes.

Two possible options:

* Store the passwords in the iOS Keychain
* Store the passwords in browser.db but encrypt them with a key that is stored in the keychain

We need to find out what this means when people backup and restore their devices.

There are a number of high profile security researchers focussing on iOS and also on third party browsers. We need to get this right for release or it will result in bad press.
tracking-fennec: ? → +
OS: Mac OS X → iOS
Hardware: x86 → All
Summary: Password should not be stored in plain text → Use encrypted database storage for passwords
One option here is to use the same SQLite.swift project that we use for Reading List. It has support for SQLCipher, which means that we could generate a random encryption key and store it in the Keychain. And then use that key to unlock/encrypt/decrypt the passwords database.
We could just do the same manually as well, or just move the sqlite3.dylib from the project into our own and use it with SwiftData. I don't want two sqlite's touching browser.db at the same time (and I'm kinda annoyed that all the Reader stuff is in its own DB making it harder to do any joins with other tables).
Actually, looking at how to do AES256 encryption (just a few lines with an NSData object), I think I'd prefer to keep this really localized. i.e. just encrypt the usernames and passwords. Being able to open the database locally and look through it/run queries on it has proven super useful. Can we do that (easily) sqlitecipher?
Assignee: nobody → wjohnston
This does the encryption in the table layer. Adds a String extension to do it.
Attachment #8601912 - Flags: review?(sarentz)
I used a key randomly generated using SecRandomCopyBytes and stored it in keychain. Encryption is being done with CCCrypt. I realize as I write this that I use the words AES256 in a few places, but this is AES128. Happy to hear if I'm using them wrong :)
I think if we actually use AES128 then we should say so in the code. Otherwise it will be a hot item in case security researchers look at this code. (Haha they think they use AES 256 but they are not)

The kCCOptionECBMode and empty IV worries me. I think we need to loop in someone who can say something about this. My gut feeling says that we need to use AES in CBC or CTR mode. Otherwise it becomes really easy to brute force passwords with a dictionary.
I think it is a better idea to use the same encrypted record format that Sync does. This is documented at 

https://docs.services.mozilla.com/sync/storageformat5.html#sync-storageformat5

It uses AES256 in CBC mode. With a random IV. And a HMAC to prove the authenticity of the record. I think that is a pretty common scheme to do per-record encryption.
Flags: needinfo?(rnewman)
Flags: needinfo?(nalexander)
We should definitely reuse an existing crypto scheme, even if it involves 'userspace' crypto. Everything Stefan mentions in Comment 6 makes sense.

In theory we have less need of HMAC locally, but there's no harm in doing so.

Reading Bug 638862 would be well worth your time, just for context -- Sync's record format isn't quite perfect.
Flags: needinfo?(rnewman)
If we were to use SQLCipher, I would note that it should get some very careful review, just as if we were landing it ourselves.

Another thing to think about: whatever storage mechanism we use needs to be designed for use by Sync.

That means supporting three-way merges (Bug 1098501), and being rebased on top of Bug 1160399.

Some additional context: Bug 1118549.
I see no particular reason to follow Sync's lead.  It's not a bad way to go, but it's not compelling to me.

I'd prefer to use the Keychain for this type of sensitive storage.  We could have our PWs schema not actually store the PW, but merely some identifier to it in the keychain.  (We're doing this for the Firefox Account state storage.)  I think we could manage the metadata updates reasonably well.  I'm not certain we could support searching by usernames or sites all that well, but I don't know what /exactly/ is required here.
Flags: needinfo?(nalexander)
I updated this to be closer to the sync spec. i.e. we use a random IV, CBC mode, and do some hashing/checks. Also only encrypts the password now (since we can't know/guess what the username is anymore for deletes). We could not use a random IV, and instead use something like the username + hostname + secret as a key if we wanted to still store that (and still use a random IV for the passwords). That would keep it from being obvious which accounts had the same username which might be nice?

The key generation still doesn't match syncs. 1.) We don't have a username to use. 2.) We don't SHA256 the encryption key or the HMAC key. Maybe its worth doing that? Since our password is random, I couldn't see a strong reason for it (but I certainly haven't finished reading everything listed above).

The hashing of the encoded data is in here, but I think its probably pretty useless in this context as well (it wasn't much to add it).

I don't hate the keychain idea either (I like it). I just want this to move forward. If you think its more likely to land that way, speak up.
(In reply to Wesley Johnston (:wesj) from comment #11)

> The key generation still doesn't match syncs. 1.) We don't have a username
> to use. 2.) We don't SHA256 the encryption key or the HMAC key. Maybe its
> worth doing that? Since our password is random, I couldn't see a strong
> reason for it (but I certainly haven't finished reading everything listed
> above).

Sync's record-level crypto uses random keys; no username needed at that level. Same is true here, I expect. Random keys, stored in the keychain, used with an IV and HMAC.

I like the idea of storage in the Keychain for simplicity, but it adds a kind of distributed transaction: when you delete a password, it needs to be deleted in two places; when you do a lookup, it can fail at either step, and we'll be doing two queries every time we need password content.

Can the user delete stuff from the iOS keychain from underneath us, e.g., by syncing it to iCloud and having changes sync back down?
Why can't we encrypt the whole thing including usernames and hostnames? I bet a lookup on a fully encrypted database would still take milliseconds.

What does the "We could not use a random IV, and instead use something like the username + hostname + secret as a key if we wanted to still store that (and still use a random IV for the passwords)." bit mean?
Long term, I'd like to be able to delete a username+hostname combination. "Remove this login for this site". If they're encrypted with individual random keys, that gets weighty. i.e. We'd have to query for the entire DB, then search through it for entries that looked like what we wanted, and then delete on those (DELETE where hostname = encryptedHostname AND username = encryptedUsername). If they're encrypted with something non-random like a hostname+username+secrectKey though, then this works. I'll put that together and encrypt those fields.
(In reply to Wesley Johnston (:wesj) from comment #14)
> Long term, I'd like to be able to delete a username+hostname combination.
> "Remove this login for this site". If they're encrypted with individual
> random keys, that gets weighty. i.e. We'd have to query for the entire DB,
> then search through it for entries that looked like what we wanted, and then
> delete on those (DELETE where hostname = encryptedHostname AND username =
> encryptedUsername). If they're encrypted with something non-random like a
> hostname+username+secrectKey though, then this works. I'll put that together
> and encrypt those fields.

This is the perfect use-case for SQLCipher. You get a fully encrypted SQLite database that you can query like any other database. We don't have to keep certain things public. Or jump though hoops to get an index. It would just work. And still be fully encrypted.

I'm still strongly in favor us using SQLCipher instead of implementing our own and making sacrifices like keeping username+hostname clear text.
We don't have to make the sacrifice, like I said in the comment above.
(In reply to Stefan Arentz [:st3fan] from comment #15)
> (In reply to Wesley Johnston (:wesj) from comment #14)
> > Long term, I'd like to be able to delete a username+hostname combination.
> > "Remove this login for this site". If they're encrypted with individual
> > random keys, that gets weighty. i.e. We'd have to query for the entire DB,
> > then search through it for entries that looked like what we wanted, and then
> > delete on those (DELETE where hostname = encryptedHostname AND username =
> > encryptedUsername). If they're encrypted with something non-random like a
> > hostname+username+secrectKey though, then this works. I'll put that together
> > and encrypt those fields.
> 
> This is the perfect use-case for SQLCipher. You get a fully encrypted SQLite
> database that you can query like any other database. We don't have to keep
> certain things public. Or jump though hoops to get an index. It would just
> work. And still be fully encrypted.
> 
> I'm still strongly in favor us using SQLCipher instead of implementing our
> own and making sacrifices like keeping username+hostname clear text.

As rnewman pointed out, SQLCipher is a non-trivial implementation of non-trivial crypto.  We'd need to familiarize ourselves with, and potentially audit, SQLCipher.  Can you provide this justification of SQLCipher, or another compelling justification?
Updated again. Now encrypts usernames and hostnames. Removed no-encryption options. And made things handle failure much better.

Encrypting hostnames didn't go as well as I wanted. We query the database with ONLY the hostname available to us. To make that work, for now I'm just encrypting them with their own name as an IV. That means every entry with the same hostname will have the same encryption. That makes me start coming around on SqlCypher... I did use a separate (runtime-randomly-generated) secret for each type of data here because I worry the overlapping data would make it easier to find one.
(In reply to Nick Alexander :nalexander from comment #17)
> As rnewman pointed out, SQLCipher is a non-trivial implementation of
> non-trivial crypto.  We'd need to familiarize ourselves with, and
> potentially audit, SQLCipher.  Can you provide this justification of
> SQLCipher, or another compelling justification?

Their design document looks pretty decent https://www.zetetic.net/sqlcipher/design/

If we have doubts about this then we can let one of our in-house crypto people look at it.

There is also this:

https://www.zetetic.net/blog/2012/3/20/sqlcipher-at-the-blackhateu-elcomsoft-presentation.html

Full presentation here: https://www.elcomsoft.com/WP/BH-EU-2012.pdf

In which a SQLCipher-powered password manager for iOS came out as one of the strongest. All the others are like ours: best effort, home made crypto. Crypto is really hard to do right. I just don't know if we did the right thing. Even with the latest changes I have questions about our implementation. Number one rule for crypto is: do not reinvent. Use something existing and proven.

(The presentation also has an interesting note about the Apple Keychain storing SHA-1 hashes of data items for fast lookups. Not sure if this is still the case for iOS8 but somethign we should think about since it would allow easy brute forcing of plaintext values stored in the keychain.)
Flags: needinfo?(nalexander)
(In reply to Stefan Arentz [:st3fan] from comment #19)
> (In reply to Nick Alexander :nalexander from comment #17)
> > As rnewman pointed out, SQLCipher is a non-trivial implementation of
> > non-trivial crypto.  We'd need to familiarize ourselves with, and
> > potentially audit, SQLCipher.  Can you provide this justification of
> > SQLCipher, or another compelling justification?
> 
> Their design document looks pretty decent
> https://www.zetetic.net/sqlcipher/design/

Reading this suggests we would replace our existing SQLite implementation with SQLCipher.  That would mean we'd be using a non-standard SQLite implementation throughout the App.  I am assuming shipping two versions of SQLite is even worse than shipping one non-standard version.

> If we have doubts about this then we can let one of our in-house crypto
> people look at it.

Good idea.

> There is also this:
> 
> https://www.zetetic.net/blog/2012/3/20/sqlcipher-at-the-blackhateu-elcomsoft-
> presentation.html
> 
> Full presentation here: https://www.elcomsoft.com/WP/BH-EU-2012.pdf
> 
> In which a SQLCipher-powered password manager for iOS came out as one of the
> strongest. All the others are like ours: best effort, home made crypto.
> Crypto is really hard to do right. I just don't know if we did the right
> thing. Even with the latest changes I have questions about our
> implementation. Number one rule for crypto is: do not reinvent. Use
> something existing and proven.

In general, I agree.

> (The presentation also has an interesting note about the Apple Keychain
> storing SHA-1 hashes of data items for fast lookups. Not sure if this is
> still the case for iOS8 but somethign we should think about since it would
> allow easy brute forcing of plaintext values stored in the keychain.)

I suggest we do the following:

* investigate whether SQLCipher runs afoul of any App store restrictions;
* request an opinion from security team.

I have no particular skin in this game; I'm happy to run SQLCipher if sec-team concurs.

st3fan, wesj, rnewman: reasonable?
Flags: needinfo?(wjohnston)
Flags: needinfo?(sarentz)
Flags: needinfo?(rnewman)
Flags: needinfo?(nalexander)
(In reply to Nick Alexander :nalexander from comment #20)

> Reading this suggests we would replace our existing SQLite implementation
> with SQLCipher.  That would mean we'd be using a non-standard SQLite
> implementation throughout the App.  I am assuming shipping two versions of
> SQLite is even worse than shipping one non-standard version.

Plus sides: we can encrypt everything, not just passwords; no maintenance of storage crypto code; in theory we can run a newer sqlite than Apple provides (and with different options: e.g., adding free-text search if it's missing).

Downsides: sqlite fork (with commercial support!); larger app size.

This worries me (from <https://www.zetetic.net/sqlcipher/ios-tutorial/>):

---
All applications that make use of cryptography, including those that use SQLCipher or iOS internal libraries like CommonCrypto and Keychain, must provide documentation to Apple that demonstrates review by the Department of Commerce (DOC) Bureau of Industry and Security (BIS) and classification of the application a mass market encryption item.
---
Flags: needinfo?(rnewman)
My main concern here is that these databases will become essentially impossible to debug. Even if we wanted to look at one, getting the key for it would be (and should be) prohibitively difficult. That said, I should have put this data in a separate signons file anyway, and the number of times we ask someone to send us our signons.db is rare.
Flags: needinfo?(wjohnston)
Yeah, that worries me too -- particularly because getting the syncing part right sometimes involves staring confusedly at a sqlite3 console!

Presumably sqlcipher yields standard sqlite DBs if created without a key? If so, perhaps we can figure out an arrangement of on/off that makes sense.

Agreed on using a separate DB file.
(In reply to Wesley Johnston (:wesj) from comment #22)
> My main concern here is that these databases will become essentially
> impossible to debug. Even if we wanted to look at one, getting the key for
> it would be (and should be) prohibitively difficult. That said, I should
> have put this data in a separate signons file anyway, and the number of
> times we ask someone to send us our signons.db is rare.

A quibble: Sync calls the engine passwords.  Our password research suggests users think of them as "logins".  Whatever they are, nobody refers to them as signons, so we shouldn't start.
Desktop calls the db 'signons.sqlite'. Variety is the spice of life.

(I agree, btw.)
(In reply to Richard Newman [:rnewman] from comment #25)
> Desktop calls the db 'signons.sqlite'. Variety is the spice of life.

Truly.
Heh. I just meant, they probably shouldn't be in the same db as history, visits, and favicons like they are now. We don't join against them, and we could at least then perhaps encrypting everything... sometimes. But I know some people who would like everything encrypted too! I'll look and see how the cipher stuff behaves if there's no key.
(In reply to Richard Newman [:rnewman] from comment #21)

> ---
> All applications that make use of cryptography, including those that use
> SQLCipher or iOS internal libraries like CommonCrypto and Keychain, must
> provide documentation to Apple that demonstrates review by the Department of
> Commerce (DOC) Bureau of Industry and Security (BIS) and classification of
> the application a mass market encryption item.
> ---

Legal already signed off on our app and the crypto it uses even though we use custom crypto code. We should ask them to be sure but I highly doubt this is an issue. I think we are except from all this because we are an open source app and we already did the paper work.
(In reply to Nick Alexander :nalexander from comment #20)
> I am assuming shipping two versions of
> SQLite is even worse than shipping one non-standard version.

We already do via ReadingList, which has its own SQLite.framework from the https://github.com/stephencelis/SQLite.swift project. That project also has SQLCipher support. (Which is not included in our app now because RL does not need it)
Flags: needinfo?(sarentz)
I like the idea of encrypting everything by default. Wish we did that on Desktop too.

We have to find out what that means wrt backups though. We need to make sure the keychain item with the master key travels to both iCloud and local iTunes backups. I know that the latter only happens if you enable encrypted backups.
This encrypts BrowserDB. I was suprised to see the SQlite.swift actually just uses the sdk sqlite. Its instructions read like it can magically find the sqlcipher one if you have it, but I haven't seen how that would work. Right now I assume its still using the SDK sql. I haked the project files quite a bit to get this working. Need to clean them back up.

I also had to fix up our error handling quite a bit in there to handle the upgrade or downgrade case. In both cases this just tosses the old db and creates the new one. I'm looking into at least forcing an upgrade from no-password to the new one (sqlite3_rekey should do that for us...)

On that same line, we already had potentials to fail opening/creating the db and now there are more. I looked into making the BrowserDB initializer fallible (I think I did make the SwiftData one fallible here). Doing so becomes... hairy, but not impossible (thanks Swift!) I ran through it for awhile and then heard bnicholson in my head saying "Why/How would we let you continue if things were broken this badly?".

You guys have feelings on how long we should limp along with no history/bookmarks/etc? We could easily let you browse in the hope that next time you restarted we could somehow fix the error. Regardless, maybe its best as a follow up.
Attachment #8606569 - Flags: feedback?(sarentz)
Attachment #8606569 - Flags: feedback?(rnewman)
Comment on attachment 8606569 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/470

Updated with some tests as well. This supports upgrades as well.

I spent too long chasing some problems (open connections?) in the tests. Punting a bit for now.
Attachment #8606569 - Flags: review?(sarentz)
Attachment #8606569 - Flags: review?(rnewman)
Attachment #8606569 - Flags: feedback?(sarentz)
Attachment #8606569 - Flags: feedback?(rnewman)
(In reply to Wesley Johnston (:wesj) from comment #31)
> Created attachment 8606569 [details] [review]
> PR https://github.com/mozilla/firefox-ios/pull/470
> 
> This encrypts BrowserDB. I was suprised to see the SQlite.swift actually
> just uses the sdk sqlite. Its instructions read like it can magically find
> the sqlcipher one if you have it, but I haven't seen how that would work.
> Right now I assume its still using the SDK sql. I haked the project files
> quite a bit to get this working. Need to clean them back up.

I'll take a look too. We may want to include a pre-build SQLCipher library in our project instead of building it as part of our application. Because I had problems with it earlier, I actually forked SQLite.swift and just disabled SQLCipher for now.

> I also had to fix up our error handling quite a bit in there to handle the
> upgrade or downgrade case. In both cases this just tosses the old db and
> creates the new one. I'm looking into at least forcing an upgrade from
> no-password to the new one (sqlite3_rekey should do that for us...)

I think at this pre-v1.0 stage it is fine to just delete the file and not worry about a migration that we would only do now during development.

My bigger worry is how do we make sure old unencrypted data is not traveling to iCloud backups.
I left a bunch of questions on the PR. The implementation looks pretty sane (modulo the optional db), but there are some hairy lifecycle questions that need to be answered before I'd want this to apply to browser.db.

Depending on the answers, this might be OK for passwords.db alone, and we'd simply accept e.g., the inability to sync passwords in the background.
Status: NEW → ASSIGNED
Attachment #8606569 - Flags: review?(rnewman) → feedback+
Comment on attachment 8606569 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/470

Updated. I actually had some code that wasn't appearing in here before either. This supports encrypting/decrypting an existing database provided you can provide the previous and the new keys.

I also updated the Keychain code. There's a PR for it at:
https://github.com/jrendel/SwiftKeychainWrapper/pull/15
Attachment #8606569 - Flags: review?(sarentz)
Attachment #8606569 - Flags: review?(rnewman)
Attachment #8606569 - Flags: feedback+
Apple just released https://www.apple.com/business/docs/iOS_Security_Guide.pdf which has a bunch of hints about keychain, icloud and backups. We should probably all read the relevant sections. I'm trying to catch up on how all of this works.
I talked to Stefan a bit right now about this. I got a bit distracted last week with iCloud backups. AFAIK from the reading the above, iCloud backs up your keychain using a key based on your device UID, meaning it can't automatically restore the keychain to any device except the one that created it. You can mark certain values as sync-able...  but I think that's outside scope for here.

For this bug we'll focus on just making this locally encrypted/decryptable. If you lose you keychain somehow (or if you back things up, but don't encrypt the backup so that your keychain isn't backed up), we're just going to toss this data next run. If you want a "backup" of passwords, FirefoxAccounts is the way to go.

I've got this using its own separate database now. In accordance with our "Don't bother with updates yet" plan, I don't clean up the old db. Just make a new one.AFAIK from my reading, we're ready.
Attachment #8606569 - Flags: review?(sarentz)
Wes, what is the status of https://github.com/mozilla/firefox-ios/pull/425 .. is that made obsolete with the sqlcipher one?
Flags: needinfo?(wjohnston)
I thought I replied to this, but yeah, its basically obsolete. I wanted to keep it around just in case.
Flags: needinfo?(wjohnston)
Attachment #8601912 - Flags: review?(sarentz)
Attachment #8601912 - Attachment is obsolete: true
The XCode server keeps trying to link x86 and x64 here and failing. I can't find any reason except that some of our Frameworks aren't being clobbered between builds maybe? I've tried making sqlcipher build a fat binary during debug builds, but since it depends on a few other projects that also fails. We'd have to fork them all and make them all fat binaries for that to work.

st3fan said he'd look at the server for me.
Flags: needinfo?(sarentz)
I don't think this is a server issue. I can't get this to compile locally either.

What I did:

 1263  hub clone git@github.com:mozilla/firefox-ios.git
 1264  cd firefox-ios
 1265  hub checkout https://github.com/mozilla/firefox-ios/pull/470
 1266  ./checkout.sh
 1267  xcnuke
 1268  open Client.xcodeproj

The first thing I see when I open the project is this:

  https://www.dropbox.com/s/9rcm8krcg3a2rh4/Screenshot%202015-06-09%2010.11.46.png?dl=0

Not sure why this is happening. Client is most certainly not a OS X app.

When I change that to for example the simulator, I get build errors like:

Utils/KeychainCache.swift:8:19: error: module 'XCGLogger' has no member named 'defaultInstance'
Utils/Extensions/HexExtensions.swift:10:16: error: use of unresolved identifier 'base16DecodeToData'

I don't really understand why this is happening.
Flags: needinfo?(sarentz)
Ha! A device build has no errors.

Do you get the same results locally on a simulator build?
Flags: needinfo?(wjohnston)
Like I said above, the code that's up there right now was an attempt to build sqlcipher as a fat binary. I saw some problems with other libraries when I did that. Reverted now. Try again?

It builds fine for me on the simulator or device.
Flags: needinfo?(wjohnston) → needinfo?(sarentz)
Yup. Builds fine on simulator. Investigating now why this fails on the server.
Flags: needinfo?(sarentz)
Landed a rebase of this pull request with a few build changes to make the buildbots happy:

https://github.com/mozilla/firefox-ios/pull/618
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 8606569 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/470

Removing myself from review. Great job landing this.
Attachment #8606569 - Flags: review?(sarentz)
Attachment #8606569 - Flags: review?(rnewman) → review+
tracking-fennec: + → ---
Whiteboard: noteworthy
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: