Discussion:
Work on adding polkit support to systemd1
(too old to reply)
Stef Walter
2014-08-06 11:23:55 UTC
Permalink
I've done initial work on adding polkit support to systemd1 DBus
methods. You can see it here:

https://github.com/stefwalter/systemd/commits/polkit-systemd1

Basic rules:

* Read access for everyone

* Methods that modifies running unit state is controlled by a polkit
action: org.freedesktop.systemd1.manage-units

* Methods that modifies unit state files is controlled by a polkit
action: org.freedesktop.systemd1.manage-unit-files

* Many methods are only callable by root callers, like: Poweroff()
Kexec() etc...

* Job.Cancel() and Manager.CancelJob() are callable by the caller(s)
that started the job.

* Setting properties is only possible by root callers.

The way that each callback in sd-bus has to handle verification seems a
bit risky to me. So I've only opened up the specific interfaces I
touched in the DBus policy file.

Eventually the DBus policy file would go away, but hopefully sd-bus will
have a less risky way of verifying callers at that point.

I need to work on testing this. Will send a patch set when I'm done.

I'd be happy to add documentation here when we're done:

http://www.freedesktop.org/wiki/Software/systemd/dbus/

Cheers,

Stef
Colin Guthrie
2014-08-06 12:23:12 UTC
Permalink
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
methods.
Hmmm, I thought this was deliberately not included as it meant a
circular dep on polkit when talking to the system that's used to start
up polkitd itself... what happens if you try to talk to systemd1
interface during early boot before polkitd has started (and before a dep
that's needed by it) has started?

I thought the main reason for logind to essentially proxy the
Power/Reboot related stuff was such that polkit support could be added
there outside of systemd1 itself and thus not have any circular dep
problems.

Perhaps things have moved on from the old days, or maybe I picked this
up wrong in the first place and this is not a concern.

Col.
--
Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
Tribalogic Limited http://www.tribalogic.net/
Open Source:
Mageia Contributor http://www.mageia.org/
PulseAudio Hacker http://www.pulseaudio.org/
Trac Hacker http://trac.edgewall.org/
Stef Walter
2014-08-06 12:44:16 UTC
Permalink
Post by Colin Guthrie
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
methods.
Hmmm, I thought this was deliberately not included as it meant a
circular dep on polkit when talking to the system that's used to start
up polkitd itself...
We were discussing this in Strasbourg. Lennart can probably summarize
better than I can. But my understanding is that these circular
dependency issues have been resolved.
Post by Colin Guthrie
what happens if you try to talk to systemd1
interface during early boot before polkitd has started (and before a dep
that's needed by it) has started?
I think it falls back on just allowing uid zero. But would be good to
double check.
Post by Colin Guthrie
I thought the main reason for logind to essentially proxy the
Power/Reboot related stuff was such that polkit support could be added
there outside of systemd1 itself and thus not have any circular dep
problems.
FWIW, we don't involve polkit on on Power/Reboot and similar
of.systemd1.Manager method calls. General purpose callers should
continue to go through logind and/or the shutdown command.

Stef
Colin Guthrie
2014-08-06 13:09:46 UTC
Permalink
Post by Stef Walter
Post by Colin Guthrie
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
methods.
Hmmm, I thought this was deliberately not included as it meant a
circular dep on polkit when talking to the system that's used to start
up polkitd itself...
We were discussing this in Strasbourg. Lennart can probably summarize
better than I can. But my understanding is that these circular
dependency issues have been resolved.
Ahh good to hear! I'll no doubt catch up at some point on the latest
cool stuff! :)

Col
--
Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
Tribalogic Limited http://www.tribalogic.net/
Open Source:
Mageia Contributor http://www.mageia.org/
PulseAudio Hacker http://www.pulseaudio.org/
Trac Hacker http://trac.edgewall.org/
Lennart Poettering
2014-08-13 18:32:06 UTC
Permalink
Post by Colin Guthrie
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
methods.
Hmmm, I thought this was deliberately not included as it meant a
circular dep on polkit when talking to the system that's used to start
up polkitd itself...
This indeed used to be a problem, since our polkit client code was
naively written synchronously, so that we might have ended up waiting
for polkit from PID1, while polkit needed to be started by PID 1 thus
resulting in a deadlock. However, I have since rewritten the logic
entirely, it's fully asynchronous now. While the cyclic dep is still not
ideal (and we should be careful when adding more cases like this), it's
not a real problem now, and in this case sounds like a good idea.
Post by Colin Guthrie
what happens if you try to talk to systemd1
interface during early boot before polkitd has started (and before a dep
that's needed by it) has started?
This should quickly fail and access be refused. Note that polkit is only
ever consulted if the client lacks priviliges, hence this actually never
happens during the normal boot process.
Post by Colin Guthrie
I thought the main reason for logind to essentially proxy the
Power/Reboot related stuff was such that polkit support could be added
there outside of systemd1 itself and thus not have any circular dep
problems.
Not really. It's mostly about the inhibition stuff and figuring out the
right policy depending on who is logged in on which seats, and things
like that...

Lennart
--
Lennart Poettering, Red Hat
Lennart Poettering
2014-08-13 18:27:43 UTC
Permalink
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
https://github.com/stefwalter/systemd/commits/polkit-systemd1
* Read access for everyone
* Methods that modifies running unit state is controlled by a polkit
action: org.freedesktop.systemd1.manage-units
* Methods that modifies unit state files is controlled by a polkit
action: org.freedesktop.systemd1.manage-unit-files
* Many methods are only callable by root callers, like: Poweroff()
Kexec() etc...
* Job.Cancel() and Manager.CancelJob() are callable by the caller(s)
that started the job.
* Setting properties is only possible by root callers.
The way that each callback in sd-bus has to handle verification seems a
bit risky to me. So I've only opened up the specific interfaces I
touched in the DBus policy file.
Eventually the DBus policy file would go away, but hopefully sd-bus will
have a less risky way of verifying callers at that point.
I need to work on testing this. Will send a patch set when I'm done.
http://www.freedesktop.org/wiki/Software/systemd/dbus/
THis looks pretty good. A few comments:

The first three patches look totally OK.

Regarding the fourth: please don't bind any checks against the UID,
check for CAP_SYS_ADMIN instead, we make it equally easy.

Why is verify_root_sync() invoked that often? I mean, for these cases
the dbus1 policy should not grant access anyway, so we don't really need
to check for permissions here again. I'd really prefer if on dbus1 would
use the dbus1 policy for as much access control work as useful, and only
do it on our own if we really need to.

reload (and probably also reexecute) should probably hook into polkit
too, with a new action? it sounds useful enough for people to invoke it
frequently.

Note that on kdbus access control works differently than on dbus1: it's
the client's responsibility to implement access control. To make this
easy our sd-bus library actually allows encoding in the method vtable
which calls are accessible for unpriviliged processes. That's what the
SD_BUS_VTABLE_UNPRIVILEGED flag in the object vtables is for (grep for
it). The flag (or its absence) has no effect on dbus1 daemons at all,
and only matters for kdbus. To make sure your changes also work
correctly and as intended on kdbus you need to add the flag to all
methods that you are now opening up via polkit. Because otherwise method
calls won't even get that far.

Or in other words: everything that is opened up in the dbus1 policy also
needs to be opened up with SD_BUS_VTABLE_UNPRIVILIGED in the object
vtables...

Sometiems you added spurious newlines to the patches, please don't.

I'd prefer if the dbus1 policy would precisely list the method calls
that are now opened up...

Looks like really good stuff I must say!

Lennart
--
Lennart Poettering, Red Hat
Stef Walter
2014-08-15 16:25:28 UTC
Permalink
Post by Lennart Poettering
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
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.
Post by Lennart Poettering
Post by Stef Walter
https://github.com/stefwalter/systemd/commits/polkit-systemd1
Updated this branch ^^

<snip>
Post by Lennart Poettering
Post by Stef Walter
The way that each callback in sd-bus has to handle verification seems a
bit risky to me. So I've only opened up the specific interfaces I
touched in the DBus policy file.
Your comments below address what I was worried about here ^^
Post by Lennart Poettering
Regarding the fourth: please don't bind any checks against the UID,
check for CAP_SYS_ADMIN instead, we make it equally easy.
Done.
Post by Lennart Poettering
Why is verify_root_sync() invoked that often? I mean, for these cases
the dbus1 policy should not grant access anyway, so we don't really need
to check for permissions here again. I'd really prefer if on dbus1 would
use the dbus1 policy for as much access control work as useful, and only
do it on our own if we really need to.
Done.
Post by Lennart Poettering
reload (and probably also reexecute) should probably hook into polkit
too, with a new action? it sounds useful enough for people to invoke it
frequently.
New action is called: org.freedesktop.systemd1.reload-daemon
Post by Lennart Poettering
Note that on kdbus access control works differently than on dbus1: it's
the client's responsibility to implement access control. To make this
easy our sd-bus library actually allows encoding in the method vtable
which calls are accessible for unpriviliged processes. That's what the
SD_BUS_VTABLE_UNPRIVILEGED flag in the object vtables is for (grep for
it). The flag (or its absence) has no effect on dbus1 daemons at all,
and only matters for kdbus. To make sure your changes also work
correctly and as intended on kdbus you need to add the flag to all
methods that you are now opening up via polkit. Because otherwise method
calls won't even get that far.
Or in other words: everything that is opened up in the dbus1 policy also
needs to be opened up with SD_BUS_VTABLE_UNPRIVILIGED in the object
vtables...
Makes sense ... but out of interest, why don't you just rely on this for
dbus1 as well? It seems that maintaining these two distinct access
control methods is risky.
Post by Lennart Poettering
Sometiems you added spurious newlines to the patches, please don't.
Removed a couple extra newlines. I hope I found them all.
Post by Lennart Poettering
I'd prefer if the dbus1 policy would precisely list the method calls
that are now opened up...
Done.

One other thing is that it seems like the ofs.Manager.LoadUnit() DBus
method call is wide open for anyone to call. Is this intentional? I
don't know all the implications, but wanted to highlight it.

Stef
Lennart Poettering
2014-08-15 16:56:36 UTC
Permalink
Post by Stef Walter
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
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.
Post by Stef Walter
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...

Patch set looks great otherwise. I'll come up with something for the
security issue, then adapt your patch, and merge it.

Thanks,

Lennart
--
Lennart Poettering, Red Hat
Stef Walter
2014-08-15 17:28:17 UTC
Permalink
Post by Lennart Poettering
Post by Stef Walter
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
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.
Post by Stef Walter
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.
Post by Lennart Poettering
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...

Stef
Lennart Poettering
2014-08-18 16:22:13 UTC
Permalink
Post by Stef Walter
Post by Lennart Poettering
Post by Stef Walter
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
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.
Post by Stef Walter
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.
Post by Lennart Poettering
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.

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().

Lennart
--
Lennart Poettering, Red Hat
Stef Walter
2014-09-01 07:51:29 UTC
Permalink
Post by Lennart Poettering
Post by Stef Walter
Post by Lennart Poettering
Post by Stef Walter
Post by Stef Walter
I've done initial work on adding polkit support to systemd1 DBus
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.
Post by Stef Walter
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.
Post by Lennart Poettering
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.
Post by Lennart Poettering
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
David Herrmann
2014-09-01 09:47:24 UTC
Permalink
Hi
Post by Stef Walter
Post by Lennart Poettering
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.
Post by Lennart Poettering
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().
* 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.
We had the idea to reserve a single bit in the dbus message header for
that. See the discussion on the dbus-ML:
http://lists.freedesktop.org/archives/dbus/2014-August/016294.html

It looks like the most sane way to resolve this issue, imho.

Thanks
David
Stef Walter
2014-09-01 13:28:47 UTC
Permalink
Post by David Herrmann
Hi
Post by Stef Walter
Post by Lennart Poettering
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.
Post by Lennart Poettering
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().
* 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.
We had the idea to reserve a single bit in the dbus message header for
http://lists.freedesktop.org/archives/dbus/2014-August/016294.html
Thanks.
Post by David Herrmann
It looks like the most sane way to resolve this issue, imho.
I guess so. Makes a lot of sense.

We'll need to see how backportable this ends up being for all of
libdbus, gdbus ... of hand it doesn't that seem *that* invasive if it's
just a flag.

Otherwise (for Cockpit) we'll end up doing the brain-dead wrapping all
systemd APIs with yet another daemon that just does interactive polkit
authentication :S

Will keep an eye on this.

Cheers,

Stef
Lennart Poettering
2014-09-03 18:40:27 UTC
Permalink
Post by Stef Walter
Post by David Herrmann
We had the idea to reserve a single bit in the dbus message header for
http://lists.freedesktop.org/archives/dbus/2014-August/016294.html
Thanks.
I have now posted a patch for the spec to the dbus ML:

http://lists.freedesktop.org/archives/dbus/2014-September/016314.html

(wanted to add you to CC, but forgot)
Post by Stef Walter
Post by David Herrmann
It looks like the most sane way to resolve this issue, imho.
I guess so. Makes a lot of sense.
We'll need to see how backportable this ends up being for all of
libdbus, gdbus ... of hand it doesn't that seem *that* invasive if it's
just a flag.
Yeah, it should be pretty trivial.

You don't even strictly need to backport this to gdbus actually, as
gdbus gives you full, direct access to the flags byte. libdbus doesn't
though, there we need an API addition first. (And gdbus should expose
the enum, too, of course...)

Lennart
--
Lennart Poettering, Red Hat
Loading...