Privilege dropping support in Upstart

Evan Broder evan at ebroder.net
Fri Nov 18 02:24:24 UTC 2011


On Fri, Nov 11, 2011 at 3:10 AM, James Hunt <james.hunt at ubuntu.com> wrote:
> Hi Evan,
>
> You beat me to it! (I started to work on a patch for this on the way back from UDS :-)

Aha, so I take it I managed to successfully distract you with my weird
sbuild/controlling tty issue? :)

Thanks for your feedback. I've pushed a handful of new commits to my
branch (http://code.launchpad.net/~broder/upstart/drop-privileges)
incorporating some of your suggestions.

> 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 [*].

Fixed, thanks.

> * 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).

Passing -1 as either the uid or gid arguments to chown and friends
results in the uid/gid (as appropriate) not being changed, so this
should be fine.

> - 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.

I'm really not sure I agree with this. I think this is philosophically
equivalent to the traditional daemon initialization process of
acquiring the resources it needs and then dropping privileges.
Similarly, daemons running under Upstart should be permitted to
express that same idiom through Upstart's config file - acquire
resources, then drop privileges.

While I can respect the need for caution, this doesn't introduce any
behavior that wasn't there previously - system jobs using the setuid
and setgid syntax would previously have been started as root and been
able to change these restrictions before dropping privileges. If we
impose restrictions on how setuid/setgid can be used, it limits the
utility of the new stanzas without any concrete gain.

> * 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.

I believe that these and other functions would be more testable if a
portion of the test suite was run under fakeroot and fakechroot -
combined, they seem to be clever enough to simulate chroot(),
setuid(), and setgid(), among others.

> 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).

I've added these tests, but left out the test for the supplementary
group - if your current uid is not 0, you can only set your gid to
your primary group:

evan at caron:~$ groups
evan adm dialout cdrom plugdev lpadmin libvirtd admin sambashare sbuild kvm
evan at caron:~$ python
Python 2.7.2+ (default, Oct  4 2011, 20:06:09)
[GCC 4.6.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import grp
>>> import os
>>> os.setgid(grp.getgrnam('cdrom').gr_gid)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 1] Operation not permitted

Changing to a supplementary group requires a setuid tool like setgid.

> 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`").

Done.

> 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.

Thanks,
 - Evan



More information about the upstart-devel mailing list