Changeset View
Standalone View
src/ioslaves/file/kauth/filehelper.cpp
Show All 17 Lines | 1 | /*** | |||
---|---|---|---|---|---|
18 | License along with this library. If not, see <http://www.gnu.org/licenses/>. | 18 | License along with this library. If not, see <http://www.gnu.org/licenses/>. | ||
19 | ***/ | 19 | ***/ | ||
20 | 20 | | |||
21 | #include "filehelper.h" | 21 | #include "filehelper.h" | ||
22 | 22 | | |||
23 | #include <utime.h> | 23 | #include <utime.h> | ||
24 | #include <fcntl.h> | 24 | #include <fcntl.h> | ||
25 | #include <unistd.h> | 25 | #include <unistd.h> | ||
26 | #include <dirent.h> | | |||
27 | #include <sys/stat.h> | | |||
28 | #include <errno.h> | 26 | #include <errno.h> | ||
27 | #include <libgen.h> | ||||
28 | #include <grp.h> | ||||
29 | #include <sys/stat.h> | ||||
30 | #include <sys/time.h> | ||||
29 | 31 | | |||
30 | #include "fdsender.h" | 32 | #include "fdsender.h" | ||
31 | #include "../file_p.h" | 33 | #include "../file_p.h" | ||
32 | 34 | | |||
35 | struct Privilege { | ||||
36 | uid_t uid; | ||||
37 | gid_t gid; | ||||
38 | }; | ||||
39 | | ||||
40 | static ActionType intToActionType(int action) | ||||
41 | { | ||||
42 | switch (action) { | ||||
43 | case 1: return CHMOD; | ||||
44 | case 2: return CHOWN; | ||||
45 | case 3: return DEL; | ||||
46 | case 4: return MKDIR; | ||||
47 | case 5: return OPEN; | ||||
48 | case 6: return OPENDIR; | ||||
49 | case 7: return RENAME; | ||||
50 | case 8: return RMDIR; | ||||
51 | case 9: return SYMLINK; | ||||
52 | case 10: return UTIME; | ||||
53 | default: return UNKNOWN; | ||||
54 | } | ||||
55 | } | ||||
56 | | ||||
33 | static bool sendFileDescriptor(int fd, const char *socketPath) | 57 | static bool sendFileDescriptor(int fd, const char *socketPath) | ||
34 | { | 58 | { | ||
35 | FdSender fdSender(socketPath); | 59 | FdSender fdSender(socketPath); | ||
36 | if (fdSender.isConnected() && fdSender.sendFileDescriptor(fd)) { | 60 | if (fdSender.isConnected() && fdSender.sendFileDescriptor(fd)) { | ||
37 | return true; | 61 | return true; | ||
38 | } | 62 | } | ||
39 | return false; | 63 | return false; | ||
40 | } | 64 | } | ||
41 | 65 | | |||
66 | static Privilege *getTargetPrivilege(int target_fd) | ||||
67 | { | ||||
68 | struct stat buf = {0}; | ||||
69 | if (fstat(target_fd, &buf) == -1) { | ||||
70 | return nullptr; | ||||
71 | } | ||||
72 | return new Privilege{buf.st_uid, buf.st_gid}; | ||||
73 | } | ||||
74 | | ||||
75 | static bool dropPrivilege(Privilege *p) | ||||
maltek: This is still subject to race conditions. You're opening the file with O_NOFOLLOW and calling… | |||||
76 | { | ||||
77 | if (!p) { | ||||
78 | return false; | ||||
79 | } | ||||
80 | | ||||
81 | uid_t newuid = p->uid; | ||||
82 | gid_t newgid = p->gid; | ||||
83 | | ||||
84 | //drop ancillary groups first because it requires root privileges. | ||||
85 | if (setgroups(1, &newgid) == -1) { | ||||
86 | return false; | ||||
87 | } | ||||
88 | //change effective gid and uid. | ||||
89 | if (setegid(newgid) == -1 || seteuid(newuid) == -1) { | ||||
90 | return false; | ||||
91 | } | ||||
92 | | ||||
93 | return true; | ||||
94 | } | ||||
95 | | ||||
96 | static void gainPrivilege(Privilege *p) | ||||
97 | { | ||||
98 | if (!p) { | ||||
99 | return; | ||||
100 | } | ||||
101 | | ||||
102 | uid_t olduid = p->uid; | ||||
103 | gid_t oldgid = p->gid; | ||||
104 | | ||||
105 | seteuid(olduid); | ||||
106 | setegid(oldgid); | ||||
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… | |||||
107 | setgroups(1, &oldgid); | ||||
108 | } | ||||
109 | | ||||
42 | ActionReply FileHelper::exec(const QVariantMap &args) | 110 | ActionReply FileHelper::exec(const QVariantMap &args) | ||
43 | { | 111 | { | ||
44 | ActionReply reply; | 112 | ActionReply reply; | ||
45 | QByteArray data = args[QStringLiteral("arguments")].toByteArray(); | 113 | QByteArray data = args[QStringLiteral("arguments")].toByteArray(); | ||
46 | QDataStream in(data); | 114 | QDataStream in(data); | ||
47 | int action; | 115 | int act; | ||
48 | QVariant arg1, arg2, arg3, arg4; | 116 | QVariant arg1, arg2, arg3, arg4; | ||
49 | in >> action >> arg1 >> arg2 >> arg3 >> arg4; | 117 | in >> act >> arg1 >> arg2 >> arg3 >> arg4; // act=action, arg1=source file, arg$n=dest file, mode, uid, gid, etc. | ||
118 | ActionType action = intToActionType(act); | ||||
119 | | ||||
120 | //chown requires privilege (CAP_CHOWN) to change user but the group can be changed without it. | ||||
121 | //It's much simpler to do it in one privileged call. | ||||
122 | if (action == CHOWN) { | ||||
123 | if (lchown(arg1.toByteArray().constData(), arg2.toInt(), arg3.toInt()) == -1) { | ||||
maltek: This needs error handling. | |||||
124 | reply.setError(errno); | ||||
125 | } | ||||
126 | return reply; | ||||
127 | } | ||||
128 | | ||||
129 | QByteArray tempPath1, tempPath2; | ||||
130 | tempPath1 = tempPath2 = arg1.toByteArray(); | ||||
131 | const QByteArray parentDir = dirname(tempPath1.data()); | ||||
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… | |||||
132 | const QByteArray baseName = basename(tempPath2.data()); | ||||
133 | int parent_fd = -1, base_fd = -1; | ||||
134 | | ||||
135 | if ((parent_fd = open(parentDir.data(), O_DIRECTORY | O_PATH | O_NOFOLLOW)) == -1) { | ||||
136 | reply.setError(errno); | ||||
137 | return reply; | ||||
138 | } | ||||
50 | 139 | | |||
51 | // the path of an existing or a new file/dir upon which the method will operate | 140 | Privilege *origPrivilege = new Privilege{geteuid(), getegid()}; | ||
52 | const QByteArray path = arg1.toByteArray(); | 141 | Privilege *targetPrivilege = nullptr; | ||
53 | 142 | | |||
143 | if (action != CHMOD && action != UTIME) { | ||||
144 | targetPrivilege = getTargetPrivilege(parent_fd); | ||||
145 | } else { | ||||
146 | if ((base_fd = openat(parent_fd, baseName.data(), O_NOFOLLOW)) != -1) { | ||||
147 | targetPrivilege = getTargetPrivilege(base_fd); | ||||
148 | } else { | ||||
149 | reply.setError(errno); | ||||
150 | } | ||||
151 | } | ||||
152 | | ||||
153 | if (dropPrivilege(targetPrivilege)) { | ||||
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… | |||||
54 | switch(action) { | 154 | switch(action) { | ||
55 | case CHMOD: { | 155 | case CHMOD: { | ||
56 | int mode = arg2.toInt(); | 156 | int mode = arg2.toInt(); | ||
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… | |||||
57 | if (chmod(path.data(), mode) == 0) { | 157 | if (fchmod(base_fd, mode) == -1) { | ||
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… | |||||
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… | |||||
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. | |||||
58 | return reply; | 158 | reply.setError(errno); | ||
59 | } | 159 | } | ||
160 | close(base_fd); | ||||
60 | break; | 161 | break; | ||
61 | } | 162 | } | ||
62 | case CHOWN: { | 163 | | ||
63 | int uid = arg2.toInt(); | 164 | case DEL: | ||
64 | int gid = arg3.toInt(); | 165 | case RMDIR: { | ||
65 | if (chown(path.data(), uid, gid) == 0) { | 166 | int flags = 0; | ||
66 | return reply; | 167 | if (action == RMDIR) { | ||
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… | |||||
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? | |||||
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… | |||||
67 | } | 168 | flags |= AT_REMOVEDIR; | ||
68 | break; | | |||
69 | } | 169 | } | ||
70 | case DEL: { | 170 | if (unlinkat(parent_fd, baseName.data(), flags) == -1) { | ||
71 | if (unlink(path.data()) == 0) { | 171 | reply.setError(errno); | ||
72 | return reply; | | |||
73 | } | 172 | } | ||
74 | break; | 173 | break; | ||
75 | } | 174 | } | ||
175 | | ||||
76 | case MKDIR: { | 176 | case MKDIR: { | ||
77 | if (mkdir(path.data(), 0777) == 0) { | 177 | int mode = arg2.toInt(); | ||
78 | return reply; | 178 | if (mkdirat(parent_fd, baseName.data(), mode) == -1) { | ||
179 | reply.setError(errno); | ||||
79 | } | 180 | } | ||
80 | break; | 181 | break; | ||
81 | } | 182 | } | ||
82 | case OPEN: { | 183 | | ||
184 | case OPEN: | ||||
185 | case OPENDIR: { | ||||
83 | int oflags = arg2.toInt(); | 186 | int oflags = arg2.toInt(); | ||
84 | int mode = arg3.toInt(); | 187 | int mode = arg3.toInt(); | ||
85 | int fd = open(path.data(), oflags, mode); | 188 | int extraFlag = O_NOFOLLOW; | ||
86 | bool success = (fd != -1) && sendFileDescriptor(fd, arg4.toByteArray().constData()); | 189 | if (action == OPENDIR) { | ||
87 | close(fd); | 190 | extraFlag |= O_DIRECTORY; | ||
88 | if (success) { | 191 | } | ||
89 | return reply; | 192 | if (int fd = openat(parent_fd, baseName.data(), oflags | extraFlag, mode) != -1) { | ||
90 | } | 193 | gainPrivilege(origPrivilege); | ||
91 | break; | 194 | if (!sendFileDescriptor(fd, arg4.toByteArray().constData())) { | ||
92 | } | 195 | reply.setError(errno); | ||
93 | case OPENDIR: { | | |||
94 | DIR *dp = opendir(path.data()); | | |||
95 | bool success = false; | | |||
96 | if (dp) { | | |||
97 | int fd = dirfd(dp); | | |||
98 | success = (fd != -1) && sendFileDescriptor(fd, arg4.toByteArray().constData()); | | |||
99 | closedir(dp); | | |||
100 | if (success) { | | |||
101 | return reply; | | |||
102 | } | 196 | } | ||
197 | } else { | ||||
198 | reply.setError(errno); | ||||
103 | } | 199 | } | ||
104 | break; | 200 | break; | ||
105 | } | 201 | } | ||
202 | | ||||
106 | case RENAME: { | 203 | case RENAME: { | ||
107 | const QByteArray newName = arg2.toByteArray(); | 204 | tempPath1 = tempPath2 = arg2.toByteArray(); | ||
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… | |||||
108 | if (rename(path.data(), newName.data()) == 0) { | 205 | const QByteArray newParentDir = dirname(tempPath1.data()); | ||
109 | return reply; | 206 | const QByteArray newBaseName = basename(tempPath2.data()); | ||
110 | } | 207 | int new_parent_fd = open(newParentDir.constData(), O_DIRECTORY | O_PATH | O_NOFOLLOW); | ||
111 | break; | 208 | if (renameat(parent_fd, baseName.data(), new_parent_fd, newBaseName.constData()) == -1) { | ||
112 | } | 209 | reply.setError(errno); | ||
113 | case RMDIR: { | | |||
114 | if (rmdir(path.data()) == 0) { | | |||
115 | return reply; | | |||
116 | } | 210 | } | ||
211 | close(new_parent_fd); | ||||
117 | break; | 212 | break; | ||
118 | } | 213 | } | ||
214 | | ||||
119 | case SYMLINK: { | 215 | case SYMLINK: { | ||
120 | const QByteArray target = arg2.toByteArray(); | 216 | const QByteArray target = arg2.toByteArray(); | ||
121 | if (symlink(target.data(), path.data()) == 0) { | 217 | if (symlinkat(target.data(), parent_fd, baseName.data()) == -1) { | ||
122 | return reply; | 218 | reply.setError(errno); | ||
123 | } | 219 | } | ||
124 | break; | 220 | break; | ||
125 | } | 221 | } | ||
222 | | ||||
126 | case UTIME: { | 223 | case UTIME: { | ||
127 | utimbuf ut; | 224 | timespec times[2]; | ||
128 | ut.actime = arg2.toULongLong(); | 225 | time_t actime = arg2.toULongLong(); | ||
129 | ut.modtime = arg3.toULongLong(); | 226 | time_t modtime = arg3.toULongLong(); | ||
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… | |||||
130 | if (utime(path.data(), &ut) == 0) { | 227 | times[0].tv_sec = actime / 1000; | ||
131 | return reply; | 228 | times[0].tv_nsec = actime * 1000; | ||
229 | times[1].tv_sec = modtime / 1000; | ||||
230 | times[1].tv_nsec = modtime * 1000; | ||||
231 | if (futimens(base_fd, times) == -1) { | ||||
232 | reply.setError(errno); | ||||
132 | } | 233 | } | ||
234 | close(base_fd); | ||||
133 | break; | 235 | break; | ||
134 | } | 236 | } | ||
135 | }; | | |||
136 | 237 | | |||
137 | reply.setError(errno ? errno : -1); | 238 | default: | ||
239 | reply.setError(ENOTSUP); | ||||
240 | break; | ||||
241 | } | ||||
242 | gainPrivilege(origPrivilege); | ||||
243 | } else { | ||||
244 | reply.setError(errno); | ||||
245 | } | ||||
246 | | ||||
247 | if (origPrivilege) { | ||||
248 | delete origPrivilege; | ||||
249 | } | ||||
250 | if (targetPrivilege) { | ||||
251 | delete targetPrivilege; | ||||
252 | } | ||||
253 | close(parent_fd); | ||||
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… | |||||
138 | return reply; | 254 | return reply; | ||
139 | } | 255 | } | ||
140 | 256 | | |||
141 | KAUTH_HELPER_MAIN("org.kde.kio.file", FileHelper) | 257 | KAUTH_HELPER_MAIN("org.kde.kio.file", FileHelper) | ||
maltek: I *think* the divisions/multiplications are reversed here. |
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.)