GNOME Bugzilla – Bug 678348
Creates a potentially insecure non-randomized /tmp/at-spi2 directory to store sockets
Last modified: 2012-06-25 22:25:03 UTC
bridge.c (register_application) has quite some room for improvement. jordi@aigua:~$ ls -ld /tmp/at-spi2 drwxrwxrwt 2 Debian-gdm Debian-gdm 4096 jun 18 19:49 /tmp/at-spi2 1) the first program to use the bridge owns the directory. After that, lots of fun can happen, this being /tmp. Second: /* could this be better, we accept some amount of race in getting the temp name*/ /* make sure the directory exists */ mkdir ("/tmp/at-spi2/", S_IRWXU|S_IRWXG|S_IRWXO|S_ISVTX); chmod ("/tmp/at-spi2/", S_IRWXU|S_IRWXG|S_IRWXO|S_ISVTX); app->app_bus_addr = g_malloc(max_addr_length * sizeof(char)); #ifndef DISABLE_P2P sprintf (app->app_bus_addr, "unix:path=/tmp/at-spi2/socket-%d-%d", getpid(), rand()); #else app->app_bus_addr [0] = '\0'; #endif This is wrong in many ways: 2) This should be using abstract sockets, or randomized directory names. 3) This should not be hardcoding /tmp. 4) It's not checking the return of chmod or mkdir.
There's a tentative patch for this in http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=678026
(In reply to comment #1) > There's a tentative patch for this in > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=678026 You should put the patch here, but a couple of problems: - you're still hard-coding /tmp, that's not good - g_dir_make_tmp() is a better option than mkdtemp() - Fix max_addr_length to use an allocated string (eg. g_strdup_printf instead of sprintf) (not even snprintf!)
Shouldn't this use XDG_RUNTIME_DIR ?
Created attachment 216971 [details] [review] Proposed patch. Yeah, XDG_RUNTIME_DIR seems like the right directory to use, at least in theory. I'm thinking of committing this patch; posting it here in case anyone wants to review it. I'm wondering if this will break accessibility of administrative applications for any of the distros, the main question being whether there are still administrative apps that run their UI as root and, if so, if XDG_RUNTIME_DIR changes when they are run. It seems not to affect YaST based on my testing, but I don't know about anything else off-hand.
Review of attachment 216971 [details] [review]: ::: atk-adaptor/adaptors/application-adaptor.c @@ +110,1 @@ const char *retval = (g_getenv ("AT_SPI_CLIENT") ? Just a coding style, but I would have declared retval at the beginning of the block, and done: retval = g_getenv ("AT_SPI_CLIENT"); if (!retval) retval = spi_global_app_data->app_bus_addr; if (!retval) retval = ""; ::: atk-adaptor/bridge.c @@ +304,2 @@ #ifndef DISABLE_P2P + env = getenv ("XDG_RUNTIME_DIR"); Use g_get_user_runtime_dir(). @@ +304,3 @@ #ifndef DISABLE_P2P + env = getenv ("XDG_RUNTIME_DIR"); + if (env) env will never be NULL. @@ +309,3 @@ + mkdir (dir, S_IRWXU); + app->app_bus_addr = g_strdup_printf ("unix:path=%s/socket-%d", dir, + getpid ()); Can't we use randomised paths now? g_mkdtemp() would do that. dir = g_build_filename (g_get_user_runtime_dir(), "at-spi2-XXXXXX", NULL); dir = g_mkdtemp (dir); app->app_bus_addr = g_strdup_printf ("unix:path=%s", dir); g_free (dir);
(In reply to comment #5) > Can't we use randomised paths now? > g_mkdtemp() would do that. I wonder if there is any reason to even have the directories, if we do that, or if it makes as much sense to place the files directly in $XDG_RUNTIME_DIR, since otherwise we have many applications, each with their own at-spi2-xxxxxx directories that each contain one socket.
Review of attachment 216971 [details] [review]: ::: atk-adaptor/bridge.c @@ +309,3 @@ + mkdir (dir, S_IRWXU); + app->app_bus_addr = g_strdup_printf ("unix:path=%s/socket-%d", dir, + getpid ()); Yes, I agree that using at-spi2-XXXXXX and g_mkdtemp would be best. Just don't copy the memory leak in his code snipplet... And the socket could just have a fixed name, since the runtime dir is already per-user (the default is /run/user/<username>)
I also didn't see any place where the socket was unlinked IIRC. Though if you choose to use a fixed name in g_get_user_runtime_dir() it matters less.
Would using at-spi2/at-spi2-XXXXXX for the directory name make more sense? Or not? I'm not really sure what problem we're trying to solve here. It would mean not cluttering $XDG_RUNTIME_DIR with a bunch of at-spi2-* directories, though maybe that doesn't really matter much... As far as unlinking the socket goes, my patch adds an unlink into deregister_application, but that requires that something call atk_bridge_adaptor_cleanup, which gtk+ currently does not do. I'm not sure we've finalized that API, though, so it might make sense to convert it to creating a GObject, then patch gtk+, etc. to ensure that it gets dereferenced.
Committed the patch with some modifications: e4f3ee.