Stéphane Graber has proposed merging lp:~stgraber/upstart/upstart-initgroups
into lp:upstart.
Requested reviews:
Upstart Reviewers (upstart-reviewers)
For more details, see:
https://code.launchpad.net/~stgraber/upstart/upstart-initgroups/+merge/136794
Call initgroups() before spawning a job to ensure that the user's group list
is properly initialized.
This avoids the following issue:
=== Example of the security problem ===
root@upstart-test:~# cat /etc/init/test.conf
setuid nobody
setgid nogroup
task
script
cat /tmp/secret-file > /tmp/public-file
chmod 666 /tmp/public-file
id > /tmp/debug
end script
root@upstart-test:~# id nobody
uid=65534(nobody) gid=65534(nogroup) groups=65534(nogroup)
root@upstart-test:~# echo secret > /tmp/secret-file
root@upstart-test:~# chmod 660 /tmp/secret-file
root@upstart-test:~# start test
test stop/waiting
root@upstart-test:~# cat /tmp/public-file
secret
root@upstart-test:~# ls -l /tmp/public-file
-rw-rw-rw- 1 nobody nogroup 7 Nov 28 20:59 /tmp/public-file
root@upstart-test:~# cat /tmp/debug
uid=65534(nobody) gid=65534(nogroup) groups=65534(nogroup),0(root)
root@upstart-test:~#
=== Same commands but with the fixed version ===
root@upstart-test:~# cat /etc/init/test.conf
setuid nobody
setgid nogroup
task
script
cat /tmp/secret-file > /tmp/public-file
chmod 666 /tmp/public-file
id > /tmp/debug
end script
root@upstart-test:~# id nobody
uid=65534(nobody) gid=65534(nogroup) groups=65534(nogroup)
root@upstart-test:~# echo secret > /tmp/secret-file
root@upstart-test:~# chmod 660 /tmp/secret-file
root@upstart-test:~# start test
start: Job failed to start
root@upstart-test:~#
The code was tested (as shown above) and the unit tests still all pass.
However, as the tests are meant to be run as non-root, it's not possible
to add new tests testing for initgroups() behaviour.
--
https://code.launchpad.net/~stgraber/upstart/upstart-initgroups/+merge/136794
Your team Upstart Reviewers is requested to review the proposed merge of
lp:~stgraber/upstart/upstart-initgroups into lp:upstart.
=== modified file 'init/job_process.c'
--- init/job_process.c 2012-11-21 06:48:08 +0000
+++ init/job_process.c 2012-11-28 21:11:21 +0000
@@ -413,6 +413,8 @@
JobClass *class;
uid_t job_setuid = -1;
gid_t job_setgid = -1;
+ struct passwd *pwd;
+ struct group *grp;
nih_assert (job != NULL);
@@ -705,6 +707,11 @@
job_process_error_abort (fds[1], JOB_PROCESS_ERROR_CHOWN, 0);
}
+ if (geteuid () == 0 && initgroups (pw->pw_name, pw->pw_gid) < 0) {
+ nih_error_raise_system ();
+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_INITGROUPS, 0);
+ }
+
if (pw->pw_gid && setgid (pw->pw_gid) < 0) {
nih_error_raise_system ();
job_process_error_abort (fds[1], JOB_PROCESS_ERROR_SETGID, 0);
@@ -844,7 +851,6 @@
* session jobs and jobs with a chroot stanza.
*/
if (class->setuid) {
- struct passwd *pwd;
/* Without resetting errno, it's impossible to
* distinguish between a non-existent user and and
* error during lookup */
@@ -867,7 +873,6 @@
}
if (class->setgid) {
- struct group *grp;
errno = 0;
grp = getgrnam (class->setgid);
if (! grp) {
@@ -891,6 +896,37 @@
job_process_error_abort (fds[1], JOB_PROCESS_ERROR_CHOWN, 0);
}
+ /* Make sure we always have the needed pwd and grp structs.
+ * Then pass those to initgroups() to setup the user's group list.
+ * Only do that if we're root as initgroups() won't work when non-root. */
+ if (geteuid () == 0) {
+ if (! pwd) {
+ errno = 0;
+ pwd = getpwuid (geteuid ());
+ if (! pwd) {
+ nih_error_raise_system ();
+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_GETPWUID, 0);
+ }
+ }
+
+ if (! grp) {
+ errno = 0;
+ grp = getgrgid (getegid ());
+ if (! grp) {
+ nih_error_raise_system ();
+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_GETGRGID, 0);
+ }
+ }
+
+ if (pwd && grp) {
+ if (initgroups (pwd->pw_name, grp->gr_gid) < 0) {
+ nih_error_raise_system ();
+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_INITGROUPS, 0);
+ }
+ }
+ }
+
+ /* Start dropping privileges */
if (job_setgid != (gid_t) -1 && setgid (job_setgid) < 0) {
nih_error_raise_system ();
job_process_error_abort (fds[1], JOB_PROCESS_ERROR_SETGID, 0);
=== modified file 'init/job_process.h'
--- init/job_process.h 2012-11-07 15:17:58 +0000
+++ init/job_process.h 2012-11-28 21:11:21 +0000
@@ -94,7 +94,9 @@
JOB_PROCESS_ERROR_PTSNAME,
JOB_PROCESS_ERROR_OPENPT_SLAVE,
JOB_PROCESS_ERROR_SIGNAL,
- JOB_PROCESS_ERROR_ALLOC
+ JOB_PROCESS_ERROR_ALLOC,
+ JOB_PROCESS_ERROR_INITGROUPS,
+ JOB_PROCESS_ERROR_GETGRGID
} JobProcessErrorType;
/**
--
upstart-devel mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
