Do not spawn a child process if setuid fails

supervisor/options.py

@@ -1160,8 +1160,8 @@ class ServerOptions(Options):
         # Drop root privileges if we have them
         if user is None:
             return "No user specified to setuid to!"
-        if os.getuid() != 0:
-            return "Can't drop privilege as nonroot user"
+
+        # get uid for user, which can be a number or username
         try:
             uid = int(user)
         except ValueError:
@@ -1175,6 +1175,19 @@ class ServerOptions(Options):
                 pwrec = pwd.getpwuid(uid)
             except KeyError:
                 return "Can't find uid %r" % uid
+
+        current_uid = os.getuid()
+
+        if current_uid == uid:
+            # do nothing and return successfully if the uid is already the
+            # current one.  this allows a supervisord running as an
+            # unprivileged user "foo" to start a process where the config
+            # has "user=foo" (same user) in it.
+            return
+
+        if current_uid != 0:
+            return "Can't drop privilege as nonroot user"
+
         gid = pwrec[3]
         if hasattr(os, 'setgroups'):
             user = pwrec[0]

supervisor/process.py

@@ -287,14 +287,19 @@ class Subprocess:
             # Presumably it also prevents HUP, etc received by
             # supervisord from being sent to children.
             options.setpgrp()
+
             self._prepare_child_fds()
             # sending to fd 2 will put this output in the stderr log
-            msg = self.set_uid()
-            if msg:
+
+            # set user
+            setuid_msg = self.set_uid()
+            if setuid_msg:
                 uid = self.config.uid
-                s = 'supervisor: error trying to setuid to %s ' % uid
-                options.write(2, s)
-                options.write(2, "(%s)\n" % msg)
+                msg = "couldn't setuid to %s: %s\n" % (uid, setuid_msg)
+                options.write(2, "supervisor: " + msg)
+                return # finally clause will exit the child process
+
+            # set environment
             env = os.environ.copy()
             env['SUPERVISOR_ENABLED'] = '1'
             serverurl = self.config.serverurl
@@ -307,6 +312,8 @@ class Subprocess:
                 env['SUPERVISOR_GROUP_NAME'] = self.group.config.name
             if self.config.environment is not None:
                 env.update(self.config.environment)
+
+            # change directory
             try:
                 cwd = self.config.directory
                 if cwd is not None:
@@ -314,23 +321,30 @@ class Subprocess:
             except OSError, why:
                 code = errno.errorcode.get(why[0], why[0])
                 msg = "couldn't chdir to %s: %s\n" % (cwd, code)
-                options.write(2, msg)
-            else:
-                try:
-                    if self.config.umask is not None:
-                        options.setumask(self.config.umask)
-                    options.execve(filename, argv, env)
-                except OSError, why:
-                    code = errno.errorcode.get(why[0], why[0])
-                    msg = "couldn't exec %s: %s\n" % (argv[0], code)
-                    options.write(2, msg)
-                except:
-                    (file, fun, line), t,v,tbinfo = asyncore.compact_traceback()
-                    error = '%s, %s: file: %s line: %s' % (t, v, file, line)
-                    options.write(2, "couldn't exec %s: %s\n" % (filename,
-                                                                 error))
+                options.write(2, "supervisor: " + msg)
+                return # finally clause will exit the child process
+
+            # set umask, then execve
+            try:
+                if self.config.umask is not None:
+                    options.setumask(self.config.umask)
+                options.execve(filename, argv, env)
+            except OSError, why:
+                code = errno.errorcode.get(why[0], why[0])
+                msg = "couldn't exec %s: %s\n" % (argv[0], code)
+                options.write(2, "supervisor: " + msg)
+            except:
+                (file, fun, line), t,v,tbinfo = asyncore.compact_traceback()
+                error = '%s, %s: file: %s line: %s' % (t, v, file, line)
+                msg = "couldn't exec %s: %s\n" % (filename, error)
+                options.write(2, "supervisor: " + msg)
+
+            # this point should only be reached if execve failed.
+            # the finally clause will exit the child process.
+
         finally:
-            options._exit(127)
+            options.write(2, "supervisor: child process was not spawned\n")
+            options._exit(127) # exit process with code for spawn failure
 
     def stop(self):
         """ Administrative stop """

supervisor/tests/base.py

@@ -48,6 +48,7 @@ class DummyOptions:
         self.written = {}
         self.fds_closed = []
         self._exitcode = None
+        self.execve_called = False
         self.execv_args = None
         self.setuid_msg = None
         self.privsdropped = None
@@ -193,6 +194,7 @@ class DummyOptions:
         self._exitcode = code
 
     def execve(self, filename, argv, environment):
+        self.execve_called = True
         if self.execv_error:
             if self.execv_error == 1:
                 raise OSError(self.execv_error)

supervisor/tests/test_options.py

@@ -1427,6 +1427,24 @@ class ServerOptionsTests(unittest.TestCase):
         self.assertRaises(OverflowError,
                           instance.openhttpservers, supervisord)
 
+    def test_dropPrivileges_user_none(self):
+        instance = self._makeOne()
+        msg = instance.dropPrivileges(None)
+        self.assertEqual(msg, "No user specified to setuid to!")
+
+    @patch('pwd.getpwuid', Mock(return_value=["foo", None, 12, 34]))
+    @patch('os.getuid', Mock(return_value=12))
+    def test_dropPrivileges_nonroot_same_user(self):
+        instance = self._makeOne()
+        msg = instance.dropPrivileges(os.getuid())
+        self.assertEqual(msg, None) # no error if same user
+
+    @patch('pwd.getpwuid', Mock(return_value=["foo", None, 55, 34]))
+    @patch('os.getuid', Mock(return_value=12))
+    def test_dropPrivileges_nonroot_different_user(self):
+        instance = self._makeOne()
+        msg = instance.dropPrivileges(42)
+        self.assertEqual(msg, "Can't drop privilege as nonroot user")
 
 class TestProcessConfig(unittest.TestCase):
     def _getTargetClass(self):

supervisor/tests/test_process.py

@@ -324,16 +324,20 @@ class SubprocessTests(unittest.TestCase):
         self.assertEqual(options.pgrp_set, True)
         self.assertEqual(len(options.duped), 3)
         self.assertEqual(len(options.fds_closed), options.minfds - 3)
-        self.assertEqual(options.written, {})
         self.assertEqual(options.privsdropped, 1)
         self.assertEqual(options.execv_args,
                          ('/good/filename', ['/good/filename']) )
-        self.assertEqual(options._exitcode, 127)
+        self.assertEqual(options.execve_called, True)
+        # if the real execve() succeeds, the code that writes the
+        # "was not spawned" message won't be reached.  this assertion
+        # is to test that no other errors were written.
+        self.assertEqual(options.written,
+             {2: "supervisor: child process was not spawned\n"})
 
     def test_spawn_as_child_setuid_fail(self):
         options = DummyOptions()
         options.forkpid = 0
-        options.setuid_msg = 'screwed'
+        options.setuid_msg = 'failure reason'
         config = DummyPConfig(options, 'good', '/good/filename', uid=1)
         instance = self._makeOne(config)
         result = instance.spawn()
@@ -344,10 +348,10 @@ class SubprocessTests(unittest.TestCase):
         self.assertEqual(len(options.duped), 3)
         self.assertEqual(len(options.fds_closed), options.minfds - 3)
         self.assertEqual(options.written,
-             {2: 'supervisor: error trying to setuid to 1 (screwed)\n'})
+             {2: "supervisor: couldn't setuid to 1: failure reason\n"
+                 "supervisor: child process was not spawned\n"})
         self.assertEqual(options.privsdropped, None)
-        self.assertEqual(options.execv_args,
-                         ('/good/filename', ['/good/filename']) )
+        self.assertEqual(options.execve_called, False)
         self.assertEqual(options._exitcode, 127)
 
     def test_spawn_as_child_cwd_ok(self):
@@ -363,11 +367,15 @@ class SubprocessTests(unittest.TestCase):
         self.assertEqual(options.pgrp_set, True)
         self.assertEqual(len(options.duped), 3)
         self.assertEqual(len(options.fds_closed), options.minfds - 3)
-        self.assertEqual(options.written, {})
         self.assertEqual(options.execv_args,
                          ('/good/filename', ['/good/filename']) )
-        self.assertEqual(options._exitcode, 127)
         self.assertEqual(options.changed_directory, True)
+        self.assertEqual(options.execve_called, True)
+        # if the real execve() succeeds, the code that writes the
+        # "was not spawned" message won't be reached.  this assertion
+        # is to test that no other errors were written.
+        self.assertEqual(options.written,
+             {2: "supervisor: child process was not spawned\n"})
 
     def test_spawn_as_child_sets_umask(self):
         options = DummyOptions()
@@ -376,11 +384,15 @@ class SubprocessTests(unittest.TestCase):
         instance = self._makeOne(config)
         result = instance.spawn()
         self.assertEqual(result, None)
-        self.assertEqual(options.written, {})
         self.assertEqual(options.execv_args,
                          ('/good/filename', ['/good/filename']) )
-        self.assertEqual(options._exitcode, 127)
         self.assertEqual(options.umaskset, 002)
+        self.assertEqual(options.execve_called, True)
+        # if the real execve() succeeds, the code that writes the
+        # "was not spawned" message won't be reached.  this assertion
+        # is to test that no other errors were written.
+        self.assertEqual(options.written,
+             {2: "supervisor: child process was not spawned\n"})
 
     def test_spawn_as_child_cwd_fail(self):
         options = DummyOptions()
@@ -397,10 +409,12 @@ class SubprocessTests(unittest.TestCase):
         self.assertEqual(len(options.duped), 3)
         self.assertEqual(len(options.fds_closed), options.minfds - 3)
         self.assertEqual(options.execv_args, None)
-        self.assertEqual(options.written,
-                         {2: "couldn't chdir to /tmp: ENOENT\n"})
+        out = {2: "supervisor: couldn't chdir to /tmp: ENOENT\n"
+                  "supervisor: child process was not spawned\n"}
+        self.assertEqual(options.written, out)
         self.assertEqual(options._exitcode, 127)
         self.assertEqual(options.changed_directory, False)
+        self.assertEqual(options.execve_called, False)
 
     def test_spawn_as_child_execv_fail_oserror(self):
         options = DummyOptions()
@@ -415,8 +429,9 @@ class SubprocessTests(unittest.TestCase):
         self.assertEqual(options.pgrp_set, True)
         self.assertEqual(len(options.duped), 3)
         self.assertEqual(len(options.fds_closed), options.minfds - 3)
-        self.assertEqual(options.written,
-                         {2: "couldn't exec /good/filename: EPERM\n"})
+        out = {2: "supervisor: couldn't exec /good/filename: EPERM\n"
+                  "supervisor: child process was not spawned\n"}
+        self.assertEqual(options.written, out)
         self.assertEqual(options.privsdropped, None)
         self.assertEqual(options._exitcode, 127)
 
@@ -434,7 +449,8 @@ class SubprocessTests(unittest.TestCase):
         self.assertEqual(len(options.duped), 3)
         self.assertEqual(len(options.fds_closed), options.minfds - 3)
         msg = options.written[2] # dict, 2 is fd #
-        self.failUnless(msg.startswith("couldn't exec /good/filename:"))
+        head = "supervisor: couldn't exec /good/filename:"
+        self.failUnless(msg.startswith(head))
         self.failUnless("exceptions.RuntimeError" in msg)
         self.assertEqual(options.privsdropped, None)
         self.assertEqual(options._exitcode, 127)
@@ -485,11 +501,15 @@ class SubprocessTests(unittest.TestCase):
         self.assertEqual(options.pgrp_set, True)
         self.assertEqual(len(options.duped), 2)
         self.assertEqual(len(options.fds_closed), options.minfds - 3)
-        self.assertEqual(options.written, {})
         self.assertEqual(options.privsdropped, 1)
         self.assertEqual(options.execv_args,
                          ('/good/filename', ['/good/filename']) )
-        self.assertEqual(options._exitcode, 127)
+        self.assertEqual(options.execve_called, True)
+        # if the real execve() succeeds, the code that writes the
+        # "was not spawned" message won't be reached.  this assertion
+        # is to test that no other errors were written.
+        self.assertEqual(options.written,
+             {2: "supervisor: child process was not spawned\n"})
 
     def test_spawn_as_parent(self):
         options = DummyOptions()