File 0001-Revert-KAuth-integration-in-document-savin... of Package ktexteditor

Overview Repositories Revisions Requests Users Attributes Meta

File 0001-Revert-KAuth-integration-in-document-saving-vol.-2.patch of Package ktexteditor (Revision 0ae66805f9384f11d2b14b29922a4aa8)

Currently displaying revision 0ae66805f9384f11d2b14b29922a4aa8 , Show latest

From fb620c4bd1621a6141c5beb3a8dd429767fb8468 Mon Sep 17 00:00:00 2001
From: Fabian Vogt <fvogt@suse.com>
Date: Mon, 15 May 2017 16:12:55 +0200
Subject: [PATCH 1/2] Revert "KAuth integration in document saving - vol. 2"

This reverts commit f7a9573d973e6ef0cd6f2c419290c0c7e46381b7.
---
 src/buffer/katesecuretextbuffer.cpp | 164 +++++++++++++++++-------------------
 src/buffer/katesecuretextbuffer_p.h |  31 ++++---
 src/buffer/katetextbuffer.cpp       | 136 +++++++++++++++++-------------
 src/buffer/katetextbuffer.h         |   5 --
 4 files changed, 173 insertions(+), 163 deletions(-)

diff --git a/src/buffer/katesecuretextbuffer.cpp b/src/buffer/katesecuretextbuffer.cpp
index 98b96cef..979bb62f 100644
--- a/src/buffer/katesecuretextbuffer.cpp
+++ b/src/buffer/katesecuretextbuffer.cpp
@@ -27,107 +27,114 @@
 
 #include <QString>
 #include <QFile>
-#include <QDir>
-#include <QFileInfo>
 #include <QTemporaryFile>
+#include <QSaveFile>
 
 KAUTH_HELPER_MAIN("org.kde.ktexteditor.katetextbuffer", SecureTextBuffer)
 
 ActionReply SecureTextBuffer::savefile(const QVariantMap &args)
 {
-    const QString sourceFile = args[QLatin1String("sourceFile")].toString();
+    const ActionMode actionMode = static_cast<ActionMode>(args[QLatin1String("actionMode")].toInt());
     const QString targetFile = args[QLatin1String("targetFile")].toString();
-    const QByteArray checksum = args[QLatin1String("checksum")].toByteArray();
     const uint ownerId = (uint) args[QLatin1String("ownerId")].toInt();
-    const uint groupId = (uint) args[QLatin1String("groupId")].toInt();
 
-    if (saveFileInternal(sourceFile, targetFile, checksum, ownerId, groupId)) {
-        return ActionReply::SuccessReply();
-    }
+    if (actionMode == ActionMode::Prepare) {
 
-    return ActionReply::HelperErrorReply();
-}
+        const QString temporaryFile = prepareTempFileInternal(targetFile, ownerId);
 
-bool SecureTextBuffer::saveFileInternal(const QString &sourceFile, const QString &targetFile,
-                                        const QByteArray &checksum, const uint ownerId, const uint groupId)
-{
-    QFileInfo targetFileInfo(targetFile);
-    if (!QDir::setCurrent(targetFileInfo.dir().path())) {
-        return false;
-    }
+        if (temporaryFile.isEmpty()) {
+            return ActionReply::HelperErrorReply();
+        }
+
+        ActionReply reply;
+        reply.addData(QLatin1String("temporaryFile"), temporaryFile);
+
+        return reply;
 
-    // get information about target file
-    const QString targetFileName = targetFileInfo.fileName();
-    targetFileInfo.setFile(targetFileName);
-    const bool newFile = !targetFileInfo.exists();
-
-    // open source and target file
-    QFile readFile(sourceFile);
-    //TODO use QSaveFile for saving contents and automatic atomic move on commit() when QSaveFile's security problem
-    // (default temporary file permissions) is fixed
-    //
-    // We will first generate temporary filename and then use it relatively to prevent an attacker
-    // to trick us to write contents to a different file by changing underlying directory.
-    QTemporaryFile tempFile(targetFileName);
-    if (!tempFile.open()) {
-        return false;
-    }
-    tempFile.close();
-    QString tempFileName = QFileInfo(tempFile).fileName();
-    tempFile.setFileName(tempFileName);
-    if (!readFile.open(QIODevice::ReadOnly) || !tempFile.open()) {
-        return false;
     }
-    const int tempFileDescriptor = tempFile.handle();
 
-    // prepare checksum maker
-    QCryptographicHash cryptographicHash(checksumAlgorithm);
+    if (actionMode == ActionMode::Move) {
 
-    // copy contents
-    char buffer[bufferLength];
-    qint64 read = -1;
-    while ((read = readFile.read(buffer, bufferLength)) > 0) {
-        cryptographicHash.addData(buffer, read);
-        if (tempFile.write(buffer, read) == -1) {
-            return false;
+        const QString sourceFile = args[QLatin1String("sourceFile")].toString();
+        const uint groupId = (uint) args[QLatin1String("groupId")].toInt();
+
+        if (moveFileInternal(sourceFile, targetFile, ownerId, groupId)) {
+            return ActionReply::SuccessReply();
         }
     }
 
-    // check that copying was successful and checksum matched
-    QByteArray localChecksum = cryptographicHash.result();
-    if (read == -1 || localChecksum != checksum || !tempFile.flush()) {
-        return false;
+    return ActionReply::HelperErrorReply();
+}
+
+bool SecureTextBuffer::moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId)
+{
+    const bool newFile = !QFile::exists(targetFile);
+    bool atomicRenameSucceeded = false;
+
+    /**
+     * There is no atomic rename operation publicly exposed by Qt.
+     *
+     * We use std::rename for UNIX and for now no-op for windows (triggers fallback).
+     *
+     * As fallback we are copying source file to destination with the help of QSaveFile
+     * to ensure targetFile is overwritten safely.
+     */
+#ifndef Q_OS_WIN
+    const int result = std::rename(QFile::encodeName(sourceFile).constData(), QFile::encodeName(targetFile).constData());
+    if (result == 0) {
+        syncToDisk(QFile(targetFile).handle());
+        atomicRenameSucceeded = true;
     }
+#else
+    atomicRenameSucceeded = false;
+#endif
 
-    tempFile.close();
+    if (!atomicRenameSucceeded) {
+        // as fallback copy the temporary file to the target with help of QSaveFile
+        QFile readFile(sourceFile);
+        QSaveFile saveFile(targetFile);
+        if (!readFile.open(QIODevice::ReadOnly) || !saveFile.open(QIODevice::WriteOnly)) {
+            return false;
+        }
+        char buffer[bufferLength];
+        qint64 read = -1;
+        while ((read = readFile.read(buffer, bufferLength)) > 0) {
+            if (saveFile.write(buffer, read) == -1) {
+                return false;
+            }
+        }
+        if (read == -1 || !saveFile.commit()) {
+            return false;
+        }
+    }
 
-    if (newFile) {
-        // ensure new file is readable by anyone
-        tempFile.setPermissions(tempFile.permissions() | QFile::Permission::ReadGroup | QFile::Permission::ReadOther);
-    } else {
-        // ensure the same file permissions
-        tempFile.setPermissions(targetFileInfo.permissions());
+    if (!newFile) {
         // ensure file has the same owner and group as before
-        setOwner(tempFileDescriptor, ownerId, groupId);
+        setOwner(targetFile, ownerId, groupId);
     }
 
-    // rename temporary file to the target file
-    if (moveFile(tempFileName, targetFileName)) {
-        // temporary file was renamed, there is nothing to remove anymore
-        tempFile.setAutoRemove(false);
-        return true;
+    return true;
+}
+
+QString SecureTextBuffer::prepareTempFileInternal(const QString &targetFile, const uint ownerId)
+{
+    QTemporaryFile tempFile(targetFile);
+    if (!tempFile.open()) {
+        return QString();
     }
-    return false;
+    tempFile.setAutoRemove(false);
+    setOwner(tempFile.fileName(), ownerId, -1);
+    return tempFile.fileName();
 }
 
-void SecureTextBuffer::setOwner(const int filedes, const uint ownerId, const uint groupId)
+void SecureTextBuffer::setOwner(const QString &filename, const uint ownerId, const uint groupId)
 {
 #ifndef Q_OS_WIN
     if (ownerId != (uint)-2 && groupId != (uint)-2) {
-        const int result = fchown(filedes, ownerId, groupId);
+        const int result = chown(QFile::encodeName(filename).constData(), ownerId, groupId);
         // set at least correct group if owner cannot be changed
         if (result != 0 && errno == EPERM) {
-            fchown(filedes, getuid(), groupId);
+            chown(QFile::encodeName(filename).constData(), getuid(), groupId);
         }
     }
 #else
@@ -135,22 +142,6 @@ void SecureTextBuffer::setOwner(const int filedes, const uint ownerId, const uin
 #endif
 }
 
-bool SecureTextBuffer::moveFile(const QString &sourceFile, const QString &targetFile)
-{
-#ifndef Q_OS_WIN
-    const int result = std::rename(QFile::encodeName(sourceFile).constData(), QFile::encodeName(targetFile).constData());
-    if (result == 0) {
-        syncToDisk(QFile(targetFile).handle());
-        return true;
-    }
-    return false;
-#else
-    // use racy fallback for windows
-    QFile::remove(targetFile);
-    return QFile::rename(sourceFile, targetFile);
-#endif
-}
-
 void SecureTextBuffer::syncToDisk(const int fd)
 {
 #ifndef Q_OS_WIN
@@ -162,5 +153,4 @@ void SecureTextBuffer::syncToDisk(const int fd)
 #else
     // no-op for windows
 #endif
-}
-
+}
\ No newline at end of file
diff --git a/src/buffer/katesecuretextbuffer_p.h b/src/buffer/katesecuretextbuffer_p.h
index a38285b6..b931aa27 100644
--- a/src/buffer/katesecuretextbuffer_p.h
+++ b/src/buffer/katesecuretextbuffer_p.h
@@ -23,7 +23,6 @@
 
 #include <QObject>
 #include <QString>
-#include <QCryptographicHash>
 
 #include <kauth.h>
 
@@ -44,29 +43,39 @@ class SecureTextBuffer : public QObject
 
 public:
 
+    /**
+     * We support Prepare action for temporary file creation
+     * and Move action for moving final file to its destination
+     */
+    enum ActionMode {
+        Prepare = 1,
+        Move = 2
+    };
+
     SecureTextBuffer() {}
 
     ~SecureTextBuffer() {}
 
     /**
-     * Common helper method
+     * Common helper methods
      */
-    static void setOwner(const int filedes, const uint ownerId, const uint groupId);
-
-    static const QCryptographicHash::Algorithm checksumAlgorithm = QCryptographicHash::Algorithm::Sha512;
+    static void setOwner(const QString &filename, const uint ownerId, const uint groupId);
+    static void syncToDisk(const int fd);
 
 private:
     static const qint64 bufferLength = 4096;
 
     /**
-     * Saves file contents using sets permissions.
+     * Creates temporary file based on given target file path.
+     * Temporary file is set to not be deleted on object destroy
+     * so KTextEditor can save contents in it.
      */
-    static bool saveFileInternal(const QString &sourceFile, const QString &targetFile,
-                                        const QByteArray &checksum, const uint ownerId, const uint groupId);
+    static QString prepareTempFileInternal(const QString &targetFile, const uint ownerId);
 
-    static bool moveFile(const QString &sourceFile, const QString &targetFile);
-
-    static void syncToDisk(const int fd);
+    /**
+     * Move file to its given destination and set owner.
+     */
+    static bool moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId);
 
 public Q_SLOTS:
     /**
diff --git a/src/buffer/katetextbuffer.cpp b/src/buffer/katetextbuffer.cpp
index aa5a4555..95603fa7 100644
--- a/src/buffer/katetextbuffer.cpp
+++ b/src/buffer/katetextbuffer.cpp
@@ -36,9 +36,8 @@
 
 #include <QSaveFile>
 #include <QTemporaryFile>
+#include <QFileDevice>
 #include <QFileInfo>
-#include <QCryptographicHash>
-#include <QBuffer>
 
 #if 0
 #define BUFFER_DEBUG qCDebug(LOG_KTE)
@@ -780,28 +779,72 @@ bool TextBuffer::save(const QString &filename)
     }
 
     /**
-     * use QSaveFile for file saving
+     * use QSaveFile for save write + rename
      */
-    QScopedPointer<QIODevice> saveFile(new QSaveFile(filename));
+    QScopedPointer<QFileDevice> saveFile(new QSaveFile(filename));
     static_cast<QSaveFile *>(saveFile.data())->setDirectWriteFallback(true);
 
-    bool usingTemporaryBuffer = false;
+    bool usingTemporaryFile = false;
+    QScopedPointer<QVariantMap> kAuthActionArgs;
+    QScopedPointer<KAuth::Action> kAuthSaveAction;
 
     // open QSaveFile for write
     if (m_alwaysUseKAuthForSave || !saveFile->open(QIODevice::WriteOnly)) {
 
         // if that fails we need more privileges to save this file
-        // -> we write to a temporary file and then send its path to KAuth action for privileged save
+        // -> we write to temporary file and then move it to target location
 
-        usingTemporaryBuffer = true;
+        usingTemporaryFile = true;
 
-        // we are now saving to a temporary buffer
-        saveFile.reset(new QBuffer());
+        QString targetFilename(filename);
 
-        // open buffer for write and read (read is used for checksum computing and writing to temporary file)
-        if (!saveFile->open(QIODevice::ReadWrite)) {
+        kAuthActionArgs.reset(new QVariantMap());
+        kAuthActionArgs->insert(QLatin1String("actionMode"), SecureTextBuffer::ActionMode::Prepare);
+        kAuthActionArgs->insert(QLatin1String("targetFile"), targetFilename);
+        kAuthActionArgs->insert(QLatin1String("ownerId"), getuid());
+
+        // call save with elevated privileges
+        if (KTextEditor::EditorPrivate::unitTestMode()) {
+
+            // unit testing purposes only
+            ActionReply reply = SecureTextBuffer::savefile(*kAuthActionArgs);
+            if (!reply.succeeded()) {
+                return false;
+            }
+            targetFilename = reply.data()[QLatin1String("temporaryFile")].toString();
+
+        } else {
+
+            // call action
+            kAuthSaveAction.reset(new KAuth::Action(QLatin1String("org.kde.ktexteditor.katetextbuffer.savefile")));
+            kAuthSaveAction->setHelperId(QLatin1String("org.kde.ktexteditor.katetextbuffer"));
+            kAuthSaveAction->setArguments(*kAuthActionArgs);
+            KAuth::ExecuteJob *job = kAuthSaveAction->execute();
+            if (!job->exec()) {
+                return false;
+            }
+
+            // get temporary file path from the reply
+            targetFilename = job->data()[QLatin1String("temporaryFile")].toString();
+
+        }
+
+        if (targetFilename.isEmpty()) {
             return false;
         }
+
+        // we are now saving to a prepared temporary file
+        saveFile.reset(new QFile(targetFilename));
+
+        // open QTemporaryFile for write
+        if (!saveFile->open(QIODevice::WriteOnly)) {
+            return false;
+        }
+
+        if (!newFile) {
+            // set original file's permissions to temporary file (QSaveFile does this automatically)
+            saveFile->setPermissions(QFile(filename).permissions());
+        }
     }
 
     /**
@@ -863,8 +906,14 @@ bool TextBuffer::save(const QString &filename)
     file.close();
 
     // flush file
-    if (!usingTemporaryBuffer) {
-        static_cast<QSaveFile *>(saveFile.data())->flush();
+    if (!saveFile->flush()) {
+        return false;
+    }
+
+    if (usingTemporaryFile) {
+        // ensure that the file is written to disk
+        // just for temporary file (QSaveFile does this automatically in commit())
+        SecureTextBuffer::syncToDisk(saveFile->handle());
     }
 
     // did save work?
@@ -874,58 +923,27 @@ bool TextBuffer::save(const QString &filename)
     // commit changes
     if (ok) {
 
-        if (usingTemporaryBuffer) {
+        if (usingTemporaryFile) {
 
-            // temporary buffer was used to save the file
-            // -> computing checksum
-            // -> saving to temporary file
-            // -> copying the temporary file to the original file location with KAuth action
+            // temporary file was used to save the file
+            // -> moving this file to original location with KAuth action
 
-            QTemporaryFile tempFile;
-            if (!tempFile.open()) {
-                return false;
-            }
-            QCryptographicHash cryptographicHash(SecureTextBuffer::checksumAlgorithm);
-
-            // go to QBuffer start
-            saveFile->seek(0);
-
-            // read contents of QBuffer and add them to checksum utility as well as to QTemporaryFile
-            char buffer[bufferLength];
-            qint64 read = -1;
-            while ((read = saveFile->read(buffer, bufferLength)) > 0) {
-                cryptographicHash.addData(buffer, read);
-                if (tempFile.write(buffer, read) == -1) {
-                    return false;
-                }
-            }
-            if (!tempFile.flush()) {
-                return false;
-            }
-
-            // compute checksum
-            QByteArray checksum = cryptographicHash.result();
-
-            // prepare data for KAuth action
-            QVariantMap kAuthActionArgs;
-            kAuthActionArgs.insert(QLatin1String("sourceFile"), tempFile.fileName());
-            kAuthActionArgs.insert(QLatin1String("targetFile"), filename);
-            kAuthActionArgs.insert(QLatin1String("checksum"), checksum);
-            kAuthActionArgs.insert(QLatin1String("ownerId"), ownerId);
-            kAuthActionArgs.insert(QLatin1String("groupId"), groupId);
+            kAuthActionArgs->insert(QLatin1String("actionMode"), SecureTextBuffer::ActionMode::Move);
+            kAuthActionArgs->insert(QLatin1String("sourceFile"), saveFile->fileName());
+            kAuthActionArgs->insert(QLatin1String("targetFile"), filename);
+            kAuthActionArgs->insert(QLatin1String("ownerId"), ownerId);
+            kAuthActionArgs->insert(QLatin1String("groupId"), groupId);
 
             // call save with elevated privileges
             if (KTextEditor::EditorPrivate::unitTestMode()) {
 
                 // unit testing purposes only
-                ok = SecureTextBuffer::savefile(kAuthActionArgs).succeeded();
+                ok = SecureTextBuffer::savefile(*kAuthActionArgs).succeeded();
 
             } else {
 
-                KAuth::Action kAuthSaveAction(QLatin1String("org.kde.ktexteditor.katetextbuffer.savefile"));
-                kAuthSaveAction.setHelperId(QLatin1String("org.kde.ktexteditor.katetextbuffer"));
-                kAuthSaveAction.setArguments(kAuthActionArgs);
-                KAuth::ExecuteJob *job = kAuthSaveAction.execute();
+                kAuthSaveAction->setArguments(*kAuthActionArgs);
+                KAuth::ExecuteJob *job = kAuthSaveAction->execute();
                 ok = job->exec();
 
             }
@@ -934,15 +952,13 @@ bool TextBuffer::save(const QString &filename)
 
             // standard save without elevated privileges
 
-            QSaveFile *saveFileLocal = static_cast<QSaveFile *>(saveFile.data());
+            ok = static_cast<QSaveFile *>(saveFile.data())->commit();
 
-            if (!newFile) {
+            if (ok && !newFile) {
                 // ensure correct owner
-                SecureTextBuffer::setOwner(saveFileLocal->handle(), ownerId, groupId);
+                SecureTextBuffer::setOwner(filename, ownerId, groupId);
             }
 
-            ok = saveFileLocal->commit();
-
         }
     }
 
diff --git a/src/buffer/katetextbuffer.h b/src/buffer/katetextbuffer.h
index f8912f24..8ec356fe 100644
--- a/src/buffer/katetextbuffer.h
+++ b/src/buffer/katetextbuffer.h
@@ -649,11 +649,6 @@ private:
      * For unit-testing purposes only.
      */
     bool m_alwaysUseKAuthForSave;
-
-    /**
-     * For copying QBuffer -> QTemporaryFile while saving document in privileged mode
-     */
-    static const qint64 bufferLength = 4096;
 };
 
 }
-- 
2.12.0

Places

File 0001-Revert-KAuth-integration-in-document-saving-vol.-2.patch of Package ktexteditor (Revision 0ae66805f9384f11d2b14b29922a4aa8)

Places