Page MenuHomePhabricator

Check for the case of database encrypted with key that is lost
ClosedPublic

Authored by marcin on Aug 16 2022, 2:30 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 10:59 AM
Unknown Object (File)
Oct 19 2024, 11:32 PM
Unknown Object (File)
Oct 12 2024, 9:41 AM
Unknown Object (File)
Oct 10 2024, 3:17 PM
Unknown Object (File)
Oct 10 2024, 3:17 PM
Unknown Object (File)
Oct 10 2024, 3:17 PM
Unknown Object (File)
Oct 10 2024, 3:17 PM
Unknown Object (File)
Oct 10 2024, 3:17 PM

Details

Summary

Previous version of "validate_encryption" function ignored the case of database that is actually encrypted but with encryption key that is no longer available. If the database is unencrypted then we can attempt encryption process. If the database is encrypted with encryption key that is not longer available then encryption process will be constantly failing and app becomes useless. We didn’t encounter such a case yet but now that we want to create new encryption key every time user logs out we need to be prepared for such unfortunate scenario.

Test Plan

Uninstall the app. Comment out code that retrieves encryption key from secure store and hardcode some random 64-character hex string. Build the app. Kill the app, bring encryption key retrieval back and bud the app again (remember not to uninstall it at tis stage!!!). Without changes from this differential app will be constantly crashing on every launch. With changes from this differential app will work correctly.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Rebase to update commit with differential link

tomek requested changes to this revision.Aug 22 2022, 6:52 AM
tomek added a reviewer: ashoat.

This logic seems to be correct, but it is hard to follow it. Could you refactor this code so that it becomes more readable? Things to consider:

  1. We check if db is not encrypted, then if it is encrypted, then use the result of 2nd check and then of the 1st. Readability suffers if we compute something and use it a lot later.
  2. It's hard to notice a difference between first check and 2nd check. Maybe we can create some functions for them? Or we can consider creating a function with common part?

Adding @ashoat because deleting the db is something serious, but we don't have any other option when a key is missing. Also there should be a backup somewhere, so I think this approach makes sense, but wanted to make sure.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
362 ↗(On Diff #15642)

Shouldn't we check the content of this error? Are there any other scenarios which result with this error that are not related to encryption?

This revision now requires changes to proceed.Aug 22 2022, 6:52 AM
In D4846#141932, @tomek wrote:

This logic seems to be correct, but it is hard to follow it. Could you refactor this code so that it becomes more readable? Things to consider:

  1. We check if db is not encrypted, then if it is encrypted, then use the result of 2nd check and then of the 1st. Readability suffers if we compute something and use it a lot later.

A remedy for this is to change the order of checks. Fist we check if the database is correctly encrypted with current encryption key (by running some select with encryption key being set). If this check fails then we advance to the second check where we try some select on database without setting encryption key first. If the second succeeds we continue with encryption process. If it fails then database is encrypted with encryption key that is no longer available and must be deleted.

  1. It's hard to notice a difference between first check and 2nd check. Maybe we can create some functions for them? Or we can consider creating a function with common part?

I will consider both options and introduce one of them.

Refactor for readability and code redundance reduction

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
362 ↗(On Diff #15642)

There are 28 error code for sqlite3_exec. The one we are actually looking for is: https://www.sqlite.org/rescode.html#notadb. It is reasonable to check for this particular code and throw in the case of others. But if we do so then we must update the code that was landed 7 moths ago which checks whether encryption key is correct. But there is one thing to note here. SQLCipher documentation does not specify https://www.sqlite.org/rescode.html#notadb. It was error code I was always seeing when testing, so it is my guess that https://www.sqlite.org/rescode.html#notadb is a code returned when running select on encrypted database without correct encryption key set.

Defer to @tomek, but this generally looks good

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
329

Link the docs

tomek requested changes to this revision.Aug 29 2022, 3:47 AM
tomek added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
337–340

If we have a pattern if (sth) return false; else return true; we can simplify it by returning !sth directly.

381–387

Is this logic correct? Here, if the database exists and is encrypted by a lost key, we delete the file so that in the subsequent code the file doesn't exist. This is a different behavior comparing to what we do in lines 368-371 where we exit if the db file doesn't exist.

This revision now requires changes to proceed.Aug 29 2022, 3:47 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
381–387

It is not incorrect but indeed inconsistent. When we exit in 368-371 we continue to execute the body of SQLiteQueryExecutor->migrate() which calls set_encryption_key and then sqlite3_open(...). This creates new database that is encrypted from the beginning. Here we do not exit but call sqlite3_open(...) (which creates new unencrypted database) and run encrypting code. The logic is not incorrect since we exit validate_encryption function with new encrypted database. But we should be consistent here end return after deleting database. Then we will exit validate_encryption without any database and let rest of SQLiteQueryExecutor->migrate() create new encrypted one.

Return if needed to delete database. Simplify return statement in 'is_database_queryable'.

This revision is now accepted and ready to land.Aug 29 2022, 9:20 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
329

https://www.zetetic.net/sqlcipher/sqlcipher-api/#testing-the-key
We used this way of encryption key correctness validation in D2874

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
329

Should have clarified that I wanted you to link the docs from the code comment