Bug 66728

Created attachment 82211 [details] [review]
[PATCH 1/2] launch-helper: fix error code parsing

Created attachment 82212 [details] [review]
[PATCH 2/2] launch-helper: return more precisely error message

(In reply to comment #0)
> If <servicehelper> used to activate a system service, it will do some user
> check, however, if <user> doesn't specified in system.conf, the user will be
> ""

This can only arise if the local sysadmin has made some rather inadvisable changes to the bus configuration. Triaging to a lower severity/priority.

Please set the patch keyword when a bug has (what you consider to be) complete patches for a solution - it's easier to filter for things to review that way.

(In reply to comment #3)
> (In reply to comment #0)
> > If <servicehelper> used to activate a system service, it will do some user
> > check, however, if <user> doesn't specified in system.conf, the user will be
> > ""
> 
> This can only arise if the local sysadmin has made some rather inadvisable
> changes to the bus configuration. Triaging to a lower severity/priority.
> 

Yes, I just found that dbus in Tizen is a case. :-(, and I also found in suddenly you're contributing to tizen.org. :-).

> Please set the patch keyword when a bug has (what you consider to be)
> complete patches for a solution - it's easier to filter for things to review
> that way.

OK, got it.

I don't see anything obviously wrong here, but the launch helper is security-sensitive, so I'll come back to this when I can concentrate on it.

Comment on attachment 82211 [details] [review]
[PATCH 1/2] launch-helper: fix error code parsing

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

::: bus/activation.c
@@ +1290,5 @@
>    switch (exit_code)
>      {
> +    case BUS_SPAWN_EXIT_CODE_CONFIG_INVALID:
> +      dbus_set_error (error, DBUS_ERROR_SPAWN_CONFIG_INVALID,
> +		      "Could not get user from config file");

It seems strange to use such a specific string. That's the only thing we use BUS_SPAWN_EXIT_CODE_CONFIG_INVALID for at the moment, but I feel as though "Invalid configuration (missing or empty <user>?)" might be a better way to phrase it?

Comment on attachment 82212 [details] [review]
[PATCH 2/2] launch-helper: return more precisely error message

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

::: bus/config-parser-trivial.c
@@ +317,5 @@
>  {
> +  if (_dbus_string_get_length (&parser->user) > 0)
> +    return _dbus_string_get_const_data (&parser->user);
> +  else
> +    return NULL;

Previously, this could never return NULL. Now, it can. That seems more risky than it needs to be.

I'd prefer to change check_dbus_user() like this:

-  if (dbus_user == NULL)
+  if (dbus_user == NULL || dbus_user[0] == '\0')
     {
       dbus_set_error (...

(In reply to comment #6)
> Comment on attachment 82211 [details] [review] [review]
> [PATCH 1/2] launch-helper: fix error code parsing
> 
> Review of attachment 82211 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: bus/activation.c
> @@ +1290,5 @@
> >    switch (exit_code)
> >      {
> > +    case BUS_SPAWN_EXIT_CODE_CONFIG_INVALID:
> > +      dbus_set_error (error, DBUS_ERROR_SPAWN_CONFIG_INVALID,
> > +		      "Could not get user from config file");

I just copy it from check_dbus_user().

> 
> It seems strange to use such a specific string. That's the only thing we use
> BUS_SPAWN_EXIT_CODE_CONFIG_INVALID for at the moment, but I feel as though
> "Invalid configuration (missing or empty <user>?)" might be a better way to
> phrase it?

Sure, so I'll change that two places.

(In reply to comment #7)
> Comment on attachment 82212 [details] [review] [review]
> [PATCH 2/2] launch-helper: return more precisely error message
> 
> Review of attachment 82212 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: bus/config-parser-trivial.c
> @@ +317,5 @@
> >  {
> > +  if (_dbus_string_get_length (&parser->user) > 0)
> > +    return _dbus_string_get_const_data (&parser->user);
> > +  else
> > +    return NULL;
> 
> Previously, this could never return NULL. Now, it can. That seems more risky
> than it needs to be.

However, if the user is "", which means only contains '\0', then _dbus_string_get_length (&parser->user) > 0 is false, it will return a anyway invalid address.

I think it's straight forward that it returns a NULL (char *) pointer and NULL is checked at all places where it's invoked.

> 
> I'd prefer to change check_dbus_user() like this:
> 
> -  if (dbus_user == NULL)
> +  if (dbus_user == NULL || dbus_user[0] == '\0')

That's not a case since if user is '\0' then _dbus_string_get_length() returns 0 in bus_config_parser_get_user().

>      {
>        dbus_set_error (...

Created attachment 87280 [details] [review]
[PATCH v2] launch-helper: fix error code parsing

fixed error string.

Comment on attachment 87280 [details] [review]
[PATCH v2] launch-helper: fix error code parsing

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

At the moment the error string from activation-helper.c is lost, but in an ideal world we'd pass the error out through a pipe or something (or just use the systemd activation protocol, and ignore this whole mess).

::: bus/activation-helper.c
@@ +497,4 @@
>    if (dbus_user == NULL)
>      {
>        dbus_set_error (error, DBUS_ERROR_SPAWN_CONFIG_INVALID,
> +                      "Invalid configuration (missing or empty <user>?)");

The error string was fine here already, since at this location, we *do* know the precise reason for the error...

::: bus/activation.c
@@ +1290,5 @@
>    switch (exit_code)
>      {
> +    case BUS_SPAWN_EXIT_CODE_CONFIG_INVALID:
> +      dbus_set_error (error, DBUS_ERROR_SPAWN_CONFIG_INVALID,
> +		      "Invalid configuration (missing or empty <user>?");

... it's only here that it seems inappropriately specific, because it would be reasonable for the helper to exit BUS_SPAWN_EXIT_CODE_CONFIG_INVALID for other reasons in future (e.g. malformed XML).

Created attachment 87311 [details] [review]
[PATCH v3] launch-helper: fix error code parsing

comments in #c11 addressed.

queued, thanks

Merged just after 1.7.6, will be in 1.7.8

(In reply to comment #14)
> Merged just after 1.7.6, will be in 1.7.8

Hmm, you mean the second patch? 

[PATCH 2/2] launch-helper: return more precisely error message

I couldn't find it in master branch, did you push it?

(In reply to comment #15)
> [PATCH 2/2] launch-helper: return more precisely error message
> 
> I couldn't find it in master branch, did you push it?

No, you're right, I didn't merge this bit. Reopened.

(In reply to comment #9)
> > > +  if (_dbus_string_get_length (&parser->user) > 0)
> > > +    return _dbus_string_get_const_data (&parser->user);
> > > +  else
> > > +    return NULL;
> > 
> > Previously, this could never return NULL. Now, it can. That seems more risky
> > than it needs to be.
> 
> However, if the user is "", which means only contains '\0', then
> _dbus_string_get_length (&parser->user) > 0 is false, it will return a
> anyway invalid address.

I would expect that _dbus_string_get_const_data() should always return something non-null, and in particular, that if the string is empty, it should return a pointer p such that *p is valid and equal to '\0' (in other words, the C string "").

If that's not true, then I think it's worth fixing in DBusString (by special-casing (real->str == NULL) to return "", perhaps).

(In reply to comment #9)
> (In reply to comment #7)
> > Comment on attachment 82212 [details] [review] [review] [review]
> > [PATCH 2/2] launch-helper: return more precisely error message
> > 
> > Review of attachment 82212 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: bus/config-parser-trivial.c
> > @@ +317,5 @@
> > >  {
> > > +  if (_dbus_string_get_length (&parser->user) > 0)
> > > +    return _dbus_string_get_const_data (&parser->user);
> > > +  else
> > > +    return NULL;
> > 
> > Previously, this could never return NULL. Now, it can. That seems more risky
> > than it needs to be.
> 
> However, if the user is "", which means only contains '\0', then
> _dbus_string_get_length (&parser->user) > 0 is false, it will return a
> anyway invalid address.
> 
> I think it's straight forward that it returns a NULL (char *) pointer and
> NULL is checked at all places where it's invoked.
> 
> > 
> > I'd prefer to change check_dbus_user() like this:
> > 
> > -  if (dbus_user == NULL)
> > +  if (dbus_user == NULL || dbus_user[0] == '\0')
> 
> That's not a case since if user is '\0' then _dbus_string_get_length()
> returns 0 in bus_config_parser_get_user().

OK, seems I was confusing by myself. Please ignore this preivous comment.

> 
> >      {
> >        dbus_set_error (...

Created attachment 87589 [details] [review]
[PATCH] config-parser: never returns a NULL pointer for <user>

agree with Simon, just found that it's cost more time to pull back the context after months. :-)

The patch looks good, but it turns out to break the tests:

Activated service 'org.freedesktop.DBus.TestSuiteEchoService' failed: Invalid configuration (missing or empty <user>?)
Did not expect error org.freedesktop.DBus.Error.Spawn.ConfigInvalid
  ./../bus/bus-test(_dbus_print_backtrace+0x1a) [0x47af6a]
  ./../bus/bus-test(_dbus_abort+0x9) [0x47e939]
  ./../bus/bus-test() [0x4670e6]
  ./../bus/bus-test() [0x42a201]
  ./../bus/bus-test(bus_dispatch_test+0x75) [0x42ad35]
  ./../bus/bus-test(main+0x180) [0x416400]
  /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x2b5199da7995]
  ./../bus/bus-test() [0x416553]

Perhaps replacing "anyrandomuser" with "root" in the activatable "system" services would fix this.

Created attachment 88541 [details] [review]
[PATCH] TEST: ignore <user> in test environment

This patch fix test config files and source code, so it ignores user in test environment.

Comment on attachment 88541 [details] [review]
[PATCH] TEST: ignore <user> in test environment

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

I'm not sure about this. It fixes the tests, but it means the daemon doesn't setuid() to the configured user if a particular environment variable is set, which seems the sort of thing likely to cause problems, and perhaps even a security flaw.

I'm not going to merge this right now, while I think about whether this is going to be a problem.

If it can't be used as a security exploit (which it probably can't because dbus-daemon isn't setuid), perhaps using a more explicit name for the environment variable (DBUS_TEST_DAEMON_KEEP_CURRENT_UID=1 or something) would mitigate the risk of accidents by making it more obvious what it does?

(In reply to comment #0)
> "
> Error org.freedesktop.DBus.Error.Spawn.PermissionsInvalid: The permission of
> the setuid helper is not correct
> "

Would it be enough to make this message a bit more vague? Something like "...PermissionsInvalid: The setuid helper has incorrect permissions or was not run by the configured dbus-daemon user"?

(The source of the error is presumably that the setuid helper looks up who ran it, and discovers that, no, their login name is not the empty string :-)

I also wonder whether the setuid helper, which is Unix-specific anyway, should use syslog() in the LOG_AUTHPRIV facility (if defined, which it is in glibc) to log better diagnostics - then the dbus-daemon's rather limited interpretation of its exit code would be less important.

LOG_AUTHPRIV messages are meant to be private to the sysadmin, because they're the appropriate log channel for things like incorrect usernames (which could have been a user mistakenly typing their password into the username box).

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/86.