Differential D5394 Diff 14020 src/buffer/katesecuretextbuffer.cpp

Changeset View

Standalone View

View Options

src/buffer/katesecuretextbuffer.cpp

Show All 21 Lines
22 22​
23#ifndef Q_OS_WIN 23​#ifndef Q_OS_WIN
24#include <unistd.h> 24​#include <unistd.h>
25#include <errno.h> 25​#include <errno.h>
26#endif 26​#endif
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();
40 ​
41 if (actionMode == ActionMode::Prepare) { ​
42 ​
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(); 42​ const uint groupId = (uint) args[QLatin1String("groupId")].toInt();
60 43​
61 if (moveFileInternal(sourceFile, targetFile, ownerId, groupId)) { 44​ if (saveFileInternal(sourceFile, targetFile, checksum, ownerId, groupId)) {
62 return ActionReply::SuccessReply(); 45​ return ActionReply::SuccessReply();
63 } 46​ }
64 } ​
65 47​
66 return ActionReply::HelperErrorReply(); 48​ return ActionReply::HelperErrorReply();
67} 49​}
68 50​
69bool SecureTextBuffer::moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId) 51​bool SecureTextBuffer::saveFileInternal(const QString &sourceFile, const QString &targetFile,
52​ const QByteArray &checksum, const uint ownerId, const uint groupId)
70{ 53​{
71 const bool newFile = !QFile::exists(targetFile); 54​ QFileInfo targetFileInfo(targetFile);
72 bool atomicRenameSucceeded = false; 55​ if (!QDir::setCurrent(targetFileInfo.dir().path())) {
73 56​ return false;
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 } 57​ }
88#else ​
89 atomicRenameSucceeded = false; ​
90#endif ​
91 58​
92 if (!atomicRenameSucceeded) { 59​ // get information about target file
93 // as fallback copy the temporary file to the target with help of QSaveFile 60​ const QString targetFileName = targetFileInfo.fileName();
61​ targetFileInfo.setFile(targetFileName);
62​ const bool newFile = !targetFileInfo.exists();
63​
64​ // open source and target file
94 QFile readFile(sourceFile); 65​ QFile readFile(sourceFile);
95 QSaveFile saveFile(targetFile); 66​ //TODO use QSaveFile for saving contents and automatic atomic move on commit() when QSaveFile's security problem
96 if (!readFile.open(QIODevice::ReadOnly) || !saveFile.open(QIODevice::WriteOnly)) { 67​ // (default temporary file permissions) is fixed
68​ //
69​ // We will first generate temporary filename and then use it relatively to prevent an attacker
70​ // to trick us to write contents to a different file by changing underlying directory.
71​ QTemporaryFile tempFile(targetFileName);
72​ if (!tempFile.open()) {
97 return false; 73​ return false;
98 } 74​ }
75​ tempFile.close();
76​ QString tempFileName = QFileInfo(tempFile).fileName();
77​ tempFile.setFileName(tempFileName);
78​ if (!readFile.open(QIODevice::ReadOnly) || !tempFile.open()) {
79​ return false;
80​ }
81​ const int tempFileDescriptor = tempFile.handle();
82​
83​ // prepare checksum maker
84​ QCryptographicHash cryptographicHash(checksumAlgorithm);
85​
86​ // copy contents
99 char buffer[bufferLength]; 87​ char buffer[bufferLength];
100 qint64 read = -1; 88​ qint64 read = -1;
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…
101 while ((read = readFile.read(buffer, bufferLength)) > 0) { 89​ while ((read = readFile.read(buffer, bufferLength)) > 0) {
102 if (saveFile.write(buffer, read) == -1) { 90​ cryptographicHash.addData(buffer, read);
91​ if (tempFile.write(buffer, read) == -1) {
103 return false; 92​ return false;
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…
104 } 93​ }
105 } 94​ }
106 if (read == -1 || !saveFile.commit()) { 95​
96​ // check that copying was successful and checksum matched
97​ QByteArray localChecksum = cryptographicHash.result();
98​ if (read == -1 || localChecksum != checksum || !tempFile.flush()) {
107 return false; 99​ return false;
108 } 100​ }
109 } ​
110 101​
111 if (!newFile) { 102​ tempFile.close();
103​
104​ if (newFile) {
105​ // ensure new file is readable by anyone
106​ tempFile.setPermissions(tempFile.permissions() | QFile::Permission::ReadGroup | QFile::Permission::ReadOther);
107​ } else {
108​ // ensure the same file permissions
109​ tempFile.setPermissions(targetFileInfo.permissions());
112 // ensure file has the same owner and group as before 110​ // ensure file has the same owner and group as before
113 setOwner(targetFile, ownerId, groupId); 111​ setOwner(tempFileDescriptor, ownerId, groupId);
114 } 112​ }
115 113​
114​ // rename temporary file to the target file
115​ if (moveFile(tempFileName, targetFileName)) {
116​ // temporary file was renamed, there is nothing to remove anymore
117​ tempFile.setAutoRemove(false);
116 return true; 118​ return true;
117} 119​ }
118 120​ return false;
119QString SecureTextBuffer::prepareTempFileInternal(const QString &targetFile, const uint ownerId) ​
120{ ​
121 QTemporaryFile tempFile(targetFile); ​
122 if (!tempFile.open()) { ​
123 return QString(); ​
124 } ​
125 tempFile.setAutoRemove(false); ​
126 setOwner(tempFile.fileName(), ownerId, -1); ​
127 return tempFile.fileName(); ​
128} 121​}
129 122​
130void SecureTextBuffer::setOwner(const QString &filename, const uint ownerId, const uint groupId) 123​void SecureTextBuffer::setOwner(const int filedes, const uint ownerId, const uint groupId)
131{ 124​{
132#ifndef Q_OS_WIN 125​#ifndef Q_OS_WIN
133 if (ownerId != (uint)-2 && groupId != (uint)-2) { 126​ if (ownerId != (uint)-2 && groupId != (uint)-2) {
134 const int result = chown(QFile::encodeName(filename).constData(), ownerId, groupId); 127​ const int result = fchown(filedes, ownerId, groupId);
135 // set at least correct group if owner cannot be changed 128​ // set at least correct group if owner cannot be changed
136 if (result != 0 && errno == EPERM) { 129​ if (result != 0 && errno == EPERM) {
137 chown(QFile::encodeName(filename).constData(), getuid(), groupId); 130​ fchown(filedes, getuid(), groupId);
138 } 131​ }
139 } 132​ }
140#else 133​#else
141 // no-op for windows 134​ // no-op for windows
142#endif 135​#endif
143} 136​}
144 137​
138​bool SecureTextBuffer::moveFile(const QString &sourceFile, const QString &targetFile)
139​{
140​#ifndef Q_OS_WIN
141​ const int result = std::rename(QFile::encodeName(sourceFile).constData(), QFile::encodeName(targetFile).constData());
142​ if (result == 0) {
143​ syncToDisk(QFile(targetFile).handle());
144​ return true;
145​ }
146​ return false;
147​#else
148​ // use racy fallback for windows
149​ QFile::remove(targetFile);
150​ return QFile::rename(sourceFile, targetFile);
151​#endif
152​}
153​
145void SecureTextBuffer::syncToDisk(const int fd) 154​void SecureTextBuffer::syncToDisk(const int fd)
146{ 155​{
147#ifndef Q_OS_WIN 156​#ifndef Q_OS_WIN
148#ifdef HAVE_FDATASYNC 157​#ifdef HAVE_FDATASYNC
149 fdatasync(fd); 158​ fdatasync(fd);
150#else 159​#else
151 fsync(fd); 160​ fsync(fd);
152#endif 161​#endif
153#else 162​#else
154 // no-op for windows 163​ // no-op for windows
155#endif 164​#endif
156} 165​}
166​
157No newline at end of file ​