After authenticating for org.kde.kio.file.exec action add it to the list of temprary authorizations.
Depends on D10641
dfaure | |
fvogt |
Frameworks |
After authenticating for org.kde.kio.file.exec action add it to the list of temprary authorizations.
Depends on D10641
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.
No Linters Available |
No Unit Test Coverage |
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.
src/core/slavebase.h | ||
---|---|---|
957 ↗ | (On Diff #26905) | Will break ABI, add new member variables only in SlaveBasePrivate |
By not using KIO or using a modified KIO. Never assume you can trust anything you get sent.
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.
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.
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.
Currently file ioslave does not support reading contents of a locked(is this the right word?) folder. So no file operations are possible.
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() ?
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?
(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.