Make use of kauth helper in copy method of file ioslave
ClosedPublic

Authored by chinmoyr on Jul 22 2017, 11:50 AM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Maniphest Tasks
T6561: Polkit support in KIO
Summary

This patch adds calls to execWithElevatedPrivilege() in copy method.
FileProtocol::copy() is an important and somewhat complex method. So I created a different differential to make it less confusing.

Depends on D6829

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
chinmoyr created this revision.Jul 22 2017, 11:50 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 22 2017, 11:50 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
chinmoyr edited the summary of this revision. (Show Details)Jul 22 2017, 2:44 PM
chinmoyr added reviewers: dfaure, Frameworks.
chinmoyr added a subscriber: elvisangelaccio.
dfaure accepted this revision.Aug 4 2017, 8:11 PM

The diff has no context in phabricator, which is a problem for this one (e.g. for the "do we need this here?" questions). Can you upload the update using arc diff for example?

src/ioslaves/file/file_unix.cpp
126

Missing "return;" no?

176

missing return;

This revision is now accepted and ready to land.Aug 4 2017, 8:11 PM
chinmoyr updated this revision to Diff 17782.Aug 6 2017, 4:55 PM
  • added 'return'
  • fixed some typos
  • dest_file.remove() -> QFile::remove(dest). because when working with fd, dest_file.remove() fails but doesn't set errno to EACCES or EPERM.
  • dest_file.setPermissions() -> QFile::setPermissions(...), for the same reason.
dfaure requested changes to this revision.Aug 7 2017, 8:45 PM
dfaure added inline comments.
src/ioslaves/file/file_unix.cpp
244–246

Double negation, looks like one too many.

290–292

same

327–328

Can you test copying a file onto a FAT partition mounted as another uid? The ownership and permissions cannot be applied due to FAT limitations. Does that then trigger a kauth prompt, with this patch? That would be annoying, and wrong since root can't do better anyway.

If I'm right (please test it) then I think this method should remember "I needed root permissions for the main operation" and only in that case use elevated privileges for the small operations at the end like chmod, chown or utime.

Please review whether other methods need the same kind of change.

This revision now requires changes to proceed.Aug 7 2017, 8:45 PM

For the case you mentioned kauth prompt will always be triggered during opening of file for writing. Operations like chmod, chown, utime will work (and then fail) without showing any prompt.

chinmoyr updated this revision to Diff 18015.Aug 11 2017, 6:35 PM
chinmoyr edited edge metadata.

Minor changes

dfaure requested changes to this revision.Aug 11 2017, 8:54 PM

You wrote: "For the case you mentioned kauth prompt will always be triggered during opening of file for writing."

I disagree. My example case is a FAT permission where the user has write permissions, but chown/chgrp/chmod fails due to FAT not supporting these operations. Please test it.
If you don't have a FAT partition, here's how to create one inside a file.

dd if=/dev/zero of=/tmp/fatdevice bs=4k count=100
mkfs.vfat /tmp/fatdevice 
mkdir /tmp/fat
sudo mount -o loop,uid=$UID /tmp/fatdevice /tmp/fat
ls -lad /tmp/fat
kioclient5 copy ~/.bashrc /tmp/fat/destfile

When I do this, I get this warning from kio_file (in ~/.xsession-errors), which is, thankfully, ignored (no user message box).
FileProtocol::copy: "Couldn't preserve group for '/tmp/fat/destfile'"

So I would say: when writing worked without privilege elevation, we should keep the current logic where chown/chgrp/chmod/utime errors are ignored.
On the other hand, if writing required kauth, then we can keep using kauth for those other operations (they won't prompt).

This revision now requires changes to proceed.Aug 11 2017, 8:54 PM
chinmoyr updated this revision to Diff 18208.Aug 15 2017, 7:16 PM
chinmoyr edited edge metadata.
  • Added method to check if filesystem supports chmod, chown and utime.
  • Added check for EUSERCANCELLED error.
chinmoyr updated this revision to Diff 18695.Aug 24 2017, 5:06 PM
  • use the new execWithElevatedPrivilege method
dfaure requested changes to this revision.Aug 24 2017, 7:41 PM
dfaure added inline comments.
src/ioslaves/file/file_unix.cpp
159

What will fileDescriptor() return if fdRecv isn't valid? -1 I suppose? So this code is going to call open(-1), a bit weird.

Maybe it would be easier if all of the above was moved to a helper function or lambda (with dest_file as input arg).

Basically we want to write

if (auto ret = tryOpen(dest_file, [...])) {
    if (!ret.wasCanceled()) {
    ...

This way you can use early returns in tryOpen when fdRecv isn't valid, when execWithElevatedPrivilege returns anything other than success, or when QFile::open fails. Much better than nested ifs, or missing ifs like above ;)

169

One should be enough ;)

175

Was it? How do you know the permissions? Don't they depent on the umask?

Unless I missed something, it would seem to me that a chmod is needed, to ensure restricted permissions. Alternatively, chown + setPermissions again, once the user owns the file.

336–339

These cases - without a variable inside the if - could be made more readable with if (exec(...).failed()) { ... } (or whatever the method name was)

This revision now requires changes to proceed.Aug 24 2017, 7:41 PM
chinmoyr updated this revision to Diff 19166.Sep 4 2017, 3:48 PM
chinmoyr edited edge metadata.
  • Added method tryOpen
  • Ensured correct permissions are set on the newly created file.
  • auto ret -> auto err
  • There's no method like failed() in PrivOpReturnValue. We have to leave those calls as they are.
chinmoyr updated this revision to Diff 19167.Sep 4 2017, 3:50 PM

removedextra semi-colon

dfaure accepted this revision.Sep 9 2017, 1:10 PM
This revision is now accepted and ready to land.Sep 9 2017, 1:10 PM
chinmoyr updated this revision to Diff 20366.Oct 5 2017, 10:15 AM

used the new header and method

chinmoyr updated this revision to Diff 24583.Jan 2 2018, 12:19 PM

Changes:

  1. Added method tryChangeAttr. This will call chown/chmod/utime in copy with elevated privileges only during the brief period authorization is kept.

2.Added check for unit test mode at beginning of copy. "copy" calls execWihElevatedPrivileges number of times but for unit test once is enough.
3.Replaced dest_file.setPermissions(...) with the static version. This will ensure that the error code set when using file decriptor is either EACCES or EPERM.

@dfaure I was curious about this block of code

if (!src_file.open(QIODevice::ReadOnly)) {
        if (auto err = tryOpen(src_file, _src, O_RDONLY, S_IRUSR)) {
            if (!err.wasCanceled()) {
                error(KIO::ERR_CANNOT_OPEN_FOR_READING, src);
            }
            return;
        }
    }

In case the operation is canceled the function returns without calling error or finished. While testing these changes with dolphin I never got any warning so I ignored.
But isn't it wrong? If that's the case then I have fix every block similar to this and with "copy" and "put" changes will definitely look ugly.

Doesn't execWithElevatedPrivilege call ERR_USER_CANCELED on cancel?
That was the idea, at least.

I can't check, it's really hard to navigate code spread over many review requests, I can't wait for all of this to be pushed ;-)

Doesn't execWithElevatedPrivilege call ERR_USER_CANCELED on cancel?

It definitely does. And here I was thinking KIO has gone nuts.
*biggest facepalm*