Add SQLCipher saved passwords

[revolter]

This PR adds the option to save a password per database file name and upon trying to open an encrypted database for which the password was saved in the Preferences, it opens up automatically, without the need for user input.

Fixed #486.

[justinclift]

Awesome. 😁

We'll need to get the Travis check working though before anything else. It's failing on the sqlcipher install part, prior to the compiling:

$ sudo apt-get install -qq libqt4-dev libsqlite3-dev libsqlcipher-dev libantlr-dev

WARNING: The following packages cannot be authenticated!

  libsqlcipher0 libsqlcipher-dev

E: There are problems and -y was used without --force-yes

The command "sudo apt-get install -qq libqt4-dev libsqlite3-dev libsqlcipher-dev libantlr-dev" failed and exited with 100 during .

Your build has been stopped.

Maybe add --force-yes to the apt-get line in the .travis.yml file as part of the PR?

It's worth a try. 😄

[revolter]

I saw that but didn't know why it failed. I'll try that tonight. Or should I pull the latest commits on my fork and then merge the master in this patch branch?

[vtronko]

Just amend it or add another commit and squash those into one.

[revolter]

I can amend to a pushed commit? :O But, I'd keep it as a separate commit because it's another thing.

[justinclift]

Yep, GitHub associates this branch in your fork with this PR, so all the changes you do in the fork are automatically "known" by GitHub to be for this PR.

It means you can completely redo all of the commits however you want them (as long as you keep them in the same branch), and they'll all be part of this PR. Generally it's very convenient. 😄

With that latest commit, that seems to have worked. I haven't had a chance to test the code yet, but probably will tomorrow.

In the meantime, code wise it looks ok to me. But I don't have much Qt/C++ experience yet. Emailed @MKleusberg to review it, but haven't heard back yet.

Maybe @innermous would be ok to review it instead? @innermous What do you reckon? 😄

[revolter]

I knew that the branch is in sync with this PR, but I didn't know I can edit commits that are pushed to GitHub. I usually edit unpushed commits in my workflow. But thanks for the explanation 😄

[justinclift]

Cool. 😄

[vtronko]

You figured out how to update commits in PR? If not, write me in Gmail I'll explain if anything is wrong

[justinclift]

Trying this out now, the .ui sizing seems to be off. This is on OSX 10.9.5, using a non-HiDPI monitor. 😉

Testing out the functionality itself... it's not working properly for me.

Created a brand new test database here "Untitled2.db", set the password to "abc123" with a page size of 2048. Added a few rows of dummy data.

Manually opening the file (before setting password info in the preferences dialog) works fine, with the password & page size prompted, then the database opening correctly when they're given.

After setting the same values in the Preferences dialog, the file no longer opens correctly. A dialog saying "Invalid file format." happens instead, and that's all. (btw, it should probably prompt using the manual password dialog when the automatic ones fail)

I didn't attach a debugging to this to figure out what's going wrong.

Are you ok to investigate? 😄

Can email the failing database file to you, if that would help?

[revolter]

I didn't test with other than 1024 page size (I don't know what page size is 😆). I'll test it these days and let you know if I need the database file.

[revolter]

Well, I can't encrypt the database. I'm following these steps:

• select File > New Database...
• Save As: > Untitles2.db
• click Save
• create a test table in the Edit table definition window that pops up
• click OK
• select File > Set Encryption
• enter abc123 in the Password and Reenter password field
• change Page size field value to 2048
• click OK
• the database closes
• the Invalid file format. popup appears

and I'm left with Untitled3.db and Untitled3.db.enctempold files.

Am I doing something wrong or is it another bug? Or I introduced it? As far as I understood, the problem with the invalid popup was when trying to open an encrypted database, right? Could you email your test database just to make sure it works with it as well after solving the problem?

[revolter]

Well, it looks like that's by design. Untitles2.db is really encrypted, and I think that Untitle2.db.enctempold is just a safety backup.

What do you mean by ".ui sizing"? The fact that I didn't managed to put the Add and Delete buttons next to each other? 😂 I did tried a lot but could figure out how to do this.

Also, I solved the "it should probably prompt using the manual password dialog when the automatic ones fail" problem, and I recall thinking of this but forgot about it, it was so obvious.

Another thing I noticed is that you entered the file with the extension. I made it so it needs just the name of the file, didn't think more thoroughly about this.

I think it needs a discussion. It kinda seems more logic to require the file extension too because of these reasons:

1. one could have test.db and test.sqlite3 files, encrypted differently
2. you, instinctively inserted the name with the file extension too, so there's a chance that that will other users do

[glawrence]

Just a couple of thoughts, has anyone tried building this on Windows? Oh and I tried reading the changes but I can't fathom where the passwords are being stored but I trust we are not storing these passwords in plain text are we?

[revolter]

@glawrence, I don't have access to a Windows OS.

And, the passwords are saved here, but, yes, they are stored in plain text, I guess. I send them without any modification to the setSettingsValue method, which uses QSettings. But it looks like QSettings is saving them in plain text.

What do you think I should do before saving them? Like MD5 them? Should I make the table cell of password type in order for it to display dots or asterisks instead of the actual password?

[revolter]

One more thing, why was the travis change needed? Why didn't it fail for everybody else?

[glawrence]

The first vital point, is "never roll your own crypto", so as long as we use some standard encryption library then we are fine. Not that you were proposing too but it is a point worth emphasising. Sorry, hope you don't mind.

I think the best approach is a "master password", a bit like Firefox, this password is then known by the user and used to decrypt, thus without it nobody can read the passwords. Remember, this is open source, so we need to make sure we protect passwords, so salting and hashing etc.

I appreciate this is a lot of work, so we could give people a statement to accept before using passwords stored in plain text. Then we have some time to consider a better solution. Does that make sense?

Sorry to be a pain and I know security is not always enjoyed but if we do this right and get it right, then we will get respect.

[revolter]

Of course I don't mind. Thanks for drawing my attention too.

I don't have much knowledge of security myself, but I do know that it's very important, even if it's not "enjoyed". So I don't think I can do this all by myself.

Another option is to not get this merged until we fix the plain text passwords problem. Or lose the master password, as it's more complicated than salting and hashing them the same way.

[justinclift]

What do you mean by ".ui sizing"? The fact that I didn't managed to put the Add and
Delete buttons next to each other? 😂 I did tried a lot but couldn't figure out how to
do this.

With that, if you're in Qt Creator you can go into the "Design" part of things to resize the various GUI elements for a dialog. It looks like the part of the dialog to contain the database name, password, and page size, aren't big enough for OSX.

In theory, it should 😄 be a fairly simple fix. You adjust things in the Qt Creator "Design" part, then with git you add the corresponding .ui file to the commit.

If that doesn't make sense, let me know and I'll try to screenshot things in the next few days to demonstrate. 😄

[revolter]

I understood, but I really don't know to do UI in Qt yet. It's very very very different from Xcode with which I'm accustomed. I don't understand how to increase the table widget's width.

[justinclift]

Ahhhh, no worries. On the left side of the Qt Creator GUI there are tabs for coding (called "Edit"), vs GUI Design, vs meta info for the project, etc. eg the Edit part:

To change GUI element sizing, double click on one of the .ui files (in the "Forms" subfolder), then the "Design" tab thing will become available. eg:

In that, you can directly click on the Database tab (in the GUI itself), which will change things to show the GUI elements you're after:

Then drag their edges to resize. eg:

Ugh, the GUI elements aren't resizing for me now for some unknown reason

Well, that sucks. 😉

At a guess, I think the GUI elements you're trying to do might need to be contained in some kind of layout widget.

But, I could be wrong. I'd need to experiment with it further to figure it out, and I don't have time today/tomorrow. 😦

Maybe @innermous or @MKleusberg can offer pointers? 😄

[revolter]

Well, I already found out where and "how" to edit the UI, but, like you noticed, I can't resize widgets.

[MKleusberg]

I'll read through all the comment and review the code tomorrow or on Friday. Then I can provide some more help and feedback, too.

But as for the resiting of widgets: the answer is, you simply can't 😉 Instead they're resized automatically based on the surrounding layouts. You can fine-tune this by adding spacers (appearing as springs in the Qt Creator form designer) or by setting the sizePolicy, minimumSize, and maximumSize properties of the wdget.

Qt docs for Layouts: http://doc.qt.io/qt-5/layout.html

[MKleusberg]

Can we move this into a separate pull request? I think it makes more sense to have this separated because it's not directly related to the cipher stuff we're doing here, and this way we can merge it sooner, too 😃

[revolter]

You'll have to tell me how to do this

[vtronko]

@revolter just open another pull request.

[revolter]

@innermous, And can I remove the latest commit from this PR too?

[MKleusberg]

@revolter I'm looking through your pull request now and will just write my notes into the diff or inside here. I hope that's fine for you 😃 First of all, thanks a lot for picking up this issue - I think you chose a nice task there, not impossible to do but certainly not easy either. If you manage to get this done you'll definitely be fit to do any changes you like. And so far you're doing really good 😃

Regarding the form layout: You can try all sorts of things here to make this look nice. For example you could try to make it look like the extensions list in the Extensions tab of the preferences dialog. I'll try to give you a step by step approach to this. If you have any trouble following me here, just let me know and I'll try to provide clearer instructions and/or screenshots. By the way: don't worry about having problems here. The Qt Creator UI designer often is super unintuitive to use 😉 Anyway, here we go:

1. Open the PeferencesDialog.ui file and switch to the Database tab
2. Right click on a free space inside the form (not on a widget). Click Lay out -> Break layout. Now you can freely move around the widgets and can do whatever you want with them. It's better not to so, however, because later we'll want to put them back into a layout.
3. Click the Add password button, hold the Ctrl key, and click the Remove password button.
4. When both buttons are selected, right click on of them. Click Lay out -> Lay out vertically.
5. Make sure the newly created layout is selected (i.e. the box around both buttons), then hold Ctrl, and click the saved password list box.
6. Right click on it, click Lay out -> Lay out horizontally.
7. Right click on a free bit of space somewhere in the form (having nothing selected) and click Lay out -> Lay out in a grid.
8. Right click again, click Lay out -> Lay out in a form layout.
9. Have a look on the widget list on the left: find the vertical spacer and pull it below the Delete password button but inside the layout surrounding the two buttons. This pushed them to the top.

Of course you can decide for another look and feel of the form, but however you want it to look like, I hope this will get you started 😃

Some other remarks on the form here:

• With this small widget, you should set the hirizontalScrollMode propery of the password table widget to avoid having this 'jumpy' ScrollPerPxel scrolling.
• For selecting the file name it would be nice to have a file dialog. It might be best to just auto open one when clicking into a cell in the first column and/or when clicking the Add password button.
• When prompting the user for a password using the CipherDialog, it'd be nice to have a checkbox in there and - when the checkbox is set - save the password and page size automatically, without having to use the preferences dialog.

[revolter]

@MKleusberg, Currently, when pressing "Add password", the file dialog opens and inserts the file name (including the extension) in the appropriate column.

I don't know how to open it when trying to edit the cell manually though. But I'm inclined to leave it like this, thinking that for the user to edit a database name cell, it means he already pressed the "Add password" before, which let him choose a database.

What do you think?

[glawrence]

@revolter I believe we should make it clear to users that passwords are stored in plain text on their local hard drive, or maybe state the location. That way we are being open and honest and the user knows what is going on, with no surprises

[revolter]

@glawrence, Agree. How will this show up in the UI?

[revolter]

Anyone knows why the Travis failed? It looks like it failed here:

553 1: Test command: 
554 Unable to find executable: test-sqlobjects
555 1/2 Test #1: test-sqlobjects ..................***Not Run   0.00 sec

[revolter]

Actually, I think it failed here:

CMakeFiles/test-sqlobjects.dir/__/sqlitedb.cpp.o: In function `DBBrowserDB::tryEncryptionSettings(QString const&, bool*, CipherDialog*&)':
sqlitedb.cpp:(.text+0x3745): undefined reference to `CipherDialog::isSavePasswordEnabled() const'
sqlitedb.cpp:(.text+0x3893): undefined reference to `CipherDialog::password() const'
sqlitedb.cpp:(.text+0x38eb): undefined reference to `CipherDialog::pageSize() const'
collect2: ld returned 1 exit status
make[2]: *** [src/tests/test-sqlobjects] Error 1
make[1]: *** [src/tests/CMakeFiles/test-sqlobjects.dir/all] Error 2
make: *** [all] Error 2

but don't know why :(

[justinclift]

Maybe add a bold text line above the "Add password" horizontal layout thing, saying something like?

   BE AWARE: These passwords are stored on your hard disk in plain text.  If someone
             gets access to your computer, they are easy to retrieve.

[justinclift]

@glawrence Your idea of a password vault thing seems decent.

eg We create a small SQLcipher encrypted database just for holding the passwords, and the user can enter a master password to open it up.

That being said... we'd want it to be the most thoroughly vetted/stable/unbuggy code anyone has ever seen. Just the thought of an accidental bug corrupting such a vault for potentially several hundred thousand users gives me the shivers. 😱

With plain text, hopefully that's less of an issue.

Hmmm... thinking about it more, we should probably encourage people to backup their vault regardless (whether text or encrypted database), just to lessen the chance of disaster. 😄

[revolter]

So what's the plan?

Also, if we use a small SQLCipher encrypted database, the password needed to access it would be in this open source code, so what would be the point of it?

[justinclift]

Nah, it'd be a "master password" set by the user. Not something we'd generate and store in the code ourselves.

[justinclift]

Hmmm... I'm tending towards thinking the master password vault thing might be the best direction to go.

@MKleusberg @glawrence Opinions?

[revolter]

Sorry, I didn't understand well the first time. It sounds good, but I don't know where I would start 🤔

[justinclift]

Ahhh, I probably explained the thought badly then. 😄

So, imagine that the first time the user clicks the "Add Password" button, a dialog pops up and says something like:

   Hi, before storing your passwords we need to create a safe place to keep them.

   We'll do that by creating an encrypted database in [some path name], and we'll
   lock it with a master password of your selection.

                                     [OK][Cancel]

If they click OK, then it asks for a password to use for encrypting this "password vault" database.

This "password vault" database is a simple SQLite database (with SQLCipher encryption), containing the fields you are showing in the images above:

• Filename
• Password
• Page size

When the user has given the "master password" to use, the code (in this PR) creates the database and encrypts it.

From then on, when a user presses the "Add Password" button, the info gets stored into this database.

Is that making sense so far?

[justinclift]

Hmmm, anyone know of decent security people we could ping to get some advice about this?

I can email Bruce Schneier and see if it's his kind of thing if that would be useful.

[revolter]

Well, I understood the thought pretty well, I was referring to the implementation of this 😂

Also, with this idea, when a user opens a crypted database, will he be required to enter that master password? If yes, that would be the exact opposite of what I tried to achieve using this PR.

In my very own personal case, I have multiple databases in multiple locations, and I often switch between them, and I want to open them by just double click.

If you think this is a bad idea and/or not useful for other people, I'll just keep this version in my fork and periodically sync and install it (😢 ).

[justinclift]

Ahhh... yeah, the "user needs to enter their password every time" was the thought.

Agreed, it does completely remove the ease-of-use approach you were going for.

Maybe the plain text approach + warning is the right idea for this after all.

[revolter]

I don't want to lead to bad decisions though. @sqlitebrowser/developers, would it be ok (do you have time) to discuss this and agree a safe solution for this, before the next release? 😄

[glawrence]

Some thoughts on this...

• FileZilla stores passwords as Base64 encoded, this has been criticised by a lot of people, hence my concern about storing in plain text
• Adding a warning that we are storing passwords in plain text is a more open option
• If the data in a database needs a password then I am sorry but in my view access should not be convenient
• we should store passwords properly, plain text and a warning is not good
• the "master password" should be used to encrypt the passwords and hence without it they are unreadable
• other open source projects work with this, like Firefox and Chrome, so it is possible, just hard work
• I am now wondering what Oracle's SQL Developer does......

[revolter]

Thank a lot for the feedback!

So, the database needs a password for it to be accessed in an iOS app, but when working on it, I would want to access it fast, while keeping it encrypted.

I am definitely not saying you are wrong or something, it was just my use case.

But I still find the master password a little too much. Any idea how is Thunderbird storing the email's passwords? I can easily accesess password protected account without the need of a password. Can we apply the same reasoning/logic/implementation in DB4S?

[justinclift]

Yeah, this is the classic security-vs-convenience dilemma.

On one hand, adding a text based password vault is maximum convenience. But completely nukes security if someone has access to the local PC.

With a text based solution, the security of the databases doesn't sound like it would be compromised for the use case where they're really only intended for external distribution - eg to specific other people on different computers. Only yourself (and the external people) should have the password. Anyone else obtaining it (eg in transit) wouldn't have it.

I wonder if there's a better way? eg support both approaches, and the user can pick the one that suits them?

[revolter]

I think supporting multiple (different) solutions is an superfluous overhead for the project. Maybe we should drop this or delay it until: one great solution is found / I have time to implement it / more users asks for this feature.

[justinclift]

The Thunderbird source should be online somewhere. Depending on how clear the code is, it might be feasible to look through that for ideas. Or even the Firefox code?

[justinclift]

k, going to close this for now. If we figure out a better solution at some point, we can open it again or make a new issue. 😄

[glawrence]

If you read https://support.mozilla.org/en-US/kb/password-manager-remember-delete-change-and-import and https://support.mozilla.org/en-US/kb/master-password you get a good idea of what is going on. The interesting item is if you reset your master password you lose everything. This is because the master is used to encrypt, without it you are stuffed. Yes, the source code will provide the answers but I think as a high-level design this is what we should work towards.

[revolter]

Well, I did take a look at Firefox's help page and started a draft comment which I'll post now as it is, not to forget about it at all, and we can discuss more when I get back from the vacation.

[revolter]

It looks like Firefox is offering a master password as an optional feature as described here:

On the other hand, putting all your logins in one place, unprotected, can be risky. Theoretically, someone who has access to your computer could open and view your logins in the Firefox Password Manager and go on an Amazon shopping spree ! Fortunately, Firefox lets you use a master password to prevent others from accessing your logins without your permission.

and here:

Even though the Password Manager stores your usernames and passwords on your hard drive in an encrypted format, someone with access to your computer can still see or use them. The Use a Master Password to protect stored logins and passwords article shows you how to prevent this and keep you protected in the event your computer is lost or stolen.

So Firefox (and probably others too) is storing the passwords on the disk encrypted, most probably with a key that's in the actual source code, so it won't need 0 difficulty to retrieve them at least.

[glawrence]

Agreed. So without a "master" password the passwords are recoverable. However with a master password they are, I believe, encrypted with the master password, so lose that and they are all lost. This means those that are not too worried can store their passwords and easily get them. However the more concerned can encrypt the password store with a "master" password, making them readable, only with the master password. I trust that makes sense.

Like you I just brain dumped here. I think we might need to start a Wiki page or something and agree some workflows or something. No hurry though, all in good time.