Bug 42531

Comment on attachment 53081 [details] [review]
Add android support

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

I cannot review the Android-specific parts. But there's a portion in cross-platform code whose change is questionable.

::: dbus/dbus-internals.c
@@ +629,2 @@
>    
> +  tempenv = _dbus_getenv("DBUS_MACHINE_ID");

This is in a function called "read uuid file". It should not read the environment.

Comment on attachment 53081 [details] [review]
Add android support

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

The two functional changes here (getting the machine ID from the environment, and changing the default access-control policy) should be separate patches. They're much more controversial than the build-system stuff.

::: Android.mk
@@ +19,5 @@
> +	CPP=$(CONFIGURE_CPP) \
> +	CPPFLAGS="$(CONFIGURE_CPPFLAGS)" \
> +	PKG_CONFIG_LIBDIR="$(CONFIGURE_PKG_CONFIG_LIBDIR)" \
> +	PKG_CONFIG_TOP_BUILD_DIR=$(PKG_CONFIG_TOP_BUILD_DIR) \
> +	ac_cv_func_posix_getpwnam_r=no \

Does Android really not have getpwnam_r? :-(

I don't see a cache variable here for whether Android supports abstract Unix sockets (as in G_UNIX_SOCKET_ADDRESS_ABSTRACT, G_UNIX_SOCKET_ADDRESS_ABSTRACT_PADDED in gio). Does it?

::: bus/Makefile.am
@@ +168,5 @@
>  libexec_PROGRAMS = dbus-daemon-launch-helper
>  endif DBUS_UNIX
>  
> +Android.mk: Makefile.am
> +	androgenizer -:PROJECT dbus -:SHARED libdbus-daemon -:TAGS eng debug \

I'd vaguely prefer this to be called libdbus-daemon-android - the fact that it's a library is completely Android-specific.

@@ +173,5 @@
> +	 -:REL_TOP $(top_srcdir) -:ABS_TOP $(abs_top_srcdir) \
> +	 -:SOURCES $(dbus_daemon_SOURCES) \
> +	 -:CFLAGS $(DEFS) $(CFLAGS) -DBUILD_AS_ANDROID_SERVICE \
> +	 -:CPPFLAGS $(CPPFLAGS) $(AM_CPPFLAGS) $(INCLUDES) $(DEFAULT_INCLUDES) \
> +	 -:LDFLAGS $(dbus_daemon_LDADD) -llog \

What is liblog and why do we need to link against it?

::: dbus/Makefile.am
@@ +286,5 @@
> +	 -:REL_TOP $(top_srcdir) -:ABS_TOP $(abs_top_srcdir) \
> +	 -:SOURCES $(libdbus_1_la_SOURCES) \
> +	 -:CFLAGS $(DEFS) $(AM_CFLAGS) $(CFLAGS) \
> +	 -:CPPFLAGS $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) -I$$\(GLIB_TOP\) \
> +	 -:LDFLAGS $(libdbus_1_0_la_LIBADD) -llog \

It's called libdbus_1_la_LIBADD.

Why is GLib mentioned here?

As before, what is liblog and why do we need to link against it?

@@ +290,5 @@
> +	 -:LDFLAGS $(libdbus_1_0_la_LIBADD) -llog \
> +	 -:STATIC libdbus-internal -:TAGS eng debug \
> +	 -:SOURCES $(libdbus_internal_la_SOURCES) \
> +	 -:CFLAGS $(DEFS) $(AM_CFLAGS) $(CFLAGS) \
> +	 -:CPPFLAGS $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) -I$$\(GLIB_TOP\) \

As before, why GLib?

::: dbus/dbus-internals.c
@@ +629,2 @@
>    
> +  tempenv = _dbus_getenv("DBUS_MACHINE_ID");

As Thiago said, this is the wrong place for this.

If it's desirable to get the machine ID from the environment, then _dbus_read_local_machine_uuid() in dbus-sysdeps-unix.c is the right place.

But why do you need to do this, and how can you guarantee that it works? (In other words: is the machine ID not created correctly on Android? and, how can you guarantee that the environment variable will be set, consistently, for every process that wants to use D-Bus?)

libdbus now looks in /etc/machine-id (the systemd machine ID) as a fallback, if that's any help?

See also Bug #13194, Bug #23679, Bug #29618 (especially the second of those).

We basically need to decide which of the semantics of the D-Bus machine ID are the most important, and put them in the spec. Possible things include:

* Every correct installation has a different machine ID.
  This is the only thing the spec currently guarantees,
  I think.

* Every correct installation has a persistent machine ID.
  This lets you tie hardware-specific config (monitor
  layouts are the thing Colin cited) to a machine ID.
  This is currently true in normal distributions, and
  in particular those that use dpkg/rpm/etc. and/or systemd,
  but is not necessarily true if you hand-installed D-Bus
  (not necessarily even system-wide) and you don't have
  a systemd machine ID either.

* Incorrect installations refuse to run without a
  machine ID. This is currently true, but bad for platforms
  where the installation behaviour currently thought of as
  correct (writing into /var/lib or /etc) is difficult.

I'd prefer this change to be separated out as something we can apply/discuss separately.

::: dbus/dbus-transport.c
@@ +678,5 @@
>                           _dbus_credentials_get_unix_uid(auth_identity),
>                           _dbus_credentials_get_unix_uid(our_identity));
> +#ifdef __BIONIC__
> +	allow = TRUE;
> +#else

What's the goal of this patch band? It looks as though it's changing the default policy (as used for the session bus) from:

* each session bus belongs to one uid
* no other uid may connect

to:

* any uid may connect

Why do you want to defeat privilege separation? Bear in mind that apps on the session bus typically implicitly trust each other and do not attempt to separate privileges, because they "know" that no differently-privileged process can connect - you're making this a false assumption.

(I don't actually know how user IDs work on Android... I'd assumed it was something a bit like Maemo, where most things either run as root or as "user".)

If there's a well-thought-out reason why you want to defeat privilege separation, it should be a separate patch, and it'd probably be better done by patching bus/session.conf.in or dropping a file into /etc/dbus-1/session.d/ to de-restrict the session bus.

::: dbus/sd-daemon.c
@@ +38,1 @@
>  #include <sys/fcntl.h>

sd-daemon.c is "owned by" systemd. Please send this upstream (to Lennart).

(Getting Android fixed so that its sys/fcntl.h is compatible with every other Unix's would be even better.)

::: tools/Makefile.am
@@ +82,5 @@
> +	 -:SOURCES $(dbus_send_SOURCES) \
> +	 -:CFLAGS $(DEFS) $(CFLAGS) $(DEFAULT_INCLUDES) $(INCLUDES) \
> +	  $(AM_CFLAGS) \
> +	 -:CPPFLAGS $(CPPFLAGS) $(AM_CPPFLAGS) \
> +	 -:LDFLAGS $(dbus_send_LDADD) -llog \

As before, what's liblog?

I can answer some of these questions...

Android really doesn't support getpwnam_r :(


The rationale for defeating the privilege separation is that the Android security model is to assign every application bundle its own unique UID at install time.  Any application that isn't part of the same .apk file the dbus daemon was installed from will be running as a different UID.  That doesn't make my solution to the problem any more sane though.


liblog supplies android's logging facility (its equivalent to syslog, I suppose).


As to the environment variable for machine id...

We can't write to /etc on Android - we can't really access anything outside of our application's install directory.  Conversely, another application that wants to connect to dbus won't have access to anything the dbus application can write to.  An application and fiddle with its own directory permissions, but there's no "well known" place to store a machine id file.

Since Android applications are written in java and the VM won't be launched from a descendant of the process that started dbus anyway, some external mechanism must be provided to set-up the session bus address variable via JNI...  this mechanism can also be expected to provide the machine id (in this case via an environment variable, but it could just as easily provide a path to a file in the dbus "application"'s storage space).

Created attachment 53256 [details] [review]
Modifications to build files to build on Android

This patch contains only the modifications to the build files, with the fixes suggested.

I also submitted a patch to systemd, with the patch for sd-daemon.c https://bugs.freedesktop.org/show_bug.cgi?id=42675

We can keep working out the other more controversial stuff.

Created attachment 53302 [details] [review]
Functions mods to fit Android's build

(In reply to comment #3)
> The rationale for defeating the privilege separation is that the Android
> security model is to assign every application bundle its own unique UID at
> install time.  Any application that isn't part of the same .apk file the dbus
> daemon was installed from will be running as a different UID.

OK, this sounds a lot like the OLPC security model.

Is there going to be a single, centralized dbus-daemon package on Android, or is it going to be something bundled in an .apk that contains a user-visible app and all of its supporting stuff, like perhaps the Telepathy stack (libraries and connection managers) and a GUI for it?

If I installed two unrelated user-visible packages (.apk?) that use D-Bus (Telepathy + a GUI and Tracker + a GUI, say) on an Android device, how many dbus-daemons would I be running, under which uids? How many copies of libdbus would I have, and what would be linked to each one?

Is the installation directory fixed at build time, or does everything have to be relocatable?

> That doesn't make my solution to the problem any more sane though.

Indeed. I think the fundamental question is: what should trust what to do what?

If there's a single dbus-daemon "provided by the system" (either in the OS, or as an add-on that apps can require), I think it'd be OK for apps to trust it, but not necessarily for it to trust apps, or for apps to trust each other.

For instance, Telepathy CMs will let anything on the session bus impersonate you in IM conversations (indeed, that's the point of Telepathy). On a GNOME, KDE, etc. system, all apps are equally privileged (they can ptrace each other if it comes to that), so this is fine - there's no additional privilege to which they can escalate. Similarly, the D-Bus-using version of GConf gives full read/write access to the whole GConf tree to everything on the session bus, and that's normally fine, because everything on the session bus could just have edited the files on disk anyway. Some GConf keys specify an arbitrary command to run, but there's no privilege escalation there, because the reader that runs the command is no more privileged than the writer that specifies it.

However, if some apps are more privileged than others, then "can interact with Telepathy" and "can interact with [subtree x in] GConf" are privileges that only some apps should have. This was never really satisfactorily solved on OLPC or Maemo either.

Telepathy and GConf are just examples here, any D-Bus session service is going to have similar issues. System services are usually fine, because the system bus is already a privilege boundary, so anything not being careful at that boundary is already broken.

> liblog supplies android's logging facility (its equivalent to syslog, I
> suppose).

Which API calls made by libdbus/dbus-daemon does it correspond to? Does it provide a Unix-compatible openlog(), syslog(), vsyslog(), closelog(), for instance?

> Since Android applications are written in java and the VM won't be launched
> from a descendant of the process that started dbus anyway, some external
> mechanism must be provided to set-up the session bus address variable via
> JNI...  this mechanism can also be expected to provide the machine id
> (in this case via an environment variable, but it could just as easily
> provide a path to a file in the dbus "application"'s storage space).

One concern I have about the environment variable is security - what's the worst that can happen if an attacker can run a setuid application with a wrong machine ID? In that respect, believing an environment-provided machine ID seems better than reading an environment-provided file, because there's no way the former can be abused to disclose the contents of a file readable by the setuid user but not the attacker.

(In reply to comment #6)
> (In reply to comment #3)
> > The rationale for defeating the privilege separation is that the Android
> > security model is to assign every application bundle its own unique UID at
> > install time.  Any application that isn't part of the same .apk file the dbus
> > daemon was installed from will be running as a different UID.
> 
> OK, this sounds a lot like the OLPC security model.
> 
> Is there going to be a single, centralized dbus-daemon package on Android, or
> is it going to be something bundled in an .apk that contains a user-visible app
> and all of its supporting stuff, like perhaps the Telepathy stack (libraries
> and connection managers) and a GUI for it?

In the specific case we are dealing with now, we are packaging dbus together with telepathy, but it runs as an android service, and provides context information available to other apps, so you could as well have dbus run on one process and telepathy and whatever else in its own processes.

> 
> If I installed two unrelated user-visible packages (.apk?) that use D-Bus
> (Telepathy + a GUI and Tracker + a GUI, say) on an Android device, how many
> dbus-daemons would I be running, under which uids? How many copies of libdbus
> would I have, and what would be linked to each one?
> 

You would only need one. As a matter of fact, the Android context we use to get dbus session variables would only allow one to be discovered.

> Is the installation directory fixed at build time, or does everything have to
> be relocatable?

That is fixed at build time

> 
> > That doesn't make my solution to the problem any more sane though.
> 
> Indeed. I think the fundamental question is: what should trust what to do what?
> 
> If there's a single dbus-daemon "provided by the system" (either in the OS, or
> as an add-on that apps can require), I think it'd be OK for apps to trust it,
> but not necessarily for it to trust apps, or for apps to trust each other.
> 
> For instance, Telepathy CMs will let anything on the session bus impersonate
> you in IM conversations (indeed, that's the point of Telepathy). On a GNOME,
> KDE, etc. system, all apps are equally privileged (they can ptrace each other
> if it comes to that), so this is fine - there's no additional privilege to
> which they can escalate. Similarly, the D-Bus-using version of GConf gives full
> read/write access to the whole GConf tree to everything on the session bus, and
> that's normally fine, because everything on the session bus could just have
> edited the files on disk anyway. Some GConf keys specify an arbitrary command
> to run, but there's no privilege escalation there, because the reader that runs
> the command is no more privileged than the writer that specifies it.
> 
> However, if some apps are more privileged than others, then "can interact with
> Telepathy" and "can interact with [subtree x in] GConf" are privileges that
> only some apps should have. This was never really satisfactorily solved on OLPC
> or Maemo either.

I think in Android case all apps have the same privileges, and no app is more privileged than the other.

(In reply to comment #7)
> > Is the installation directory fixed at build time, or does everything have to
> > be relocatable?
> 
> That is fixed at build time

In that case, instead of playing with environment variables for the machine ID, couldn't you set --localstatedir=/opt/dbus/var (replace /opt/dbus with the fixed installation directory) and just read the machine ID based on a hard-coded filename in that directory, as usual?

You could even make dbus-daemon write out a machine ID on startup - it could be as simple as system ("/opt/dbus/bin/dbus-uuidgen --ensure") when running on Android? (In normal Unix this would be done during installation or in the system bus init script - and it's somewhat advantageous to have it be something that only root can do - but if other packages are trusting the dbus package's user anyway, then that user might as well own its own machine ID).

> I think in Android case all apps have the same privileges, and no app is more
> privileged than the other.

If that's true, then why do they have different uids? I'd expect a system where everything is equally privileged to look more like Maemo (where almost everything runs as either 'root' or 'user').

(In reply to comment #8)
> (In reply to comment #7)
> > > Is the installation directory fixed at build time, or does everything have to
> > > be relocatable?
> > 
> > That is fixed at build time
> 
> In that case, instead of playing with environment variables for the machine ID,
> couldn't you set --localstatedir=/opt/dbus/var (replace /opt/dbus with the
> fixed installation directory) and just read the machine ID based on a
> hard-coded filename in that directory, as usual?
> 
> You could even make dbus-daemon write out a machine ID on startup - it could be
> as simple as system ("/opt/dbus/bin/dbus-uuidgen --ensure") when running on
> Android? (In normal Unix this would be done during installation or in the
> system bus init script - and it's somewhat advantageous to have it be something
> that only root can do - but if other packages are trusting the dbus package's
> user anyway, then that user might as well own its own machine ID).
> 

Ok, we'll check this way and see what we can come up with.

> > I think in Android case all apps have the same privileges, and no app is more
> > privileged than the other.
> 
> If that's true, then why do they have different uids? I'd expect a system where
> everything is equally privileged to look more like Maemo (where almost
> everything runs as either 'root' or 'user').

My bad. Apps can have different permissions. So yes, you could have 2 apps with access to dbus, but only one of them is allowed to send chat messages.
Is there a way around this situation?

(In reply to comment #9)
> My bad. Apps can have different permissions. So yes, you could have 2 apps with
> access to dbus, but only one of them is allowed to send chat messages.
> Is there a way around this situation?

Not trivially, but yes: you basically need to make the session bus look like a normal system bus.

You can do that by installing a different session.conf rather than patching the source code (I'd be inclined to replace session.conf entirely as part of your packaging, rather than patching the one dbus provides - you're doing something completely different with it).

Things to change include:

* log to syslog, maybe?: <syslog/>

* allow all users to connect: <allow user="*"/> in the default policy

* deny owning names by default (so you can't impersonate a well-known service):
  <deny own="*"/> in the default policy

* disallow method calls by default: <deny own="*"/> in the default policy

Then, you need to "poke holes in" this default-deny policy, using drop-in config files similar to those normally found in /etc/dbus-1/system.d.

For that to work, you need to either:

* have some way to identify privileged users (user or group IDs -
  group IDs are probably easiest) and allow them to own particular names
  and/or call particular methods, for instance you might let members
  of the 'telepathy' group talk to Telepathy CMs and Mission Control; or

* allow everyone to call methods on a particular service, if you know that
  that service is "safe", either because it doesn't do anything potentially
  private/damaging or because it checks access itself using PolicyKit

This requires you to do some hard work (thinking about a security model and implementing it via group IDs or whatever); but that's unavoidable really, once you go beyond "anyone may do anything".

(In reply to comment #10)
> * disallow method calls by default: <deny own="*"/> in the default policy

By which I meant <deny send_type="method_call"/>, of course.

Basically, look at what's inside <policy context="default"> in bus/system.conf.in - you probably want most, or even all, of that.

(In reply to comment #7)
> In the specific case we are dealing with now, we are packaging dbus together
> with telepathy

The other thing you could do for this specific, limited situation is to have your dbus-daemon be restricted so only the user ID of its package is allowed to connect (i.e. don't patch out the "same user" restriction), and then you're back in the "trust your own uid but nobody else" model used on normal session buses. If this is the way you're going, then each D-Bus-using "app" (cluster of related processes under the same uid) can only use D-Bus internally, and they can't share - you'd want one "session bus" per "app".

Comment on attachment 53256 [details] [review]
Modifications to build files to build on Android

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

Is this still applicable to current master (soon to be dbus-1.6)?

::: bus/Makefile.am
@@ +168,5 @@
>  libexec_PROGRAMS = dbus-daemon-launch-helper
>  endif DBUS_UNIX
>  
> +Android.mk: Makefile.am
> +	androgenizer -:PROJECT dbus -:SHARED libdbus-daemon -:TAGS eng debug \

As before, I would prefer this to be called libdbus-daemon-android or something, since the fact that it is a library at all is Android-specific.

Comment on attachment 53302 [details] [review]
Functions mods to fit Android's build

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

::: bus/main.c
@@ +45,5 @@
>  #ifdef DBUS_UNIX
>  
> +#ifdef BUILD_AS_ANDROID_SERVICE
> +int dbus_daemon_main (int argc, char **argv);
> +#endif

This is relatively uncontroversial and, if it was a separate patch ("replace main() with a library entry point on Android" or something), could be applied.

@@ +347,5 @@
>  
>  int
> +#ifdef BUILD_AS_ANDROID_SERVICE
> +dbus_daemon_main (int argc, char **argv)
> +#else

Conceptually part of the same patch as the other BUILD_AS_ANDROID_SERVICE bit.

::: dbus/dbus-internals.c
@@ +629,5 @@
>    
> +  tempenv = _dbus_getenv("DBUS_MACHINE_ID");
> +  if (tempenv)
> +    _dbus_string_init_const (&contents, tempenv);
> +  else

As Thiago pointed out, a function called "read UUID file" should read the UUID file, and nothing else: if its purpose has changed to "get the UUID from somewhere or other", it at least deserves renaming.

If you really need to read an environment variable, the existing function _dbus_read_local_machine_uuid() seems a more appropriate place to do this.

Coding style, while I'm reviewing anyway: space before opening parenthesis; the "else" has {}, so the "if" should have them too.

tempenv = _dbus_getenv ("DBUS_MACHINE_ID");

if (tempenv)
  {
    _dbus_string_init_const (&contents, tempenv);
  }
else
  {
    ...

::: dbus/dbus-message-util.c
@@ +42,5 @@
>  
> +#ifdef __BIONIC__
> +#include <sys/select.h>
> +#include <linux/time.h>
> +#endif

If this block is here because of a deficiency in some Android system header, then it deserves a comment explaining why system dependencies are here in an otherwise mostly-OS-independent file, and should be a separate patch, but looks basically OK.

::: dbus/dbus-transport.c
@@ +683,3 @@
>        _dbus_transport_disconnect (transport);
>        allow = FALSE;
> +#endif

This part is a change to security policy, and should not be mixed into a patch for miscellaneous build-system bits.

Another way to achieve the same result would be to alter session.conf in the Android packaging. I think this would be more appropriate, in fact, because if you're allowing processes with arbitrary UIDs onto the session bus, either you have no security and are undermining the platform's security model, or you will need to "lock down" the session bus into a mini-system-bus; the latter requires session.conf to be replaced anyway.

"Robert <robertwbaquet@gmail.com>", who are you and why are you reassigning and re-prioritizing bugs without explanation?

This is not going to get merged without getting broken up into individual "change one thing" patches that each come with a justification. Is that something that is likely to happen, or is that WONTFIX?

This is a 6 year old patch for a way old version of Android, from a project that is long dead. It's just not gonna happen.
If the problem persists, there's likely a better way to fix it.