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

[Malte Kraus]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190618/45ec6534/attachment.html>