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

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

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

[Malte Kraus]

maltek added inline comments.

INLINE COMMENTS

> chinmoyr wrote in filehelper.cpp:133
> Ah! Since I was testing inside /opt I didn't notice. I think the order here 
> should be: drop privilege -> change grp -> gain privilege -> change user.

IMO, it's fine (and less complicated) to just do both in one single privileged 
`fchmod` call.

> chinmoyr wrote in filehelper.cpp:150
> Do you think it'll be a bad idea to skip the case for symlinks in utime, 
> chmod, chown, for now? Right now there's no code in KIO that requires these 
> operations to be performed on the link itself.

Fine by me - I'm only really here to look for security problems, not to decide 
on which features are required for this to land.

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

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

[Malte Kraus]

maltek accepted this revision.
maltek added a comment.
This revision is now accepted and ready to land.


  Looks good to me now!

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D14467

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

D23692: kdesu: set kernel flags to prevent ptrace instead of relying on setgid

[Malte Kraus]

maltek created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
maltek requested review of this revision.

REVISION SUMMARY
  So I noticed that kdesu is setgid 'nogroup'. That group is the fallback for 
groups from a remote NFS share that do not exist on the local machine. Since 
kdesu does not deal with NFS, I wanted to get rid of this (ab)use of 'nogroup'.
  
  From all that I could gather (inline comments and a discussion on the KDE su 
handbook ), the 
goal of the setgid bit on the binary is not to access any file as 'nogroup', 
but to prevent other processes of the calling user from accessing cached 
passwords, e.g. through ptrace(), core dumps or /proc//memory. While 
setgid is one way to achieve that, both Linux and FreeBSD allow setting a 
kernel flag to directly to disable such access. So I went for that.

REPOSITORY
  R299 KDESu

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

AFFECTED FILES
  src/client.cpp
  src/client.h
  src/kdesud/CMakeLists.txt
  src/kdesud/kdesud.cpp

To: maltek
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23692: kdesu: set kernel flags to prevent ptrace instead of relying on setgid

[Malte Kraus]

maltek added a reviewer: adridg.
maltek added a comment.


  I have no idea who to specify as reviewer, so I'm picking the maintainer from 
`src/README`, Adriaan de Groot.

REPOSITORY
  R299 KDESu

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

To: maltek, adridg
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns