Bug 94591

Created attachment 122379 [details] [review]
Use the ConsoleKit database for at_console support

This patch allows dbus to use the consolekit database to lookup the user and determine if at least one session is local. Failing to find the user is local, it will continute to fallback to checking the console auth dir as before.

(In reply to Eric Koegel from comment #0)
> Overall the goal is to have dbus attempt to read the consolekit database to
> determine if a user is local

Is this for <allow ... at_console="true"/> policies?

The D-Bus maintainers consider the dbus-daemon's XML policy language to be the wrong layer to do fine-grained access control, and in particular there are moves to deprecate at_console (Bug #39611). dbus-daemon is primarily a name registry and a message-passing intermediary: the only thing it should be concerned with is whether a bus name is allowed to be owned by specific system users (usually correct) or by anyone, and whether a service accepts messages from specific system users or from everyone.

Finer-grained access-control policies (requiring a locally-logged-in user, limiting access to particular methods or domain-specific resources, etc.) is a job for the specific service in question, either directly or by delegating the decision to a service like polkit (formerly PolicyKit) or Tizen's Cynara. See <http://smcv.pseudorandom.co.uk/2015/why_polkit/> for more on the reasons why polkit exists, and why it's better at this stuff than dbus-daemon.

As a purely practical consideration, dbus-daemon's XML policy language is extremely easy to get wrong, because every snippet can affect all system services, leading to vulnerabilities like CVE-2014-8148 and CVE-2014-8156. If your service uses polkit to decide whether to take privileged actions, it will be immune to those issues.

Comment on attachment 122378 [details] [review]
Add a bus_configuration_file_load helper function

Review of attachment 122378 [details] [review]:
-----------------------------------------------------------------

(Doing a brief review anyway.)

It's a pity the ConsoleKit2 people have chosen to implement something that is so close to the standard .desktop syntax, without actually being that syntax...

::: bus/desktop-file.h
@@ +40,4 @@
>  
>  BusDesktopFile *bus_desktop_file_load (DBusString     *filename,
>  				       DBusError      *error);
> +BusDesktopFile *bus_configuration_file_load (DBusString     *filename,

The dbus-daemon has configuration files that are not in .desktop-style format (notably the XML stuff), so a better name would be bus_desktop_like_file_load(), bus_ini_file_load(), bus_key_file_load() (by analogy with GLib's GKeyFile) or something.

@@ +40,5 @@
>  
>  BusDesktopFile *bus_desktop_file_load (DBusString     *filename,
>  				       DBusError      *error);
> +BusDesktopFile *bus_configuration_file_load (DBusString     *filename,
> +                                             dbus_bool_t     strict,

Boolean arguments are often best avoided. Without looking at the header, does

    bus_configuration_file_load ("myfile.ini", TRUE, &error)

mean "strict=TRUE", or "permissive=TRUE"?

It would be better as an enum, something like

typedef enum
{
  BUS_DESKTOP_FILE_FORMAT_STANDARD,
  BUS_DESKTOP_FILE_FORMAT_LOOSE
} BusDesktopFileFormat;

because then the call is self-documenting.

Comment on attachment 122379 [details] [review]
Use the ConsoleKit database for at_console support

Review of attachment 122379 [details] [review]:
-----------------------------------------------------------------

Again, I don't think we want to merge this much code to support a deprecated feature, but doing a quick review anyway in case it's helpful.

::: cmake/CMakeLists.txt
@@ +450,3 @@
>  else (WIN32)
>  	set (DBUS_CONSOLE_AUTH_DIR "/var/run/console/")
> +	set (DBUS_CONSOLEKIT_DB "/var/run/ConsoleKit/database")

Is this file API, in the sense that the ConsoleKit2 developers promise that its format, contents and semantics will never have incompatible changes?

If this file was a new addition in ConsoleKit2, it might be less confusing to use ConsoleKit2 (with appropriate case adjustments) in the various names, messages and so on.

::: configure.ac
@@ +1595,5 @@
>  AC_SUBST(DBUS_CONSOLE_AUTH_DIR)
>  AC_DEFINE_UNQUOTED(DBUS_CONSOLE_AUTH_DIR, "$DBUS_CONSOLE_AUTH_DIR", [Directory to check for console ownerhip])
>  
> +#### Location of the ConsoleKit database for at_console support
> +if ! test -z "$with_consolekit_db"; then

New conditional things should use AS_IF. The difference usually doesn't matter, but occasionally does, and it's easier to get right by insisting on AS_IF everywhere.

(One of these days I'll get round to fixing the existing if/fi and case/esac.)

Please accept "no" as a value that disables it, so that "--without-consolekit-db" works.

@@ +1602,5 @@
> +   DBUS_CONSOLEKIT_DB=/var/run/ConsoleKit/database
> +fi
> +
> +AC_SUBST(DBUS_CONSOLEKIT_DB)
> +AC_DEFINE_UNQUOTED(DBUS_CONSOLEKIT_DB, "$DBUS_CONSOLEKIT_DB", [Location of the ConsoleKit database for at_console support])

This is underquoted. Rule of thumb: there should be one level of [] per level of (). Search for "underquoted" in Autoconf documentation for more details.

::: dbus/Makefile.am
@@ +117,5 @@
>  	$(NULL)
>  
>  DBUS_UTIL_arch_sources = 			\
> +	$(top_builddir)/bus/desktop-file.c	\
> +	$(top_builddir)/bus/desktop-file.h	\

If .desktop-style parsing is needed in libdbus, it needs to be moved from bus/ into dbus/ and re-namespaced from bus_foo to _dbus_foo. One day there'll probably be some feature that needs that change, but I don't think this is that feature.

(Also, desktop-file.[ch] are in the $(top_srcdir), not the $(top_builddir).)

::: dbus/dbus-sysdeps-util-unix.c
@@ +643,5 @@
> +      return FALSE;
> +    }
> +
> +  /* Create the User group to look into */
> +  if (!_dbus_string_append_printf (&user_group, "User %lu", uid))

DBUS_UID_FORMAT, not %lu

@@ +645,5 @@
> +
> +  /* Create the User group to look into */
> +  if (!_dbus_string_append_printf (&user_group, "User %lu", uid))
> +    {
> +      _DBUS_SET_OOM (error);

The string still needs to be freed here.

@@ +661,5 @@
> +       */
> +      if (dbus_error_is_set (error))
> +        {
> +          dbus_error_free (error);
> +        }

If a function with a DBusError fails, you can assume that the error is set. The only reason it might not be is programmer error (wrong arguments like NULL strings or whatever), and you don't fix programmer error by second-guessing the API, you fix programmer error by changing your source code to not contain that error.

@@ +668,5 @@
> +    {
> +      if (local_str != NULL)
> +        {
> +          is_local = (strcmp (local_str, "true") == 0);
> +          free (local_str);

If it's allocated with dbus_malloc() (I haven't checked, but it probably is) then it needs to be freed with dbus_free(). They are not necessarily compatible with malloc() and free().

(In reply to Simon McVittie from comment #2)
> (In reply to Eric Koegel from comment #0)
> > Overall the goal is to have dbus attempt to read the consolekit database to
> > determine if a user is local
> 
> Is this for <allow ... at_console="true"/> policies?
> 
Correct, this is for the <allow ... at_console="true"/> policies and I didn't realize that this was being deprecated. In that case I agree, don't bother to add more code to it.

Thanks for the quick reply and code review. I'll close this report.