Merge lp:~stgraber/upstart/upstart-initgroups into lp:upstart

[Stéphane Graber]

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.

[Stéphane Graber (stgraber)]

So after more testing, this bug can't be exploited on regular hosts (physical machines, VMs) as the kernel group list is empty on those, so there's no "root" group to inherit.

On those, the bug is only that you don't inherit the groups of the setuid user, which is problematic but not a security issue.

However for users of containers, the initial group list does contain root, so for those, it's a potential security issue. But the number of users of containers being far lower than those of regular systems, this somewhat lowers the priority of this fix.

[James Hunt (jamesodhunt)]

The first call to initgroups for user jobs looks fine, but the 2nd call for system jobs (that specify setuid/setgid stanzas) has a few problems:

(1) Shouldn't we only be calling this if we're root _and_ 'if (class->setuid || class->setgid)' and then performing the password and group lookup for job_setuid and job_setgid?

(2) The checks on pwd and grp for near the 2nd call to initgroups are not currently legitimate - pwd and grp should be initialised to NULL to avoid undefined behaviour.

(3) You don't need those two errno resets just before the 2nd call since the code never explicitly checks errno there.

review: Needs Fixing

[Stéphane Graber (stgraber)]

> The first call to initgroups for user jobs looks fine, but the 2nd call for
> system jobs (that specify setuid/setgid stanzas) has a few problems:
>
> (1) Shouldn't we only be calling this if we're root _and_ 'if (class->setuid
> || class->setgid)' and then performing the password and group lookup for
> job_setuid and job_setgid?

No, in the case where the root user is in additional groups, we need the initgroups call to populate the group list.

> (2) The checks on pwd and grp for near the 2nd call to initgroups are not
> currently legitimate - pwd and grp should be initialised to NULL to avoid
> undefined behaviour.

Oops, indeed, fixed.

> (3) You don't need those two errno resets just before the 2nd call since the
> code never explicitly checks errno there.

Fixed.

[lp:~stgraber/upstart/upstart-initgroups]

1395. By Stéphane Graber on 2012-11-30

Don't reset errno when we're not using it. Initialise passwd and group to NULL.

[James Hunt (jamesodhunt)]

> > The first call to initgroups for user jobs looks fine, but the 2nd call for
> > system jobs (that specify setuid/setgid stanzas) has a few problems:
> >
> > (1) Shouldn't we only be calling this if we're root _and_ 'if (class->setuid
> > || class->setgid)' and then performing the password and group lookup for
> > job_setuid and job_setgid?
>
> No, in the case where the root user is in additional groups, we need the initgroups call to populate the group list.

Ah - I did wonder about that. We've survived for a long time without calling initgroups for system jobs, but that appears to have been an oversight from way back :)

>
> > (2) The checks on pwd and grp for near the 2nd call to initgroups are not
> > currently legitimate - pwd and grp should be initialised to NULL to avoid
> > undefined behaviour.
>
> Oops, indeed, fixed.
>
> > (3) You don't need those two errno resets just before the 2nd call since the
> > code never explicitly checks errno there.
>
> Fixed.

The final change required is to update job_process_error_read() for the new JobProcessErrorTypes you've added. Once you've done this, please can you force such an error in a manual test, to ensure expected behaviour (currently, you should find that init will assert).

I've raised bug 1085836 to document the requirement to create a new test program which will embody all tests that need to be run as the root user.

review: Needs Fixing

[lp:~stgraber/upstart/upstart-initgroups]

1396. By Stéphane Graber on 2012-12-03

Add the new error codes to job_process_error_read

1397. By Stéphane Graber on 2012-12-03

Add a basic test for initgroups

[Stéphane Graber (stgraber)]

Added the entries to job_process_error_read.

I also tried to add a test for the initgroups() code that'd work both as a user and as root by checking that the error path is triggered in one case and that the job starts properly in the other.

The test is pretty minimal and appears to work when run as a user, when running it as root, the test suite appears to fail earlier on so even if the code appears to be fine, I'm not 100% sure it'll work.
I also tried to use nih_error_get() to match the exact INITGROUPS error code but without much success, I gave up after a few hours of trying to figure out why the simple fact of calling nih_error_get would fail.

[James Hunt (jamesodhunt)]

Thanks for this. Great that we now have some form of test for this scenario. Just a couple of points:

* init/tests/test_job_process.c:
  - Can you change the getpwuid() call to getpwnam ("nobody") and
    likewise for getgrgid() to getgrnam() as it's much clearer and
    should protect us in the case where the tests are run on
    systems with 32-bit uid_t's. This will also solve the 2 typo issues
    (655534 rather than 65534).
  - Also, can you put some more comments on that test to make it clear
    why it is expected to work when run as "nobody".

[lp:~stgraber/upstart/upstart-initgroups]

1398. By Stéphane Graber on 2012-12-04

Use nobody/nogroup instead of 65534/65534, use more comprehensive comment.

[Stéphane Graber (stgraber)]

There you go. Confirmed that the test still passes.

[James Hunt (jamesodhunt)]

Thanks Stéphane - merged.

review: Approve

=== modified file 'init/job_process.c'
--- init/job_process.c	2012-11-21 06:48:08 +0000
+++ init/job_process.c	2012-12-04 19:59:27 +0000
@@ -413,6 +413,8 @@
 	JobClass       *class;
 	uid_t           job_setuid = -1;
 	gid_t           job_setgid = -1;
+	struct passwd   *pwd = NULL;
+	struct group    *grp = NULL;
 
 
 	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,35 @@
 		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) {
+			pwd = getpwuid (geteuid ());
+			if (! pwd) {
+				nih_error_raise_system ();
+				job_process_error_abort (fds[1], JOB_PROCESS_ERROR_GETPWUID, 0);
+			}
+		}
+
+		if (! grp) {
+			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);
@@ -1142,6 +1176,11 @@
 				  err, _("unable to getpwuid: %s"),
 				  strerror (err->errnum)));
 		break;
+	case JOB_PROCESS_ERROR_GETGRGID:
+		err->error.message = NIH_MUST (nih_sprintf (
+				  err, _("unable to getgrgid: %s"),
+				  strerror (err->errnum)));
+		break;
 	case JOB_PROCESS_ERROR_BAD_SETUID:
 		err->error.message = NIH_MUST (nih_sprintf (
 				  err, _("unable to find setuid user")));
@@ -1200,6 +1239,11 @@
 				  err, _("unable to allocate memory: %s"),
 				  strerror (err->errnum)));
 		break;
+	case JOB_PROCESS_ERROR_INITGROUPS:
+		err->error.message = NIH_MUST (nih_sprintf (
+				  err, _("unable to initgroups: %s"),
+				  strerror (err->errnum)));
+		break;
 	default:
 		nih_assert_not_reached ();
 	}

=== modified file 'init/job_process.h'
--- init/job_process.h	2012-11-07 15:17:58 +0000
+++ init/job_process.h	2012-12-04 19:59:27 +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;
 
 /**

=== modified file 'init/tests/test_job_process.c'
--- init/tests/test_job_process.c	2012-12-04 17:12:46 +0000
+++ init/tests/test_job_process.c	2012-12-04 19:59:27 +0000
@@ -4007,6 +4007,74 @@
 	}
 
 	/************************************************************/
+
+	/* Check that initgroups gets called.
+	 * The test will run a job as nobody/nogroup (setuid/setgid) target.
+	 *
+	 * When run from an unprivileged user, the check will ensure that upstart
+	 * fails to start the job (returning -1) as initgroups() is a privileged
+	 * call (similar to setuid and setgid).
+	 *
+	 * When run from a privileged user (root), the check will ensure that
+	 * upstart succeeds in dropping privileges, which includes calling
+	 * initgroup, setuid and setgid.
+	 *
+	 * If the test is started by user nobody/nogroup, then it'll succeed as
+	 * there's no privilege changes to be done in such case (same uid/gid).
+	 */
+	TEST_FEATURE ("with setuid");
+	TEST_HASH_EMPTY (job_classes);
+
+	TEST_NE_P (output, NULL);
+	TEST_ALLOC_FAIL {
+		TEST_ALLOC_SAFE {
+			class = job_class_new (NULL, "test", NULL);
+			class->process[PROCESS_MAIN] = process_new (class);
+			class->process[PROCESS_MAIN]->command = nih_sprintf (
+				class->process[PROCESS_MAIN],
+				"touch %s", filename);
+
+			pwd = getpwnam ("nobody");
+			TEST_NE (pwd, NULL);
+			class->setuid = nih_strdup (class, pwd->pw_name);
+
+			grp = getgrnam ("nogroup");
+			TEST_NE (grp, NULL);
+			class->setgid = nih_strdup (class, grp->gr_name);
+
+			job = job_new (class, "");
+			job->goal = JOB_START;
+			job->state = JOB_SPAWNED;
+
+			output = tmpfile ();
+		}
+
+		TEST_DIVERT_STDERR (output) {
+			ret = job_process_run (job, PROCESS_MAIN);
+			if (geteuid() == 0 || getuid() == pwd->pw_uid) {
+				TEST_EQ (ret, 0);
+			}
+			else {
+				TEST_EQ (ret, -1);
+			}
+		}
+
+		if (geteuid() == 0 || getuid() == pwd->pw_uid) {
+			TEST_NE (job->pid[PROCESS_MAIN], 0);
+
+			waitpid (job->pid[PROCESS_MAIN], NULL, 0);
+			TEST_EQ (stat (filename, &statbuf), 0);
+		}
+		else {
+			TEST_EQ (stat (filename, &statbuf), -1);
+		}
+
+		unlink (filename);
+		nih_free (class);
+
+	}
+
+	/************************************************************/
 	TEST_FEATURE ("with multiple processes and log");
 	TEST_HASH_EMPTY (job_classes);