Hi, On debian, lintian (an tool testing for common issues in the packages) is complaining that the org.freedesktop.GeoClue2.Agent policy file has too broad permissions. https://lintian.debian.org/maintainer/bigon@debian.org.html#geoclue-2.0 After talking to smvc he was suggesting to use <policy user="@dbus_srv_user@"> instead of using <policy context="default">
(In reply to Laurent Bigonville from comment #0) > Hi, > > On debian, lintian (an tool testing for common issues in the packages) is > complaining that the org.freedesktop.GeoClue2.Agent policy file has too > broad permissions. > > https://lintian.debian.org/maintainer/bigon@debian.org.html#geoclue-2.0 That is cause agent is supposed to run as normal user, one for each user. gnome-shell is currently the only real agent. Agents are supposed to be third-party system apps. In future, I'm hoping systemd/(some sandboxing entity) will be the agent and we probably won't have to install this config file at all.
(I wrote the Lintian checks that complain about this, and I maintain D-Bus in Debian and upstream.) The issue here is that every file in /etc/dbus-1/system.d applies to everything on the system bus - there is no way to limit policies to particular packages. So Geoclue2's policy allows any uid to call any method on the Properties interface at the path /org/freedesktop/GeoClue2/Agent, in *any* destination. You might think "why would any other service have an object at /org/freedesktop/GeoClue2/Agent?", but not all services distinguish between object paths: those that are implemented in terms of simplistic libdbus filters[1] typically do not. The short term solution, which would downgrade the Lintian error to a warning, is to limit the caller to geoclue's uid; that mitigates this problem a lot. The ideal long-term solution, which would suppress the warning too, is to have every Geoclue agent queue for a particular well-known name, let's say org.freedesktop.GeoClue2.AnyAgent, to opt-in to being treated as an agent - they don't have to *own* it, as long as they are in the queue for it - and use a send_destination that matches that well-known name. The Geoclue daemon would still send its messages to a particular agent's unique name - dbus-daemon will still match the send_destination rule for any recipient that is queueing for that name. However, this would require code changes in every Geoclue agent, so it's technically a D-Bus API break, and should have a transitional period. -- [1] which is generally a bad idea, but sometimes people do it anyway, because people like to minimize dependencies
As I understand it, the setup is something like this, right? So the same shape as NetworkManager? * system service runs as @dbus_srv_user@ * agents (gnome-shell or whatever) run as e.g. smcv * clients (gnome-maps etc.) and agents want to send method calls to geoclue - this is fine, the rules allowing this can have a send_destination * system service wants to send broadcast signals - this is fine, it's allowed by default anyway * system service wants to send method calls to agent - this is what the problematic rule is intended to allow * agent wants to send replies to those method calls - this is fine, requested replies are allowed by default If that is the case, then tightening the problematic rule from "anyone can send method calls to anywhere if they look like an agent method call" to "only the geoclue system service can send those method calls" seems safe. My current Lintian checks say that a D-Bus rule is excessively broad unless at least one of these is true: * it has a send_destination; or * it has a send_interface that is not org.freedesktop.DBus.Properties (o.fd.DBus.Properties is "too vague", it could mean any underlying interface); or * it is in <policy user="root">, because root is all-powerful anyway
(In reply to Simon McVittie from comment #2) > (I wrote the Lintian checks that complain about this, and I maintain D-Bus > in Debian and upstream.) > > The issue here is that every file in /etc/dbus-1/system.d applies to > everything on the system bus - there is no way to limit policies to > particular packages. So Geoclue2's policy allows any uid to call any method > on the Properties interface at the path /org/freedesktop/GeoClue2/Agent, in > *any* destination. > > You might think "why would any other service have an object at > /org/freedesktop/GeoClue2/Agent?", but not all services distinguish between > object paths: those that are implemented in terms of simplistic libdbus > filters[1] typically do not. > > The short term solution, which would downgrade the Lintian error to a > warning, is to limit the caller to geoclue's uid; that mitigates this > problem a lot. Ah yes, caller could be restricted to geoclue alone for sure. I was thinking in term of destination only. > The ideal long-term solution, which would suppress the warning too, is to > have every Geoclue agent queue for a particular well-known name, let's say > org.freedesktop.GeoClue2.AnyAgent, to opt-in to being treated as an agent - > they don't have to *own* it, as long as they are in the queue for it - and > use a send_destination that matches that well-known name. The Geoclue daemon > would still send its messages to a particular agent's unique name - > dbus-daemon will still match the send_destination rule for any recipient > that is queueing for that name. However, this would require code changes in > every Geoclue agent, so it's technically a D-Bus API break, and should have > a transitional period. Not sure that is worth all the effort. Things will completely change with sandboxing anyway.
Created attachment 117058 [details] [review] 0001-Retrict-org.freedesktop.DBus.Properties-interface-to.patch
Created attachment 117059 [details] [review] 0002-Do-not-ship-generated-dbusservice-files-in-the-tarba.patch
Comment on attachment 117058 [details] [review] 0001-Retrict-org.freedesktop.DBus.Properties-interface-to.patch Review of attachment 117058 [details] [review]: ----------------------------------------------------------------- Thanks for providing the patches. Comments below: Please try to follow https://wiki.gnome.org/Git/CommitMessages . Specifically: * Shorter shotlog * Rationale/details in details section of log. ::: data/org.freedesktop.GeoClue2.Agent.conf.in @@ +1,5 @@ > +<!DOCTYPE busconfig PUBLIC > + "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN" > + "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd"> > +<busconfig> > + <policy user="@dbus_srv_user@"> I think we'd want to define a default policy too so non-root apps can be agent? @@ +5,5 @@ > + <policy user="@dbus_srv_user@"> > + <allow send_interface="org.freedesktop.GeoClue2.Agent" > + send_path="/org/freedesktop/GeoClue2/Agent"/> > + <allow send_interface="org.freedesktop.DBus.Properties" > + send_path="/org/freedesktop/GeoClue2/Agent"/> Wasn't the decision to restrict this to receive_* rather than send_*? Have you tested if this doesn't break gnome-shell as agent?
Comment on attachment 117059 [details] [review] 0002-Do-not-ship-generated-dbusservice-files-in-the-tarba.patch Review of attachment 117059 [details] [review]: ----------------------------------------------------------------- Same comment about commit log. ::: configure.ac @@ -187,5 @@ > src/geocode-glib/Makefile > src/public-api/Makefile > po/Makefile.in > - data/org.freedesktop.GeoClue2.conf > - data/org.freedesktop.GeoClue2.Agent.conf * you added the last line in the other patch, would prefer if you re-order to not have to add a line that you'll remove in a later patch. * AFAIK this line dicates that these files need to be generated, nothing to do with distribution. ::: data/Makefile.am @@ +25,5 @@ > +dbusservice_in_files = org.freedesktop.GeoClue2.conf.in \ > + org.freedesktop.GeoClue2.Agent.conf.in > +dbusservice_DATA = $(dbusservice_in_files:.conf.in=.conf) > +$(dbusservice_DATA): $(dbusservice_in_files) Makefile > + sed -e "s|\@dbus_srv_user\@|$(dbus_srv_user)|" $@.in > $@ Ah so you are moving the generation from configure to makefile, why? This looks much hairier solution than configur.ac one. @@ +41,4 @@ > > DISTCLEANFILES = $(service_DATA) \ > $(systemconf_DATA) \ > + $(dbusservice_DATA) \ I'd just have this line here and remove $(dbusservice_DATA) from EXTRA_DIST like you are doing but not replace it with _in_files.
(In reply to Zeeshan Ali from comment #7) > I think we'd want to define a default policy too so non-root apps can be > agent? You're misunderstanding the proposed policy file. This allows @dbus_srv_user@ to send messages that will be received by any process, if those messages are for the desired interface and path; it says nothing about the uid of the recipient. send_* rules can only be affected by the uid of the sender, not the uid of the recipient. > Wasn't the decision to restrict this to receive_* rather than send_*? By default, on the system bus, sending is restrictive (default-deny) and receiving is permissive (default-allow). If you want to restrict receiving, you would need to have a <policy> that is more restrictive than the default, and another <policy> with lower precedence that provides limited exceptions. However, if you want to control receiving, you cannot base the decision on the uid of the sender, only the uid of the recipient. It sounds as though receive_* rules are unsuitable for what you want, because you want a specific uid to be able to send messages to a general recipient; so you must be specific at the sending side, not the receiving side. (Yes, the dbus-daemon policy language is horrible, and if I designed a language for that purpose today, it would be orders of magnitude simpler.)
(In reply to Simon McVittie from comment #9) > (In reply to Zeeshan Ali from comment #7) > > I think we'd want to define a default policy too so non-root apps can be > > agent? > > You're misunderstanding the proposed policy file. This allows > @dbus_srv_user@ to send messages that will be received by any process, if > those messages are for the desired interface and path; it says nothing about > the uid of the recipient. > > send_* rules can only be affected by the uid of the sender, not the uid of > the recipient. > > > Wasn't the decision to restrict this to receive_* rather than send_*? > > By default, on the system bus, sending is restrictive (default-deny) and > receiving is permissive (default-allow). If you want to restrict receiving, > you would need to have a <policy> that is more restrictive than the default, > and another <policy> with lower precedence that provides limited exceptions. > > However, if you want to control receiving, you cannot base the decision on > the uid of the sender, only the uid of the recipient. It sounds as though > receive_* rules are unsuitable for what you want, because you want a > specific uid to be able to send messages to a general recipient; so you must > be specific at the sending side, not the receiving side. No, I was looking to only restrict the receiving side so if these patches achieve that and been tested, I'll accept them but I'd want some explanation of this in the commit log. > (Yes, the dbus-daemon policy language is horrible, and if I designed a > language for that purpose today, it would be orders of magnitude simpler.) Indeed. :)
Created attachment 117814 [details] [review] 0001-Restrict-who-can-send-messages-from-the-Agent-path.patch
I added the 2nd patch because it seems that make dist is currently shipping the generated file in the tarball which is probably not needed
Comment on attachment 117814 [details] [review] 0001-Restrict-who-can-send-messages-from-the-Agent-path.patch Review of attachment 117814 [details] [review]: ----------------------------------------------------------------- If you have test against gnome-shell, it's good then. Can you please follow the shorter shorlog convention I have been pointing out: https://wiki.gnome.org/Git/CommitMessages
Comment on attachment 117059 [details] [review] 0002-Do-not-ship-generated-dbusservice-files-in-the-tarba.patch Review of attachment 117059 [details] [review]: ----------------------------------------------------------------- Could you please provide answer comments (inline using the 'Review' link) here and/or provide a new patch addressing them?
Created attachment 117898 [details] [review] 0002-Do-not-ship-generated-dbusservice-files-in-the-tarba.patch
Created attachment 117899 [details] [review] 0001-Only-allow-the-geoclue-user-to-send-messages-from-th.patch
Comment on attachment 117898 [details] [review] 0002-Do-not-ship-generated-dbusservice-files-in-the-tarba.patch Review of attachment 117898 [details] [review]: ----------------------------------------------------------------- Short log still exceeds 50 chars and could easily be fixed by just "Do not" -> "Don't". :) But i'll fix that before pushing.
Comment on attachment 117899 [details] [review] 0001-Only-allow-the-geoclue-user-to-send-messages-from-th.patch Review of attachment 117899 [details] [review]: ----------------------------------------------------------------- * Shortlog still pretty long. * Now I'm confused again. I repeat: agent can run as any user and the only agent we currently have (gnome-shell) does not run as geoclue user or root. So either the log really needs to be fixed/improved or this patch breaks gnome-shell's agent.
Comment on attachment 117899 [details] [review] 0001-Only-allow-the-geoclue-user-to-send-messages-from-th.patch Review of attachment 117899 [details] [review]: ----------------------------------------------------------------- The code change looks right. The confusion might be because this is backwards: > Only allow the geoclue user to send messages from the Agent path This should be "only allow the geoclue user to call agents' methods", or something. That way round, it would make more sense?
To be clear: if geoclue is allowed to call methods on an arbitrary agent, then that same arbitrary agent is allowed to send exactly one reply per method call, which is what gives gnome-shell the ability to reply.
(In reply to Simon McVittie from comment #19) > Comment on attachment 117899 [details] [review] [review] > 0001-Only-allow-the-geoclue-user-to-send-messages-from-th.patch > > Review of attachment 117899 [details] [review] [review]: > ----------------------------------------------------------------- > > The code change looks right. The confusion might be because this is > backwards: > > > Only allow the geoclue user to send messages from the Agent path > > This should be "only allow the geoclue user to call agents' methods", or > something. That way round, it would make more sense? Yeah that makes sense but what I still don't understand is why Laurent doesn't say anything himself. :) One question still remains: Has both patches been tested?
Comment on attachment 117899 [details] [review] 0001-Only-allow-the-geoclue-user-to-send-messages-from-th.patch Review of attachment 117899 [details] [review]: ----------------------------------------------------------------- To be honest, I'm still sometime confused about dbus, oh well... Do you want me to rework the description? (According to the link you pointed me to, the description should be max 70char, with a preference to 50 :p) And yes the patches have been tested with gnome-shell
Comment on attachment 117899 [details] [review] 0001-Only-allow-the-geoclue-user-to-send-messages-from-th.patch Review of attachment 117899 [details] [review]: ----------------------------------------------------------------- Yes please. The shortlog is supposed to be 50 chars max, but you can't always fit a meaningful enough summary of the change in that limit so another limit is provided when that's the case. :) Anway, always try you best!
(In reply to Zeeshan Ali from comment #23) > Comment on attachment 117899 [details] [review] [review] > 0001-Only-allow-the-geoclue-user-to-send-messages-from-th.patch > > Review of attachment 117899 [details] [review] [review]: > ----------------------------------------------------------------- > > Yes please. The shortlog is supposed to be 50 chars max, but you can't > always fit a meaningful enough summary of the change in that limit so > another limit is provided when that's the case. :) Anway, always try you > best! Oh and I think you might be confusing shortlog with description. I was referring to shortlog being too long. Each line of description should fit 74 chars and that you can always follow since you can have as many lines as you need in description part.
Created attachment 117922 [details] [review] 0001-Only-allow-the-geoclue-user-to-call-agents-methods.patch
Pushed with shortlogs and descriptions improved. Thanks!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.