[PATCH] Hal privilige seperation

[David Zeuthen]

Hey Sjoerd!

On Fri, 2006-01-20 at 12:08 +0100, Sjoerd Simons wrote:
> Hi,
> 
>   As most people probably know by now, various people don't really like that
>   hal running as root. We'd much rather see only a small process running as
>   root and the main hal process running unpriviledged. Which is exactly what
>   this patch does :) 
> 
>   How does it work? Just before drops it's root privs. a small program is
>   startup which will remain running as root and does the real execution of the
>   addons/probes/callouts on hals behalf. Communication between hald
>   and hald-runner is done via a p2p dbus connection. Resulting in a process
>   tree like this:
> 
>     hal       /usr/sbin/hald
>     root      \_ /usr/lib/hal/hald-runner
>     root          \_ /usr/lib/hal/hald-addon-acpi
>     root          \_ /usr/lib/hal/hald-addon-storage
>     root          \_ /usr/lib/hal/hald-addon-storage
> 
>   The patch consists out of two parts. First the implementation of hald-runner,
>   which is about 700 lines of code. And then a part transforming the hald code
>   from the current spawning code in utils to an interface that can talk to the
>   runner.

This is very nice. I looked through the code and it looks pretty good.
I've also installed a build with this and everything works well on
Fedora including use of g-p-m, gnome-mount and g-v-m. 

So.. I've committed your patch with the following changes to
hald/hald-runner.c

> +static void
> +handle_connection(DBusServer *server,
> +                  DBusConnection *new_connection,
> +                  void *data) {
> +
> +  if (runner_connection == NULL) {
> +    runner_connection = new_connection;
> +    dbus_connection_ref (new_connection);
> +    dbus_connection_setup_with_g_main (new_connection, NULL);
> +    dbus_server_unref(server);
> +  }
> +}

I had to remove the dbus_server_unref(server) otherwise D-BUS would
assert. Anyone else seen this?

> +  if (runner_location == NULL) {
> +    runner_location = g_strdup_printf("%s/hald-runner", PACKAGE_LIBEXEC_DIR);
> +  } else {
> +    runner_location = g_strdup(runner_location);
> +  }

I've added a HAL_INFO statement with the runner location

> +  if (!g_spawn_async("/", argv, env, G_SPAWN_DO_NOT_REAP_CHILD,
> +        NULL, NULL, &pid, &error)) {
> +    goto error;
> +  }

Changed "/" to NULL as we may want to pass ../hald-runner/hald-runner
using --with-runner. Also adapted run-hald.sh, debug-hald.sh and
valgrind-hald.sh to pass --with-runner=../hald-runner/hald-runner.
Without this fix it doesn't work. Also adapted to print out error
message and free the error if g_spawn_sync fails.

There is only one thing I'm concerned about. We pass the socket to the
server in the environment variable HALD_RUNNER_DBUS_ADDRESS - some
people say that some Unices allow unprivileged users to peek at the
environment for any process. Any chance you could cook up a patch to
pass this on stdin instead? Also we probably only want to accept
connections from uid 0 as an extra check just in case someone guesses
the address..

>   For debian people who want to test this, i've upload a hal 0.5.6 package to
>   experimental with this patch. It's been running on my personal machines fine
>   for a few days (i.e. vanilla hal with retain privs and patch hal show the
>   same devices/device informations).
> 
>   Obviously i don't want to maintain this as a specific patch for Ubuntu and
>   Debian, so please let me know what issues you see with it, if any.

It's committed now and it appears to work great! Thank you so much for
this, it's highly appreciated! I've also added you to the AUTHORS file
as this is a significant contribution! 

Great work, thanks again! Rock on!

Cheers,
David