Skip to content
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

Can't remove password from an encrypted database any more #1829

Closed
justinclift opened this Issue Mar 30, 2019 · 14 comments

Comments

Projects
None yet
2 participants
@justinclift
Copy link
Member

justinclift commented Mar 30, 2019

Just discovered an important bug when testing out an initial build of 3.11.2 release binaries.

Previously (eg in 3.10.1), to remove the encryption from a database it just needed to be saved ("Set encyption") with an empty password. eg nothing in the password and password confirmation fields

With our current v3.11.x branch, if nothing is put in the password and password confirmation fields then something seems to internally automatically reuse the current password.

So, removing the password from an encrypted database doesn't seem to work any more.

Completely guessing here... maybe some internal variable just needs the new "empty" setting applied?

@MKleusberg I think this is one for you. 😄

@justinclift justinclift added this to the 3.11.2 milestone Mar 30, 2019

@justinclift justinclift referenced this issue Mar 30, 2019

Closed

3.11.2 - outstanding pieces #1773

13 of 13 tasks complete
@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Mar 30, 2019

I'm still using SQLCipher 3.4.1 here and can't seem to reproduce it with that. So this is going to be a blind one 😉 As a first guess, can you try with this patch applied?

diff --git a/src/MainWindow.cpp b/src/MainWindow.cpp
index 06ea7a0b..b1c44a52 100644
--- a/src/MainWindow.cpp
+++ b/src/MainWindow.cpp
@@ -3225,14 +3225,18 @@ void MainWindow::editEncryption()
             ok = db.executeSQL(QString("ATTACH DATABASE '%1' AS sqlitebrowser_edit_encryption KEY %2;").arg(db.currentFile() + ".enctemp").arg(cipherSettings.getPassword()),
                                false, false);
         qApp->processEvents();
-        if(ok)
-            ok = db.executeSQL(QString("PRAGMA sqlitebrowser_edit_encryption.cipher_page_size = %1").arg(cipherSettings.getPageSize()), false, false);
-        if(ok)
-            ok = db.executeSQL(QString("PRAGMA sqlitebrowser_edit_encryption.cipher_hmac_algorithm = %1").arg(cipherSettings.getHmacAlgorithm()), false, false);
-        if(ok)
-            ok = db.executeSQL(QString("PRAGMA sqlitebrowser_edit_encryption.cipher_kdf_algorithm = %1").arg(cipherSettings.getKdfAlgorithm()), false, false);
-        if(ok)
-            ok = db.executeSQL(QString("PRAGMA sqlitebrowser_edit_encryption.kdf_iter = %1").arg(cipherSettings.getKdfIterations()), false, false);
+
+        if(cipherSettings.getPassword() != "''")
+        {
+            if(ok)
+                ok = db.executeSQL(QString("PRAGMA sqlitebrowser_edit_encryption.cipher_page_size = %1").arg(cipherSettings.getPageSize()), false, false);
+            if(ok)
+                ok = db.executeSQL(QString("PRAGMA sqlitebrowser_edit_encryption.cipher_hmac_algorithm = %1").arg(cipherSettings.getHmacAlgorithm()), false, false);
+            if(ok)
+                ok = db.executeSQL(QString("PRAGMA sqlitebrowser_edit_encryption.cipher_kdf_algorithm = %1").arg(cipherSettings.getKdfAlgorithm()), false, false);
+            if(ok)
+                ok = db.executeSQL(QString("PRAGMA sqlitebrowser_edit_encryption.kdf_iter = %1").arg(cipherSettings.getKdfIterations()), false, false);
+        }
 
         // Export the current database to the new one
         qApp->processEvents();

Also, did you check whether you can change the password instead of removing it?

@justinclift

This comment has been minimized.

Copy link
Member Author

justinclift commented Mar 30, 2019

Sure, gimme a few minutes. 😄

@justinclift

This comment has been minimized.

Copy link
Member Author

justinclift commented Mar 30, 2019

As a thought, for experimental patches like this... just chuck them in another branch and point to it. I have to put them in another branch + update the build script to use that alternative branch anyway. 😄

justinclift added a commit that referenced this issue Mar 30, 2019

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Mar 30, 2019

Sure, I'll do that next time 😄

@justinclift

This comment has been minimized.

Copy link
Member Author

justinclift commented Mar 30, 2019

k, it's not working for removing the password.

@justinclift

This comment has been minimized.

Copy link
Member Author

justinclift commented Mar 30, 2019

Interestingly... using a different password doesn't use the different password either. It's still encrypted using the old one.

Guessing that's the real problem. 😄

@justinclift

This comment has been minimized.

Copy link
Member Author

justinclift commented Mar 30, 2019

Just in case it makes a difference, I've only been trying this with the "SQLCipher 4 defaults" option selected.

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Mar 30, 2019

That's weird. But encrypting from plain database works? I have pushed a new commit to the same branch with my next attempt to fix this. I've also just tried the SQLCipher 4 defaults option and it made no difference for me. So if that last commit doesn't help I'll probably need some time to come up with a new idea 😉

@justinclift

This comment has been minimized.

Copy link
Member Author

justinclift commented Mar 30, 2019

But encrypting from plain database works?

Yep. Or at least it did before we started adding these patches. I'll rebuild and try again. 😄

@justinclift

This comment has been minimized.

Copy link
Member Author

justinclift commented Mar 30, 2019

Yep, that last patch fixed the problem. Now the new password takes effect, and passwords can be removed by leaving the new password fields blank.

Well done @MKleusberg. 😄

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Mar 30, 2019

Awesome! I'll make a proper commit out of it and apply it to master and v3.11.x in a second 😄

@justinclift

This comment has been minimized.

Copy link
Member Author

justinclift commented Mar 30, 2019

Tested with both SQLCipher 3 and SQLCipher 4 defaults. All good. 😄

Thanks. 😄

We forgot to notify the translators, which @mgrojo has just done. So, we'd better leave the actual binary building for a bit after all. Oops, 🤕

I'm calling today a good practise run. Heh. 😉

MKleusberg added a commit that referenced this issue Mar 30, 2019

Fix editing encryption when using SQLCipher 4
It seems like we need to explicitly detach the newly encrypted database
when changing the encryption settings and using SQLCipher 4.

See issue #1829.

MKleusberg added a commit that referenced this issue Mar 30, 2019

Fix editing encryption when using SQLCipher 4
It seems like we need to explicitly detach the newly encrypted database
when changing the encryption settings and using SQLCipher 4.

See issue #1829.
@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Mar 30, 2019

Heh heh, no worries 😄

The fix is applied to both branches now. Good catch by the way 👍

@justinclift

This comment has been minimized.

Copy link
Member Author

justinclift commented Mar 30, 2019

Awesome. Thanks heaps. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.