Sanitize the bus address argument
This is a follow-up to #43 (comment 1337067)
After the changes in 412bdee0, @sbacher reports that Ubuntu's GTK tests fail with this:
dbus-daemon[30592]: [session uid=2001 pid=30592] Activating service name='org.a11y.Bus' requested by ':1.0' (uid=2001 pid=31979 comm="/build/gtk+3.0-nBmMO7/gtk+3.0-3.24.30/debian/build" label="unconfined")
dbus-daemon[30592]: [session uid=2001 pid=30592] Successfully activated service 'org.a11y.Bus'
dbus-daemon[32002]: Failed to start message bus: In D-Bus address, character '+' should have been escaped
As far as I can tell, the offending code is where the socket address gets computed:
xdg_runtime_dir = g_get_user_runtime_dir ();
if (xdg_runtime_dir)
{
const gchar *display = g_getenv ("DISPLAY");
gchar *at_spi_dir = g_strconcat (xdg_runtime_dir, "/at-spi", NULL);
gchar *p;
mkdir (at_spi_dir, 0700);
app->socket_name = g_strconcat (at_spi_dir, "/bus", display, NULL);
g_free (at_spi_dir);
p = strchr (app->socket_name, ':');
if (p)
*p = '_';
if (strlen (app->socket_name) >= 100)
{
g_free (app->socket_name);
app->socket_name = NULL;
}
}
Some comments:
-
For dbus-daemon, the code later concatenates
--address=unix:path=
andapp->socket_name
. I think thesocket_name
needs to be escaped first withdbus_address_escape_value()
, and later freed withdbus_free()
. The docs for that function say, "Escapes the given string as a value in a key=value pair for a D-Bus address", so I guess that's the thing to do here for theunix:path
key. -
I think that escaping needs to be done only for dbus-daemon in
ensure_a11y_bus_daemon()
, and not for dbus-broker. For the latter,ensure_a11y_bus_broker()
constructs theapp->a11y_bus_address
from theaddr.sun_path
which came from theapp->socket_name
. -
In the code above, the length of
app->socket_name
is tested against 100 because it gets copied to asockaddr_un.sun_path
. It should compare against the actual size of that field and leave room for the\0
at the beginning if it's an abstract socket. See the code fromdbus/dbus/dbus-sysdeps-unix.c
:
addr.sun_family = AF_UNIX;
path_len = strlen (path);
if (abstract)
{
#ifdef __linux__
addr.sun_path[0] = '\0'; /* this is what says "use abstract" */
path_len++; /* Account for the extra nul byte added to the start of sun_path */
if (path_len > _DBUS_MAX_SUN_PATH_LENGTH)
{
dbus_set_error (error, DBUS_ERROR_BAD_ADDRESS,
"Abstract socket name too long\n");
_dbus_close (fd, NULL);
return -1;
}
strncpy (&addr.sun_path[1], path, sizeof (addr.sun_path) - 2);
/* _dbus_verbose_bytes (addr.sun_path, sizeof (addr.sun_path)); */
#else /* !__linux__ */
dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED,
"Operating system does not support abstract socket namespace\n");
_dbus_close (fd, NULL);
return -1;
#endif /* !__linux__ */
}
else
{
if (path_len > _DBUS_MAX_SUN_PATH_LENGTH)
{
dbus_set_error (error, DBUS_ERROR_BAD_ADDRESS,
"Socket name too long\n");
_dbus_close (fd, NULL);
return -1;
}
strncpy (addr.sun_path, path, sizeof (addr.sun_path) - 1);
}
I think a test for this could create a temporary directory with funny characters in its name, set XDG_RUNTIME_DIR
to that, and try to run the test suite.
(Sorry for quoting the whole incantation, but otherwise I'll forget where a "correct" implementation is...)