Revoke temporary authorization of KIO slave before sending status to IdleSlave
AbandonedPublic
Actions

Authored by chinmoyr on Feb 18 2018, 4:48 PM.

Details

Reviewers

Summary

An idle slave authorized for privilege operation can be easily misused. So when it asks SlaveBase for its status then
try to revoke all the authorizations before sending back the status (which includes the authorization status as well)
to IdleSlave.

Depends on D10568 and D10638

Test Plan

1.An over-simplified version of how the slave is sent to klauncher:
2.SlaveBase calls connectSlave(d->poolSocket)
3.This in turn emits newConnection
4.In klauncher this signal connects to acceptSlave which creates a new IdleSlave.
5.Then mConnectionServer gets the connection backend of the Slave and sets it as the connection backed in IdleSlave.
6.IdleSlave then sends CMD_SLAVE_STATUS command and gets Slave's details. (pid, protocol etc)
7.kaluncher then stores this IdleSlave.

Test as mentioned in D10437

Diff Detail

Repository

R241 KIO

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

chinmoyr created this revision.Feb 18 2018, 4:48 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 18 2018, 4:48 PM

Restricted Application added a subscriber: Frameworks. · View Herald Transcript

chinmoyr requested review of this revision.Feb 18 2018, 4:48 PM

chinmoyr added a dependent revision: D10437: Update file ioslave's temporary authorization list.Feb 18 2018, 5:03 PM

chinmoyr edited the test plan for this revision. (Show Details)Feb 18 2018, 5:11 PM

Split this diff into three parts D108{18,20} and this.
Updated summary and title.

Removed other files which were included in previous commit by mistake

chinmoyr mentioned this in T8075: Fix security issues with KAuth support in KIO.Feb 25 2018, 11:44 AM

chinmoyr added a dependency: D10820: Send slave's polkit authorization status to the host.Feb 25 2018, 11:56 AM

chinmoyr removed a dependency: D10568: Handle privilege operation confirmation prompts in SlaveBase.

chinmoyr removed a dependent revision: D10437: Update file ioslave's temporary authorization list.Feb 25 2018, 12:01 PM

dfaure requested changes to this revision.Mar 4 2018, 10:39 AM

dfaure added inline comments.

src/core/slavebase.cpp
147	That looks like horrible API ;) A getter that optionally makes changes, based on a boolean... urgh ;) "Duplicating" a for loop isn't really duplication, go for a different method.
554	It turns out that indeed this method is only called when the slave is going to Idle, but, hmm, in this code nothing says so, it looks like a method that just sends current status, and that could be called at any moment... I would suggest to at least add a comment here, something like // This method is only called from the IdleSlave constructor, the slave has just been returned to klauncher, clear any authorization so that another application cannot benefit from it. ... above the method call that you'll have to add to clear the auths, due to the previous comment ;)

This revision now requires changes to proceed.Mar 4 2018, 10:39 AM

chinmoyr added inline comments.Mar 4 2018, 10:53 AM

src/core/slavebase.cpp
147	Now I feel stupid for doing this. Apart from that I am not even confident about this change because it involves changes in KAuth (Obviously I am not sure about them either). How about we drop this and the related revisions for now and just delete the slave whenever there's authorization?

dfaure added inline comments.Mar 4 2018, 10:54 AM

src/core/slavebase.cpp
147	If by delete you mean kill, I think you're right, it's probably simpler and safer, and this should happen rarely enough for this not to be a performance issue.

Not required right now

Revision Contents
Changeset List

			Path	Packages
M			src/core/CMakeLists.txt (4 lines)
M			src/core/job.cpp (142 lines)
M			src/core/job_p.h (5 lines)
M			src/core/simplejob.cpp (2 lines)
M			src/core/slavebase.h (9 lines)
M			src/core/slavebase.cpp (7 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	27481	652564f		Feb 18 2018, 4:48 PM	★	★
Diff 2	27995	0f8c2d7	1. Split this diff into three parts D108{18,20} and this.	Feb 25 2018, 10:39 AM	★	★
Diff 3	27996	d23c9f8	Removed other files which were included in previous commit by mistake	Feb 25 2018, 10:41 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
2a383b9be659	18c4b30bc901	d23c9f8bc9aa	Chinmoy Ranjan Pradhan	Revoke temporary authorization of KIO slave (Show More…)	Feb 25 2018, 10:29 AM

Status	Author	Revision
Closed	chinmoyr	D10824 Delete IdleSlave having temporary authorization
Closed	chinmoyr	D10822 Store temporary authorization status in IdleSlave
Closed	chinmoyr	D10437 Update file ioslave's temporary authorization list
Abandoned	chinmoyr	D10641 Revoke temporary authorization of KIO slave before sending status to IdleSlave
Abandoned	chinmoyr	D10638 [KAuth] Implement revokeTemporaryAuthorization in Polkit1Backend
Abandoned	chinmoyr	D10636 Add support for revoking temporary authorization in KAuth
Closed	chinmoyr	D10820 Send slave's polkit authorization status to the host
Closed	chinmoyr	D10818 Store PolicyKit action which the slave is authorized to perform
Closed	chinmoyr	D10568 Handle privilege operation confirmation prompts in SlaveBase
Closed	chinmoyr	D10567 Remove handling of privilege operation confirmation prompts from KIO::Job
Closed	chinmoyr	D11010 Add MSG_SLAVE_STATUS_V2 to slave interface