chown is performed with elevated privileges. For chmod and utime, the uid and gid of the target file is considered when relinquishing privileges. For the remaining actions, the uid and gid of target's parent directory is considered.
Details
Details
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Branch
- arcpatch-D14467
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 12512 Build 12530: arc lint + arc unit
Comment Actions
+1, the idea seems sane to me, but I'm no expert with this kind of API so I'd like someone else to check it.
Comment Actions
chinmoyr asked me to review this patch since I was involved with A CVE in similar code in kate / ktexteditor a while ago.
Back then the logic was special purpose to replace a file in the file system with content provided via D-Bus. This here is a way more generic interface with a lot of file operations that complicates the security review quite a bit. I don't know whether this is well thought out in all cases ... for example: The OPEN action allows to specify arbitrary open flags and mode. So it can also be used to create new files. The calling application receives the resulting file descriptor ... this is a lot of power and each application that employs this interface would need to be checked in turn for safe usage of that file descriptor.
Also the mkdir() with mode 0777 is a bit optimistic, shouldn't the caller specify the exact permissions? Maybe it should become a private directory.
Regarding the dropPrivileges() I first have some coding related comments. It takes an int action where it should rather take an enum ActionType parameter. This adds a bit of type safety and makes more clear what kind of parameter is expected. Then I think the function wants to do to many things at once:
- it checks whether a privilege drop is required at all
- if so then it performs the privilege drop
- the return value signals some kind of "should we continue" flag but not whether privileges have been dropped or not
- it handles single-file operations as well as dual-file operations (rename)
Better separating the logic might be a good idea. One function that determines whether a privilege drop is necessary and to which uid/gid privileges should be dropped to. Then another function that unconditionally performs the privilege drop. The case of a single file operation should be better separated from the dual-file operation which can become quite more complex to check.
Now for a couple of security issues with this proposal:
- in line 70 we have a case where no privileges are dropped but operation is still continued. When a rename of two paths is requested and both are owned by different users then there is no safe way to perform the rename as root user (actually the permissions of the parent directories are more important here but that is another issue). Such a case is fishy and I would maybe rather not perform the action than performing it in an unsafe way.
- the stat() call is used to determine ownership. This follows symlinks.
- you are operating on the path names all the time. This involves race conditions. I.e. the file object stat() is called for can be a different one than the one that chown() etc. is called upon.
So you should open the files to operate on one time at the start of the call and then only operate on the file descriptors to be on the safe side. The open() should use O_NOFOLLOW. There is fstat() to get the ownership info of an open file descriptor. Then we have fchmod(), fchown(), mkdirat(), unlinkat(), symlinkat(), futimes() and so on. Only this way you can make sure that you're always operating on the same file system object.
An example case for MKDIR. You get the request to MKDIR "/var/www/localhost/app/CGI". Following ownership situation:
-rw-r--r-- 1 apache apache 2.2K 3. Jan 10:53 /var/www/localhost/app -rw-r--r-- 1 apache apache 2.4K 3. Jan 10:51 /var/www/localhost
So your dropPrivileges() will look at /var/www/localhost/app to determine which user to drop privileges to. Suppose the apache user is compromised and simply replaces /var/www/localhost/app by a symlink to /root or so. Now your function thinks it needs to drop to root i.e. drop nothing at all. Before the actual mkdir() happens the evil apace user replaces the symlink by another one pointing to /etc. As a result the MKDIR operation will create a directory /etc/CGI.
To prevent such a situation one safe approach is the following:
int pardir_fd = open("/var/www/localhost/app", O_DIRECTORY | O_PATH | O_NOFOLLOW);
struct stat info;
if( fstat(pardir_fd, &info) != 0 )
// error handling
// drop privileges to info.st_uid and info.st_gid
// finally create the directory using just the basename
mkdirat(pardir_fd, "CGI", mode);Comment Actions
- int -> ActionType
- separated the logic in dropPrivileges() to two parts
- accepting mode argument in mkdir
- used *at() functions
- minor cosmetic changes
@mgerstner In case of rename, when owners are different I am trying to rename twice. First after changing euid
and egid to the owner of src's parent dir and then the owner of dest's parent dir, hoping that at least one of
them is a privileged user. Do you think it's a good idea?
Comment Actions
He is not available for the next couple of weeks. However he did ask one of his colleagues to have a look at it.
Comment Actions
I've gone over the code and found some issues. I haven't fully thought through the design on a conceptual level, because I assume Matthias already did.
| src/ioslaves/file/kauth/filehelper.cpp | ||
|---|---|---|
| 74 | 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.) | |
| 105 | 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. | |
| 144 | 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(). | |
| 222 | I *think* the divisions/multiplications are reversed here. | |
| 231 | Somewhere here, I'd expect the privileges to be escalated again, to restore the state from before the call to dropPrivileges(). | |
Comment Actions
I noticed a few more things on the second read.
| src/ioslaves/file/kauth/filehelper.cpp | ||
|---|---|---|
| 122 ↗ | (On Diff #59290) | This needs error handling. |
| 170 ↗ | (On Diff #59290) | typo: there's a second & missing after the first condition. (I don't think it ends up affecting the result.) |
| 173 ↗ | (On Diff #59290) | There's no error handling here, which will likely lead to weird EBADF errors getting returned later. |
| 174 ↗ | (On Diff #59290) | 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. |
| 178 ↗ | (On Diff #59290) | I just realized that this wouldn't allow changing the owner of symbolic links. The way to go here is lchown. |
| 215 ↗ | (On Diff #59290) | 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. |
| 237 ↗ | (On Diff #59290) | 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? |
| src/ioslaves/file/kauth/filehelper.cpp | ||
|---|---|---|
| 174 ↗ | (On Diff #59290) | 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. |
| 178 ↗ | (On Diff #59290) | 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. |
| src/ioslaves/file/kauth/filehelper.cpp | ||
|---|---|---|
| 174 ↗ | (On Diff #59290) | IMO, it's fine (and less complicated) to just do both in one single privileged fchmod call. |
| 178 ↗ | (On Diff #59290) | 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. |
Comment Actions
It would appear that the commit of this change disturbed somethng with FreeBSD builds - see https://build.kde.org/view/Failing/job/Frameworks/job/kio/job/kf5-qt5%20FreeBSDQt5.12/135/
Could someone take a look into that please?