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

[Matthias Gerstner]

mgerstner added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14467

To: chinmoyr, dfaure, ngraham, elvisangelaccio, #frameworks, #dolphin
Cc: mgerstner, fvogt, kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20190131/b0d77048/attachment.htm>