Differential D5394 Diff 13539 src/buffer/katesecuretextbuffer.cpp

Changeset View

Standalone View

View Options

src/buffer/katesecuretextbuffer.cpp

Context not available.
28#include <QString> 28​#include <QString>
29#include <QFile> 29​#include <QFile>
30#include <QTemporaryFile> 30​#include <QTemporaryFile>
31#include <QSaveFile> ​
32 31​
33KAUTH_HELPER_MAIN("org.kde.ktexteditor.katetextbuffer", SecureTextBuffer) 32​KAUTH_HELPER_MAIN("org.kde.ktexteditor.katetextbuffer", SecureTextBuffer)
34 33​
35ActionReply SecureTextBuffer::savefile(const QVariantMap &args) 34​ActionReply SecureTextBuffer::savefile(const QVariantMap &args)
36{ 35​{
37 const ActionMode actionMode = static_cast<ActionMode>(args[QLatin1String("actionMode")].toInt()); 36​ const QString sourceFile = args[QLatin1String("sourceFile")].toString();
38 const QString targetFile = args[QLatin1String("targetFile")].toString(); 37​ const QString targetFile = args[QLatin1String("targetFile")].toString();
38​ const QByteArray checksum = args[QLatin1String("checksum")].toByteArray();
39 const uint ownerId = (uint) args[QLatin1String("ownerId")].toInt(); 39​ const uint ownerId = (uint) args[QLatin1String("ownerId")].toInt();
40​ const uint groupId = (uint) args[QLatin1String("groupId")].toInt();
40 41​
41 if (actionMode == ActionMode::Prepare) { 42​ if (saveFileInternal(sourceFile, targetFile, checksum, ownerId, groupId)) {
42 43​ return ActionReply::SuccessReply();
43 const QString temporaryFile = prepareTempFileInternal(targetFile, ownerId); ​
44 ​
45 if (temporaryFile.isEmpty()) { ​
46 return ActionReply::HelperErrorReply(); ​
47 } ​
48 ​
49 ActionReply reply; ​
50 reply.addData(QLatin1String("temporaryFile"), temporaryFile); ​
51 ​
52 return reply; ​
53 ​
54 } ​
55 ​
56 if (actionMode == ActionMode::Move) { ​
57 ​
58 const QString sourceFile = args[QLatin1String("sourceFile")].toString(); ​
59 const uint groupId = (uint) args[QLatin1String("groupId")].toInt(); ​
60 ​
61 if (moveFileInternal(sourceFile, targetFile, ownerId, groupId)) { ​
62 return ActionReply::SuccessReply(); ​
63 } ​
64 } 44​ }
65 45​
66 return ActionReply::HelperErrorReply(); 46​ return ActionReply::HelperErrorReply();
67} 47​}
68 48​
69bool SecureTextBuffer::moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId) 49​bool SecureTextBuffer::saveFileInternal(const QString &sourceFile, const QString &targetFile,
50​ const QByteArray &checksum, const uint ownerId, const uint groupId)
70{ 51​{
71 const bool newFile = !QFile::exists(targetFile); 52​ const bool newFile = !QFile::exists(targetFile);
72 bool atomicRenameSucceeded = false; 53​
73 54​ // open source and target file
74 /** 55​ QFile readFile(sourceFile);
75 * There is no atomic rename operation publicly exposed by Qt. 56​ //TODO use QSaveFile for saving contents and automatic atomic move on commit() when QSaveFile's security problem
76 * 57​ // (default temporary file permissions) is fixed
77 * We use std::rename for UNIX and for now no-op for windows (triggers fallback). 58​ QTemporaryFile tempFile(targetFile);
78 * 59​ if (!readFile.open(QIODevice::ReadOnly) || !tempFile.open()) {
79 * As fallback we are copying source file to destination with the help of QSaveFile 60​ return false;
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 } 61​ }
88#else 62​ const int tempFileDescriptor = tempFile.handle();
89 atomicRenameSucceeded = false; ​
90#endif ​
91 63​
92 if (!atomicRenameSucceeded) { 64​ // prepare checksum maker
93 // as fallback copy the temporary file to the target with help of QSaveFile 65​ QCryptographicHash cryptographicHash(checksumAlgorithm);
94 QFile readFile(sourceFile); 66​
95 QSaveFile saveFile(targetFile); 67​ // copy contents
96 if (!readFile.open(QIODevice::ReadOnly) || !saveFile.open(QIODevice::WriteOnly)) { 68​ char buffer[bufferLength];
97 return false; 69​ qint64 read = -1;
98 } 70​ while ((read = readFile.read(buffer, bufferLength)) > 0) {
99 char buffer[bufferLength]; 71​ cryptographicHash.addData(buffer, read);
100 qint64 read = -1; 72​ if (tempFile.write(buffer, 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; 73​ return false;
108 } 74​ }
109 } 75​ }
110 76​
77​ // check that copying was successful and checksum matched
78​ QByteArray localChecksum = cryptographicHash.result();
79​ if (read == -1 || localChecksum != checksum || !tempFile.flush()) {
80​ return false;
81​ }
82​
83​ tempFile.close();
84​
111 if (!newFile) { 85​ if (!newFile) {
86​ // ensure the same file permissions
87​ tempFile.setPermissions(QFile::permissions(targetFile));
112 // ensure file has the same owner and group as before 88​ // ensure file has the same owner and group as before
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…
113 setOwner(targetFile, ownerId, groupId); 89​ setOwner(tempFileDescriptor, ownerId, groupId);
114 } 90​ }
115 91​
116 return true; 92​ // rename temporary file to the target file
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…
117} 93​ if (moveFile(tempFile.fileName(), targetFile)) {
118 94​ // temporary file was renamed, there is nothing to remove anymore
119QString SecureTextBuffer::prepareTempFileInternal(const QString &targetFile, const uint ownerId) 95​ tempFile.setAutoRemove(false);
120{ 96​ return true;
121 QTemporaryFile tempFile(targetFile); ​
122 if (!tempFile.open()) { ​
123 return QString(); ​
124 } 97​ }
125 tempFile.setAutoRemove(false); 98​ return false;
126 setOwner(tempFile.fileName(), ownerId, -1); ​
127 return tempFile.fileName(); ​
128} 99​}
129 100​
130void SecureTextBuffer::setOwner(const QString &filename, const uint ownerId, const uint groupId) 101​void SecureTextBuffer::setOwner(const int filedes, const uint ownerId, const uint groupId)
131{ 102​{
132#ifndef Q_OS_WIN 103​#ifndef Q_OS_WIN
133 if (ownerId != (uint)-2 && groupId != (uint)-2) { 104​ if (ownerId != (uint)-2 && groupId != (uint)-2) {
134 const int result = chown(QFile::encodeName(filename).constData(), ownerId, groupId); 105​ const int result = fchown(filedes, ownerId, groupId);
135 // set at least correct group if owner cannot be changed 106​ // set at least correct group if owner cannot be changed
136 if (result != 0 && errno == EPERM) { 107​ if (result != 0 && errno == EPERM) {
137 chown(QFile::encodeName(filename).constData(), getuid(), groupId); 108​ fchown(filedes, getuid(), groupId);
138 } 109​ }
139 } 110​ }
140#else 111​#else
Context not available.
142#endif 113​#endif
143} 114​}
144 115​
116​bool SecureTextBuffer::moveFile(const QString &sourceFile, const QString targetFile)
117​{
118​#ifndef Q_OS_WIN
119​ const int result = std::rename(QFile::encodeName(sourceFile).constData(), QFile::encodeName(targetFile).constData());
120​ if (result == 0) {
121​ syncToDisk(QFile(targetFile).handle());
122​ return true;
123​ }
124​ return false;
125​#else
126​ // use racy fallback for windows
127​ QFile::remove(targetFile);
128​ return QFile::rename(sourceFile, targetFile);
129​#endif
130​}
131​
145void SecureTextBuffer::syncToDisk(const int fd) 132​void SecureTextBuffer::syncToDisk(const int fd)
146{ 133​{
147#ifndef Q_OS_WIN 134​#ifndef Q_OS_WIN
Context not available.
153#else 140​#else
154 // no-op for windows 141​ // no-op for windows
155#endif 142​#endif
156} 143​}
157No newline at end of file 144​
Context not available.