Merge lp:~broder/upstart/drop-privileges into lp:upstart

[Evan Broder]

[James Hunt (jamesodhunt)]

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

review: Needs Fixing

[lp:~broder/upstart/drop-privileges]

1336. By Evan Broder on 2011-11-11

* init/man/init.5: Correct spelling typo and clarify that system jobs
which drop privilege are still system jobs, not user jobs.

1337. By Evan Broder on 2011-11-11

* contrib/vim/syntax/upstart.vim: Add the new setuid and setgid
  stanzas

1338. By Evan Broder on 2011-11-18

* init/tests/test_job_process.c, util/tests/test_user_sessions.sh:
  Test setuid and setgid stanzas as thoroughly as an unprivileged user
  can (i.e. make sure dropping to yourself works, and escalating to
  anything else doesn't)

1339. By Evan Broder on 2011-12-07

* init/job_process.c: Cast signed constants to uid_t and gid_t before
  comparing them with the same to correct warnings from the
  build. uid_t and gid_t are unsigned, but -1 is still their dummy
  value

=== modified file 'contrib/vim/syntax/upstart.vim'
--- contrib/vim/syntax/upstart.vim	2011-06-14 13:53:27 +0000
+++ contrib/vim/syntax/upstart.vim	2011-12-07 15:52:24 +0000
@@ -33,7 +33,7 @@
 " one argument
 syn keyword upstartStatement description author version instance expect
 syn keyword upstartStatement pid kill normal console env exit export
-syn keyword upstartStatement umask nice oom chroot chdir exec
+syn keyword upstartStatement umask nice oom chroot chdir exec setiud setgid
 
 " two arguments
 syn keyword upstartStatement limit
 
=== modified file 'init/errors.h'
--- init/errors.h	2011-05-12 20:42:28 +0000
+++ init/errors.h	2011-12-07 15:52:24 +0000
@@ -36,6 +36,8 @@
 
 	/* Errors while handling job processes */
 	JOB_PROCESS_ERROR,
+	JOB_PROCESS_INVALID_SETUID,
+	JOB_PROCESS_INVALID_SETGID,
 
 	/* Errors while parsing configuration files */
 	PARSE_ILLEGAL_INTERVAL,
@@ -59,6 +61,8 @@
 #define ENVIRON_UNKNOWN_PARAM_STR	N_("Unknown parameter")
 #define ENVIRON_EXPECTED_OPERATOR_STR	N_("Expected operator")
 #define ENVIRON_MISMATCHED_BRACES_STR	N_("Mismatched braces")
+#define JOB_PROCESS_INVALID_SETUID_STR	N_("Invalid setuid user name does not exist")
+#define JOB_PROCESS_INVALID_SETGID_STR	N_("Invalid setgid group name does not exist")
 #define PARSE_ILLEGAL_INTERVAL_STR	N_("Illegal interval, expected number of seconds")
 #define PARSE_ILLEGAL_EXIT_STR		N_("Illegal exit status, expected integer")
 #define PARSE_ILLEGAL_SIGNAL_STR	N_("Illegal signal status, expected integer")
 
=== modified file 'init/job_class.c'
--- init/job_class.c	2011-08-11 20:47:00 +0000
+++ init/job_class.c	2011-12-07 15:52:24 +0000
@@ -216,6 +216,9 @@
 	class->chroot = NULL;
 	class->chdir = NULL;
 
+	class->setuid = NULL;
+	class->setgid = NULL;
+
 	class->deleted = FALSE;
 	class->debug   = FALSE;
 
 
=== modified file 'init/job_class.h'
--- init/job_class.h	2011-08-11 20:47:00 +0000
+++ init/job_class.h	2011-12-07 15:52:24 +0000
@@ -156,6 +156,8 @@
  * @limits: resource limits indexed by resource,
  * @chroot: root directory of process (implies @chdir if not set),
  * @chdir: working directory of process,
+ * @setuid: user name to drop to before starting process,
+ * @setgid: group name to drop to before starting process,
  * @deleted: whether job should be deleted when finished.
  *
  * This structure holds the configuration of a known task or service that
@@ -206,6 +208,8 @@
 	struct rlimit  *limits[RLIMIT_NLIMITS];
 	char           *chroot;
 	char           *chdir;
+	char           *setuid;
+	char           *setgid;
 
 	int             deleted;
 	int             debug;
 
=== modified file 'init/job_process.c'
--- init/job_process.c	2011-08-11 21:08:52 +0000
+++ init/job_process.c	2011-12-07 15:52:24 +0000
@@ -41,6 +41,7 @@
 #include <utmp.h>
 #include <utmpx.h>
 #include <pwd.h>
+#include <grp.h>
 
 #include <nih/macros.h>
 #include <nih/alloc.h>
@@ -380,6 +381,8 @@
 	FILE           *fd;
 	int             user_job = FALSE;
 	nih_local char *user_dir = NULL;
+	uid_t           job_setuid = -1;
+	gid_t           job_setgid = -1;
 
 
 	nih_assert (class != NULL);
@@ -654,6 +657,67 @@
 		job_process_error_abort (fds[1], JOB_PROCESS_ERROR_CHDIR, 0);
 	}
 
+	/* Change the user and group of the process to the one
+	 * configured in the job. We must wait until now to lookup the
+	 * UID and GID from the names to accommodate both chroot
+	 * session jobs and jobs with a chroot stanza.
+	 */
+	if (class->setuid) {
+		/* Without resetting errno, it's impossible to
+		 * distinguish between a non-existent user and and
+		 * error during lookup */
+		errno = 0;
+		struct passwd *pwd = getpwnam (class->setuid);
+		if (! pwd) {
+			if (errno != 0) {
+				nih_error_raise_system ();
+				job_process_error_abort (fds[1], JOB_PROCESS_ERROR_GETPWNAM, 0);
+			} else {
+				nih_error_raise (JOB_PROCESS_INVALID_SETUID,
+						 JOB_PROCESS_INVALID_SETUID_STR);
+				job_process_error_abort (fds[1], JOB_PROCESS_ERROR_BAD_SETUID, 0);
+			}
+		}
+
+		job_setuid = pwd->pw_uid;
+		/* This will be overridden if setgid is also set: */
+		job_setgid = pwd->pw_gid;
+	}
+
+	if (class->setgid) {
+		errno = 0;
+		struct group *grp = getgrnam (class->setgid);
+		if (! grp) {
+			if (errno != 0) {
+				nih_error_raise_system ();
+				job_process_error_abort (fds[1], JOB_PROCESS_ERROR_GETGRNAM, 0);
+			} else {
+				nih_error_raise (JOB_PROCESS_INVALID_SETGID,
+						 JOB_PROCESS_INVALID_SETGID_STR);
+				job_process_error_abort (fds[1], JOB_PROCESS_ERROR_BAD_SETGID, 0);
+			}
+		}
+
+		job_setgid = grp->gr_gid;
+	}
+
+	if (script_fd != -1 &&
+	    (job_setuid != (uid_t) -1 || job_setgid != (gid_t) -1) &&
+	    fchown (script_fd, job_setuid, job_setgid) < 0) {
+		nih_error_raise_system ();
+		job_process_error_abort (fds[1], JOB_PROCESS_ERROR_CHOWN, 0);
+	}
+
+	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);
+	}
+
+	if (job_setuid != (uid_t)-1 && setuid (job_setuid) < 0) {
+		nih_error_raise_system ();
+		job_process_error_abort (fds[1], JOB_PROCESS_ERROR_SETUID, 0);
+	}
+
 	/* Reset all the signal handlers back to their default handling so
 	 * the child isn't unexpectedly ignoring any, and so we won't
 	 * surprisingly handle them before we've exec()d the new process.
@@ -880,6 +944,24 @@
 				  err, _("unable to execute: %s"),
 				  strerror (err->errnum)));
 		break;
+	case JOB_PROCESS_ERROR_GETPWNAM:
+		err->error.message = NIH_MUST (nih_sprintf (
+				  err, _("unable to getpwnam: %s"),
+				  strerror (err->errnum)));
+		break;
+	case JOB_PROCESS_ERROR_GETGRNAM:
+		err->error.message = NIH_MUST (nih_sprintf (
+				  err, _("unable to getgrnam: %s"),
+				  strerror (err->errnum)));
+		break;
+	case JOB_PROCESS_ERROR_BAD_SETUID:
+		err->error.message = NIH_MUST (nih_sprintf (
+				  err, _("unable to find setuid user")));
+		break;
+	case JOB_PROCESS_ERROR_BAD_SETGID:
+		err->error.message = NIH_MUST (nih_sprintf (
+				  err, _("unable to find setgid group")));
+		break;
 	case JOB_PROCESS_ERROR_SETUID:
 		err->error.message = NIH_MUST (nih_sprintf (
 				  err, _("unable to setuid: %s"),
 
=== modified file 'init/job_process.h'
--- init/job_process.h	2011-07-25 14:08:47 +0000
+++ init/job_process.h	2011-12-07 15:52:24 +0000
@@ -57,6 +57,10 @@
 	JOB_PROCESS_ERROR_CHDIR,
 	JOB_PROCESS_ERROR_PTRACE,
 	JOB_PROCESS_ERROR_EXEC,
+	JOB_PROCESS_ERROR_GETPWNAM,
+	JOB_PROCESS_ERROR_GETGRNAM,
+	JOB_PROCESS_ERROR_BAD_SETUID,
+	JOB_PROCESS_ERROR_BAD_SETGID,
 	JOB_PROCESS_ERROR_SETUID,
 	JOB_PROCESS_ERROR_SETGID,
 	JOB_PROCESS_ERROR_CHOWN
 
=== modified file 'init/man/init.5'
--- init/man/init.5	2011-07-25 15:01:24 +0000
+++ init/man/init.5	2011-12-07 15:52:24 +0000
@@ -659,6 +659,32 @@
 .B unlimited
 may be specified for either.
 .\"
+.TP
+.B setuid \fIUSERNAME
+Changes to the user
+.I USERNAME
+before running the job's process.
+
+If this stanza is unspecified, the job will run as root in the case of
+system jobs, and as the user in the case of User Jobs.
+
+Note that System jobs using the setuid stanza are still system jobs,
+and can not be controlled by an unprivileged user, even if the setuid
+stanza specifies that user.
+.\"
+.TP
+.B setgid \fIGROUPNAME
+Changes to the group
+.I GROUPNAME
+before running the job's process.
+
+If this stanza is unspecified, the primary group of the user specified
+in the
+.B setuid
+block is used. If both stanzas are unspecified, the job will run with
+its group ID set to 0 in the case of system jobs, and as the primary
+group of the user in the case of User Jobs.
+.\"
 .SS Override File Handling
 Override files allow a jobs environment to be changed without modifying
 the jobs configuration file. Rules governing override files:
 
=== modified file 'init/parse_job.c'
--- init/parse_job.c	2011-06-06 12:52:08 +0000
+++ init/parse_job.c	2011-12-07 15:52:24 +0000
@@ -209,6 +209,14 @@
 			       const char *file, size_t len,
 			       size_t *pos, size_t *lineno)
 	__attribute__ ((warn_unused_result));
+static int stanza_setuid      (JobClass *class, NihConfigStanza *stanza,
+			       const char *file, size_t len,
+			       size_t *pos, size_t *lineno)
+	__attribute__ ((warn_unused_result));
+static int stanza_setgid      (JobClass *class, NihConfigStanza *stanza,
+			       const char *file, size_t len,
+			       size_t *pos, size_t *lineno)
+	__attribute__ ((warn_unused_result));
 static int stanza_debug       (JobClass *class, NihConfigStanza *stanza,
 			       const char *file, size_t len,
 			       size_t *pos, size_t *lineno)
@@ -253,6 +261,8 @@
 	{ "limit",       (NihConfigHandler)stanza_limit       },
 	{ "chroot",      (NihConfigHandler)stanza_chroot      },
 	{ "chdir",       (NihConfigHandler)stanza_chdir       },
+	{ "setuid",      (NihConfigHandler)stanza_setuid      },
+	{ "setgid",      (NihConfigHandler)stanza_setgid      },
 	{ "debug",       (NihConfigHandler)stanza_debug       },
 	{ "manual",      (NihConfigHandler)stanza_manual      },
 
@@ -2538,3 +2548,77 @@
 
 	return nih_config_skip_comment (file, len, pos, lineno);
 }
+
+/**
+ * stanza_setuid:
+ * @class: job class being parsed,
+ * @stanza: stanza found,
+ * @file: file or string to parse,
+ * @len: length of @file,
+ * @pos: offset within @file,
+ * @lineno: line number.
+ *
+ * Parse a setuid stanza from @file, extracting a single argument
+ * containing a user name.
+ *
+ * Returns: zero on success, negative value on error.
+ **/
+static int
+stanza_setuid (JobClass        *class,
+	       NihConfigStanza *stanza,
+	       const char      *file,
+	       size_t           len,
+	       size_t          *pos,
+	       size_t          *lineno)
+{
+	nih_assert (class != NULL);
+	nih_assert (stanza != NULL);
+	nih_assert (file != NULL);
+	nih_assert (pos != NULL);
+
+	if (class->setuid)
+		nih_unref (class->setuid, class);
+
+	class->setuid = nih_config_next_arg (class, file, len, pos, lineno);
+	if (! class->setuid)
+		return -1;
+
+	return nih_config_skip_comment (file, len, pos, lineno);
+}
+
+/**
+ * stanza_setgid:
+ * @class: job class being parsed,
+ * @stanza: stanza found,
+ * @file: file or string to parse,
+ * @len: length of @file,
+ * @pos: offset within @file,
+ * @lineno: line number.
+ *
+ * Parse a setgid stanza from @file, extracting a single argument
+ * containing a group name.
+ *
+ * Returns: zero on success, negative value on error.
+ **/
+static int
+stanza_setgid (JobClass        *class,
+	       NihConfigStanza *stanza,
+	       const char      *file,
+	       size_t           len,
+	       size_t          *pos,
+	       size_t          *lineno)
+{
+	nih_assert (class != NULL);
+	nih_assert (stanza != NULL);
+	nih_assert (file != NULL);
+	nih_assert (pos != NULL);
+
+	if (class->setgid)
+		nih_unref (class->setgid, class);
+
+	class->setgid = nih_config_next_arg (class, file, len, pos, lineno);
+	if (! class->setgid)
+		return -1;
+
+	return nih_config_skip_comment (file, len, pos, lineno);
+}
 
=== modified file 'init/tests/test_job_class.c'
--- init/tests/test_job_class.c	2011-06-06 17:05:11 +0000
+++ init/tests/test_job_class.c	2011-12-07 15:52:24 +0000
@@ -139,6 +139,10 @@
 
 		TEST_EQ_P (class->chroot, NULL);
 		TEST_EQ_P (class->chdir, NULL);
+
+		TEST_EQ_P (class->setuid, NULL);
+		TEST_EQ_P (class->setgid, NULL);
+
 		TEST_FALSE (class->deleted);
 
 		nih_free (class);
 
=== modified file 'init/tests/test_job_process.c'
--- init/tests/test_job_process.c	2011-06-06 17:05:11 +0000
+++ init/tests/test_job_process.c	2011-12-07 15:52:24 +0000
@@ -38,6 +38,8 @@
 #include <unistd.h>
 #include <utmp.h>
 #include <utmpx.h>
+#include <pwd.h>
+#include <grp.h>
 
 #include <nih/macros.h>
 #include <nih/string.h>
@@ -124,13 +126,15 @@
 void
 test_run (void)
 {
-	JobClass   *class = NULL;
-	Job         *job = NULL;
-	FILE        *output;
-	struct stat  statbuf;
-	char         filename[PATH_MAX], buf[80];
-	int          ret = -1, status, first;
-	siginfo_t    info;
+	JobClass      *class = NULL;
+	Job           *job = NULL;
+	FILE          *output;
+	struct stat    statbuf;
+	char           filename[PATH_MAX], buf[80];
+	int            ret = -1, status, first;
+	siginfo_t      info;
+	struct passwd *pwd;
+	struct group  *grp;
 
 	TEST_FUNCTION ("job_process_run");
 	job_class_init ();
@@ -787,6 +791,44 @@
 
 		nih_free (class);
 	}
+
+	/* Check that we can succesfully setuid and setgid to
+	 * ourselves. This should always work, privileged or
+	 * otherwise.
+	 */
+	TEST_FEATURE ("with setuid me");
+	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 = getpwuid (getuid ());
+			TEST_NE (pwd, NULL);
+			class->setuid = nih_strdup (class, pwd->pw_name);
+
+			grp = getgrgid (getgid ());
+			TEST_NE (grp, NULL);
+			class->setuid = nih_strdup (class, grp->gr_name);
+
+			job = job_new (class, "");
+			job->goal = JOB_START;
+			job->state = JOB_SPAWNED;
+		}
+
+		ret = job_process_run (job, PROCESS_MAIN);
+		TEST_EQ (ret, 0);
+
+		TEST_NE (job->pid[PROCESS_MAIN], 0);
+
+		waitpid (job->pid[PROCESS_MAIN], NULL, 0);
+		TEST_EQ (stat (filename, &statbuf), 0);
+
+		unlink (filename);
+		nih_free (class);
+	}
 }
 
 
 
=== modified file 'init/tests/test_parse_job.c'
--- init/tests/test_parse_job.c	2011-06-06 12:52:08 +0000
+++ init/tests/test_parse_job.c	2011-12-07 15:52:24 +0000
@@ -7933,6 +7933,238 @@
 	nih_free (err);
 }
 
+void
+test_stanza_setuid (void)
+{
+	JobClass*job;
+	NihError *err;
+	size_t    pos, lineno;
+	char      buf[1024];
+
+	TEST_FUNCTION ("stanza_setuid");
+
+	/* Check that a setuid stanza with an argument results in it
+	 * being stored in the job.
+	 */
+	TEST_FEATURE ("with single argument");
+	strcpy (buf, "setuid www-data\n");
+
+	TEST_ALLOC_FAIL {
+		pos = 0;
+		lineno = 1;
+		job = parse_job (NULL, NULL, NULL, "test", buf, strlen (buf),
+				 &pos, &lineno);
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (job, NULL);
+
+			err = nih_error_get ();
+			TEST_EQ (err->number, ENOMEM);
+			nih_free (err);
+
+			continue;
+		}
+
+		TEST_EQ (pos, strlen (buf));
+		TEST_EQ (lineno, 2);
+
+		TEST_ALLOC_SIZE (job, sizeof (JobClass));
+
+		TEST_ALLOC_PARENT (job->setuid, job);
+		TEST_EQ_STR (job->setuid, "www-data");
+
+		nih_free (job);
+	}
+
+
+	/* Check that the last of multiple setuid stanzas is used.
+	 */
+	TEST_FEATURE ("with multiple stanzas");
+	strcpy (buf, "setuid www-data\n");
+	strcat (buf, "setuid pulse\n");
+
+	TEST_ALLOC_FAIL {
+		pos = 0;
+		lineno = 1;
+		job = parse_job (NULL, NULL, NULL, "test", buf, strlen (buf),
+				 &pos, &lineno);
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (job, NULL);
+
+			err = nih_error_get ();
+			TEST_EQ (err->number, ENOMEM);
+			nih_free (err);
+
+			continue;
+		}
+
+		TEST_EQ (pos, strlen (buf));
+		TEST_EQ (lineno, 3);
+
+		TEST_ALLOC_SIZE (job, sizeof (JobClass));
+
+		TEST_ALLOC_PARENT (job->setuid, job);
+		TEST_EQ_STR (job->setuid, "pulse");
+
+		nih_free (job);
+	}
+
+
+	/* Check that a setuid stanza without an argument results in
+	 * a syntax error.
+	 */
+	TEST_FEATURE ("with missing argument");
+	strcpy (buf, "setuid\n");
+
+	pos = 0;
+	lineno = 1;
+	job = parse_job (NULL, NULL, NULL, "test", buf, strlen (buf), &pos, &lineno);
+
+	TEST_EQ_P (job, NULL);
+
+	err = nih_error_get ();
+	TEST_EQ (err->number, NIH_CONFIG_EXPECTED_TOKEN);
+	TEST_EQ (pos, 6);
+	TEST_EQ (lineno, 1);
+	nih_free (err);
+
+
+	/* Check that a setuid stanza with an extra second argument
+	 * results in a syntax error.
+	 */
+	TEST_FEATURE ("with extra argument");
+	strcpy (buf, "setuid www-data foo\n");
+
+	pos = 0;
+	lineno = 1;
+	job = parse_job (NULL, NULL, NULL, "test", buf, strlen (buf), &pos, &lineno);
+
+	TEST_EQ_P (job, NULL);
+
+	err = nih_error_get ();
+	TEST_EQ (err->number, NIH_CONFIG_UNEXPECTED_TOKEN);
+	TEST_EQ (pos, 16);
+	TEST_EQ (lineno, 1);
+	nih_free (err);
+}
+
+void
+test_stanza_setgid (void)
+{
+	JobClass*job;
+	NihError *err;
+	size_t    pos, lineno;
+	char      buf[1024];
+
+	TEST_FUNCTION ("stanza_setgid");
+
+	/* Check that a setgid stanza with an argument results in it
+	 * being stored in the job.
+	 */
+	TEST_FEATURE ("with single argument");
+	strcpy (buf, "setgid kvm\n");
+
+	TEST_ALLOC_FAIL {
+		pos = 0;
+		lineno = 1;
+		job = parse_job (NULL, NULL, NULL, "test", buf, strlen (buf),
+				 &pos, &lineno);
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (job, NULL);
+
+			err = nih_error_get ();
+			TEST_EQ (err->number, ENOMEM);
+			nih_free (err);
+
+			continue;
+		}
+
+		TEST_EQ (pos, strlen (buf));
+		TEST_EQ (lineno, 2);
+
+		TEST_ALLOC_SIZE (job, sizeof (JobClass));
+
+		TEST_ALLOC_PARENT (job->setgid, job);
+		TEST_EQ_STR (job->setgid, "kvm");
+
+		nih_free (job);
+	}
+
+
+	/* Check that the last of multiple setgid stanzas is used.
+	 */
+	TEST_FEATURE ("with multiple stanzas");
+	strcpy (buf, "setgid kvm\n");
+	strcat (buf, "setgid fuse\n");
+
+	TEST_ALLOC_FAIL {
+		pos = 0;
+		lineno = 1;
+		job = parse_job (NULL, NULL, NULL, "test", buf, strlen (buf),
+				 &pos, &lineno);
+
+		if (test_alloc_failed) {
+			TEST_EQ_P (job, NULL);
+
+			err = nih_error_get ();
+			TEST_EQ (err->number, ENOMEM);
+			nih_free (err);
+
+			continue;
+		}
+
+		TEST_EQ (pos, strlen (buf));
+		TEST_EQ (lineno, 3);
+
+		TEST_ALLOC_SIZE (job, sizeof (JobClass));
+
+		TEST_ALLOC_PARENT (job->setgid, job);
+		TEST_EQ_STR (job->setgid, "fuse");
+
+		nih_free (job);
+	}
+
+
+	/* Check that a setgid stanza without an argument results in
+	 * a syntax error.
+	 */
+	TEST_FEATURE ("with missing argument");
+	strcpy (buf, "setgid\n");
+
+	pos = 0;
+	lineno = 1;
+	job = parse_job (NULL, NULL, NULL, "test", buf, strlen (buf), &pos, &lineno);
+
+	TEST_EQ_P (job, NULL);
+
+	err = nih_error_get ();
+	TEST_EQ (err->number, NIH_CONFIG_EXPECTED_TOKEN);
+	TEST_EQ (pos, 6);
+	TEST_EQ (lineno, 1);
+	nih_free (err);
+
+
+	/* Check that a setgid stanza with an extra second argument
+	 * results in a syntax error.
+	 */
+	TEST_FEATURE ("with extra argument");
+	strcpy (buf, "setgid kvm foo\n");
+
+	pos = 0;
+	lineno = 1;
+	job = parse_job (NULL, NULL, NULL, "test", buf, strlen (buf), &pos, &lineno);
+
+	TEST_EQ_P (job, NULL);
+
+	err = nih_error_get ();
+	TEST_EQ (err->number, NIH_CONFIG_UNEXPECTED_TOKEN);
+	TEST_EQ (pos, 11);
+	TEST_EQ (lineno, 1);
+	nih_free (err);
+}
+
 int
 main (int   argc,
       char *argv[])
@@ -7979,6 +8211,8 @@
 	test_stanza_limit ();
 	test_stanza_chroot ();
 	test_stanza_chdir ();
+	test_stanza_setuid ();
+	test_stanza_setgid ();
 
 	return 0;
 }
 
=== modified file 'util/tests/test_user_sessions.sh'
--- util/tests/test_user_sessions.sh	2011-07-25 14:49:17 +0000
+++ util/tests/test_user_sessions.sh	2011-12-07 15:52:24 +0000
@@ -470,6 +470,45 @@
   rm -f "$job_file"
 }
 
+test_user_job_setuid_setgid()
+{
+    group="user job with setuid and setgid me"
+    job_name="setuid_setgid_me_test"
+    script="\
+setuid $(id -un)
+setgid $(id -gn)
+exec true"
+    test_user_job "$group" "$job_name" "$script" no ""
+
+    TEST_GROUP "user job with setuid and setgid root"
+    script="\
+setuid root
+setgid root
+exec true"
+
+    job_name="setuid_setgid_root_test"
+    job_file="${test_dir}/${job_name}.conf"
+    job="${test_dir_suffix}/${job_name}"
+
+    echo "$script" > $job_file
+
+    ensure_job_known "$job" "$job_name"
+
+    TEST_FEATURE "ensure job fails to start as root"
+    cmd="start ${job}"
+    output=$(eval "$cmd")
+    rc=$?
+    TEST_EQ "$cmd" $rc 1
+
+    TEST_FEATURE "ensure 'start' indicates job failure"
+    error=$(echo "$output"|grep failed)
+    TEST_NE "error" "$error" ""
+
+    TEST_FEATURE "ensure 'initctl' does not list job"
+    initctl list|grep -q "^$job stop/waiting" || \
+        TEST_FAILED "job $job_name not listed as stopped"
+}
+
 test_user_jobs()
 {
   test_user_job_binary
@@ -480,6 +519,8 @@
   test_user_job_single_line_script_task
   test_user_job_multi_line_script_task
 
+  test_user_job_setuid_setgid
+
   test_user_emit_events
 }