File 0001-Revert-KAuth-integration-in-document-saving-vol.-2.patch of Package ktexteditor
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
