Update file ioslave's temporary authorization list
ClosedPublic

Authored by chinmoyr on Feb 11 2018, 8:51 AM.

Details

Reviewers
dfaure
fvogt
Group Reviewers
Frameworks
Summary

After authenticating for org.kde.kio.file.exec action add it to the list of temprary authorizations.

Depends on D10641

Test Plan

Before testing set permission in org.kde.kio.file.policy to auth_admin_keep.

create files /opt/{a,b}
open kioslavetest
delete /opt/a
open ksysguard and send interrupt signal to kioslavetest
again open kioslavetest
delete /opt/b

Without this patch "/opt/b" will be deleted but password won't be asked.
With this patch kauth authentication dialog as well as other prompts are shown properly.

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
chinmoyr created this revision.Feb 11 2018, 8:51 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 11 2018, 8:51 AM
chinmoyr requested review of this revision.Feb 11 2018, 8:51 AM

If I'm not mistaken, the application sends its own pid to the slave.
This means it could just fake it.

The whole work is being done inside KIO::Job. If the application uses regular Jobs then I can't see how it can fake it.

anthonyfieroni added inline comments.
src/core/slavebase.h
957 ↗(On Diff #26905)

Will break ABI, add new member variables only in SlaveBasePrivate

The whole work is being done inside KIO::Job. If the application uses regular Jobs then I can't see how it can fake it.

By not using KIO or using a modified KIO. Never assume you can trust anything you get sent.

markg added a subscriber: markg.Feb 11 2018, 12:58 PM

I just tried this:
kdesu gwenview (type root password and go to the root homefolder).
In gwenview i can now go to that folder.
In dolphin (started as user, not root) i can't get into that folder.

I don't really see what you try to fix here as it seems to be working just fine here.
Or i don't understand it.. In that case, please provide reproducible steps.

The whole work is being done inside KIO::Job. If the application uses regular Jobs then I can't see how it can fake it.

By not using KIO or using a modified KIO. Never assume you can trust anything you get sent.

Going by this logic, it seems any attempt at providing security from job's side is pointless. So how about moving the handling of prompts to slave's side? At least we can be sure a prompt will be shown all the time.

fvogt added a comment.Feb 11 2018, 1:17 PM

The whole work is being done inside KIO::Job. If the application uses regular Jobs then I can't see how it can fake it.

By not using KIO or using a modified KIO. Never assume you can trust anything you get sent.

Going by this logic, it seems any attempt at providing security from job's side is pointless.

It is.

So how about moving the handling of prompts to slave's side? At least we can be sure a prompt will be shown all the time.

Sounds good. Once polkit granted file.so authorized access to the helper, it needs to be treated as privilege boundary so it needs to prompt.

I just tried this:
kdesu gwenview (type root password and go to the root homefolder).
In gwenview i can now go to that folder.
In dolphin (started as user, not root) i can't get into that folder.

I don't really see what you try to fix here as it seems to be working just fine here.
Or i don't understand it.. In that case, please provide reproducible steps.

Currently file ioslave does not support reading contents of a locked(is this the right word?) folder. So no file operations are possible.

markg added a comment.Feb 11 2018, 1:22 PM

I just tried this:
kdesu gwenview (type root password and go to the root homefolder).
In gwenview i can now go to that folder.
In dolphin (started as user, not root) i can't get into that folder.

I don't really see what you try to fix here as it seems to be working just fine here.
Or i don't understand it.. In that case, please provide reproducible steps.

Currently file ioslave does not support reading contents of a locked(is this the right word?) folder. So no file operations are possible.

Could you provide steps to reproduce what you try to fix?

Could you provide steps to reproduce what you try to fix?

Steps:
create files /opt/{a,b}
open kioslavetest
delete /opt/a
open ksysguard and send interrupt signal to kioslavetest
again open kioslavetest
delete /opt/b

Without this patch "/opt/b" will be deleted but password won't be asked.
With this patch an error will be shown.

Indeed the sender could definitely fake the PID.
One could generate and send a sha1 and store it in the slave (and send it as metadata with every command), but this can still be sniffed.
I assume the KAuth security principle is that an intruder (who would have access to your session, and therefore can do a lot of things already, including installing a keylogger), shouldn't be able to get root access?
In that case, either kio_file should lose priviledges immediately (sounds annoying for the user, but maybe that's the price of this feature?), or the app (libkio) should perform the file operations directly.

I'm surprised you didn't hit that yet, btw. E.g. deleting local files, will not involve kio_file. DeleteJobPrivate::deleteNextFile calls QFile::remove() directly. Of course this isn't the case for all file operations (otherwise your patch wouldn't work at all), and it might not even be a good idea to generalize this (it's already visible that deleting a 6 GB local file will freeze the app, because of this - which was written under the assumption that deleting is fast).

Brainstorming further: the other possibility is that kio_file processes that gained root auth, cannot be reused by another app later on. This could be done somehow in the KIO scheduler or in klauncher, if they can be told that this slave should be killed rather than reused once idle. The design of that stuff isn't fully clear in my mind (I didn't write that part), but make sure not to get confused by "idle slave which is associated to my process" (KIO::Scheduler's IdleJob, kills the slave after 3 minutes), and "idle slave that has been returned to klauncher" (in frameworks/kinit), for use by another process (or killed after 30s). But I can't find the code that returns an idle slave to klauncher (only a "slave that has been put on hold", which is a different use case (documented in kio/docs/krun-passing-slaves.txt).

@dfaure I was thinking about revoking authorization just before the idle slave is reassigned by klauncher. polkit-qt-1 provides revokeTemporaryAuthorization just for this purpose. We only need to implement it on KAuth's side. Then on slave's side we can do something like KAuth::Action("org.kde.kio.file.exec").revoke(). How does this sound?

Sounds good, but the tricky part will be finding how to run some code in the (idle) slave at the right time. Maybe in SlaveBase::disconnectSlave() ?

chinmoyr updated this revision to Diff 27482.Feb 18 2018, 4:57 PM

Changed approach. Now temporary authorization is revoked.

chinmoyr updated this revision to Diff 27483.Feb 18 2018, 5:03 PM
chinmoyr retitled this revision from Limit the use of file.so for privilege operation to one application to Revoke authorization of file ioslave before it is used by another process.
chinmoyr edited the summary of this revision. (Show Details)
chinmoyr edited the test plan for this revision. (Show Details)

Updated test plan

chinmoyr updated this revision to Diff 28009.Feb 25 2018, 11:28 AM
chinmoyr retitled this revision from Revoke authorization of file ioslave before it is used by another process to Update file ioslave's temporary authorization list.
chinmoyr edited the summary of this revision. (Show Details)

Updated function name.
Updated title and summary.
With this it is the third time I am completely changing the title and summary. When will it be too much? Or should I create a new patch everytime there's a change in approach?

dfaure accepted this revision.Mar 4 2018, 10:48 AM

(Reusing the same phab request is fine in this case, because there were no review comments yet.
Once there are, and they don't apply to a completely redesigned commit, then indeed better start a new phab request.)

This commit looks ok, but I would merge it with the one that adds the method addTemporaryAuthorization(actionId), they go together.

This revision is now accepted and ready to land.Mar 4 2018, 10:48 AM
chinmoyr closed this revision.Apr 3 2018, 6:07 PM

Merged with D10818