Differential D5394 Diff 13621 src/buffer/katesecuretextbuffer.cpp

Changeset View

Standalone View

View Options

src/buffer/katesecuretextbuffer.cpp

Context not available.
27 27​
28#include <QString> 28​#include <QString>
29#include <QFile> 29​#include <QFile>
30​#include <QDir>
31​#include <QFileInfo>
30#include <QTemporaryFile> 32​#include <QTemporaryFile>
31#include <QSaveFile> ​
32 33​
33KAUTH_HELPER_MAIN("org.kde.ktexteditor.katetextbuffer", SecureTextBuffer) 34​KAUTH_HELPER_MAIN("org.kde.ktexteditor.katetextbuffer", SecureTextBuffer)
34 35​
35ActionReply SecureTextBuffer::savefile(const QVariantMap &args) 36​ActionReply SecureTextBuffer::savefile(const QVariantMap &args)
36{ 37​{
37 const ActionMode actionMode = static_cast<ActionMode>(args[QLatin1String("actionMode")].toInt()); 38​ const QString sourceFile = args[QLatin1String("sourceFile")].toString();
38 const QString targetFile = args[QLatin1String("targetFile")].toString(); 39​ const QString targetFile = args[QLatin1String("targetFile")].toString();
40​ const QByteArray checksum = args[QLatin1String("checksum")].toByteArray();
39 const uint ownerId = (uint) args[QLatin1String("ownerId")].toInt(); 41​ const uint ownerId = (uint) args[QLatin1String("ownerId")].toInt();
42​ const uint groupId = (uint) args[QLatin1String("groupId")].toInt();
40 43​
41 if (actionMode == ActionMode::Prepare) { 44​ if (saveFileInternal(sourceFile, targetFile, checksum, ownerId, groupId)) {
42 45​ return ActionReply::SuccessReply();
43 const QString temporaryFile = prepareTempFileInternal(targetFile, ownerId); 46​ }
44 47​
45 if (temporaryFile.isEmpty()) { 48​ return ActionReply::HelperErrorReply();
46 return ActionReply::HelperErrorReply(); 49​}
47 } ​
48 50​
49 ActionReply reply; 51​bool SecureTextBuffer::saveFileInternal(const QString &sourceFile, const QString &targetFile,
50 reply.addData(QLatin1String("temporaryFile"), temporaryFile); 52​ const QByteArray &checksum, const uint ownerId, const uint groupId)
53​{
54​ QFileInfo targetFileInfo(targetFile);
55​ if (!QDir::setCurrent(targetFileInfo.dir().path())) {
56​ return false;
57​ }
51 58​
52 return reply; 59​ const bool newFile = !targetFileInfo.exists();
53 60​
61​ // open source and target file
62​ QFile readFile(sourceFile);
63​ //TODO use QSaveFile for saving contents and automatic atomic move on commit() when QSaveFile's security problem
64​ // (default temporary file permissions) is fixed
65​ QString targetFileName = targetFileInfo.fileName();
66​ QTemporaryFile tempFile(targetFileName);
67​ if (!readFile.open(QIODevice::ReadOnly) || !tempFile.open()) {
68​ return false;
54 } 69​ }
70​ const int tempFileDescriptor = tempFile.handle();
55 71​
56 if (actionMode == ActionMode::Move) { 72​ // prepare checksum maker
57 73​ QCryptographicHash cryptographicHash(checksumAlgorithm);
58 const QString sourceFile = args[QLatin1String("sourceFile")].toString(); ​
59 const uint groupId = (uint) args[QLatin1String("groupId")].toInt(); ​
60 74​
61 if (moveFileInternal(sourceFile, targetFile, ownerId, groupId)) { 75​ // copy contents
62 return ActionReply::SuccessReply(); 76​ char buffer[bufferLength];
77​ qint64 read = -1;
78​ while ((read = readFile.read(buffer, bufferLength)) > 0) {
79​ cryptographicHash.addData(buffer, read);
80​ if (tempFile.write(buffer, read) == -1) {
81​ return false;
63 } 82​ }
64 } 83​ }
65 84​
66 return ActionReply::HelperErrorReply(); 85​ // check that copying was successful and checksum matched
67} 86​ QByteArray localChecksum = cryptographicHash.result();
68 87​ if (read == -1 || localChecksum != checksum || !tempFile.flush()) {
69bool SecureTextBuffer::moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId) 88​ return false;
fvogtUnsubmitted
Done

This is racy: If the newly set permissions allow someone to delete the file, it can be replaced with a symlink and the chown will take effect on the symlink target, which can be literally anything -> escalation.

This is not an issue for the rename call as if the file permissions allow deleting, they allow deleting for the destination file as well -> no escalation.

Solution: Use fchown.

fvogt: This is racy: If the newly set permissions allow someone to delete the file, it can be replaced…
70{ ​
71 const bool newFile = !QFile::exists(targetFile); ​
72 bool atomicRenameSucceeded = false; ​
73 ​
74 /** ​
75 * There is no atomic rename operation publicly exposed by Qt. ​
76 * ​
77 * We use std::rename for UNIX and for now no-op for windows (triggers fallback). ​
78 * ​
79 * As fallback we are copying source file to destination with the help of QSaveFile ​
80 * to ensure targetFile is overwritten safely. ​
81 */ ​
82#ifndef Q_OS_WIN ​
83 const int result = std::rename(QFile::encodeName(sourceFile).constData(), QFile::encodeName(targetFile).constData()); ​
84 if (result == 0) { ​
85 syncToDisk(QFile(targetFile).handle()); ​
86 atomicRenameSucceeded = true; ​
87 } 89​ }
88#else ​
89 atomicRenameSucceeded = false; ​
90#endif ​
91 90​
92 if (!atomicRenameSucceeded) { 91​ tempFile.close();
93 // as fallback copy the temporary file to the target with help of QSaveFile ​
94 QFile readFile(sourceFile); ​
95 QSaveFile saveFile(targetFile); ​
96 if (!readFile.open(QIODevice::ReadOnly) || !saveFile.open(QIODevice::WriteOnly)) { ​
97 return false; ​
98 } ​
99 char buffer[bufferLength]; ​
100 qint64 read = -1; ​
101 while ((read = readFile.read(buffer, bufferLength)) > 0) { ​
102 if (saveFile.write(buffer, read) == -1) { ​
103 return false; ​
104 } ​
105 } ​
106 if (read == -1 || !saveFile.commit()) { ​
107 return false; ​
108 } ​
109 } ​
110 92​
fvogtUnsubmitted
Done

The destructor of QTemporaryFile here tries to unlink the temporary file here, which fails if the rename was successful.

fvogt: The destructor of QTemporaryFile here tries to unlink the temporary file here, which fails if…
111 if (!newFile) { 93​ if (!newFile) {
94​ // ensure the same file permissions
95​ tempFile.setPermissions(targetFileInfo.permissions());
112 // ensure file has the same owner and group as before 96​ // ensure file has the same owner and group as before
113 setOwner(targetFile, ownerId, groupId); 97​ setOwner(tempFileDescriptor, ownerId, groupId);
114 } 98​ }
115 99​
116 return true; 100​ // rename temporary file to the target file
117} 101​ QString tempFileName = QFileInfo(tempFile).fileName();
118 102​ if (moveFile(tempFileName, targetFileName)) {
119QString SecureTextBuffer::prepareTempFileInternal(const QString &targetFile, const uint ownerId) 103​ // temporary file was renamed, there is nothing to remove anymore
120{ 104​ tempFile.setAutoRemove(false);
121 QTemporaryFile tempFile(targetFile); 105​ return true;
122 if (!tempFile.open()) { ​
123 return QString(); ​
124 } 106​ }
125 tempFile.setAutoRemove(false); 107​ return false;
126 setOwner(tempFile.fileName(), ownerId, -1); ​
127 return tempFile.fileName(); ​
128} 108​}
129 109​
130void SecureTextBuffer::setOwner(const QString &filename, const uint ownerId, const uint groupId) 110​void SecureTextBuffer::setOwner(const int filedes, const uint ownerId, const uint groupId)
131{ 111​{
132#ifndef Q_OS_WIN 112​#ifndef Q_OS_WIN
133 if (ownerId != (uint)-2 && groupId != (uint)-2) { 113​ if (ownerId != (uint)-2 && groupId != (uint)-2) {
134 const int result = chown(QFile::encodeName(filename).constData(), ownerId, groupId); 114​ const int result = fchown(filedes, ownerId, groupId);
135 // set at least correct group if owner cannot be changed 115​ // set at least correct group if owner cannot be changed
136 if (result != 0 && errno == EPERM) { 116​ if (result != 0 && errno == EPERM) {
137 chown(QFile::encodeName(filename).constData(), getuid(), groupId); 117​ fchown(filedes, getuid(), groupId);
138 } 118​ }
139 } 119​ }
140#else 120​#else
Context not available.
142#endif 122​#endif
143} 123​}
144 124​
125​bool SecureTextBuffer::moveFile(const QString &sourceFile, const QString &targetFile)
126​{
127​#ifndef Q_OS_WIN
128​ const int result = std::rename(QFile::encodeName(sourceFile).constData(), QFile::encodeName(targetFile).constData());
129​ if (result == 0) {
130​ syncToDisk(QFile(targetFile).handle());
131​ return true;
132​ }
133​ return false;
134​#else
135​ // use racy fallback for windows
136​ QFile::remove(targetFile);
137​ return QFile::rename(sourceFile, targetFile);
138​#endif
139​}
140​
145void SecureTextBuffer::syncToDisk(const int fd) 141​void SecureTextBuffer::syncToDisk(const int fd)
146{ 142​{
147#ifndef Q_OS_WIN 143​#ifndef Q_OS_WIN
Context not available.
153#else 149​#else
154 // no-op for windows 150​ // no-op for windows
155#endif 151​#endif
156} 152​}
157No newline at end of file 153​
Context not available.