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
