SQLCipher disk encryption for Android + iOS #907

cjroth · 2020-12-23T00:12:05Z

This builds on @afiller's PR #597 and also adds optional encryption for iOS. My understanding is that disk encryption is not totally necessary for iOS but offers an additional layer of security.

Notes:

This uses FMDB from CocoaPods instead of hardwiring it.
- EDIT: Looks like the desire is to include support for non CocoaPods users? I think the options are to either update the hardwired FMDB source to the one that supports SQLCipher or to just put in the docs that SQLCipher will only work for CocoaPods users. I'm happy to break this into a separate PR for the sake of keeping things organized if it makes sense to switch to Cocoapods. It seems like most RN libraries are 100% Cocoapods now, but maybe I'm getting ahead of the community? If desired, I am happy to update this PR / open a new one where I undo the removal of the hardwired FMDB.
This doesn't add password support for web. I don't see anything about encryption in LokiJS's docs for their various storage adapters. If it's used only for in-memory storage then it shouldn't really matter.
better-sqlite3 (what WatermelonDB uses for Node) does work with SQLCipher if you compile it with it enabled - it sounds like it shouldn't be super hard to get it working for that too, which may be good for the tests. However, I'm not sure if it's worth the effort, honestly, given that it seems like the Node adapter is mainly used for testing. I could be wrong.
We probably would want to add some tests for this - I need to think about what said tests might look like.

…tcher

cjroth · 2020-12-23T00:18:32Z

@radex I assume this will require some discussion and additional work but wanted to put it on your radar, so I went ahead and opened the PR. I'm not sure if this is a feature that you're even interested in merging. I assume we'd want to figure out how to only include the sqlcipher version of sqlite if the user opts in - not sure how to set that up in Kotlin atm. Also assuming this will require further documentation changes.

This reverts commit 78b8fae.

cjroth · 2020-12-23T02:13:53Z

native/android/build.gradle

@@ -22,6 +22,8 @@ dependencies {
    //noinspection GradleDynamicVersion
    implementation 'com.facebook.react:react-native:+'
    implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
+    implementation "net.zetetic:android-database-sqlcipher:4.4.2"


It seems like we'd want to branch these based on the user's desire to use SQLCipher or not. If not, I imagine we'd want to use the native implementation.

Yeah, I feel a little uncomfortable forcing this dependency. android.database.sqlite.SQLiteDatabase is built in, requires no extra code, and "just works".

Can we easily make it so that it's optional? For example, there would be a separate .kt file which adds to Database support for android-database-sqlicipher, and unless we explicitly add this to app's setup we fall back to standard implementation.

I'm not super familiar with ins and outs of Android. @rozPierog ideas?

There is no clear way to create dependency that's conditional that I know of.
What I would do is to move and duplicate Database.kt to separate modules: standard, cipher and require users to include another path in settings.gradle+build.gradle to one or the other. That way person using library can have full control what is in their app.
In theory if both are included it will throw error on compile that there are two classes with same name/package
In theory if non are included it will throw error on compile that there is no such class
Database initialization would need to be handled via Class.forName

To make it easier for maintenance there should be another module common that has common code from both Database files so that any change shouldn't be duplicated. (But I don't know how to cleanly handle type of SQLiteDatabase [ import android.database.sqlite.SQLiteDatabase vs import net.sqlcipher.database.SQLiteDatabase])

Disclaimer: I have no idea what I'm doing. I've never attempted stuff like this and I'm not familiar with pitfalls of modularization like that.

@cjroth Can you give your input here? How would you handle this?

Disclaimer: I have no idea what I'm doing. I've never attempted stuff like this and I'm not familiar with pitfalls of modularization like that.

Haha! Me neither, I don't know even know Kotlin :)

I think I agree with both of your comments though - I'm not aware of any way to do optional dependencies, so separating into modules seems like the best option. I do think this is kind of a bummer for users that don't need to use SQLCipher since it adds one more step to setup, but maybe it's worth it.

cjroth · 2020-12-23T02:14:02Z

native/android/src/main/java/com/nozbe/watermelondb/Database.kt

@@ -2,12 +2,17 @@ package com.nozbe.watermelondb

 import android.content.Context
 import android.database.Cursor
-import android.database.sqlite.SQLiteDatabase
+import net.sqlcipher.database.SQLiteDatabase


cjroth · 2020-12-23T02:15:36Z

native/ios/WatermelonDB/Database.swift

@@ -32,6 +34,7 @@ class Database {
    }

    func inTransaction(_ executeBlock: () throws -> Void) throws {
+        fmdb.setKey(self.password)


It's not clear to me why it was necessary to call setKey every time and I worry about performance with this. I think we want to figure this out before merging. Originally I tried calling setKey only in the open method but it did not encrypt the database - the method gets called, but it is effectively ignored by SQLite.

yup, agreed, should be figured out before merge, I don't like this either

cjroth · 2020-12-23T02:16:33Z

src/Model/index.d.ts

@@ -53,6 +53,7 @@ declare module '@nozbe/watermelondb/Model' {

    public observe(): Observable<this>

+    // @ts-ignore


This was necessary to get TypeScript not to complain. Could add more descriptive comment if needed.

cjroth · 2020-12-23T02:17:10Z

src/adapters/lokijs/index.js

@@ -109,6 +109,9 @@ export default class LokiJSAdapter implements DatabaseAdapter {
    this._dbName = dbName

    if (process.env.NODE_ENV !== 'production') {
+      invariant('password' in options,


It would be bad if someone didn't realize that encryption wasn't supported, so best to safeguard.

…ed and fix tests

radex · 2021-01-07T09:08:28Z

'm not sure if this is a feature that you're even interested in merging

it is! I'm not promising when I'll take a closer look at this, but I would very much like it as a built-in feature

cjroth · 2021-01-07T21:03:55Z

@radex sounds good and let me know what I can do to help. I will try to work with you to get this done.

klandell · 2021-03-11T16:58:31Z

Hello! Just wondering if there are any updates on this since the last comment in January. I'm thinking about migrating from Realm to Watermelon, but the current lack of encryption is a deal breaker.

radex

Excellent work! Thank you! I have somewhat mixed feelings about the approach of passing password through JavaScript into the native thread. I'm not saying it's necessarily a bad thing… What I'd prefer in my own apps is to keep encryption info purely in native code - to treat JavaScript as insecure, and shield encryption keys from it. Having said that, this would only be a small extra level of protection, considering JS has access to the data anyway, so rogue JS could still read and transmit all of it, and many/most users would not be able to figure out the native app code required to set this up. So I think it's acceptable.

Have you checked if native/iosTest works correctly? It didn't used to use WatermelonDB via CocoaPods but via old-school linking, so I'm not sure if we can make the breaking change of removing non-CP support without fixing this first

radex · 2021-03-24T10:14:18Z

docs-master/Installation.md

+          3. Finally, add this line:
+
+             ```ruby
+             pod 'FMDB/SQLCipher', :modular_headers => true


I don't think this should be required. Seems appropriate to me to add to docs-master/Advanced a new .md for how to set up encryption. Alternatively, just put this at the end and specify that this is optional

radex · 2021-03-24T10:15:54Z

native/android/src/main/java/com/nozbe/watermelondb/Database.kt

-            it.moveToFirst()
-            val index = it.getColumnIndex("name")
-            if (index > -1) {
-                do {
-                    allTables.add(it.getString(index))
-                } while (it.moveToNext())
+            if (it.count > 0) {
+                it.moveToFirst()
+                val index = it.getColumnIndex("name")
+                if (index > -1) {
+                    do {
+                        allTables.add(it.getString(index))
+                    } while (it.moveToNext())
+                }


why is this necessary?

radex · 2021-03-24T10:17:39Z

native/android/build.gradle

@@ -22,6 +22,8 @@ dependencies {
    //noinspection GradleDynamicVersion
    implementation 'com.facebook.react:react-native:+'
    implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
+    implementation "net.zetetic:android-database-sqlcipher:4.4.2"


Yeah, I feel a little uncomfortable forcing this dependency. android.database.sqlite.SQLiteDatabase is built in, requires no extra code, and "just works".

Can we easily make it so that it's optional? For example, there would be a separate .kt file which adds to Database support for android-database-sqlicipher, and unless we explicitly add this to app's setup we fall back to standard implementation.

I'm not super familiar with ins and outs of Android. @rozPierog ideas?

radex · 2021-03-24T10:20:02Z

native/ios/WatermelonDB/Database.swift

@@ -32,6 +34,7 @@ class Database {
    }

    func inTransaction(_ executeBlock: () throws -> Void) throws {
+        fmdb.setKey(self.password)


yup, agreed, should be figured out before merge, I don't like this either

radex · 2021-03-24T10:20:52Z

native/ios/WatermelonDB/Database.swift


-    init(path: String) {
+    init(path: String, password: String) {


shouldn't password be optional?

radex · 2021-03-24T10:21:47Z

native/iosTest/WatermelonTesterTests/DatabaseDriverTests.swift

@@ -27,7 +27,7 @@ let testSchema = """
    """

 func newDatabase() -> DatabaseDriver {
-    return DatabaseDriver(dbName: ":memory:", setUpWithSchema: (version: 1, sql: testSchema))
+    return DatabaseDriver(dbName: ":memory:", password: "password", setUpWithSchema: (version: 1, sql: testSchema))


do we need to pass password here? Seems irrelevant to what we want to do in this test

radex · 2021-03-24T10:22:01Z

src/adapters/lokijs/index.d.ts

-    useWebWorker?: boolean
-    useIncrementalIndexedDB?: boolean


radex · 2021-03-24T10:22:22Z

src/adapters/lokijs/index.js

@@ -109,6 +109,9 @@ export default class LokiJSAdapter implements DatabaseAdapter {
    this._dbName = dbName

    if (process.env.NODE_ENV !== 'production') {
+      invariant(!('password' in options),
+          'LokiJSAdapter `password` option not supported. Encryption is only supported on mobile.',


Suggested change

'LokiJSAdapter `password` option not supported. Encryption is only supported on mobile.',

'LokiJSAdapter `password` option not supported. Encryption is only supported in SQLiteAdapter.',

radex · 2021-03-24T10:22:56Z

src/adapters/sqlite/index.js

    this.schema = schema
    this.migrations = migrations
    this._dbName = this._getName(dbName)

+    // Password is not supported in web/Node but we pass it through in order to keep the API
+    // consistent and we throw an error if a password has been provided in order to prevent mistakes.
+    this._password = password || ''


why are we defaulting to '' instead of having password be a ?string ?

radex · 2021-03-24T10:24:31Z

src/adapters/sqlite/makeDispatcher/index.js

+    if (process.env.NODE_ENV !== 'production') {
+      invariant(
+        !password,
+        'SQLiteAdapter `password` option not supported. Encryption is only supported on mobile`',


Suggested change

'SQLiteAdapter `password` option not supported. Encryption is only supported on mobile`',

'`password` option not currently supported in Node implementation of SQLiteAdapter`',

cjroth · 2021-03-24T16:48:41Z

Excellent work! Thank you! I have somewhat mixed feelings about the approach of passing password through JavaScript into the native thread. I'm not saying it's necessarily a bad thing… What I'd prefer in my own apps is to keep encryption info purely in native code - to treat JavaScript as insecure, and shield encryption keys from it. Having said that, this would only be a small extra level of protection, considering JS has access to the data anyway, so rogue JS could still read and transmit all of it, and many/most users would not be able to figure out the native app code required to set this up. So I think it's acceptable.

How do you get the password from the user to the native thread? Currently we still use a React Native TextInput for this and then pass it over to RNSensitiveInfo, so it does at one moment go through JS. I'm not sure what the alternative would be here - maybe if you could configure the specific encrypted keychain location to read the password from, which would then prompt the user to do a FaceID/Fingerprint/etc? This would require the user to have a keychain library installed and would be a bit of work to have the two native modules communicate. I think this JS concept is ok for now too for the reasons that you mentioned but I'm open to brainstorming more ideas on this.

Have you checked if native/iosTest works correctly? It didn't used to use WatermelonDB via CocoaPods but via old-school linking, so I'm not sure if we can make the breaking change of removing non-CP support without fixing this first

I ran the tests which are passing - does that run when you run yarn test or is there another step for that?

Thank you for the code review :) I'll review your specific comments when I get some time.

radex · 2021-03-24T21:28:18Z

Have you checked if native/iosTest works correctly? It didn't used to use WatermelonDB via CocoaPods but via old-school linking, so I'm not sure if we can make the breaking change of removing non-CP support without fixing this first

Yes, just today. It's gotten so out of date that I could no longer even build it correctly on an M1 Mac... I forgot that modern RN doesn't even support non-CP installs, so I think we're fine with just CP support. I don't want to specifically test for RN versions older than 0.62.

How do you get the password from the user to the native thread?

My assumption was that the password isn't going to be directly input by the user, but instead a system Keychain API would be used - either the biometrics+system password fallback combo or just random password generated and stored in the Keychain (I'm not super familiar with Android security model, but on iOS, keychain is far, far more secure than disk storage, since the former is managed by the SEP, and the second is only protected by sandboxing).

I think what I'd do is have an API exposed so that the app developer can hook up DatabaseDriver.passwordProvider = { dbName in .... add your custom password request implementation here ... }

Anyway. I don't think you have to worry about it. It's a fairly easy addition to make and shouldn't block this PR IMO.

klandell · 2021-03-24T21:50:32Z

For what it's worth to the above conversation:

Currently on Android I store the encryption key for Realm in Android's EncryptedSharedPreferences which is itself encrypted by a MasterKey managed by Android's keystore.

Realm's key is randomly generated on the Native Side on the first app open, then passed to the JS side to configure Realm, then passed back to be stored. Each time the app starts up, the value needs to be read from Native and sent to JS, where it's sent back to native I presume with all of the other Realm configs.

IF this could all be handled in native code in Android without all of the back and forth it would be really cool.

radex · 2021-03-25T12:01:24Z

@cjroth iosTest changes are merged to master, so when you merge in master here, CP-only is going to be fine.

PS: If you can configure the PR so that I can modify your branch, that will help me with review

afiller · 2021-03-25T22:53:58Z

@cjroth and @radex thank you so much for taking on this task and your intensive discussions. My suggestion (PR) was just a proprietary solution because we needed it urgently in a project. 🙏 👍

cjroth · 2021-03-26T05:00:51Z

PS: If you can configure the PR so that I can modify your branch, that will help me with review

@radex what do you mean exactly? Happy to do whatever helps you!

Sorry still haven't had much time to review all comments in detail, hoping to find some time soon. I also don't know Kotlin so it would be amazing if someone could help me out on that front.

radex · 2021-03-26T07:38:44Z

Sorry still haven't had much time

No rush!

@radex what do you mean exactly? Happy to do whatever helps you!

This checkbox: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

cjroth · 2021-03-27T05:18:09Z

This checkbox: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Not sure why but I don't see this option:

radex · 2021-05-07T10:10:56Z

@cjroth hey, are you planning to finish this in the near future? Conflicts are creating themselves :(

cjroth · 2021-05-21T06:10:06Z

@radex hey I'm so sorry I have not had any time to work on this and probably won't any time soon :( I will see if one of my team members has any time to help though!

dzcpy · 2021-07-30T04:20:27Z

Any updates?

waqas19921 · 2022-01-26T05:18:37Z

@radex @cjroth Anyone who will resolve the conflicts and give it a final touch?

dzcpy · 2022-01-26T06:24:46Z

Anyone's still interested in working on it? Maybe we can try fixing it

radex · 2022-01-26T07:59:48Z

@dzcpy There were some issues left - look at the comments; happy to review a PR with these issues fixed

UnknownH · 2022-01-28T19:48:16Z

Does anyone know what are the chances of this ever being implemented? Would it be possible to go back to #597 as an alternative (since iOS apparently didn't need this encryption to begin with), so that safe storage of data can also be unlocked/available on Android? We've been keen to implement a React Native project with WatermelonDB for the longest time but this is always stopping us from adoption. Thanks and regards.

radex · 2022-01-31T08:11:41Z

@UnknownH This is an open source project. If you need this PR, then use a fork of WatermelonDB and use it. Or contribute, help bring it up to the standard required to get it merged.

ororsatti · 2023-08-02T14:35:43Z

I have a PR open to do that for JSI, @radex if you can take a look that'd be awesome

avinashlingaloo · 2024-06-11T10:21:15Z

Is this PR still relevant ? I would need this feature. @radex

Lemon-King · 2024-08-09T21:13:04Z

Is this PR still relevant ? I would need this feature. @radex

I'm also interested in knowing if this is still being worked on or if there is an equivalent feature already in?

ororsatti · 2024-08-15T12:20:43Z

Is this PR still relevant ? I would need this feature. @radex

I'm also interested in knowing if this is still being worked on or if there is an equivalent feature already in?

@avinashlingaloo @Lemon-King

Is this PR still relevant ? I would need this feature. @radex

I'm also interested in knowing if this is still being worked on or if there is an equivalent feature already in?

I have a pr open on that, for the new arch, take a look: #1635
perhaps youll be able to use it.

cjroth added 9 commits December 18, 2020 18:02

merge afiller:features/encrypted-android-db-pull-request

820db09

update a few more locations where password is required (native only)

9c8de8f

use FMDB from cocoapods instead of hard coding it

6074631

wip - get sqlcipher working for fmdb

e4832cc

accept password form javascriptland

82dd122

fix typescript issues in development

577d2e5

ignore eslint warning that password argument is not used in makeDispa…

1799b96

…tcher

bump sqlcipher version on android

43c643a

update changelog

a9bdd9d

cjroth added 6 commits December 22, 2020 17:40

remove password from web/node dispatcher

cb480db

fix kotlin typo

78b8fae

Revert "fix kotlin typo"

3237c52

This reverts commit 78b8fae.

revert typescript fix

08bd625

update node dispatcher to match native, tests passing

7bb0341

safeguard against accidental usage of password with LokiJSAdapter

91216e2

cjroth commented Dec 23, 2020

View reviewed changes

ensure that password is never provided when encryption is not support…

d68c04c

…ed and fix tests

diegolmello mentioned this pull request Jan 12, 2021

[Security] Sensitive data stored in unencrypted database RocketChat/Rocket.Chat.ReactNative#2785

Open

radex mentioned this pull request Mar 24, 2021

Encrypted SQLite DB for Android #597

Closed

radex requested changes Mar 24, 2021

View reviewed changes

radex assigned cjroth Mar 24, 2021

waqas19921 mentioned this pull request Jun 23, 2021

Is watermelon safe to store sensitive data? #51

Closed

radex mentioned this pull request May 23, 2022

Does WatermelonDB support encryption? #1328

Closed

killerchip mentioned this pull request Jan 17, 2024

Add encrypted SQLCipher WatermelonDB JSI #1635

Open

7 tasks

		@@ -53,6 +53,7 @@ declare module '@nozbe/watermelondb/Model' {

		public observe(): Observable<this>

		// @ts-ignore

	'LokiJSAdapter `password` option not supported. Encryption is only supported on mobile.',
	'LokiJSAdapter `password` option not supported. Encryption is only supported in SQLiteAdapter.',

	'SQLiteAdapter `password` option not supported. Encryption is only supported on mobile`',
	'`password` option not currently supported in Node implementation of SQLiteAdapter`',

SQLCipher disk encryption for Android + iOS #907

Are you sure you want to change the base?

SQLCipher disk encryption for Android + iOS #907

Conversation

cjroth commented Dec 23, 2020 • edited Loading

Notes:

cjroth commented Dec 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radex commented Jan 7, 2021

cjroth commented Jan 7, 2021

klandell commented Mar 11, 2021 • edited Loading

radex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjroth commented Mar 24, 2021

radex commented Mar 24, 2021

klandell commented Mar 24, 2021

radex commented Mar 25, 2021

afiller commented Mar 25, 2021 • edited Loading

cjroth commented Mar 26, 2021 • edited Loading

radex commented Mar 26, 2021

cjroth commented Mar 27, 2021

radex commented May 7, 2021

cjroth commented May 21, 2021

dzcpy commented Jul 30, 2021

waqas19921 commented Jan 26, 2022

dzcpy commented Jan 26, 2022 • edited Loading

radex commented Jan 26, 2022

UnknownH commented Jan 28, 2022

radex commented Jan 31, 2022

ororsatti commented Aug 2, 2023

avinashlingaloo commented Jun 11, 2024

Lemon-King commented Aug 9, 2024

ororsatti commented Aug 15, 2024

cjroth commented Dec 23, 2020 •

edited

Loading

klandell commented Mar 11, 2021 •

edited

Loading

afiller commented Mar 25, 2021 •

edited

Loading

cjroth commented Mar 26, 2021 •

edited

Loading

dzcpy commented Jan 26, 2022 •

edited

Loading