Differential D14467 Diff 38719 src/ioslaves/file/kauth/filehelper.cpp

Changeset View

Standalone View

View Options

src/ioslaves/file/kauth/filehelper.cpp

Show All 18 Lines
19***/ 19​***/
20 20​
21#include <utime.h> 21​#include <utime.h>
22#include <fcntl.h> 22​#include <fcntl.h>
23#include <unistd.h> 23​#include <unistd.h>
24#include <dirent.h> 24​#include <dirent.h>
25#include <sys/stat.h> 25​#include <sys/stat.h>
26#include <errno.h> 26​#include <errno.h>
27​#include <libgen.h>
28​#include <grp.h>
27 29​
28#include "filehelper.h" 30​#include "filehelper.h"
29#include "fdsender.h" 31​#include "fdsender.h"
30#include "../file_p.h" 32​#include "../file_p.h"
31 33​
32static bool sendFileDescriptor(int fd, const char *socketPath) 34​static bool sendFileDescriptor(int fd, const char *socketPath)
33{ 35​{
34 FdSender fdSender(socketPath); 36​ FdSender fdSender(socketPath);
35 if (fdSender.isConnected() && fdSender.sendFileDescriptor(fd)) { 37​ if (fdSender.isConnected() && fdSender.sendFileDescriptor(fd)) {
36 return true; 38​ return true;
37 } 39​ }
38 return false; 40​ return false;
39} 41​}
40 42​
43​static bool dropPrivileges(int action, const QByteArray &path, const QByteArray &newPath/*for rename action*/)
44​{
45​ //for chmod, chown, utime, drop privileges to that of owner of the target file/dir
46​ //for other actions use owner of the parent directory.
47​ QByteArray target = path;
48​ if (action != CHMOD && action != CHOWN && action != UTIME) {
49​ target = dirname(target.data());
50​ }
51​
52​ struct stat buf = {0};
maltekUnsubmitted
Done

This is still subject to race conditions. You're opening the file with O_NOFOLLOW and calling fstat on it, correctly. But then, you don't return this file descriptor to further work on it. So there's no way to know if the file checked here and the file that's changed later on are the same.

(It's also leaking the opened file descriptor, currently.)

maltek: This is still subject to race conditions. You're opening the file with O_NOFOLLOW and calling…
53​ if (stat(target.data(), &buf) == -1) {
54​ return false;
55​ }
56​
57​ uid_t newuid = buf.st_uid;
58​ gid_t newgid = buf.st_gid;
59​ if (newuid == 0 || newgid == 0) {
60​ return true;
61​ }
62​
63​ //rename requires write access in both old and new location so drop privileges only when owners
64​ //are same otherwise continue with root privileges.
65​ if (action == RENAME) {
66​ struct stat buf_new = {0};
67​ const QByteArray newFilePath = dirname(QByteArray(newPath).data());
68​ stat(newFilePath.data(), &buf_new);
69​ if (newuid != buf_new.st_uid || newgid != buf_new.st_gid) {
70​ return true;
71​ }
72​ }
73​
74​ //drop ancillary groups first because it requires root privileges.
75​ if (setgroups(1, &newgid) == -1) {
76​ return false;
77​ }
78​ //change real, effective, saved gid and uid.
79​ if (setregid(newgid, newgid) == -1 || setreuid(newuid, newuid) == -1) {
80​ return false;
81​ }
82​
83​ return true;
maltekUnsubmitted
Done

After this return false, the process is in some undefined state with unknown groups, since there is no attempt to restore the original groups. The chance of these calls failing are quite slim, however - it can only really happen due to programming error when the program doesn't have privileges to call sete[ug]id. Personally, I'd just abort the program in such circumstances, but since it should never be happening, reasonable people might disagree.

maltek: After this `return false`, the process is in some undefined state with unknown groups, since…
84​}
85​
41ActionReply FileHelper::exec(const QVariantMap &args) 86​ActionReply FileHelper::exec(const QVariantMap &args)
42{ 87​{
43 ActionReply reply; 88​ ActionReply reply;
44 QByteArray data = args["arguments"].toByteArray(); 89​ QByteArray data = args["arguments"].toByteArray();
45 QDataStream in(data); 90​ QDataStream in(data);
46 int action; 91​ int action;
47 QVariant arg1, arg2, arg3, arg4; 92​ QVariant arg1, arg2, arg3, arg4;
48 in >> action >> arg1 >> arg2 >> arg3 >> arg4; 93​ in >> action >> arg1 >> arg2 >> arg3 >> arg4;
49 94​
50 // the path of an existing or a new file/dir upon which the method will operate 95​ // the path of an existing or a new file/dir upon which the method will operate
51 const QByteArray path = arg1.toByteArray(); 96​ const QByteArray path = arg1.toByteArray();
97​ // for rename
98​ const QByteArray newPath = action == RENAME ? arg2.toByteArray() : QByteArray();
52 99​
100​ if (dropPrivileges(action, path, newPath)) {
maltekUnsubmitted
Done

This needs error handling.

maltek: This needs error handling.
53 switch(action) { 101​ switch(action) {
54 case CHMOD: { 102​ case CHMOD: {
55 int mode = arg2.toInt(); 103​ int mode = arg2.toInt();
56 if (chmod(path.data(), mode) == 0) { 104​ if (chmod(path.data(), mode) == 0) {
57 return reply; 105​ return reply;
58 } 106​ }
59 break; 107​ break;
60 } 108​ }
61 case CHOWN: { 109​ case CHOWN: {
62 int uid = arg2.toInt(); 110​ int uid = arg2.toInt();
63 int gid = arg3.toInt(); 111​ int gid = arg3.toInt();
64 if (chown(path.data(), uid, gid) == 0) { 112​ if (chown(path.data(), uid, gid) == 0) {
65 return reply; 113​ return reply;
maltekUnsubmitted
Done

AT_SYMLINK_NOFOLLOW is not implemented for fchmodat, so this will always fail with ENOTSUP. It seems the only safe way to do this is to open() the file with O_NOFOLLOW, and then use fchmod().

maltek: AT_SYMLINK_NOFOLLOW is not implemented for fchmodat, so this will always fail with ENOTSUP. It…
66 } 114​ }
67 break; 115​ break;
68 } 116​ }
69 case DEL: { 117​ case DEL: {
70 if (unlink(path.data()) == 0) { 118​ if (unlink(path.data()) == 0) {
71 return reply; 119​ return reply;
72 } 120​ }
73 break; 121​ break;
74 } 122​ }
75 case MKDIR: { 123​ case MKDIR: {
76 if (mkdir(path.data(), 0777) == 0) { 124​ if (mkdir(path.data(), 0777) == 0) {
77 return reply; 125​ return reply;
78 } 126​ }
79 break; 127​ break;
80 } 128​ }
81 case OPEN: { 129​ case OPEN: {
82 int oflags = arg2.toInt(); 130​ int oflags = arg2.toInt();
83 int mode = arg3.toInt(); 131​ int mode = arg3.toInt();
maltekUnsubmitted
Done

typo: there's a second & missing after the first condition. (I don't think it ends up affecting the result.)

maltek: typo: there's a second & missing after the first condition. (I don't think it ends up affecting…
84 int fd = open(path.data(), oflags, mode); 132​ int fd = open(path.data(), oflags, mode);
85 bool success = (fd != -1) && sendFileDescriptor(fd, arg4.toByteArray().constData()); 133​ bool success = (fd != -1) && sendFileDescriptor(fd, arg4.toByteArray().constData());
86 close(fd); 134​ close(fd);
maltekUnsubmitted
Done

There's no error handling here, which will likely lead to weird EBADF errors getting returned later.

maltek: There's no error handling here, which will likely lead to weird `EBADF` errors getting returned…
87 if (success) { 135​ if (success) {
maltekUnsubmitted
Done

For chown, dropping privileges here means that the chown later can't succeed - it's not possible to 'gift' a file to another user. I think it should be handled more like DEL/RMDIR/MKDIR etc.

maltek: For `chown`, dropping privileges here means that the `chown` later can't succeed - it's not…
chinmoyrAuthorUnsubmitted
Not Done

Ah! Since I was testing inside /opt I didn't notice. I think the order here should be: drop privilege -> change grp -> gain privilege -> change user.

chinmoyr: Ah! Since I was testing inside /opt I didn't notice. I think the order here should be: drop…
maltekUnsubmitted
Not Done

IMO, it's fine (and less complicated) to just do both in one single privileged fchmod call.

maltek: IMO, it's fine (and less complicated) to just do both in one single privileged `fchmod` call.
88 return reply; 136​ return reply;
89 } 137​ }
90 break; 138​ break;
91 } 139​ }
92 case OPENDIR: { 140​ case OPENDIR: {
93 DIR *dp = opendir(path.data()); 141​ DIR *dp = opendir(path.data());
94 bool success = false; 142​ bool success = false;
maltekUnsubmitted
Not Done

I just realized that this wouldn't allow changing the owner of symbolic links. The way to go here is lchown.

maltek: I just realized that this wouldn't allow changing the owner of symbolic links. The way to go…
chinmoyrAuthorUnsubmitted
Not Done

Do you think it'll be a bad idea to skip the case for symlinks in utime, chmod, chown, for now? Right now there's no code in KIO that requires these operations to be performed on the link itself.

chinmoyr: Do you think it'll be a bad idea to skip the case for symlinks in utime, chmod, chown, for now?
maltekUnsubmitted
Not Done

Fine by me - I'm only really here to look for security problems, not to decide on which features are required for this to land.

maltek: Fine by me - I'm only really here to look for security problems, not to decide on which…
95 if (dp) { 143​ if (dp) {
96 int fd = dirfd(dp); 144​ int fd = dirfd(dp);
97 success = (fd != -1) && sendFileDescriptor(fd, arg4.toByteArray().constData()); 145​ success = (fd != -1) && sendFileDescriptor(fd, arg4.toByteArray().constData());
98 closedir(dp); 146​ closedir(dp);
99 if (success) { 147​ if (success) {
100 return reply; 148​ return reply;
101 } 149​ }
102 } 150​ }
103 break; 151​ break;
104 } 152​ }
105 case RENAME: { 153​ case RENAME: {
106 const QByteArray newName = arg2.toByteArray(); 154​ if (rename(path.data(), newPath.data()) == 0) {
107 if (rename(path.data(), newName.data()) == 0) { ​
108 return reply; 155​ return reply;
109 } 156​ }
110 break; 157​ break;
111 } 158​ }
112 case RMDIR: { 159​ case RMDIR: {
113 if (rmdir(path.data()) == 0) { 160​ if (rmdir(path.data()) == 0) {
114 return reply; 161​ return reply;
115 } 162​ }
116 break; 163​ break;
117 } 164​ }
118 case SYMLINK: { 165​ case SYMLINK: {
119 const QByteArray target = arg2.toByteArray(); 166​ const QByteArray target = arg2.toByteArray();
120 if (symlink(target.data(), path.data()) == 0) { 167​ if (symlink(target.data(), path.data()) == 0) {
121 return reply; 168​ return reply;
122 } 169​ }
123 break; 170​ break;
124 } 171​ }
125 case UTIME: { 172​ case UTIME: {
126 utimbuf ut; 173​ utimbuf ut;
maltekUnsubmitted
Done

I *think* the divisions/multiplications are reversed here.

maltek: I *think* the divisions/multiplications are reversed here.
127 ut.actime = arg2.toULongLong(); 174​ ut.actime = arg2.toULongLong();
128 ut.modtime = arg3.toULongLong(); 175​ ut.modtime = arg3.toULongLong();
129 if (utime(path.data(), &ut) == 0) { 176​ if (utime(path.data(), &ut) == 0) {
130 return reply; 177​ return reply;
131 } 178​ }
132 break; 179​ break;
maltekUnsubmitted
Done

In the error case, this attempts sending fd -1. I haven't checked the underlying code, but this will probably pollute errno with something unrelated to the underlying error.

maltek: In the error case, this attempts sending fd `-1`. I haven't checked the underlying code, but…
133 } 180​ }
134 }; 181​ };
182​ }
135 183​
maltekUnsubmitted
Done

Somewhere here, I'd expect the privileges to be escalated again, to restore the state from before the call to dropPrivileges().

maltek: Somewhere here, I'd expect the privileges to be escalated again, to restore the state from…
136 reply.setError(errno ? errno : -1); 184​ reply.setError(errno ? errno : -1);
137 return reply; 185​ return reply;
138} 186​}
139 187​
140KAUTH_HELPER_MAIN("org.kde.kio.file", FileHelper) 188​KAUTH_HELPER_MAIN("org.kde.kio.file", FileHelper)
maltekUnsubmitted
Done

This early return skips all the deintialization code in the end of the function. Shouldn't it just be reply.setError(errno); like for all the other operations?

maltek: This early return skips all the deintialization code in the end of the function. Shouldn't it…