Don't keep the sqlCipher passphrase in memory longer than necessary

[groue]

This PR is a replacement for #599. cc @michaelkirk-signal

The way to set the passphrase of an encrypted SQLCipher database has changed. The old way is still supported, but deprecated:

// Deprecated
var config = Configuration()
config.passphrase = "secret"
let dbQueue = try DatabaseQueue(path: ..., configuration: config)

One now sets the passphrase in the database preparation function:

// New
var config = Configuration()
config.prepareDatabase = { db in
    try db.usePassphrase("secret")
}
let dbQueue = try DatabaseQueue(path: ..., configuration: config)

This new technique allows the most demanding users to manage the security of the passphrase at the lowest possible level, the sqlite3_key C function:

// Some security experts will appreciate this
var config = Configuration()
config.prepareDatabase = { db in
    ... // Carefully load passphrase bytes
    let code = sqlite3_key(db.sqliteConnection, /* passphrase bytes */)
    ... // Carefully dispose passphrase bytes
    guard code == SQLITE_OK else {
        throw DatabaseError(resultCode: code, message: db.lastErrorMessage)
    }
}
let dbQueue = try DatabaseQueue(path: ..., configuration: config)

This pull request is not complete. The change(passphrase:) method has to be deprecated, too:

try dbQueue.change(passphrase: "newSecret")

It has to be deprecated because it stores the new password in memory, for the remaining lifetime of the database queue or pool. And it does not allow demanding users to change the passphrase at the lowest possible level.

I'm facing a few difficulties there. Details will come.

[groue]

How to run SQLCipher tests from Xcode:

• Clone GRDB
• cd Tests/CocoaPods/SQLCipher4
• pod update
• Open Tests/CocoaPods/SQLCipher4/GRDBTests.xcworkspace
• Run the GRDBTests test scheme

And from the command line:

• make test_framework_SQLCipher4

[groue]

The change(passphrase:) method has to be deprecated, too. [...] I'm facing a few difficulties there.

#603 aims at solving those difficulties. It waits for it to be merged. This pull request has been rebased on top of it.

Changing the passphrase of a database queue should soon look like:

// Deprecated
try dbQueue.change(passphrase: "newSecret")

// New
try dbQueue.inDatabase { db in
    try db.changePassphrase("newSecret")
}

And for a database pool:

// Deprecated
try dbPool.change(passphrase: "newSecret")

// New: use a barrier so that no read can happen
// during the modification of the passphrase:
try dbPool.barrierWriteWithoutTransaction { db in
    try db.changePassphrase("newSecret")
    // All readers should now use the new passphrase
    dbPool.invalidateReadOnlyConnections()
}

Those new techniques allow the most demanding users to manage the security of the passphrase at the lowest possible level, with the raw sqlite3_rekey() C function. Just like usePassphrase(_:) introduced above, changePassphrase(_:) is a convenience method for not-so-demanding users who are OK with using Swift strings.

---

An important consequence of this pull request is that GRDB no longer stores passphrases. It is now the applications that provides the necessary storage. This is particularly visible when an app needs to change the passphrase of an encrypted database pool:

// Initialize storage for the passphrase.
// Here it is the memory, but it could be the system keychain.
var passphrase = "oldSecret"

var config = Configuration()
config.prepareDatabase = { db in
    // Read passphrase from the storage
    try db.usePassphrase(passphrase)
}
let dbPool = try DatabasePool(path: ..., configuration: config)

try dbPool.barrierWriteWithoutTransaction { db in
    // Store new passphrase
    passphrase = "newSecret"
    try db.changePassphrase(passphrase)
    dbPool.invalidateReadOnlyConnections()
}

@michaelkirk-signal, what do you think?

Also, barrierWriteWithoutTransaction, introduced by #603, does not take database snapshots in account: it allows snapshots to run concurrently with the database re-keying, with consequences I have not analyzed. Maybe the re-keying fails, I don't know. Maybe concurrent snapshot reads fail, I don't know. Surely some future snapshot reads will fail. After all, snapshots created before the re-keying use an obsolete passphrase: application has to stop using them. As you are a contributor to https://github.com/secure-sign/securesign-ios, a repository that uses database snapshots, you are the only user I know with some experience of them. I wonder if you have an opinion on this?

[groue]

@darrenclark, @hartbit, @Marus, you are users of SQLCipher (or have been). Would you mind telling if this pull request looks like a correct way forward to you?

For full disclosure of the consequences of this PR, see also #599 (comment).

[hartbit]

If users write the following code:

var passphrase = "secret"

var config = Configuration()
config.prepareDatabase = { db in
    try db.usePassphrase(passphrase)
}

won’t the prepareDatabase closure capture the passphrase variable, thus causing the secret to be kept in memory?

If that’s correct, then this change just transposes the problem to the client side. No?

[groue]

Hi David,

If that’s correct, then this change just transposes the problem to the client side. No?

Totally, that's precisely the goal 😈

There are now at least three levels of security:

Level 1: The worst level of security is the code snippet you comment. It indeed captures the passphrase variable, and keeps the secret in memory. This is not much different from the current implementation of GRDB, which keeps the secret in a Configuration value, for at least as long as the database queue/pool that uses it (this is what @michaelkirk-signal wanted to solve).

So... it doesn't introduce any new bad behavior. But maybe we should not show this in the documentation.

Level 2: The second one is somehow better (but I don't really know how much). At least GRDB doesn't retain String instances:

var config = Configuration()
config.prepareDatabase = { db in
    let passphrase = try fetchPassphraseFromSystemKeychain()
    try db.usePassphrase(passphrase)
    // <- passphrase is released here
}

The passphrase remains in memory as long as... Swift does. I'm sure String is not designed to be secure. But it's convenient: some users will be OK with that.

And I guess this is what should be shown in documentation.

Level 3: And finally, if you are a very demanding user, you can manage the passphrase bytes as you want, as close as possible from the SQLCipher metal (you can't be closer):

var config = Configuration()
config.prepareDatabase = { db in
    ... // Load passphrase bytes as securely as you can
    let code = sqlite3_key(db.sqliteConnection, /* passphrase bytes */)
    ... // Dispose bytes as securely as you can
    guard let code = SQLITE_OK else { throw DatabaseError(...) }
}

This pull request does not provide any ready-made API at this level. Heck, if passphrase comes from some secure UITextField, then the passphrase has been floating around wrapped in an NSString, and I'm not even sure it is useful to perform any byte-juggling after that.

I guess this low-level technique will be suggested in the doc, without much detail.

In a nutshell, I aim at this sweet spot: let security experts do their best, without pretending we are security experts.

[groue]

[...] won’t the prepareDatabase closure capture the passphrase variable, thus causing the secret to be kept in memory?

Do you think, @hartbit, that we should explicitly warn against such capture? Or think about a way to prevent it in the first place?

[groue]

Some news on the change(passphrase:) front.

It will be deprecated, but now I'm not sure it will get a replacement.

There are several reasons for that.

Some blind spot created by the deprecated Configuration.passphrase property prevented me from realizing that when the passphrase comes from a failable storage like the system keychain, things are very difficult for the application. This is because there are two passphrases to deal with: the one in the storage, managed by the app, and the one that encrypts the database, managed by SQLCipher. When one passphrase change fails, and the other succeeds, you end up with a database that is impossible to decrypt - unless some backup of the previous passphrase has been kept. Applications have to implement some kind of journaling of the passphrase. Applications have to be able to perform several passphrase attempts.

This means that providing sample code for changing database passphrase becomes close to impossible. Too simple, the sample code would have no value. And GRDB is not supposed to tell how to perform journaling of keychain items. This is the job of the application, or another library.

Next, the sqlite3_rekey() function that performs in-place passphrase change is discouraged in favor of this technique.

Besides, I'm pretty sure there exist indirect strategies that allow the database passphrase to never change and still support user password updates.

All those reasons tell me that a high-level API for in-place passphrase change has little value. Too much to lose here, too little to win for the users.

[groue]

The Encryption documentation chapter has been rewritten. It does not hide intrinsic difficulties, and is precise up to the point where the brain of the reader has to kick in. I think it's not bad.

Folks, as far as I know, I have addressed questions and concerns: the pull request is ready to be merged.

Please chime in quickly if you see something wrong, or this will ship as is in GRDB 4.4!

[michaelkirk-signal]

Overall the API, inherent limitations, and documentation make sense to me.

I don't have any real world use with the changePassphrase API, so I can't speak to it much. At a high level the API and limitations make sense to me.

I added some nit-picky feedback to the documentation in c2e68d9. Feel free to take-it-or-leave-it.

Thanks for seeing this through!

[groue]

Overall the API, inherent limitations, and documentation make sense to me.

Thanks @michaelkirk-signal! After all, you are the one who kicked all this brainstorm, so satisfying your initial request is the least I can do :-)

I don't have any real world use with the changePassphrase API, so I can't speak to it much. At a high level the API and limitations make sense to me.

Yes, same for me. I'm just wondering if people can deal properly with keychain error AND/OR database error that may happen when they change the password. I can wait a couple more days before I merge the PR and hope for the best.

I added some nit-picky feedback to the documentation in c2e68d9. Feel free to take-it-or-leave-it.

Your help is very welcome, Michael :-)

Thanks for seeing this through!

Thank you! We have a much more secure GRDB now!

[hartbit]

I understand better why ˋprepareDatabase` is necessary. And with that better understanding, I agree that your current solution seems like the best.

But they are not. At least, not for pools. For pools, you must change the passphrase for future reads inside the barrier, which blocks all concurrent reads:

Those code snippets are complex enough (especially for pools) that I think it might make sense to wrap the passphrase change in a convenience function, that is obviously named differently than change(passphrase:):

func triggerPassphraseChange(to passphrase: String) {
    try write { db in
        try db.changePassphrase(passphrase)
    }
}

func triggerPassphraseChange(to passphrase: String) {
    try barrierWriteWithoutTransaction { db in
        try db.changePassphrase(passphrase)
        self.invalidateReadOnlyConnections()
    }
}
ˋˋˋ

[groue]

Those code snippets are complex enough (especially for pools) that I think it might make sense to wrap the passphrase change in a convenience function, that is obviously named differently than change(passphrase:):

I wish it were possible. But #602 (comment): you must update the passphrase in the keychain (or anywhere else) from within the barrier in order to make sure future reads will get the new passphrase.

Do you yourself use an encrypted pool, and change its passphrase? If yes... Then how do you manage the change of passphrase, in both the database and the keychain (assuming you use the keychain)? How do you recover from partial errors (passphrase change in keychain is OK, but not in the db, or the opposite)?

This also is a use case that we don't want to hide by providing a convenience method that can't handle it. It would not be convenient: it would be naive. It's OK if barrierWriteWithoutTransaction and invalidateReadOnlyConnections look like magic incantations to some users. It won't stay magic for long after they understand that they must not allow keychain and database to get out of sync.

... And on top of that, those String-based convenience methods prevent access to the raw C SQLCipher APIs, which is undesirable in the context of this PR. The only String-based convenience methods we want to allow are on the Database class, because it exposes its raw SQLite connection handle and allows demanding users to avoid unsecure Swift strings.

[groue]

🚀 Shipped in version 4.4.0! Thank you all :-)