Privilege dropping support in Upstart

[James Hunt]

On 08/11/11 21:01, Evan Broder wrote:
> The attached patchset (also available at
> http://code.launchpad.net/~broder/upstart/drop-privileges) adds new
> setuid and setgid stanzas to the config format, each of which accept a
> user/group name (not UID/GID), respectively. (See also
> https://bugs.launchpad.net/upstart/+bug/586942)
> 
> If the stanzas are set, Upstart drops privileges after handling the
> chroot and chdir stanzas and before resetting signal handlers. This
> means that the arguments to the stanzas are evaluated within the
> chroot where the job will run. They are also evaluated after dropping
> privilege for user jobs, and after setting rlimits.
> 
> If the setuid stanza is set and the setgid stanza is unset, then the
> primary group of the user specified is used. If the setgid stanza is
> set and the setuid stanza is unset, the job runs with root's (or the
> unprivileged user's) UID and the specified group. If neither is
> specified, the job runs with root's user and group.
> 
> If either the user or group specified do not exist in the job's
> environment, the job throws an error.
> 
> Since setuid(2) and setgid(2) are both privileged operations, I was
> unable to write automated tests for them that could run unprivileged.
> However, I did test all combinations of setting and unsetting setuid
> and setgid, as well as invalid values for both setuid and setgid, and
> confirmed that those combinations work as expected.
> 
> Here is the concatenated bzr log of changes:
> 
>   * init/man/init.5: Document new setuid and setgid stanzas, including
>   their behavior when unspecified.
>   * init/job_process.c, init/job_process.h, init/errors.h: If setuid or
>   setgid stanzas are specified, drop privileges just before executing
>   the job.
>   * init/tests/test_parse_job.c: Test new setuid and setgid stanza
>   parsing
>   * init/parse_job.c: Parse setuid and setgid stanzas from config files
>   taking a user and group name argument, respectively.
>   * init/tests/test_job_class.c: Test new setuid and setgid JobClass
>   attributes
>   * init/job_class.c, init/job_class.h: Add new setuid and setgid fields
>   to JobClass
> 
> Thanks for your feedback,
>  - Evan
> 
> 
> 
Hi Evan,

You beat me to it! (I started to work on a patch for this on the way back from UDS :-)


Comments:

* init/man/init.5:

- Typo 'unspceified'.
- To avoid any confusion, I think it's worth explaining that system jobs which specify "setuid X"
cannot be controlled by user X since they are not user jobs: they are simply jobs running as the
user in question [*].

* init/job_process.c:job_process_spawn():

- That fchown() looks potentially unsafe:

  if job_setgid != -1, but job_setuid == -1, we get:

    fchown (script_fd, -1, job_setgid);

However, something like the following is also too simplistic...

    fchown (script_fd,
      job_setuid == -1 ? 0 : job_setuid,
      job_setgid == -1 ? 0 : job_setgid);

... since it doesn't take account of user jobs (and in my snippet above could allow privilege
escalation).

- Resource Limits/OOM/Priority

The fact that the setgid+setuid calls come *after* the chroot+user session code means we're
effectively allowing non-privileged (system) jobs to elevate their resource limits, OOM score and
priority. It could be argued that these are after all system jobs, so why not allow such behaviour?
But until compelling examples are given, I'd prefer we take the cautious approach and disallowed
this behaviour. Again, to avoid confusion we should document in init.5 that system jobs running as
non-privileged users cannot elevate their resource limits beyond that users limits.

* Testing

Yes, I appreciate the issues testing some of these scenarios. It will admittedly complicated by
adding setuid+setgid support to the already interesting combination of user jobs and chroot support
:) What we need is a fully automated set of tests for these features. Effort is being put into this
for the current Ubuntu cycle.

What I would recommend is adding some tests to util/tests/test_user_sessions.sh that...

  - essentially duplicate the tests you've created in test_parse_job.c to ensure that say specifying
multiple values after the setuid stanza fails as expected.

  - ensure 'setuid root' fails as expected
  - ensure 'setdid root' fails as expected
  - ensure 'setgid <group>' works (assuming <group> is the users primary group).
  - ensure 'setgid <group>' works (assuming <group> is a valid supplementary group).

Also, could you add a test to init/tests/test_job_process.c that ensures it is possible to run a job
which specifies the *current* user and group of the user running the tests (essentially specifying
"setuid `id -u`" and "setgid `id -g`").


A final point to note is that once we've added this feature, life becomes a lot easier for those
wanting to run jobs encapsulating Java applications (which apparently require additional "helper
code" to change uid/gid on their own).

Kind regards,

James.

[*] - we should maybe discuss exposing uid+gid info for jobs in initctl's 'list', 'status' and
'show-config' commands.

--
James Hunt
____________________________________
http://upstart.ubuntu.com/cookbook
http://upstart.ubuntu.com/cookbook/upstart_cookbook.pdf