|
Description
Mathieu Bridon
2018-04-25 13:38:06 UTC
Created attachment 139106 [details]
Debug logs
Here are some logs obtained by setting G_MESSAGES_DEBUG=Geoclue.
This is with a build of Git master (e726d8c), and a debug patch added on top from Zeenix.
Looking at the patched (to add debug logs) source code, I see:
guint i = 0;
g_print ("STARTING: %u\n", i++); // STARTING: 0
[... snip ...]
g_print ("STARTING: %u\n", i++); // STARTING: 1
[... snip ...]
g_print ("STARTING: %u\n", i++); // STARTING: 2
[... snip ...]
g_print ("STARTING: %u\n", i++); // STARTING: 3
data = g_slice_new (StartData);
data->client = g_object_ref (client);
data->invocation = g_object_ref (invocation);
data->desktop_id = g_strdup (desktop_id);
data->accuracy_level = gclue_dbus_client_get_requested_accuracy_level (client);
data->accuracy_level = CLAMP (data->accuracy_level,
GCLUE_ACCURACY_LEVEL_COUNTRY,
GCLUE_ACCURACY_LEVEL_EXACT);
/* No agent == No authorization needed */
if (priv->agent_proxy == NULL ||
gclue_config_is_system_component (config, desktop_id) ||
app_perm == GCLUE_APP_PERM_ALLOWED) {
complete_start (data);
return TRUE;
}
g_print ("STARTING: %u\n", i++); // Never got printed
So, hum… I don't have an agent? :-/
From the log your provided: https://paste.gnome.org/p5cf5cjs1/kge2ya/raw it's very likely a race-condition, the client request for location comes *before* gnome-shell gets to register itself as an (app-authorizing) agent. Since only GNOME implements an agent, I think Geoclue is correct in authorizing all apps if no agent is registered. This an extension (or gnome-shell component) that uses geolocation, right? (In reply to Zeeshan Ali from comment #3) > Since only GNOME implements an agent, I think Geoclue is correct in > authorizing all apps if no agent is registered. I think it might be time to implement a really crappy small GTK+ app that can run in the background on systems which aren't GNOME. Because otherwise none of the other desktops will ever implement an agent, and we run into this sort of problems. (In reply to Zeeshan Ali from comment #3) > This an extension (or > gnome-shell component) that uses geolocation, right? No, it's an application that's auto-started in the background. I don't quite know how it would get started before gnome-shell in a session though. Mathieu, any ideas? Is it saved in your session? Do you use gnome-shell under X11 or Wayland? > No, it's an application that's auto-started in the background.
Not even that!
I manually start the app after logging in to my session. With my mouse. GNOME Calendar is in fact the 5th app I start.
Am I somehow too fast? :D
Oops, sorry, I sent the previous comment too early…
> Do you use gnome-shell under X11 or Wayland?
Wayland.
(In reply to Mathieu Bridon from comment #5) > > No, it's an application that's auto-started in the background. > > Not even that! > > I manually start the app after logging in to my session. With my mouse. > GNOME Calendar is in fact the 5th app I start. > > Am I somehow too fast? :D No, you're probably starting it without realising. Check the journal for an autostarted org.gnome.Calendar. It's possible that it gets started by its search provider which you can disable in the Search panel. I think I know what the problem is. And I was looking in the right direction by looking at the service timeout in bug 106249. Geoclue disappears after a couple of seconds not being used in gdm or at the start of gnome-shell. When Mathieu starts Calendar, its prods on the bus, which autostarts geoclue. Calendar then makes its calls before gnome-shell has the chance to register the agent. We need an agent to be there before the call is made, which means that comment 4 is all the more important., We also need geoclue to wait a couple of seconds after startup for an agent to appear, rather than replying straight away. Otherwise agents won't have the chance to register before applications make their calls. Removing the timeout from the service file can be a short-term work-around, but it doesn't close all the ways to abuse those problems. Created attachment 139125 [details] [review] service-client: Refuse access without an agent To avoid races between clients and agents, make sure that an agent is required for each UID that wants its applications to be able to access location services. This will mean that desktop environments (and DIY ones) will need to have an agent running in the background to be able to ask authorisation questions. Comment on attachment 139125 [details] [review] service-client: Refuse access without an agent Review of attachment 139125 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-service-client.c @@ +489,5 @@ > + uid); > + return TRUE; > + } > + > + if (gclue_config_is_system_component (config, desktop_id) || Even if we wanted this change, you need to at least make exception for the system components to access location (they don't need agent). But I don't think this is a good idea. Geoclue2 is mostly (only?) being used by GNOME apps currently and they will break overnight on other DEs and that would look bad on the apps from the user's POV, not the DE in question. If we are going to make this change, we need to inform the major DEs and give them some time before doing this. For now, we want to simply give agent some time (push this call on a timeout for 1-2 seconds). (In reply to Zeeshan Ali from comment #10) > Comment on attachment 139125 [details] [review] [review] > service-client: Refuse access without an agent > > Review of attachment 139125 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: src/gclue-service-client.c > @@ +489,5 @@ > > + uid); > > + return TRUE; > > + } > > + > > + if (gclue_config_is_system_component (config, desktop_id) || > > Even if we wanted this change, you need to at least make exception for the > system components to access location (they don't need agent). They auto-start Geoclue though, which means that an agent would register, which means that an agent should be present when the condition is checked. > But I don't > think this is a good idea. Geoclue2 is mostly (only?) being used by GNOME > apps currently and they will break overnight on other DEs and that would > look bad on the apps from the user's POV, not the DE in question. If we are > going to make this change, we need to inform the major DEs and give them > some time before doing this. It's a security issue. I don't think you should have allowed access without an agent, ever. > For now, we want to simply give agent some time (push this call on a timeout > for 1-2 seconds). If you continue allowing sessions without an agent, this just makes the race condition a little bit harder to replicate, but a slow computer such as one using a rotary hard disk would likely still hit it. (In reply to Bastien Nocera from comment #11) > (In reply to Zeeshan Ali from comment #10) > > Comment on attachment 139125 [details] [review] [review] [review] > > service-client: Refuse access without an agent > > > > Review of attachment 139125 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: src/gclue-service-client.c > > @@ +489,5 @@ > > > + uid); > > > + return TRUE; > > > + } > > > + > > > + if (gclue_config_is_system_component (config, desktop_id) || > > > > Even if we wanted this change, you need to at least make exception for the > > system components to access location (they don't need agent). > > They auto-start Geoclue though, So do apps. > which means that an agent would register, > which means that an agent should be present when the condition is checked. Not sure why you think that but it's not the case. The other part of the condition here takes into account the other known apps that we allow (browsers and such from geoclue.conf) that also need to be excluded the from the agent check. > > But I don't > > think this is a good idea. Geoclue2 is mostly (only?) being used by GNOME > > apps currently and they will break overnight on other DEs and that would > > look bad on the apps from the user's POV, not the DE in question. If we are > > going to make this change, we need to inform the major DEs and give them > > some time before doing this. > > It's a security issue. I don't think you should have allowed access without > an agent, ever. Ok, let's break the world then. :) Could you update the patch to move the system/allowed apps check before the check for agent? (In reply to Zeeshan Ali from comment #12) > Ok, let's break the world then. :) Could you update the patch to move the > system/allowed apps check before the check for agent? I don't think that we should allow anything to pass through unless there's an agent. There are no non-session components in the whitelist, so every one of those should have an agent running in the same session. This patch is just made to plug the security issue. Postponing answers a little until the agent has a chance to register, for example, are follow-up fixes, and refinements. (In reply to Bastien Nocera from comment #13) > (In reply to Zeeshan Ali from comment #12) > > Ok, let's break the world then. :) Could you update the patch to move the > > system/allowed apps check before the check for agent? > > I don't think that we should allow anything to pass through unless there's > an agent. There are no non-session components in the whitelist, so every one > of those should have an agent running in the same session. Actually you are right. When geolocation is disabled (by agent), we don't want anyone accessing geolocation. In which case, the check for system components/allowed-apps should go further down instead. :) I modified the patch a bit. Let me upload.. Created attachment 139127 [details] [review] service-client: Refuse access without an agent To avoid races between clients and agents, make sure that an agent is required for each UID that wants its applications to be able to access location services. This will mean that desktop environments (and DIY ones) will need to have an agent running in the background to be able to ask authorisation questions. (In reply to Zeeshan Ali from comment #14) > (In reply to Bastien Nocera from comment #13) > > (In reply to Zeeshan Ali from comment #12) > > > Ok, let's break the world then. :) Could you update the patch to move the > > > system/allowed apps check before the check for agent? > > > > I don't think that we should allow anything to pass through unless there's > > an agent. There are no non-session components in the whitelist, so every one > > of those should have an agent running in the same session. > > Actually you are right. When geolocation is disabled (by agent), we don't > want anyone accessing geolocation. In which case, the check for system > components/allowed-apps should go further down instead. :) That's not what the patch does though, is it? The only difference I can see between the 2 patches is when to check on the max accuracy. System components and whitelisted apps still go through without querying the agent. (In reply to Bastien Nocera from comment #16) > (In reply to Zeeshan Ali from comment #14) > > (In reply to Bastien Nocera from comment #13) > > > (In reply to Zeeshan Ali from comment #12) > > > > Ok, let's break the world then. :) Could you update the patch to move the > > > > system/allowed apps check before the check for agent? > > > > > > I don't think that we should allow anything to pass through unless there's > > > an agent. There are no non-session components in the whitelist, so every one > > > of those should have an agent running in the same session. > > > > Actually you are right. When geolocation is disabled (by agent), we don't > > want anyone accessing geolocation. In which case, the check for system > > components/allowed-apps should go further down instead. :) > > That's not what the patch does though, is it? The only difference I can see > between the 2 patches is when to check on the max accuracy. No, that max_accuracy is how the agent disables geolocation. Currently Shell only sets it to either EXACT or NONE but theoretically an agent can dictate the accuracy it wants all apps to have. > System > components and whitelisted apps still go through without querying the agent. They do but with my patch if agents disables geolocation (NONE accuracy), they don't get to access location info. I pushed the latest patch. Created attachment 139138 [details] [review] Always build and install the demo agent Most desktops don't have an agent. As a result, now that GeoClue refuses access without an agent, apps won't be able to obtain the location on those desktops. Installing the demo agent by default keeps things working just the same for those desktops: all accesses will continue being granted. However, since GNOME has its own agent, we don't want the demo agent to run on GNOME. Created attachment 139139 [details] [review] Always build and install the demo agent Most desktops don't have an agent. As a result, now that GeoClue refuses access without an agent, apps won't be able to obtain the location on those desktops. Installing the demo agent by default keeps things working just the same for those desktops: all accesses will continue being granted. However, since GNOME has its own agent, we don't want the demo agent to run on GNOME. Comment on attachment 139138 [details] [review] Always build and install the demo agent Review of attachment 139138 [details] [review]: ----------------------------------------------------------------- Rest looks fine. ::: configure.ac @@ -175,5 @@ > - libnotify]) > -else > - AC_DEFINE([BUILD_DEMO_AGENT], [0], [Build demo agent?]) > -fi > -AM_CONDITIONAL([BUILD_DEMO_AGENT], [test "x$enable_demo_agent" = "xyes"]) I think it's best to still keep it configurable but just default to 'yes'. Created attachment 139158 [details] [review] Always build and install the demo agent Most desktops don't have an agent. As a result, now that GeoClue refuses access without an agent, apps won't be able to obtain the location on those desktops. Installing the demo agent by default keeps things working just the same for those desktops: all accesses will continue being granted. However, since GNOME has its own agent, we don't want the demo agent to run on GNOME. Created attachment 139159 [details] [review] Build and install the demo agent by default Most desktops don't have an agent. As a result, now that GeoClue refuses access without an agent, apps won't be able to obtain the location on those desktops. Installing the demo agent by default keeps things working just the same for those desktops: all accesses will continue being granted. However, since GNOME has its own agent, we don't want the demo agent to run on GNOME. Comment on attachment 139159 [details] [review] Build and install the demo agent by default Review of attachment 139159 [details] [review]: ----------------------------------------------------------------- Thanks so much. I think the change to install the demo agent by default and adding it to autostart for non-gnome are two different changes so we need to divide them into two patches. If you don't have time to do it, just let me know and I'll do it on your behalf when merging. Thanks again. Created attachment 139164 [details] [review] Only install the demo agent desktop file when requested Created attachment 139165 [details] [review] Build and install the demo agent by default Most desktops don't have an agent. As a result, now that GeoClue refuses access without an agent, apps won't be able to obtain the location on those desktops. Installing the demo agent, as long as they start it, keeps things working just the same for those desktops: all accesses will continue being granted. Created attachment 139166 [details] [review] Autostart the demo agent by default This is necessary for most desktops to continue working as they were before GeoClue required an agent. However, GNOME already has its own agent, which allows authorizing apps individually, and as such doesn't need the demo agent. > I think the change to install the demo agent by default and adding it to autostart for non-gnome are two different changes
Done.
While I was at it, I also split a third part, which just fixes a tiny issue: in master the desktop file for the demo agent was always installed, even when it hadn't been enabled. (as such it pointed to an executable which didn't exist)
Attachment 139164 [details] pushed as d3e71e9 - Only install the demo agent desktop file when requested Attachment 139165 [details] pushed as 16d0f13 - Build and install the demo agent by default Attachment 139166 [details] pushed as a5e5885 - Autostart the demo agent by default Comment on attachment 139127 [details] [review] service-client: Refuse access without an agent Review of attachment 139127 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-service-client.c @@ +471,1 @@ > return TRUE; Just realised that this is leaking the newly allocated data struct. This will be fixed in the timeout patch though, coming soon. @@ -468,4 @@ > return TRUE; > } > > - if (priv->agent_proxy != NULL) yes, thanks. Created attachment 139264 [details] [review] service-client: Fix warning with newer versions of GLib geoclue/src/gclue-service-client.c: In function ‘gclue_service_client_handle_start’: geoclue/src/gclue-service-client.c:574:22: warning: assignment to ‘GClueServiceClient *’ {aka ‘struct _GClueServiceClient *’} from incompatible pointer type ‘GClueDBusClient *’ {aka ‘struct _GClueDBusClient *’} [-Wincompatible-pointer-types] data->client = g_object_ref (client); ^ Created attachment 139265 [details] [review] service-client: Split off post agent auth check Separate the authorization checks that happen after the agent availability is checked, to make it possible to delay that portion. Created attachment 139266 [details] [review] service-client: Delay "no agent" authorization decision To avoid applications being denied access to location services when gnome-shell hasn't had a chance to register its agent, either because Geoclue got auto-started by the application, or because the shell hasn't finished starting up, delay the authorization check until either an agent appears, or 5 seconds after the application requested the authorization. I've only compile tested the last 3 patches, let me know if you run into any problems. Comment on attachment 139264 [details] [review] service-client: Fix warning with newer versions of GLib Review of attachment 139264 [details] [review]: ----------------------------------------------------------------- ack. If you could instead say "Fix warning against GLib VERSION", I'd appreciate it even more. :) Comment on attachment 139265 [details] [review] service-client: Split off post agent auth check Review of attachment 139265 [details] [review]: ----------------------------------------------------------------- ack Created attachment 139275 [details] [review] service-client: Fix warning with GLib >= 2.56 geoclue/src/gclue-service-client.c: In function ‘gclue_service_client_handle_start’: geoclue/src/gclue-service-client.c:574:22: warning: assignment to ‘GClueServiceClient *’ {aka ‘struct _GClueServiceClient *’} from incompatible pointer type ‘GClueDBusClient *’ {aka ‘struct _GClueDBusClient *’} [-Wincompatible-pointer-types] data->client = g_object_ref (client); ^ See https://bugzilla.gnome.org/show_bug.cgi?id=790697 for the gobject change that triggers this warning. (In reply to Bastien Nocera from comment #33) > Created attachment 139266 [details] [review] [review] > service-client: Delay "no agent" authorization decision > > To avoid applications being denied access to location services when > gnome-shell hasn't had a chance to register its agent, either because > Geoclue got auto-started by the application, or because the shell hasn't > finished starting up, delay the authorization check until either an > agent appears, or 5 seconds after the application requested the > authorization. > #DEFAULT_AGENT_STARTUP_WAIT_SECS 5 I think that's too long. Isn't it even longer (or as long as) the default dbus timeout? I doubt we need more than a second here. > set_pending_auths_timeout() Let's not use boolean to use the same function for two different actions. Let's split this into two different functions. > 584 if (priv->agent_proxy == NULL) { I am not sure we should keep postponing, if agent is never registered. How about we just register a timeout once from _init() or _constructed() and until it triggers, we postpone all requests and once timeout happens, we handle all pending requests and any subsequent request are not queued but denied. Also clients are not supposed to send multiple Start() calls in parallel. Putting all requests in the queue, could easily mean a OOM/DOS attack (especially if the client has as much as 5 seconds). So I think we should only queue one call and simply deny subsequent ones, until the first one has been handled. (In reply to Zeeshan Ali from comment #38) > (In reply to Bastien Nocera from comment #33) > > Created attachment 139266 [details] [review] [review] [review] > > service-client: Delay "no agent" authorization decision > > > > 584 if (priv->agent_proxy == NULL) { > > I am not sure we should keep postponing, if agent is never registered. How > about we just register a timeout once from _init() or _constructed() and > until it triggers, we postpone all requests and once timeout happens, we > handle all pending requests and any subsequent request are not queued but > denied. > > Also clients are not supposed to send multiple Start() calls in parallel. > Putting all requests in the queue, could easily mean a OOM/DOS attack > (especially if the client has as much as 5 seconds). So I think we should > only queue one call and simply deny subsequent ones, until the first one has > been handled. Oh and we shouldn't keep waiting for time out if agent is set in the meantime. Created attachment 139279 [details] [review] service-client: Delay "no agent" authorization decision To avoid applications being denied access to location services when gnome-shell hasn't had a chance to register its agent, either because Geoclue got auto-started by the application, or because the shell hasn't finished starting up, delay the authorization check until either an agent appears, or 5 seconds after the application requested the authorization. Comment on attachment 139279 [details] [review] service-client: Delay "no agent" authorization decision Review of attachment 139279 [details] [review]: ----------------------------------------------------------------- looks good otherwise. ::: src/gclue-service-client.c @@ +50,4 @@ > const char *path; > GDBusConnection *connection; > GClueAgent *agent_proxy; > + gpointer pending_auth_start_data; When there is a specific type, why use gpointer? Warning: You'll need a very compelling argument to convince me about this. :) (In reply to Zeeshan Ali from comment #41) > Comment on attachment 139279 [details] [review] [review] > service-client: Delay "no agent" authorization decision > > Review of attachment 139279 [details] [review] [review]: > ----------------------------------------------------------------- > > looks good otherwise. > > ::: src/gclue-service-client.c > @@ +50,4 @@ > > const char *path; > > GDBusConnection *connection; > > GClueAgent *agent_proxy; > > + gpointer pending_auth_start_data; > > When there is a specific type, why use gpointer? Warning: You'll need a very > compelling argument to convince me about this. :) Because it's that, or moving all the StartData related code above this. Attachment 139265 [details] pushed as c26ecf7 - service-client: Split off post agent auth check Attachment 139275 [details] pushed as 9d42708 - service-client: Fix warning with GLib >= 2.56 (In reply to Bastien Nocera from comment #42) > (In reply to Zeeshan Ali from comment #41) > > Comment on attachment 139279 [details] [review] [review] [review] > > service-client: Delay "no agent" authorization decision > > > > Review of attachment 139279 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > looks good otherwise. > > > > ::: src/gclue-service-client.c > > @@ +50,4 @@ > > > const char *path; > > > GDBusConnection *connection; > > > GClueAgent *agent_proxy; > > > + gpointer pending_auth_start_data; > > > > When there is a specific type, why use gpointer? Warning: You'll need a very > > compelling argument to convince me about this. :) > > Because it's that, or moving all the StartData related code above this. I was afraid you are going to say that. That is not at all a good argument for ditching types, it's one example of how C sucks big time in some ways. Created attachment 139285 [details] [review] service-client: Split off post agent auth check Separate the authorization checks that happen after the agent availability is checked, to make it possible to delay that portion. Created attachment 139286 [details] [review] service-client: Fix warning with GLib >= 2.56 geoclue/src/gclue-service-client.c: In function ‘gclue_service_client_handle_start’: geoclue/src/gclue-service-client.c:574:22: warning: assignment to ‘GClueServiceClient *’ {aka ‘struct _GClueServiceClient *’} from incompatible pointer type ‘GClueDBusClient *’ {aka ‘struct _GClueDBusClient *’} [-Wincompatible-pointer-types] data->client = g_object_ref (client); ^ See https://bugzilla.gnome.org/show_bug.cgi?id=790697 for the gobject change that triggers this warning. Created attachment 139287 [details] [review] service-client: Delay "no agent" authorization decision To avoid applications being denied access to location services when gnome-shell hasn't had a chance to register its agent, either because Geoclue got auto-started by the application, or because the shell hasn't finished starting up, delay the authorization check until either an agent appears, or 5 seconds after the application requested the authorization. All patches pushed. I'm pretty sure I mentioned that I didn't test this. And neither did anybody else :/ #0 0x000055c14b574862 in handle_post_agent_check_auth (data=0x0) at gclue-service-client.c:412 #1 0x000055c14b574c10 in handle_pending_auth (user_data=<optimized out>) at gclue-service-client.c:474 #2 0x00007f48597a2a79 in g_object_new_internal () at /lib64/libgobject-2.0.so.0 #3 0x00007f48597a464e in g_object_new_valist () at /lib64/libgobject-2.0.so.0 #4 0x00007f4859a42cbe in g_initable_new_valist () at /lib64/libgio-2.0.so.0 #5 0x00007f4859a42d7d in g_initable_new () at /lib64/libgio-2.0.so.0 #6 0x000055c14b5759d8 in gclue_service_client_new (info=info@entry=0x7f483c0054d0, path=path@entry=0x55c14d6bc260 "/org/freedesktop/GeoClue2/Client/1", connection=0x55c14d67d000, agent_proxy=agent_proxy@entry=0x55c14d684550, error=error@entry=0x7ffc4f6e6bc0) at gclue-service-client.c:942 #7 0x000055c14b573b6d in complete_get_client (data=0x7f483c001960) at gclue-service-manager.c:147 #8 0x00007f48594c3291 in g_timeout_dispatch () at /lib64/libglib-2.0.so.0 #9 0x00007f48594c27cd in g_main_context_dispatch () at /lib64/libglib-2.0.so.0 #10 0x00007f48594c2b98 in g_main_context_iterate.isra () at /lib64/libglib-2.0.so.0 #11 0x00007f48594c2ec2 in g_main_loop_run () at /lib64/libglib-2.0.so.0 #12 0x000055c14b57258e in main (argc=<optimized out>, argv=<optimized out>) at gclue-main.c:192 Created attachment 139301 [details] [review] service-client: Fix crash on startup Make sure to not dereference a NULL StartData when geoclue gets started. (In reply to Bastien Nocera from comment #50) > Created attachment 139301 [details] [review] [review] > service-client: Fix crash on startup > > Make sure to not dereference a NULL StartData when geoclue gets started. This patch is also untested. I'm testing it out now. (In reply to Bastien Nocera from comment #49) > I'm pretty sure I mentioned that I didn't test this. And neither did anybody > else :/ Oh, i misread "compile tested" as "compiled and tested". (In reply to Bastien Nocera from comment #51) > (In reply to Bastien Nocera from comment #50) > > Created attachment 139301 [details] [review] [review] [review] > > service-client: Fix crash on startup > > > > Make sure to not dereference a NULL StartData when geoclue gets started. > > This patch is also untested. I'm testing it out now. Seems to work (doesn't crash on startup). I ran into fallout from bug 97776 though. Attachment 139301 [details] pushed as f7b0c52 - service-client: Fix crash on startup
Tested myself too before pushing.
|
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.