Add kauth helper to file ioslave
ClosedPublic

Authored by chinmoyr on Jun 12 2017, 4:18 PM.

Details

Summary

This patch adds a kauth helper to be used by file ioslave.

Depends on D6709

Diff Detail

Branch
first
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
dfaure requested changes to this revision.Jul 1 2017, 12:07 PM
dfaure added inline comments.
src/core/jobuidelegateextension.h
230 ↗(On Diff #15729)

This class is public, adding virtual methods is BIC.
You have to make a V2 class that inherits from this one (with a KF6 TODO to merge them), or a completely separate class that's specific to privileged operations.

Remember to add @since 5.37 to the class.

src/ioslaves/file/file_unix.cpp
684 ↗(On Diff #15729)

s/the the/the/

src/ioslaves/file/kauth/helper.h
29 ↗(On Diff #15729)

Best class ever. Add a comment to explain what it does?
(not much, AFAICS....)

This revision now requires changes to proceed.Jul 1 2017, 12:07 PM
chinmoyr updated this revision to Diff 16738.Jul 15 2017, 1:07 PM
chinmoyr edited edge metadata.
chinmoyr marked 4 inline comments as done.
chinmoyr edited the test plan for this revision. (Show Details)

In my previous revision the logic for showing warning from ioslave was flawed. In case of deleteRecursive everything would have worked out fine but in case of copy the logic for warning would have failed. Since CopyJob creates number of sub jobs, there are as many number of slaves. If there happen to be more than one file with read access restricted then ioslave's warning would have been shown multiple times.

In this revision, I added a variable m_enablePrivilegeExecution, a public method isPrivilegeExecutionEnabled and an additional job flag PrivilegeExecution to the KIO Job class.

Now if an application want's to execute a privilege file operation, it will

  1. create a job with PrivilegeExecution flag.
  2. the flag will cause the job to set m_enablePrivilegeExecution to true.
  3. when execWithElevatedPrivilege() is called it will first emit dataReq() signal.
  4. the job will respond with a message "ElevatePrivilege" if it supports it and the slave will continue.

The step 2 is very important. Even if the flag is set its upto us to decide which job should support it. And the jobs which support it will also show warnings prior to notifying the slave. This prevents misuse of the job during the brief period of elevated privilege.

@dfaure what do you say about the feasibility of this approach?

@davidedmundson will this revision solve the problem you mentioned D6198. I will include the code for warning in the job itself.

chinmoyr updated this revision to Diff 17013.Jul 22 2017, 10:59 AM
chinmoyr retitled this revision from Add basic KAuth support to file ioslave to Add kauth helper to file ioslave.
chinmoyr edited the summary of this revision. (Show Details)
chinmoyr removed subscribers: davidedmundson, dfaure, eliasp, aacid.

Removed everything except the helper's code
Replaced all the helper method with on method, exec().

dfaure requested changes to this revision.Jul 29 2017, 6:12 AM
dfaure added inline comments.
src/ioslaves/file/kauth/filehelper.cpp
72

this calls for a switch()

87

opendir can return 0, in which case the next line will probably crash.

Overall, this whole method seriously lacks error handling in my opinion. Just checking for errno at the end isn't enough, e.g. in cases like this where multiple calls are being made.

And even otherwise, I'm unsure if ignoring the return value of chmod/chown/unlink/mkdir and *just* checking errno is enough or not. Does anyone else know?

src/ioslaves/file/kauth/filehelper.h
23

should be FILEHELPER_H to match the filename

34

This isn't public API -> no @since.

This revision now requires changes to proceed.Jul 29 2017, 6:12 AM
chinmoyr updated this revision to Diff 17507.Aug 1 2017, 5:11 PM
chinmoyr edited edge metadata.
  • used switch..case
chinmoyr added inline comments.Aug 1 2017, 5:21 PM
src/ioslaves/file/kauth/filehelper.cpp
87

In case of OPEN and OPENDIR, I can see why we need error checking, opendir() can return null and there are some issues (I read on internet, can't recall the source) if close() fails. But in case of single function calls like chmod() I can't see how return value matters. Say if unlink fails for whatever reasons, we can return the error code and the checking for exact error code can be done by ioslave. Is there something I am overlooking?

dfaure requested changes to this revision.Aug 3 2017, 7:48 AM
dfaure added inline comments.
src/ioslaves/file/kauth/filehelper.cpp
41 ↗(On Diff #17507)

I assume this enum is duplicated elsewhere (in whoever is serializing the arguments for FileHelper), right?
Can the duplication be avoided by sharing a private header? Otherwise this at least needs a comment like "keep in sync with..." in both places.

102 ↗(On Diff #17507)

missing else or break here, no?
You're sending the valid fd, followed by -1.

87

The documentation for chown and others says:

Upon successful completion, these functions shall return 0. Otherwise, these functions shall return −1 and set errno to indicate the error.

It does NOT say, that errno isn't set when the function is successful. It seems to me that it would be perfectly valid for a libc implementation to do something like "try this, it fails (and sets errno), then try that, it worked, return 0".

For this reason I would feel much safer (especially in code run by root!) if the error handling was more classic, i.e. by checking return values.

This revision now requires changes to proceed.Aug 3 2017, 7:48 AM
chinmoyr updated this revision to Diff 17780.Aug 6 2017, 4:30 PM
chinmoyr edited edge metadata.
  • Add private header
  • minor fixes
chinmoyr added inline comments.Aug 6 2017, 4:35 PM
src/ioslaves/file/kauth/filehelper.cpp
87

It does NOT say, that errno isn't set when the function is successful. It seems to me that it would be perfectly valid for a libc implementation to do something like "try this, it fails (and sets errno), then try that, it worked, return 0".

It is valid and isn't that uncommon.

For this reason I would feel much safer (especially in code run by root!) if the error handling was more classic, i.e. by checking return values.

Currently helper sets the error and slave terminates due to Kauth::ErrorReply. Do you want this to not happen, like in case where chown succeeds but errno is set? And what error code should I check for and whether it should be for each case or at the end of method?

dfaure added a comment.Aug 6 2017, 7:16 PM

Not sure I understand your question. If we agree on using return values then there is only one way to do that. E.g. chmod() returns 0 on success, so the code could be like

if (chmod(...) == 0)
     return reply;
 break;

which jumps to the if (errno) code when chmod returns non-zero.
Same for all other methods, check the man page for each to avoid making any assumptions, but presumably they all return 0 on success.

chinmoyr updated this revision to Diff 18012.Aug 11 2017, 6:04 PM

Improved error checking in helper.

dfaure requested changes to this revision.Aug 13 2017, 10:17 PM
dfaure added inline comments.
src/ioslaves/file/kauth/filehelper.cpp
40 ↗(On Diff #17507)

this basically abuses a global variable for something that could just be a return value from this function.

45 ↗(On Diff #17507)

should be unnecessary now, with the above suggested change

135 ↗(On Diff #17507)

I hope we never have the opposite problem, where a libc function returns non-zero but "forgets" to set errno.
Now that you added return statements on success everywhere, maybe this line could say Q_ASSERT(errno != 0), or

reply.setError(errno ? errno : -1);

to make sure we never return success when an error happened.

This revision now requires changes to proceed.Aug 13 2017, 10:17 PM
chinmoyr updated this revision to Diff 18201.Aug 15 2017, 4:38 PM
chinmoyr edited edge metadata.
  • fixed minor issues
chinmoyr added inline comments.Aug 15 2017, 4:49 PM
src/ioslaves/file/kauth/filehelper.cpp
45 ↗(On Diff #17507)

In my system errno is set to 11. It seems like the case where function call succeeds but errno is set. But then it happens right after control reaches this method so I can't figure out which call might have set the error. I can't say if its unique to my system or not but there it is. So I think its best to set errno to 0 just to be on safe side.

dfaure requested changes to this revision.Aug 15 2017, 8:54 PM
dfaure added inline comments.
src/ioslaves/file/kauth/filehelper.cpp
47 ↗(On Diff #17507)

Well, this proves exactly my earlier point: we should only test errno *when* a libc function fails.

If this code is still checking errno after success somewhere, then *that* it what should be fixed. And once it's fixed, there's no need to reset errno here.

(BTW strerror(11) is EAGAIN, "Resource temporarily unavailable", rather frequent for non-blocking sockets, which is probably what triggered the slave to wake up in the first place.)

So, where is this code checking for errno even after success? In the caller of this method? It's hard to review all these separate review requests because I never have a global overview or the ability to search across the whole codebase -- but at the same time, everything in a single merge request would kill this slow webbrowser... [QtWebEngine compiled in debug mode] :-)

This revision now requires changes to proceed.Aug 15 2017, 8:54 PM
chinmoyr updated this revision to Diff 18235.Aug 16 2017, 1:02 PM
chinmoyr edited edge metadata.
  • Remove errno assignment
chinmoyr added inline comments.Aug 16 2017, 1:11 PM
src/ioslaves/file/kauth/filehelper.cpp
47 ↗(On Diff #17507)

Actually action OPEN and OPENDIR were relying on this errno assignment (in the previous revision). I forgot that in this revision both of them return on success.
I have now removed that statement. Sorry for the trouble.

dfaure requested changes to this revision.Aug 16 2017, 11:08 PM
dfaure added inline comments.
src/ioslaves/file/kauth/filehelper.cpp
35 ↗(On Diff #17507)

BTW what happens if two instances of this helper (and two instances of kio_file) try to do this at the same time? Don't they need different socket paths, to avoid interferring with each other?

I suppose the PID of the kio_file process should be added to this path, which means passing it in the QVariantMap to exec(), right?

105 ↗(On Diff #17507)

You send fd=-1 if opendir fails, but not if dirfd fails.
Is it even needed to send -1, if we're going to return an error reply anyway?
(disclaimer, I don't know sendFileDescriptor nor what the code on the calling side does)

This revision now requires changes to proceed.Aug 16 2017, 11:08 PM

Is it even needed to send -1, if we're going to return an error reply anyway?

The current logic is "after a successful sendFileDescriptor close the socket". So sending -1 was to ensure socket is closed.

chinmoyr updated this revision to Diff 18300.Aug 17 2017, 5:44 PM
chinmoyr edited edge metadata.
This comment was removed by chinmoyr.
chinmoyr updated this revision to Diff 18301.Aug 17 2017, 5:54 PM

-Removed the hard-coded socket path. Now it will take socket path from ioslave.
-Removed sendFileDescriptor(-1) calls.

chinmoyr updated this revision to Diff 18504.Aug 21 2017, 4:45 PM
  • Minor changes in sendFileDescriptor function because constructor of FdSender changed in D6709.
dfaure accepted this revision.Aug 23 2017, 9:08 PM
elvisangelaccio added inline comments.
src/ioslaves/file/kauth/file.actions
4 ↗(On Diff #17507)

We could use more descriptive sentences here. Note also that we are not necessarily asking for the root password. How about something like:

  • Name=File Operation
  • Description=The file operation that you requested requires elevated privileges
dfaure added a comment.Sep 3 2017, 8:35 AM

(strange that Phabricator still says "Needs Review" when this has two approvals)

elvisangelaccio accepted this revision.Sep 6 2017, 8:16 PM

(strange that Phabricator still says "Needs Review" when this has two approvals)

Probably because "Frameworks" as group reviewer was still in the "requested changes" status.

This revision is now accepted and ready to land.Sep 6 2017, 8:16 PM
chinmoyr updated this revision to Diff 20310.Oct 3 2017, 4:58 PM

replaced occurence of sharefd.{h, cpp} with fdsender.{h, cpp}

dfaure accepted this revision.Oct 7 2017, 4:20 PM
This revision was automatically updated to reflect the committed changes.
chinmoyr reopened this revision.Oct 29 2017, 4:10 PM
This revision is now accepted and ready to land.Oct 29 2017, 4:10 PM
This revision was automatically updated to reflect the committed changes.