Bug 66728 - [PATCH] launch-helper: fix error code parsing
Summary: [PATCH] launch-helper: fix error code parsing
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: low minor
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-07-09 07:51 UTC by Chengwei Yang
Modified: 2018-10-12 21:16 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/2] launch-helper: fix error code parsing (2.90 KB, patch)
2013-07-09 07:55 UTC, Chengwei Yang
Details | Splinter Review
[PATCH 2/2] launch-helper: return more precisely error message (969 bytes, patch)
2013-07-09 07:55 UTC, Chengwei Yang
Details | Splinter Review
[PATCH v2] launch-helper: fix error code parsing (3.19 KB, patch)
2013-10-08 13:41 UTC, Chengwei Yang
Details | Splinter Review
[PATCH v3] launch-helper: fix error code parsing (2.85 KB, patch)
2013-10-09 00:52 UTC, Chengwei Yang
Details | Splinter Review
[PATCH] config-parser: never returns a NULL pointer for <user> (1.42 KB, patch)
2013-10-14 10:34 UTC, Chengwei Yang
Details | Splinter Review
[PATCH] TEST: ignore <user> in test environment (2.97 KB, patch)
2013-11-03 00:23 UTC, Chengwei Yang
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chengwei Yang 2013-07-09 07:51:51 UTC
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 "" because it always make sure parser->user initialized as a DBusString, I think the idea behind is to avoid OOM later.

However, when the above situation occurrs, the error is a little confusing.

"
Error org.freedesktop.DBus.Error.Spawn.PermissionsInvalid: The permission of the setuid helper is not correct
"

I think it's better to compalin like below.

"
Error org.freedesktop.DBus.Error.Spawn.ConfigInvalid: Could not get user from config file
"
Comment 1 Chengwei Yang 2013-07-09 07:55:22 UTC
Created attachment 82211 [details] [review]
[PATCH 1/2] launch-helper: fix error code parsing
Comment 2 Chengwei Yang 2013-07-09 07:55:47 UTC
Created attachment 82212 [details] [review]
[PATCH 2/2] launch-helper: return more precisely error message
Comment 3 Simon McVittie 2013-07-16 11:45:51 UTC
(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.
Comment 4 Chengwei Yang 2013-07-16 13:29:48 UTC
(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.
Comment 5 Simon McVittie 2013-08-22 18:01:15 UTC
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 6 Simon McVittie 2013-10-08 11:53:13 UTC
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 7 Simon McVittie 2013-10-08 11:55:17 UTC
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 (...
Comment 8 Chengwei Yang 2013-10-08 13:29:05 UTC
(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.
Comment 9 Chengwei Yang 2013-10-08 13:40:10 UTC
(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 (...
Comment 10 Chengwei Yang 2013-10-08 13:41:52 UTC
Created attachment 87280 [details] [review]
[PATCH v2] launch-helper: fix error code parsing

fixed error string.
Comment 11 Simon McVittie 2013-10-08 15:06:27 UTC
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).
Comment 12 Chengwei Yang 2013-10-09 00:52:43 UTC
Created attachment 87311 [details] [review]
[PATCH v3] launch-helper: fix error code parsing

comments in #c11 addressed.
Comment 13 Simon McVittie 2013-10-09 09:49:26 UTC
queued, thanks
Comment 14 Simon McVittie 2013-10-10 17:26:25 UTC
Merged just after 1.7.6, will be in 1.7.8
Comment 15 Chengwei Yang 2013-10-11 02:00:38 UTC
(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?
Comment 16 Simon McVittie 2013-10-11 10:12:04 UTC
(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).
Comment 17 Chengwei Yang 2013-10-14 07:28:20 UTC
(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 (...
Comment 18 Chengwei Yang 2013-10-14 10:34:00 UTC
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. :-)
Comment 19 Simon McVittie 2013-11-01 12:13:27 UTC
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.
Comment 20 Chengwei Yang 2013-11-03 00:23:54 UTC
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 21 Simon McVittie 2013-11-27 16:46:57 UTC
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?
Comment 22 Simon McVittie 2013-11-27 16:49:30 UTC
(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 :-)
Comment 23 Simon McVittie 2013-11-27 16:55:53 UTC
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).
Comment 24 GitLab Migration User 2018-10-12 21:16:27 UTC
-- 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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.