Hello community, here is the log from the commit of package ktexteditor for openSUSE:Factory checked in at 2017-06-01 16:21:21 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Comparing /work/SRC/openSUSE:Factory/ktexteditor (Old) and /work/SRC/openSUSE:Factory/.ktexteditor.new (New) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Package is "ktexteditor" Thu Jun 1 16:21:21 2017 rev:41 rq:495067 version:5.34.0 Changes: -------- --- /work/SRC/openSUSE:Factory/ktexteditor/ktexteditor.changes 2017-04-30 21:21:04.636561758 +0200 +++ /work/SRC/openSUSE:Factory/.ktexteditor.new/ktexteditor.changes 2017-06-01 16:21:22.427238728 +0200 @@ -1,0 +2,17 @@ +Mon May 15 13:53:15 CEST 2017 - fabian@ritter-vogt.de + +- Update to 5.34.0 + * New feature release + * For more details please see: + * https://www.kde.org/announcements/kde-frameworks-5.34.0.php +- Changes since 5.33.0: + * KAuth integration in document saving - vol. 2 - various security improvements + * Fix assertion when applying code folding that changes cursor position + * Use non-deprecated <gui> root element in ui.rc file + * Add scroll-bar-marks also to the built-in search&replace + * KAuth integration in document saving +- Revert upstream feature as disliked by the security team (boo#1033055) + * 0001-Revert-KAuth-integration-in-document-saving-vol.-2.patch + * 0002-Revert-KAuth-integration-in-document-saving.patch + +------------------------------------------------------------------- Old: ---- ktexteditor-5.33.0.tar.xz New: ---- 0001-Revert-KAuth-integration-in-document-saving-vol.-2.patch 0002-Revert-KAuth-integration-in-document-saving.patch ktexteditor-5.34.0.tar.xz ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Other differences: ------------------ ++++++ ktexteditor.spec ++++++ --- /var/tmp/diff_new_pack.gNA5np/_old 2017-06-01 16:21:23.091145109 +0200 +++ /var/tmp/diff_new_pack.gNA5np/_new 2017-06-01 16:21:23.091145109 +0200 @@ -17,9 +17,9 @@ %bcond_without lang -%define _tar_path 5.33 +%define _tar_path 5.34 Name: ktexteditor -Version: 5.33.0 +Version: 5.34.0 Release: 0 %define kf5_version %{version} BuildRequires: cmake >= 3.0 @@ -58,6 +58,10 @@ Url: http://www.kde.org Source: http://download.kde.org/stable/frameworks/%{_tar_path}/%{name}-%{version}.tar.xz Source1: baselibs.conf +# PATCH-MISFEATURE-OPENSUSE - Disliked by the security team (boo#1033055) +Patch1: 0001-Revert-KAuth-integration-in-document-saving-vol.-2.patch +# PATCH-MISFEATURE-OPENSUSE - Disliked by the security team (boo#1033055) +Patch2: 0002-Revert-KAuth-integration-in-document-saving.patch BuildRoot: %{_tmppath}/%{name}-%{version}-build %description @@ -79,6 +83,8 @@ %lang_package %prep %setup -q +%patch1 -p1 +%patch2 -p1 %build %cmake_kf5 -d build -- -DSYSCONF_INSTALL_DIR=%{_kf5_sysconfdir} ++++++ 0001-Revert-KAuth-integration-in-document-saving-vol.-2.patch ++++++
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; + QScopedPointerKAuth::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 ++++++ 0002-Revert-KAuth-integration-in-document-saving.patch ++++++ ++++ 705 lines (skipped) ++++++ ktexteditor-5.33.0.tar.xz -> ktexteditor-5.34.0.tar.xz ++++++ ++++ 23134 lines of diff (skipped)