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

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

Details

Reviewers
dfaure
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 edited the test plan for this revision. (Show Details)Feb 18 2018, 5:11 PM
chinmoyr updated this revision to Diff 27995.Feb 25 2018, 10:39 AM
chinmoyr retitled this revision from Revoke temporary authorization of KIO slave before sending it to klauncher to Revoke temporary authorization of KIO slave before sending status to IdleSlave.
chinmoyr edited the summary of this revision. (Show Details)
  1. Split this diff into three parts D108{18,20} and this.
  2. Updated summary and title.
chinmoyr updated this revision to Diff 27996.Feb 25 2018, 10:41 AM

Removed other files which were included in previous commit by mistake

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.

chinmoyr abandoned this revision.Apr 4 2018, 4:11 AM

Not required right now