Auth Support: Drop privileges if target is not owned by root
ClosedPublic

Authored by chinmoyr on Jul 29 2018, 5:45 PM.

Details

Summary
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.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
chinmoyr created this revision.Jul 29 2018, 5:45 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 29 2018, 5:45 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
chinmoyr requested review of this revision.Jul 29 2018, 5:45 PM
dfaure added a comment.Aug 5 2018, 8:53 PM

+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.

fvogt added a subscriber: fvogt.Jan 31 2019, 12:17 PM

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);
chinmoyr updated this revision to Diff 59290.Jun 6 2019, 7:07 PM
  • 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?

chinmoyr updated this revision to Diff 59319.Jun 7 2019, 9:00 AM

use seteuid to make the logic for rename work

@mgerstner How is this looking now? Yea/nay?

@mgerstner How is this looking now? Yea/nay?

He is not available for the next couple of weeks. However he did ask one of his colleagues to have a look at it.

maltek requested changes to this revision.Jun 18 2019, 4:15 PM
maltek added a subscriber: maltek.

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
75

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.)

106

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.

131

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().

252–253

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

257

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

This revision now requires changes to proceed.Jun 18 2019, 4:15 PM
chinmoyr updated this revision to Diff 60167.Thu, Jun 20, 6:39 PM
chinmoyr marked 5 inline comments as done.

Thanks @maltek. I've fixed all the issues.

maltek requested changes to this revision.Fri, Jun 21, 10:04 AM

I noticed a few more things on the second read.

src/ioslaves/file/kauth/filehelper.cpp
123

This needs error handling.

153

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

156

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

157

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.

167

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

204

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.

226

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?

This revision now requires changes to proceed.Fri, Jun 21, 10:04 AM
chinmoyr updated this revision to Diff 60215.Fri, Jun 21, 11:20 AM
chinmoyr marked 6 inline comments as done.

Addresed the issues.

chinmoyr added inline comments.Fri, Jun 21, 11:22 AM
src/ioslaves/file/kauth/filehelper.cpp
157

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.

167

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.

maltek added inline comments.Fri, Jun 21, 1:54 PM
src/ioslaves/file/kauth/filehelper.cpp
157

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

167

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.

chinmoyr updated this revision to Diff 60249.Fri, Jun 21, 3:23 PM

Separated (l)chown since it's to be done with elavated privileges.

maltek accepted this revision.Fri, Jun 21, 3:28 PM

Looks good to me now!

This revision is now accepted and ready to land.Fri, Jun 21, 3:28 PM

Thanks for your help!

This revision was automatically updated to reflect the committed changes.
chinmoyr edited the summary of this revision. (Show Details)Fri, Jun 21, 4:05 PM

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?

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?

https://commits.kde.org/kio/91901a64adcaab9e444bb67a75bebd2ea77c3db2 should fix this.