GNOME Bugzilla – Bug 721887
Implement geoclue agent
Last modified: 2016-02-22 18:32:03 UTC
Geoclue should not simply share user's location info to every other application but rather ask the user if they want to share it. Every desktop will have to implement an agent that takes care of that for geolcue. The reason geoclue can't do this on its own is that it runs on the system bus as some system user. So an agent is needed that runs on the session as logged-in user and handles this authentication for geoclue. This permission will have to be per-application and persistent. We'll have to maintain a white-list for known apps that either only access user location of their own request (e.g gnome-maps, control-center) or have their own mechanism for asking the user (e.g browsers).
(Just speculating...) Is this the right occasion to implement a general framework for asking the user permission? Like PolicyKit, but in the session. With manifests for apps that want the permission always, a setting storage for apps that were previously approved, and a simple API to build the g-c-c view. After all, we will need this for a number of semi-privileged actions (accessing the filesystem, the camera, the microphone, etc.) once we have sandboxed applications. On a different question, how do we identify the application making the request? Under our current security model, every application is potentially a vehicle of untrusted code (think of ptrace and LD_PRELOAD), so the whitelist could be trivially exploited. Then again, if the app is collaborative, it will need to ask the user at some point, and this takes the responsibility off it, so it's still useful.
This is obviously necessary if we are to have a useful GPS/geolocation capability. I share Giovanni's concerns though, and would like to make sure that we are clear on the overall approach we are taking to application sandboxing and permissions before taking this on. The design page for this is currently empty: https://wiki.gnome.org/Design/OS/Location
(In reply to comment #1) > (Just speculating...) > > Is this the right occasion to implement a general framework for asking the user > permission? > Like PolicyKit, but in the session. With manifests for apps that want the > permission always, a setting storage for apps that were previously approved, > and a simple API to build the g-c-c view. > > After all, we will need this for a number of semi-privileged actions (accessing > the filesystem, the camera, the microphone, etc.) once we have sandboxed > applications. In the geolocation case, I'd expect Geoclue to keep track of the applications. In the session, for microphones, Geoclue's place would be taken by pulseaudio and would have a completely different API for this. > On a different question, how do we identify the application making the request? > Under our current security model, every application is potentially a vehicle of > untrusted code (think of ptrace and LD_PRELOAD), so the whitelist could be > trivially exploited. This security isn't foolproof, just as one could LD_PRELOAD on top of a polkit client, but it provides the UI for what we hope to achieve in the future. > Then again, if the app is collaborative, it will need to ask the user at some > point, and this takes the responsibility off it, so it's still useful. (In reply to comment #2) > This is obviously necessary if we are to have a useful GPS/geolocation > capability. I share Giovanni's concerns though, and would like to make sure > that we are clear on the overall approach we are taking to application > sandboxing and permissions before taking this on. I don't think we need to wait for application sandboxing to implement this. Not anymore than we needed to wait for it for PolicyKit. > The design page for this is currently empty: > > https://wiki.gnome.org/Design/OS/Location Best get started on it then :)
(In reply to comment #3) > (In reply to comment #1) > > (Just speculating...) > > > > Is this the right occasion to implement a general framework for asking the user > > permission? > > Like PolicyKit, but in the session. With manifests for apps that want the > > permission always, a setting storage for apps that were previously approved, > > and a simple API to build the g-c-c view. > > > > After all, we will need this for a number of semi-privileged actions (accessing > > the filesystem, the camera, the microphone, etc.) once we have sandboxed > > applications. > > In the geolocation case, I'd expect Geoclue to keep track of the applications. > In the session, for microphones, Geoclue's place would be taken by pulseaudio > and would have a completely different API for this. > > > On a different question, how do we identify the application making the request? > > Under our current security model, every application is potentially a vehicle of > > untrusted code (think of ptrace and LD_PRELOAD), so the whitelist could be > > trivially exploited. > > This security isn't foolproof, just as one could LD_PRELOAD on top of a polkit > client, but it provides the UI for what we hope to achieve in the future. This non-foolproof identification is currently based on the commandline of the apps FWIW. While implementing the agent, I'm puzzed with another question: How does geoclue identify/authenticate agents themselves? Currenty the "solution" I have in geoclue is hardcoded whitelist of commandlines of agents. Would this be an adequate enough for now if this whitelist exists in a config file under /etc?
(In reply to comment #4) <snip> > While implementing the agent, I'm puzzed with another question: How does > geoclue identify/authenticate agents themselves? Currenty the "solution" I have > in geoclue is hardcoded whitelist of commandlines of agents. Would this be an > adequate enough for now if this whitelist exists in a config file under /etc? In the future, you'd just need to check the dbus sender, and something else would make sure that the application owning that name is indeed what it says it is, but that's good enough for now. As you mentioned, it doesn't need to be hard-coded either, and a file in etc sounds like a good plan in the short-term.
Created attachment 266006 [details] [review] Add a dummy Geoclue agent Add an app authentication agent for Geoclue. This patch adds a dummy agent that always authenticates all apps without asking the user.
Review of attachment 266006 [details] [review]: Comments on the implementation: 1) why do you remove the object path when Geoclue appears and disappears? Wouldn't it be simpler to always have it? 2) what's the API on geoclue side, is there a RemoveAgent() call? I think there should be one to disable() the component (which would happen when locking the screen). Alternatively, the agent would be always registered but would reply negatively when disabled. 3) ideally, the proxy creation would be for GeoclueManager would be async, or no proxy would be needed at all, and you just use g_dbus_connection_call() Now on the API: 1) I guess bus_name is the system bus name of the app that's requesting access? That's enough to retrieve the PID (and from that the UID) but wouldn't it be simpler to pass the PID from the start? Or maybe something similar to a PolicyKit subject. 2) What's title, and who fills it? 3) On gnome-shell side, going through the PID to retrieve the app is a bit of a pain (apps group windows, not processes, to handle eg. libreoffice), and most important would not work when no window is visible. Can we get a desktop file id/gapplication id or something? I understand it would be client generated and thus forgeable, but we can cross-check it with the other data
Created attachment 266023 [details] [review] Add a dummy Geoclue agent Add an app authentication agent for Geoclue. This patch adds a dummy agent that always authenticates all apps without asking the user.
Created attachment 266024 [details] [review] geoclueAgent: Ask user to authenticate the app This is really not a complete patch. We only succeed to present the notification the first time and even that doesn't exactly popup. Could use some help from a gnome-shell hacker on this.
Comment on attachment 266023 [details] [review] Add a dummy Geoclue agent Sorry for duplicates, git-bz somehow messed-up.
(In reply to comment #7) > Review of attachment 266006 [details] [review]: > > Comments on the implementation: > 1) why do you remove the object path when Geoclue appears and disappears? > Wouldn't it be simpler to always have it? I don't think its good idea to expose API if its not going to be used. No strong feelings though so I can change that if you feel strongly about it. > 2) what's the API on geoclue side, is there a RemoveAgent() call? Currently there is no API to remove it. > I think there should be one to disable() the component (which would happen when > locking the screen). > Alternatively, the agent would be always registered but would reply negatively > when disabled. Doesn't removing the object/unexporting it achieve the same already? > 3) ideally, the proxy creation would be for GeoclueManager would be async, or > no proxy would be needed at all, and you just use g_dbus_connection_call() Sure, although I see that many proxies in existing code are created sync so I thought thats the usual trend. > Now on the API: > 1) I guess bus_name is the system bus name of the app that's requesting access? > That's enough to retrieve the PID (and from that the UID) but wouldn't it be > simpler to pass the PID from the start? Or maybe something similar to a > PolicyKit subject. Sure but this API was based on my assumption that agent will keep the list of authorized apps rather than geoclue. In that case you are right and it would actually be app's commandline that we'll need to grab. However I think Bastien's idea of geoclue itself maintaining this list of authorized apps is the way to go, in which case I wonder if we need to pass bus_name or pid to agent at all. > 2) What's title, and who fills it? Its unfortunately provided currently by the app itself. I'm open to ideas on how we can get a human readable name for apps to present to user. > 3) On gnome-shell side, going through the PID to retrieve the app is a bit of a > pain (apps group windows, not processes, to handle eg. libreoffice), and most > important would not work when no window is visible. Can we get a desktop file > id/gapplication id or something? > > I understand it would be client generated and thus forgeable, but we can > cross-check it with the other data Sure that sounds good but how would it work? I mean how do we get desktop file entry given a bus_name or PID?
(In reply to comment #11) > (In reply to comment #7) > > Review of attachment 266006 [details] [review] [details]: > > Now on the API: > > 1) I guess bus_name is the system bus name of the app that's requesting access? > > That's enough to retrieve the PID (and from that the UID) but wouldn't it be > > simpler to pass the PID from the start? Or maybe something similar to a > > PolicyKit subject. > > Sure but this API was based on my assumption that agent will keep the list of > authorized apps rather than geoclue. In that case you are right and it would > actually be app's commandline that we'll need to grab. However I think > Bastien's idea of geoclue itself maintaining this list of authorized apps is > the way to go, in which case I wonder if we need to pass bus_name or pid to > agent at all. On second thought, I'm not sure if Bastien actually meant that geoclue itself should maintain list of authorized apps here: > In the geolocation case, I'd expect Geoclue to keep track of the applications. Since the app authorization is per-user, I think it belongs in the agent. In which case I think Geoclue should send the commandline of the app to agent rather than its bus id.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #7) > > > Review of attachment 266006 [details] [review] [details] [details]: > > > Now on the API: > > > 1) I guess bus_name is the system bus name of the app that's requesting access? > > > That's enough to retrieve the PID (and from that the UID) but wouldn't it be > > > simpler to pass the PID from the start? Or maybe something similar to a > > > PolicyKit subject. > > > > Sure but this API was based on my assumption that agent will keep the list of > > authorized apps rather than geoclue. In that case you are right and it would > > actually be app's commandline that we'll need to grab. However I think > > Bastien's idea of geoclue itself maintaining this list of authorized apps is > > the way to go, in which case I wonder if we need to pass bus_name or pid to > > agent at all. > > On second thought, I'm not sure if Bastien actually meant that geoclue itself > should maintain list of authorized apps here: > > > In the geolocation case, I'd expect Geoclue to keep track of the applications. > > Since the app authorization is per-user, I think it belongs in the agent. In > which case I think Geoclue should send the commandline of the app to agent > rather than its bus id. OK, given a lot more thought I realized this is not a good idea: agents will run as same user as the apps so wherever agent stores the list of permitted apps, the apps can access it too. So better if geoclue keeps its list. Also we don't want geoclue to always ask the agent for each call made by app. I had a chat with Lennart about commandline and learnt that /proc/$PID/cmdline is very easily overwritable by apps so we better use /proc/$PID/exe. I'll make the appropriate changes to geoclue. About the API, I think title coming from app is fine as long as geoclue ensures its uniqueness for each user. Do we need the bus_name for anything? Does geoclue need to pass any other info to agents?
(In reply to comment #13) > I had a chat with Lennart about commandline and learnt that /proc/$PID/cmdline > is very easily overwritable by apps so we better use /proc/$PID/exe. I'll make > the appropriate changes to geoclue. Can't apps also overwrite this with prctl(PR_SET_NAME, "foo"); ?
(In reply to comment #14) > (In reply to comment #13) > > I had a chat with Lennart about commandline and learnt that /proc/$PID/cmdline > > is very easily overwritable by apps so we better use /proc/$PID/exe. I'll make > > the appropriate changes to geoclue. > > Can't apps also overwrite this with prctl(PR_SET_NAME, "foo"); ? I don't think so, the /proc/$PID/exe is a symlink to the binary file but I'm not sure.
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > I had a chat with Lennart about commandline and learnt that /proc/$PID/cmdline > > > is very easily overwritable by apps so we better use /proc/$PID/exe. I'll make > > > the appropriate changes to geoclue. > > > > Can't apps also overwrite this with prctl(PR_SET_NAME, "foo"); ? > > I don't think so, the /proc/$PID/exe is a symlink to the binary file but I'm > not sure. To verify I wrote this code: ------------------- #include <glib.h> #include <sys/prctl.h> #include <errno.h> int main (int argc, char **argv) { GMainLoop *main_loop; main_loop = g_main_loop_new (NULL, 0); if (prctl(PR_SET_NAME, "foo") != 0) g_error ("Failed to change name: %s", strerror (errno)); strcpy (argv[0], "foo"); g_main_loop_run (main_loop); return 0; } -------------------- While the prctl is successful, it seems to affect neither /proc/PID/cmdline nor /proc/PID/exe. OTOH the strcpy does overwrite /proc/PID/cmdline but not /proc/PID/exe.
Review of attachment 266024 [details] [review]: ::: js/ui/components/geoclueAgent.js @@ +34,3 @@ + + this._notify = new MessageTray.Source(_("Geolocation"), 'dialog-question-symbolic'); + this._notify.policy = new MessageTray.NotificationApplicationPolicy('geoclue'); Do we have a geoclue.desktop? @@ +55,3 @@ log (title + "(" + bus_name + ") authorized"); + + var banner = _("Allow %s to access your location information?").format (title); I still believe that getting a desktop file ID from the app, and retrieving the application name from that would be better. For once, we would also get the application icon. @@ +57,3 @@ + var banner = _("Allow %s to access your location information?").format (title); + var notification = new MessageTray.Notification(this._notify, _("Geolocation"), banner); + notification.setUrgency(MessageTray.Urgency.HIGH); I guess this wants to be CRITICAL (which makes it like a modal dialog). HIGH just reorders it first. @@ +58,3 @@ + var notification = new MessageTray.Notification(this._notify, _("Geolocation"), banner); + notification.setUrgency(MessageTray.Urgency.HIGH); + notification.setTransient(); .setTransient() without parameters is equivalent to .setTransient(false) (not very useful) Anyway, this notification should not be transient. @@ +67,3 @@ + invocation.return_value(new GLib.Variant('(b)', [false])); + }); + this._notify.pushNotification(notification); You want to .notify(), not pushNotification(), otherwise the banner is skipped.
Review of attachment 266024 [details] [review]: Thanks for the input. ::: js/ui/components/geoclueAgent.js @@ +34,3 @@ + + this._notify = new MessageTray.Source(_("Geolocation"), 'dialog-question-symbolic'); + this._notify.policy = new MessageTray.NotificationApplicationPolicy('geoclue'); Nope, I wondered if I should be using that policy class. @@ +55,3 @@ log (title + "(" + bus_name + ") authorized"); + + var banner = _("Allow %s to access your location information?").format (title); Yeah, I can change that. Sorry for my ignorance but do we have any API to get that info given the ID?
(In reply to comment #18) > Review of attachment 266024 [details] [review]: > > Thanks for the input. > > ::: js/ui/components/geoclueAgent.js > @@ +34,3 @@ > + > + this._notify = new MessageTray.Source(_("Geolocation"), > 'dialog-question-symbolic'); > + this._notify.policy = new > MessageTray.NotificationApplicationPolicy('geoclue'); > > Nope, I wondered if I should be using that policy class. Well, NotificationApplicationPolicy() allows the user to configure notifications from the control-center, but must be associated with an app or pseudo apps. For other system notifications we usually pick the related control center panel, or just ship a desktop file. Otherwise, the options are NotificationGenericPolicy(), which applies the global settings from the panel, or NotificationPolicy() which is hardcoded special cases. > @@ +55,3 @@ > log (title + "(" + bus_name + ") authorized"); > + > + var banner = _("Allow %s to access your location information?").format > (title); > > Yeah, I can change that. Sorry for my ignorance but do we have any API to get > that info given the ID? let appSystem = Shell.AppSystem.get_default(); let app = appSystem.lookup_app(id); let name = app.get_name();
Just rolled out a new geoclue release for 3.11.4 and uploaded new docs: http://www.freedesktop.org/software/geoclue/docs/ http://www.freedesktop.org/software/geoclue/docs/gdbus-org.freedesktop.GeoClue2.Agent.html As you can see, I've removed 'bus_name' argument and replaced optional 'title' with mandatory 'desktop_id' argument.
Created attachment 266414 [details] [review] Add a dummy Geoclue agent Add an app authentication agent for Geoclue. This patch adds a dummy agent that always authenticates all apps without asking the user.
Created attachment 266415 [details] [review] geoclueAgent: Ask user to authenticate the app This is really not a complete patch. We only succeed to present the notification the first time.
Created attachment 266577 [details] [review] Add a dummy Geoclue agent New version: Adapt to new accuracy level params in geoclue agent API.
Created attachment 266578 [details] [review] geoclueAgent: Ask user to authenticate the app This is as basic as it gets. We simply ask the user and user either says 'yes' or 'no'. We do also get the requested accuracy level from user and we have the means to dictate what accuracy level should user be actually allowed but we currently don't make use of these abilities. Unlike the previous versions of this patch, notifications now work reliably.
Why is it a notification instead of a system modal dialogue like other authorisation requests?
(In reply to comment #25) > Why is it a notification instead of a system modal dialogue like other > authorisation requests? Because I'm a noob when it comes to shell source code and nobody pointed this out until now. :) I'll modify the patch..
Created attachment 267009 [details] [review] Add a dummy Geoclue agent Add an app authentication agent for Geoclue. This patch adds a dummy agent that always authenticates all apps without asking the user.
Created attachment 267010 [details] [review] geoclueAgent: Ask user to authenticate the app This is as basic as it gets. We simply ask the user and user either says 'yes' or 'no'. We do also get the requested accuracy level from user and we have the means to dictate what accuracy level should user be actually allowed but we currently don't make use of these abilities.
We (me, Jasper and Bastien) had a long discussion about this on IRC today. The conclusion was that until we have some real security in place (app sandboxing tech), we: * Put this up as an exension on extensions.gnome.org and make it clear in the descritpion that it doesn't provide any real security. * modify geoclue to only use an agent if one is available. * Still fix bug#709372, hopefully in time for 3.12.
I'm reopening this, as it *will* be needed, so the bug might as well stay opened.
This is as fixed by bug#762119 as it possibly can be.