Fix permissions changing on database save

* Saving a database in unsafe mode retains the existing permissions on the kdbx file * New databases (save as, save backup, new database) and new key files are saved with 0600 permissions (user read/write), fixes keepassxreboot#2575
iSC-Labs · Jun 7, 2020 · fbebf30 · fbebf30
1 parent 1ad0184
commit fbebf30
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 4 deletions.
diff --git a/src/core/Database.cpp b/src/core/Database.cpp
@@ -253,10 +253,14 @@ bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool
 
     QFileInfo fileInfo(filePath);
     auto realFilePath = fileInfo.exists() ? fileInfo.canonicalFilePath() : fileInfo.absoluteFilePath();
+    bool isNewFile = !QFile::exists(realFilePath);
     bool ok = AsyncTask::runAndWaitForFuture([&] { return performSave(realFilePath, error, atomic, backup); });
     if (ok) {
         markAsClean();
         setFilePath(filePath);
+        if (isNewFile) {
+            QFile::setPermissions(realFilePath, QFile::ReadUser | QFile::WriteUser);
+        }
         m_fileWatcher->start(realFilePath, 30, 1);
     } else {
         // Saving failed, don't rewatch file since it does not represent our database
@@ -304,6 +308,7 @@ bool Database::performSave(const QString& filePath, QString* error, bool atomic,
             }
 
             // Delete the original db and move the temp file in place
+            auto perms = QFile::permissions(filePath);
             QFile::remove(filePath);
 
             // Note: call into the QFile rename instead of QTemporaryFile
@@ -312,6 +317,7 @@ bool Database::performSave(const QString& filePath, QString* error, bool atomic,
             if (tempFile.QFile::rename(filePath)) {
                 // successfully saved the database
                 tempFile.setAutoRemove(false);
+                QFile::setPermissions(filePath, perms);
                 return true;
             } else if (!backup || !restoreDatabase(filePath)) {
                 // Failed to copy new database in place, and
@@ -454,9 +460,12 @@ bool Database::backupDatabase(const QString& filePath)
 
     auto match = re.match(filePath);
     auto backupFilePath = filePath;
+    auto perms = QFile::permissions(filePath);
     backupFilePath = backupFilePath.replace(re, "") + ".old" + match.captured(1);
     QFile::remove(backupFilePath);
-    return QFile::copy(filePath, backupFilePath);
+    bool res = QFile::copy(filePath, backupFilePath);
+    QFile::setPermissions(backupFilePath, perms);
+    return res;
 }
 
 /**
@@ -472,11 +481,13 @@ bool Database::restoreDatabase(const QString& filePath)
     static auto re = QRegularExpression("^(.*?)(\\.[^.]+)?$");
 
     auto match = re.match(filePath);
+    auto perms = QFile::permissions(filePath);
     auto backupFilePath = match.captured(1) + ".old" + match.captured(2);
     // Only try to restore if the backup file actually exists
     if (QFile::exists(backupFilePath)) {
         QFile::remove(filePath);
         return QFile::copy(backupFilePath, filePath);
+        QFile::setPermissions(filePath, perms);
     }
     return false;
 }

diff --git a/src/keys/FileKey.cpp b/src/keys/FileKey.cpp
@@ -200,6 +200,7 @@ bool FileKey::create(const QString& fileName, QString* errorMsg, int size)
     }
     create(&file, size);
     file.close();
+    file.setPermissions(QFile::ReadUser);
 
     if (file.error()) {
         if (errorMsg) {

diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp
@@ -680,7 +680,7 @@ void TestGuiFdoSecrets::testCollectionCreate()
 
         QCOMPARE(spyCollectionCreated.count(), 1);
         {
-            auto args = spyCollectionCreated.takeFirst();
+            args = spyCollectionCreated.takeFirst();
             QCOMPARE(args.size(), 1);
             QCOMPARE(args.at(0).value<Collection*>(), coll);
         }
@@ -754,7 +754,7 @@ void TestGuiFdoSecrets::testCollectionDelete()
 
     QCOMPARE(spyCollectionDeleted.count(), 1);
     {
-        auto args = spyCollectionDeleted.takeFirst();
+        args = spyCollectionDeleted.takeFirst();
         QCOMPARE(args.size(), 1);
         QCOMPARE(args.at(0).value<Collection*>(), rawColl);
     }
@@ -977,7 +977,7 @@ void TestGuiFdoSecrets::testItemDelete()
 
     QCOMPARE(spyItemDeleted.count(), 1);
     {
-        auto args = spyItemDeleted.takeFirst();
+        args = spyItemDeleted.takeFirst();
         QCOMPARE(args.size(), 1);
         QCOMPARE(args.at(0).value<Item*>(), rawItem);
     }