Use secure temp directory for remote sync (#10911)

share/translations/keepassxc_en.ts

@@ -8960,6 +8960,14 @@ This option is deprecated, use --set-key-file instead.</source>
         <source>Failed to upload merged database. Command `%1` exited with status code: %2</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Could not create temporary directory &apos;%1&apos;</source>
+        <translation type="unfinished"></translation>
+    </message>
+    <message>
+        <source>Could not change permissions of temporary directory &apos;%1&apos; to owner</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>ReportsWidgetBrowserStatistics</name>

src/gui/DatabaseWidget.cpp

@@ -1145,6 +1145,8 @@ void DatabaseWidget::uploadAndFinishSync(const RemoteParams* params, RemoteHandl
 
 void DatabaseWidget::finishSync(const RemoteParams* params, RemoteHandler::RemoteResult result)
 {
+    QScopedPointer<RemoteHandler> remoteHandler(new RemoteHandler(this));
+    remoteHandler->cleanup(result.filePath);
     setDisabled(false);
     emit updateSyncProgress(-1, "");
     if (result.success) {

src/gui/remote/DatabaseSettingsWidgetRemote.cpp

@@ -177,24 +177,27 @@ void DatabaseSettingsWidgetRemote::testDownload()
     params->downloadCommand = m_ui->downloadCommand->text();
     params->downloadInput = m_ui->inputForDownload->toPlainText();
 
-    QScopedPointer<RemoteHandler> remoteHandler(new RemoteHandler(this));
     if (params->downloadCommand.isEmpty()) {
         m_ui->messageWidget->showMessage(tr("Download command cannot be empty."), MessageWidget::Warning);
         return;
     }
 
+    QScopedPointer<RemoteHandler> remoteHandler(new RemoteHandler(this));
     RemoteHandler::RemoteResult result = remoteHandler->download(params);
     if (!result.success) {
         m_ui->messageWidget->showMessage(tr("Download failed with error: %1").arg(result.errorMessage),
                                          MessageWidget::Error);
+        remoteHandler->cleanup(result.filePath);
         return;
     }
 
     if (!QFile::exists(result.filePath)) {
         m_ui->messageWidget->showMessage(tr("Download finished, but file %1 could not be found.").arg(result.filePath),
                                          MessageWidget::Error);
+        remoteHandler->cleanup(result.filePath);
         return;
     }
 
+    remoteHandler->cleanup(result.filePath);
     m_ui->messageWidget->showMessage(tr("Download successful."), MessageWidget::Positive);
-}
+}

src/gui/remote/RemoteHandler.cpp

@@ -23,16 +23,6 @@
 #include "core/AsyncTask.h"
 #include "core/Database.h"
 
-namespace
-{
-    QString getTempFileLocation()
-    {
-        QString uuid = QUuid::createUuid().toString().remove(0, 1);
-        uuid.chop(1);
-        return QDir::toNativeSeparators(QDir::temp().absoluteFilePath("RemoteDatabase-" + uuid + ".kdbx"));
-    }
-} // namespace
-
 std::function<QScopedPointer<RemoteProcess>(QObject*)> RemoteHandler::m_createRemoteProcess([](QObject* parent) {
     return QScopedPointer<RemoteProcess>(new RemoteProcess(parent));
 });
@@ -57,7 +47,15 @@ RemoteHandler::RemoteResult RemoteHandler::download(const RemoteParams* params)
             return result;
         }
 
-        auto filePath = getTempFileLocation();
+        QString error;
+        auto filePath = getTempFileLocation(&error);
+        result.filePath = filePath;
+        if (!error.isEmpty()) {
+            result.success = false;
+            result.errorMessage = error;
+            return result;
+        }
+
         auto remoteProcess = m_createRemoteProcess(nullptr); // use nullptr parent, otherwise there is a warning
         remoteProcess->setTempFileLocation(filePath);
         remoteProcess->start(params->downloadCommand);
@@ -82,7 +80,6 @@ RemoteHandler::RemoteResult RemoteHandler::download(const RemoteParams* params)
                 result.errorMessage = tr("Command `%1` failed to download database.").arg(params->downloadCommand);
             } else {
                 result.success = true;
-                result.filePath = filePath;
             }
         } else if (finished) {
             result.success = false;
@@ -103,6 +100,7 @@ RemoteHandler::RemoteResult RemoteHandler::upload(const QString& filePath, const
 {
     return AsyncTask::runAndWaitForFuture([filePath, params] {
         RemoteResult result;
+        result.filePath = filePath;
         if (!params) {
             result.success = false;
             result.errorMessage = tr("Invalid database pointer or upload parameters provided.");
@@ -143,3 +141,39 @@ RemoteHandler::RemoteResult RemoteHandler::upload(const QString& filePath, const
         return result;
     });
 }
+
+QString RemoteHandler::getTempFileLocation(QString* error)
+{
+    QString uuid = QUuid::createUuid().toString().remove(0, 1);
+    uuid.chop(1);
+    QString writableLocation = QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation);
+    if (writableLocation.isEmpty()) {
+        writableLocation = QStandardPaths::writableLocation(QStandardPaths::TempLocation);
+    }
+
+    QString tempDirLocation = QDir(writableLocation).absoluteFilePath(PREFIX + uuid);
+    QString tempFileLocation =
+        QDir::toNativeSeparators(QDir(tempDirLocation).absoluteFilePath("RemoteDatabase-" + uuid + ".kdbx"));
+
+    if (!QDir().mkdir(tempDirLocation)) {
+        *error = tr("Could not create temporary directory '%1'").arg(tempDirLocation);
+        return "";
+    }
+
+    if (!QFile::setPermissions(tempDirLocation,
+                               QFileDevice::ReadOwner | QFileDevice::WriteOwner | QFileDevice::ExeOwner)) {
+        QDir(tempDirLocation).removeRecursively();
+        *error = tr("Could not change permissions of temporary directory '%1' to owner").arg(tempDirLocation);
+        return "";
+    }
+
+    return tempFileLocation;
+}
+
+void RemoteHandler::cleanup(QString& tempFileLocation)
+{
+    QFileInfo file(tempFileLocation);
+    if (file.absoluteDir().exists() && file.absoluteDir().dirName().startsWith(PREFIX)) {
+        file.absoluteDir().removeRecursively();
+    }
+}

src/gui/remote/RemoteHandler.h

@@ -44,12 +44,16 @@ class RemoteHandler : public QObject
     RemoteResult download(const RemoteParams* params);
     RemoteResult upload(const QString& filePath, const RemoteParams* params);
 
+    void cleanup(QString& tempFileLocation);
+
     // Used for testing only
     static void setRemoteProcessFunc(std::function<QScopedPointer<RemoteProcess>(QObject*)> func);
 
 private:
+    static QString getTempFileLocation(QString* error);
+
     static std::function<QScopedPointer<RemoteProcess>(QObject*)> m_createRemoteProcess;
-    static QString m_tempFileLocation;
+    inline static const QString PREFIX = "KPXC-Sync-";
 
     Q_DISABLE_COPY(RemoteHandler)
 };

src/gui/remote/RemoteProcess.cpp

@@ -17,7 +17,6 @@
 
 #include "RemoteProcess.h"
 
-#include <QTemporaryDir>
 #include <QUuid>
 
 RemoteProcess::RemoteProcess(QObject* parent)