Add automatic crypted databases open via dotenvs #1404

revolter · 2018-06-02T21:43:55Z

This adds support for .env files next to the crypted databases that
are to be opened that contains the needed cipher settings.

The only required one is the plain-text password as a value for the key
with the name of the database like this:

myCryptedDatabase.sqlite = MyPassword

This way, databases with a different extension are supported too:

myCryptedDatabase.db = MyPassword

You can also specify a custom page size adding a different line
(anywhere in the file) like this:

myCryptedDatabase.db_pageSize = 2048

If not specified, 1024 is used.

You can also specify the format of the specified key using the
associated integer id:

anotherCryptedDatabase.sqlite = 0xCAFEBABE
anotherCryptedDatabase.sqlite_keyFormat = 1

where 1 means a Raw key. If not specified, 0 is used, which means a
simple text Passphrase.

Dotenv files (.env) are already used on other platforms and by
different tools to manage environment variables, and it's recommended
to be ignored from version control systems, so they won't leak.

Related to #625.

revolter · 2018-06-02T21:55:45Z

Related answer on StackOverflow about dotenv files and use cases: https://stackoverflow.com/a/42014598/865175.

justinclift · 2018-06-02T22:00:35Z

Hmmm, not sure about the .env approach. Hadn't heard of it before, but there's a decent amount of info around detailing what it's about.

Personally, I don't like the idea of storing the plaintext password for an encrypted database... in the exact same filesystem directory as the database itself. That feels to me like a bad security approach.

eg if someone gains access to the directory the database is in somehow, they've pretty much automatically got access to the password too.

We could store the password in the same central local database which we're storing other stuff in (eg Remote database info).

That being said, I haven't reviewed this code yet to see if the passwords themselves are actually encrypted (by a master password) in the .env files. If they are, then my concern above probably isn't that warranted. 😄

revolter · 2018-06-02T22:05:27Z

Personally, I don't like the idea of storing the plaintext password for an encrypted database... in the exact same filesystem directory as the database itself. That feels to me like a bad security approach.

Yes, but it wouldn't be DB4S' responsibility. We could even not mention and document this at all so only technical persons that happen to find it would use it.

eg if someone gains access to the directory the database is in somehow, they've pretty much automatically got access to the password too.

Also, in my case, the password is already in plain text in the codebase where the database is too, so it's the exact same thing regarding the security. This is because the database is used programmatically, hence the password needs to be in the code, and not manually (except when debugging and stuff) by people.

That being said, I haven't reviewed this code yet to see if the passwords themselves are actually encrypted (by a master password) in the .env files.

They are not, but again, it's the whole responsibility of the users if they want to create these .env files or not.

justinclift · 2018-06-02T22:05:36Z

Ahhh, the Travis failure is a real failure too. You ok to fix it? 😄

[ 71%] Linking CXX executable sqlitebrowser
CMakeFiles/sqlitebrowser.dir/src/Settings.cpp.o: In function `Settings::getDotenvFormat()':
Settings.cpp:(.text+0x51): undefined reference to `DotenvFormat::readEnvFile(QIODevice&, QMap<QString, QVariant>&)'
CMakeFiles/sqlitebrowser.dir/src/CipherDialog.cpp.o: In function `CipherDialog::getCipherSettings() const':
CipherDialog.cpp:(.text+0xa3): undefined reference to `CipherSettings::getKeyFormat(int)'
CipherDialog.cpp:(.text+0xde): undefined reference to `CipherSettings::setKeyFormat(CipherSettings::KeyFormats const&)'
CipherDialog.cpp:(.text+0xe9): undefined reference to `CipherSettings::setPassword(QString const&)'
CipherDialog.cpp:(.text+0xf3): undefined reference to `CipherSettings::setPageSize(int)'
CMakeFiles/sqlitebrowser.dir/src/CipherDialog.cpp.o: In function `CipherDialog::checkInputFields()':
CipherDialog.cpp:(.text+0x25b): undefined reference to `CipherSettings::getKeyFormat(int)'
collect2: error: ld returned 1 exit status
make[2]: *** [sqlitebrowser] Error 1
make[1]: *** [CMakeFiles/sqlitebrowser.dir/all] Error 2
make: *** [all] Error 2

revolter · 2018-06-02T22:06:24Z

Saw that, but I don't have any idea how to fix it yet.

justinclift · 2018-06-02T22:07:51Z

When you say programmatically, are you're meaning the program just needs to be able to open the database automatically without the user needing to type the password manually (every time)?

If that's the case, would it be ok to have the password stored elsewhere? eg in a different directory or something?

revolter · 2018-06-02T22:13:02Z

When you say programmatically, are you're meaning the program just needs to be able to open the database automatically without the user needing to type the password manually (every time)?

Indeed.

If that's the case, would it be ok to have the password stored elsewhere? eg in a different directory or something?

In my case, no, it's not feasible.

Also, .env files are generally used for storing secure keys and passwords in clear text, like API keys/tokens or secret keys.

justinclift · 2018-06-02T22:35:42Z

Also, .env files are generally used for storing secure keys and passwords in clear text, like API keys/tokens or secret keys.

Yeah, I understand that. They seem like a solution mostly for source code, where the main threat is accidentally including the secure key (etc) when commiting to version control. It's a very real problem, and this seems like a decent solution to that specific one.

My understanding of the threat model for encrypted databases though, is that accidentally uploading a plain text version of the key with the database... isn't really in the list. If we automatically create files with the plain text key... right next to the database... that seems like a really bad idea.

Hmmm, let me sleep on it. Might have a different take on it tomorrow.

@mgrojo and @MKleusberg might have a different viewpoint on things anyway. 😄

revolter · 2018-06-02T23:09:28Z

If we automatically create files with the plain text key... right next to the database... that seems like a really bad idea.

This is not at all what this PR does. It will never write to any file, only read:

static const QSettings::Format DotenvFormat = QSettings::registerFormat("env", &DotenvFormat::readEnvFile, nullptr);

(the third parameters is used for specifying a function for writing/serializing values into a file, but is null so not even accidental writes happen)

This only checks if an .env file is present and read from it. If not, it uses the default logic.
That's why I'm saying that it's only the user's responsibility regarding these files.

justinclift · 2018-06-02T23:10:56Z

Ahhhh, gotcha. Ok, that sounds reasonable to me then. 😄

This adds support for `.env` files next to the crypted databases that are to be opened that contains the needed cipher settings. The only required one is the plain-text password as a value for the key with the name of the database like this: myCryptedDatabase.sqlite = MyPassword This way, databases with a different extension are supported too: myCryptedDatabase.db = MyPassword You can also specify a custom page size adding a different line (anywhere in the file) like this: myCryptedDatabase.db_pageSize = 2048 If not specified, `1024` is used. You can also specify the format of the specified key using the associated integer id: anotherCryptedDatabase.sqlite = 0xCAFEBABE anotherCryptedDatabase.sqlite_keyFormat = 1 where `1` means a Raw key. If not specified, `0` is used, which means a simple text Passphrase. Dotenv files (`.env`) are already used on other platforms and by different tools to manage environment variables, and it's recommended to be ignored from version control systems, so they won't leak.

revolter · 2018-06-09T19:06:47Z

FINALLY! I fixed the build errors. ❤️ (I also rebased to fix the conflicts)

@MKleusberg, could you please take a look? 😃 It's easier if you look at each commit separately, while the most important one is 94bbb46.

MKleusberg

I have marked some pieces of code here which in my opinion should be changed before merging.

In general however I think this is a good approach for storing the encryption. As Justin correctly pointed out it's not safe to have the password in plain text and in the same directory as the database. But I think there are only two alternatives for storing passwords: hiding them somewhere, maybe even encrypted (but where should we hide the encryption key?), or using a master password approach. Hiding the passwords would be problematic because it makes the users feel safe when they aren't. The master password approach is definitely the best way but on the other hand then you could just install a proper password manager and use that. And again, the password manager is probably better checked and maintained for security. So yeah, this approach is definitely not safe but on the other hand it is so unsafe that everybody will notice and hopefully be able to judge if it's the right solution for them.

Because this is such a hidden feature we should also add a Wiki page for it. And there we could also describe the security implications of this feature, just to make sure.

Not sure about the dotenv format because I haven't used/seen it before but it seems simple enough to use and to parse, so I don't have any objections here either.

MKleusberg · 2018-06-21T10:26:49Z

src/Settings.cpp

+    static const QSettings::Format DotenvFormat = QSettings::registerFormat("env", &DotenvFormat::readEnvFile, nullptr);
+
+    return DotenvFormat;
+}


I would delete this function or at least move it to some other file. The reason is that the Settings.cpp and Settings.h file were added to have a class which is as small and simple as possible because the settings are queried all the time throughout the entire program. The extra function wouldn't hurt much but because it's only used once and because it is essentially a one-liner I think it's cleaner to just move that one line to sqlitedb.cpp. This way we can also avoid the extra include in Settings.h.

MKleusberg · 2018-06-21T10:28:56Z

src/sqlitedb.h

@@ -13,6 +13,7 @@

 struct sqlite3;
 class CipherDialog;
+class CipherSettings;


You can delete the old class CipherDialog; line here.

MKleusberg · 2018-06-21T10:32:57Z

src/sqlitedb.cpp

        {
            QMessageBox::warning(nullptr, qApp->applicationName(), lastErrorMessage);
            return false;
        }
    }
+    delete cipherSettings;


Well spotted 😄

MKleusberg · 2018-06-21T10:40:19Z

src/sqlitedb.cpp

+                    cipherSettings->setPassword(password);
+                    cipherSettings->setPageSize(pageSize);
+
+                    // Close and reopen database first to be in a clean state after the failed read attempt from above


Can you change the code here (maybe by introducing some flag or moving it into a separate function) to avoid duplicating the lines below? Everything from this line until the *encrypted = true; line is basically a copy of the code below and I feel like this code is long and complex enough to not be twice in the code base - especially because it might be changed in the future.

MKleusberg · 2018-06-21T10:41:52Z

src/sqlitedb.cpp

-                sqlite3_exec(dbHandle, QString("PRAGMA key = %1").arg(cipherSettings->password()).toUtf8(), NULL, NULL, NULL);
-                if(cipherSettings->pageSize() != 1024)
-                    sqlite3_exec(dbHandle, QString("PRAGMA cipher_page_size = %1;").arg(cipherSettings->pageSize()).toUtf8(), NULL, NULL, NULL);
+                if (cipherSettings)


No need for the if here. Deleting a nullptr is basically a no-op.

MKleusberg · 2018-06-21T10:42:50Z

src/sqlitedb.cpp


                *encrypted = true;
            } else {
                sqlite3_close(dbHandle);
                *encrypted = false;
-                delete cipherSettings;
-                cipherSettings = nullptr;
+                if (cipherSettings)


Same here. No need for the if here, the old code did the same but was a bit shorter and easier to read.

(related to ac51c23)

(introduced by 94bbb46)

(introduced by e5a0293)

revolter · 2018-06-23T11:09:55Z

@MKleusberg, This is ready for another review 😃

MKleusberg · 2018-07-10T18:46:00Z

Looks good 👍 And I couldn't trigger any errors either. So let's merge this 😄 Sorry for the delay on my side, @revolter!

revolter · 2018-07-11T08:37:42Z

No problem. Thank you so much! This is one of my happiest days. I can finally open a database file by simply double-clicking it, without entering any password ❤️

This idea was the reason of one of my first Pull Requests on one of the first repositories I contributed to. (From 2 years ago, one day after my birthday, so that means I was working on it on my birthday too 😆)

revolter requested review from MKleusberg and mgrojo June 2, 2018 21:44

justinclift added the enhancement Feature requests. label Jun 2, 2018

revolter force-pushed the feature/sqlcipher-passwords branch from e70510f to c9ede24 Compare June 9, 2018 13:46

revolter added 11 commits June 9, 2018 16:58

Rename confusing variables

dd6768b

Fix some project warnings

a644bce

Fix code style

9049e72

Add constant for the default page size

bb14734

Move KeyFormats enum to CipherSettings

095f77b

Fix code style

c13dc42

Fix memory leak

6910663

Stop relying on CipherDialog for encryption settings management

e5a0293

Fix code style

387de52

Add .env format for QSettings

ac51c23

revolter force-pushed the feature/sqlcipher-passwords branch from c9ede24 to 94bbb46 Compare June 9, 2018 14:03

revolter added 2 commits June 9, 2018 17:06

Add new files to CMakeLists

4eca07c

Move DotenvFormat include to the implementation

2eaae54

revolter force-pushed the feature/sqlcipher-passwords branch from e7f4b2c to f0ecdf9 Compare June 9, 2018 18:32

Fix build error

f9db3dc

revolter force-pushed the feature/sqlcipher-passwords branch from f0ecdf9 to f9db3dc Compare June 9, 2018 18:48

MKleusberg reviewed Jun 21, 2018

View reviewed changes

revolter added 7 commits June 23, 2018 12:59

Remove superfluous method

255f7bd

(related to ac51c23)

Remove superfluous checks

c421bde

Fix memory leaks

58e6677

(introduced by 94bbb46)

Fix code style

2075a59

Make dotenv related variable and comment clearer

be10599

Remove duplicated code

1713f54

Remove unused forward declaration

e6159ce

(introduced by e5a0293)

MKleusberg merged commit 3cdc65a into sqlitebrowser:master Jul 10, 2018

revolter deleted the feature/sqlcipher-passwords branch July 11, 2018 08:37

MKleusberg mentioned this pull request Jul 14, 2018

Automatically enter crypted database's password #486

Closed

chrisjlocke mentioned this pull request Aug 16, 2018

Storing passwords for encrypted databases? #1505

Closed

revolter mentioned this pull request Feb 2, 2019

3.11.0 release - outstanding pieces? #1656

Closed

23 tasks

Uh oh!

Add automatic crypted databases open via dotenvs #1404

Add automatic crypted databases open via dotenvs #1404

Uh oh!

Conversation

revolter commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

revolter commented Jun 2, 2018

Uh oh!

justinclift commented Jun 2, 2018

Uh oh!

revolter commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinclift commented Jun 2, 2018

Uh oh!

revolter commented Jun 2, 2018

Uh oh!

justinclift commented Jun 2, 2018 • edited by revolter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

revolter commented Jun 2, 2018

Uh oh!

justinclift commented Jun 2, 2018

Uh oh!

revolter commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinclift commented Jun 2, 2018

Uh oh!

revolter commented Jun 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MKleusberg left a comment

Choose a reason for hiding this comment

Uh oh!

MKleusberg Jun 21, 2018 • edited by revolter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MKleusberg Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

MKleusberg Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

MKleusberg Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

MKleusberg Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

MKleusberg Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

revolter commented Jun 23, 2018

Uh oh!

MKleusberg commented Jul 10, 2018

Uh oh!

revolter commented Jul 11, 2018

Uh oh!

Uh oh!

revolter commented Jun 2, 2018 •

edited

Loading

revolter commented Jun 2, 2018 •

edited

Loading

justinclift commented Jun 2, 2018 •

edited by revolter

Loading

revolter commented Jun 2, 2018 •

edited

Loading

revolter commented Jun 9, 2018 •

edited

Loading

MKleusberg Jun 21, 2018 •

edited by revolter

Loading