[systemd-devel] Work on adding polkit support to systemd1

[Stef Walter]

On 18.08.2014 18:22, Lennart Poettering wrote:
> On Fri, 15.08.14 19:28, Stef Walter (stefw at redhat.com) wrote:
> 
>>
>> On 15.08.2014 18:56, Lennart Poettering wrote:
>>> On Fri, 15.08.14 18:25, Stef Walter (stefw at redhat.com) wrote:
>>>
>>>>
>>>> On 13.08.2014 20:27, Lennart Poettering wrote:
>>>>> On Wed, 06.08.14 13:23, Stef Walter (stefw at redhat.com) wrote:
>>>>>
>>>>>> I've done initial work on adding polkit support to systemd1 DBus
>>>>>> methods. You can see it here:
>>>>
>>>> Thanks for the review. Worked on this a bit more.
>>>>
>>>> I might drop off the face of the earth for a couple weeks. In case I do,
>>>> I thought I'd update my public branch. But if I'm around, I'll test and
>>>> prepare a patch set early next week.
>>>>
>>>>>> https://github.com/stefwalter/systemd/commits/polkit-systemd1
>>>
>>> Hmm, yuck. There's a security issue here... Reading the capabilities
>>> from the sender on dbus1 is racy, since we have to read it from
>>> /proc/$PID/stat and don't get it sent along with the message, like we do
>>> on kdbus. A rogue client could send a message, quickly invoke some suid
>>> binary, and we'd consider the client trusted.
>>>
>>> Now for the low-level implementation of the vtable bit we are actually
>>> smart, and check by UID on dbus1, and by cap on kdbus, in order to avoid
>>> the vulnerability.
>>>
>>> Hmm, now I wonder how to best handle this for cases like this, we
>>> probably need some generic way how clients can make this decision in an
>>> always safe way...
>>>
>>> I need to think more about this...
>>
>> By the way, there's some similar problematic code in the modified
>> KillUnit() method implementation ... changed from specifying the
>> CAP_KILL in the vtable, and now it does a manual check.
>>
>>> Patch set looks great otherwise. I'll come up with something for the
>>> security issue, then adapt your patch, and merge it.
>>
>> I haven't tested the updated branch at all :) So it may go boom...
> 
> I have now pushed this, after reworking this on top some major changes
> to bus_verify_polkit(), which avoids having to pass the original
> callbacks through to the function that ultimately does the verification.
> 
> While merging I also made another change, you are probably not going to
> like: I turned of the interactivity for the polkit checks. Interactivity
> needs to be optional, and it currently is for all out polkit-enabled bus
> methods. And we should do the same for the PID 1 offered methods.

Ugh.

> Now, of course, we should open this up for inetractive (after all,
> that's what polkit is good for), but we probably need a new set of
> methods for that, which take the original arguments but also take a
> boolean argument to enable ineractivity. Hence, we probably should have
> StartUnit2() in addition to StartUnit().

That seems ugly. I think we should either:

 * Have a method which we can invoke to make a client opt into
   interactive polkit prompting for any invoked method.

 * Version all the org.freedesktop.systemd1.Manager to
   org.freedesktop.systemd1.Manager2 or something like that and support
   both interfaces.

Cheers,

Stef