Re: [systemd-devel] Work on adding polkit support to systemd1
On Mon, 01.09.14 15:28, Stef Walter (s...@thewalter.net) wrote: 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 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) 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
On 18.08.2014 18:22, Lennart Poettering wrote: On Fri, 15.08.14 19:28, Stef Walter (st...@redhat.com) wrote: On 15.08.2014 18:56, Lennart Poettering wrote: On Fri, 15.08.14 18:25, Stef Walter (st...@redhat.com) wrote: On 13.08.2014 20:27, Lennart Poettering wrote: On Wed, 06.08.14 13:23, Stef Walter (st...@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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
Hi On Mon, Sep 1, 2014 at 9:51 AM, Stef Walter st...@redhat.com wrote: On 18.08.2014 18:22, Lennart Poettering wrote: 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. 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
On 01.09.2014 11:47, David Herrmann wrote: Hi On Mon, Sep 1, 2014 at 9:51 AM, Stef Walter st...@redhat.com wrote: On 18.08.2014 18:22, Lennart Poettering wrote: 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. 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 Thanks. 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
On Fri, 15.08.14 19:28, Stef Walter (st...@redhat.com) wrote: On 15.08.2014 18:56, Lennart Poettering wrote: On Fri, 15.08.14 18:25, Stef Walter (st...@redhat.com) wrote: On 13.08.2014 20:27, Lennart Poettering wrote: On Wed, 06.08.14 13:23, Stef Walter (st...@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. 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
On 13.08.2014 20:27, Lennart Poettering wrote: On Wed, 06.08.14 13:23, Stef Walter (st...@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 Updated this branch ^^ snip 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 ^^ Regarding the fourth: please don't bind any checks against the UID, check for CAP_SYS_ADMIN instead, we make it equally easy. Done. 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. 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 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. Sometiems you added spurious newlines to the patches, please don't. Removed a couple extra newlines. I hope I found them all. 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
On Fri, 15.08.14 18:25, Stef Walter (st...@redhat.com) wrote: On 13.08.2014 20:27, Lennart Poettering wrote: On Wed, 06.08.14 13:23, Stef Walter (st...@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... 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
On 15.08.2014 18:56, Lennart Poettering wrote: On Fri, 15.08.14 18:25, Stef Walter (st...@redhat.com) wrote: On 13.08.2014 20:27, Lennart Poettering wrote: On Wed, 06.08.14 13:23, Stef Walter (st...@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... Stef ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
On Wed, 06.08.14 13:23, Stef Walter (st...@redhat.com) wrote: 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/ 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
On Wed, 06.08.14 13:23, Colin Guthrie (gm...@colin.guthr.ie) wrote: Stef Walter wrote on 06/08/14 12:23: 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. 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. 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
Stef Walter wrote on 06/08/14 12:23: 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/ ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
On 06.08.2014 14:23, Colin Guthrie wrote: Stef Walter wrote on 06/08/14 12:23: 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. 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. 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Work on adding polkit support to systemd1
Stef Walter wrote on 06/08/14 13:44: On 06.08.2014 14:23, Colin Guthrie wrote: Stef Walter wrote on 06/08/14 12:23: 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/ ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
