New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add automatic crypted databases open via dotenvs #1404

Merged
merged 21 commits into from Jul 10, 2018

Conversation

Projects
None yet
3 participants
@revolter
Copy link
Member

revolter commented Jun 2, 2018

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 revolter requested review from MKleusberg and mgrojo Jun 2, 2018

@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 2, 2018

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

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jun 2, 2018

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

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 2, 2018

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

This comment has been minimized.

Copy link
Member

justinclift commented Jun 2, 2018

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

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 2, 2018

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

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jun 2, 2018

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

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 2, 2018

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

This comment has been minimized.

Copy link
Member

justinclift commented Jun 2, 2018

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

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 2, 2018

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

This comment has been minimized.

Copy link
Member

justinclift commented Jun 2, 2018

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

@revolter revolter force-pushed the revolter:feature/sqlcipher-passwords branch from e70510f to c9ede24 Jun 9, 2018

revolter added some commits Jun 1, 2018

Add automatic crypted databases open via dotenvs
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 revolter force-pushed the revolter:feature/sqlcipher-passwords branch from c9ede24 to 94bbb46 Jun 9, 2018

@revolter revolter force-pushed the revolter:feature/sqlcipher-passwords branch from e7f4b2c to f0ecdf9 Jun 9, 2018

@revolter revolter force-pushed the revolter:feature/sqlcipher-passwords branch from f0ecdf9 to f9db3dc Jun 9, 2018

@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 9, 2018

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
Copy link
Member

MKleusberg left a comment

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.

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

return DotenvFormat;
}

This comment has been minimized.

@MKleusberg

MKleusberg Jun 21, 2018

Member

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.

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

struct sqlite3;
class CipherDialog;
class CipherSettings;

This comment has been minimized.

@MKleusberg

MKleusberg Jun 21, 2018

Member

You can delete the old class CipherDialog; line here.

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

This comment has been minimized.

@MKleusberg

MKleusberg Jun 21, 2018

Member

Well spotted 😄

cipherSettings->setPassword(password);
cipherSettings->setPageSize(pageSize);

// Close and reopen database first to be in a clean state after the failed read attempt from above

This comment has been minimized.

@MKleusberg

MKleusberg Jun 21, 2018

Member

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.

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)

This comment has been minimized.

@MKleusberg

MKleusberg Jun 21, 2018

Member

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


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

This comment has been minimized.

@MKleusberg

MKleusberg Jun 21, 2018

Member

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

@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jun 23, 2018

@MKleusberg, This is ready for another review 😃

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Jul 10, 2018

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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@revolter

This comment has been minimized.

Copy link
Member Author

revolter commented Jul 11, 2018

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 revolter deleted the revolter:feature/sqlcipher-passwords branch Jul 11, 2018

@revolter revolter referenced this pull request Feb 2, 2019

Closed

3.11.0 release - outstanding pieces? #1656

12 of 23 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment