maltek requested changes to this revision.
maltek added a comment.
This revision now requires changes to proceed.
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.
INLINE COMMENTS
> filehelper.cpp:74
> + } else {
> + target_fd = openat(parent_fd, baseName.data(), O_NOFOLLOW);
> + }
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.)
> filehelper.cpp:105
> + if (setegid(newgid) == -1 || seteuid(newuid) == -1) {
> + return false;
> + }
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.
> filehelper.cpp:144
> + int mode = arg2.toInt();
> + if (fchmodat(parent_fd, baseName.data(), mode,
> AT_SYMLINK_NOFOLLOW) == -1) {
> + reply.setError(errno);
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().
> filehelper.cpp:222
> + times[1].tv_sec = modtime * 1000;
> + times[1].tv_nsec = modtime / 1000;
> + if (utimensat(parent_fd, baseName.data(), times,
> AT_SYMLINK_NOFOLLOW) == -1) {
I *think* the divisions/multiplications are reversed here.
> filehelper.cpp:231
> }
> - };
>
> + close(parent_fd);
Somewhere here, I'd expect the privileges to be escalated again, to restore the
state from before the call to dropPrivileges().
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D14467
To: chinmoyr, dfaure, ngraham, elvisangelaccio, #frameworks, #dolphin, maltek
Cc: maltek, mreeves, mgerstner, fvogt, kde-frameworks-devel, LeGast00n,
michaelh, ngraham, bruns
