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 noticed a few more things on the second read.

INLINE COMMENTS

> filehelper.cpp:123
> +    const QByteArray baseName = basename(tempPath2.data());
> +    int parent_fd = open(parentDir.data(), O_DIRECTORY | O_PATH | O_NOFOLLOW);
> +    int base_fd = -1;

This needs error handling.

> filehelper.cpp:129
> +
> +    if (action != CHMOD & action != CHOWN && action != UTIME) {
> +        targetPrivilege = getTargetPrivilege(parent_fd);

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

> filehelper.cpp:132
> +    } else {
> +        base_fd = openat(parent_fd, baseName.data(), O_NOFOLLOW);
> +        targetPrivilege = getTargetPrivilege(base_fd);

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

> filehelper.cpp:133
> +        base_fd = openat(parent_fd, baseName.data(), O_NOFOLLOW);
> +        targetPrivilege = getTargetPrivilege(base_fd);
>      }

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.

> filehelper.cpp:150
> +                int gid = arg3.toInt();
> +                if (fchown(base_fd, uid, gid) == -1) {
> +                    reply.setError(errno);

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

> filehelper.cpp:187
> +                gainPrivilege(origPrivilege);
> +                bool sendSuccess = sendFileDescriptor(fd, arg4.toByteArray().constData());
> +                if (fd != -1 && sendSuccess) {

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.

> filehelper.cpp:209
> +                if (symlinkat(target.data(), parent_fd, baseName.data()) == -1) {
> +                    return reply;
> +                }

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?

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: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20190621/eb9ca76b/attachment.htm>