diff --git a/src/buffer/katesecuretextbuffer.cpp b/src/buffer/katesecuretextbuffer.cpp --- a/src/buffer/katesecuretextbuffer.cpp +++ b/src/buffer/katesecuretextbuffer.cpp @@ -27,121 +27,130 @@ #include #include +#include +#include #include -#include KAUTH_HELPER_MAIN("org.kde.ktexteditor.katetextbuffer", SecureTextBuffer) ActionReply SecureTextBuffer::savefile(const QVariantMap &args) { - const ActionMode actionMode = static_cast(args[QLatin1String("actionMode")].toInt()); + const QString sourceFile = args[QLatin1String("sourceFile")].toString(); 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 (actionMode == ActionMode::Prepare) { - - const QString temporaryFile = prepareTempFileInternal(targetFile, ownerId); - - if (temporaryFile.isEmpty()) { - return ActionReply::HelperErrorReply(); - } - - ActionReply reply; - reply.addData(QLatin1String("temporaryFile"), temporaryFile); - - return reply; - - } - - if (actionMode == ActionMode::Move) { - - const QString sourceFile = args[QLatin1String("sourceFile")].toString(); - const uint groupId = (uint) args[QLatin1String("groupId")].toInt(); - - if (moveFileInternal(sourceFile, targetFile, ownerId, groupId)) { - return ActionReply::SuccessReply(); - } + if (saveFileInternal(sourceFile, targetFile, checksum, ownerId, groupId)) { + return ActionReply::SuccessReply(); } return ActionReply::HelperErrorReply(); } -bool SecureTextBuffer::moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId) +bool SecureTextBuffer::saveFileInternal(const QString &sourceFile, const QString &targetFile, + const QByteArray &checksum, 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; + QFileInfo targetFileInfo(targetFile); + if (!QDir::setCurrent(targetFileInfo.dir().path())) { + return false; } -#else - atomicRenameSucceeded = false; -#endif - 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()) { + // 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); + + // 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; } } - if (!newFile) { - // ensure file has the same owner and group as before - setOwner(targetFile, ownerId, groupId); + // check that copying was successful and checksum matched + QByteArray localChecksum = cryptographicHash.result(); + if (read == -1 || localChecksum != checksum || !tempFile.flush()) { + return false; } - return true; -} + tempFile.close(); -QString SecureTextBuffer::prepareTempFileInternal(const QString &targetFile, const uint ownerId) -{ - QTemporaryFile tempFile(targetFile); - if (!tempFile.open()) { - return QString(); + 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()); + // ensure file has the same owner and group as before + setOwner(tempFileDescriptor, ownerId, groupId); } - tempFile.setAutoRemove(false); - setOwner(tempFile.fileName(), ownerId, -1); - return tempFile.fileName(); + + // 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 false; } -void SecureTextBuffer::setOwner(const QString &filename, const uint ownerId, const uint groupId) +void SecureTextBuffer::setOwner(const int filedes, const uint ownerId, const uint groupId) { #ifndef Q_OS_WIN if (ownerId != (uint)-2 && groupId != (uint)-2) { - const int result = chown(QFile::encodeName(filename).constData(), ownerId, groupId); + const int result = fchown(filedes, ownerId, groupId); // set at least correct group if owner cannot be changed if (result != 0 && errno == EPERM) { - chown(QFile::encodeName(filename).constData(), getuid(), groupId); + fchown(filedes, getuid(), groupId); } } #else // no-op for windows #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 @@ -153,4 +162,5 @@ #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 --- a/src/buffer/katesecuretextbuffer_p.h +++ b/src/buffer/katesecuretextbuffer_p.h @@ -23,6 +23,7 @@ #include #include +#include #include @@ -43,39 +44,29 @@ 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 methods + * Common helper method */ - static void setOwner(const QString &filename, const uint ownerId, const uint groupId); - static void syncToDisk(const int fd); + static void setOwner(const int filedes, const uint ownerId, const uint groupId); + + static const QCryptographicHash::Algorithm checksumAlgorithm = QCryptographicHash::Algorithm::Sha512; private: static const qint64 bufferLength = 4096; /** - * 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. + * Saves file contents using sets permissions. */ - static QString prepareTempFileInternal(const QString &targetFile, const uint ownerId); + static bool saveFileInternal(const QString &sourceFile, const QString &targetFile, + const QByteArray &checksum, const uint ownerId, const uint groupId); - /** - * Move file to its given destination and set owner. - */ - static bool moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId); + static bool moveFile(const QString &sourceFile, const QString &targetFile); + + static void syncToDisk(const int fd); public Q_SLOTS: /** diff --git a/src/buffer/katetextbuffer.h b/src/buffer/katetextbuffer.h --- a/src/buffer/katetextbuffer.h +++ b/src/buffer/katetextbuffer.h @@ -649,6 +649,11 @@ * For unit-testing purposes only. */ bool m_alwaysUseKAuthForSave; + + /** + * For copying QBuffer -> QTemporaryFile while saving document in privileged mode + */ + static const qint64 bufferLength = 4096; }; } diff --git a/src/buffer/katetextbuffer.cpp b/src/buffer/katetextbuffer.cpp --- a/src/buffer/katetextbuffer.cpp +++ b/src/buffer/katetextbuffer.cpp @@ -36,8 +36,9 @@ #include #include -#include #include +#include +#include #if 0 #define BUFFER_DEBUG qCDebug(LOG_KTE) @@ -779,72 +780,28 @@ } /** - * use QSaveFile for save write + rename + * use QSaveFile for file saving */ - QScopedPointer saveFile(new QSaveFile(filename)); + QScopedPointer saveFile(new QSaveFile(filename)); static_cast(saveFile.data())->setDirectWriteFallback(true); - bool usingTemporaryFile = false; - QScopedPointer kAuthActionArgs; - QScopedPointer kAuthSaveAction; + bool usingTemporaryBuffer = false; // 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 temporary file and then move it to target location + // -> we write to a temporary file and then send its path to KAuth action for privileged save - usingTemporaryFile = true; + usingTemporaryBuffer = true; - QString targetFilename(filename); + // we are now saving to a temporary buffer + saveFile.reset(new QBuffer()); - 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()) { + // open buffer for write and read (read is used for checksum computing and writing to temporary file) + if (!saveFile->open(QIODevice::ReadWrite)) { 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()); - } } /** @@ -906,14 +863,8 @@ file.close(); // flush file - 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()); + if (!usingTemporaryBuffer) { + static_cast(saveFile.data())->flush(); } // did save work? @@ -923,42 +874,75 @@ // commit changes if (ok) { - if (usingTemporaryFile) { + if (usingTemporaryBuffer) { - // temporary file was used to save the file - // -> moving this file to original location with KAuth action + // 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 - 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); + 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); // call save with elevated privileges if (KTextEditor::EditorPrivate::unitTestMode()) { // unit testing purposes only - ok = SecureTextBuffer::savefile(*kAuthActionArgs).succeeded(); + ok = SecureTextBuffer::savefile(kAuthActionArgs).succeeded(); } else { - kAuthSaveAction->setArguments(*kAuthActionArgs); - KAuth::ExecuteJob *job = kAuthSaveAction->execute(); + KAuth::Action kAuthSaveAction(QLatin1String("org.kde.ktexteditor.katetextbuffer.savefile")); + kAuthSaveAction.setHelperId(QLatin1String("org.kde.ktexteditor.katetextbuffer")); + kAuthSaveAction.setArguments(kAuthActionArgs); + KAuth::ExecuteJob *job = kAuthSaveAction.execute(); ok = job->exec(); } } else { // standard save without elevated privileges - ok = static_cast(saveFile.data())->commit(); + QSaveFile *saveFileLocal = static_cast(saveFile.data()); - if (ok && !newFile) { + if (!newFile) { // ensure correct owner - SecureTextBuffer::setOwner(filename, ownerId, groupId); + SecureTextBuffer::setOwner(saveFileLocal->handle(), ownerId, groupId); } + ok = saveFileLocal->commit(); + } }