Rewrote actions/privilegedactions.py to be less exploitable.

I'm sure there are still some exploits in the code, but there are
certainly fewer now.  Instead of just executing whatever arguments are
passed into privilegedactions.privilegedaction_run, we now limit the
actions that can be run in the following ways:

- Only actions that exist in the actions directory can be executed.
  Attempting to run the action "echo; rm -rf /" will look for a file
  named "actions/echo; rm -rf /", of which there are none.

- Shell literals are escaped: attempting to run the "echo" action with
  options like "'hi'; rm -rf /") will echo "'hi'; rm -rf /".

- It is difficult to interact with the spawned process through this
  interface.  We can't control whether the spawned process allows
  interaction.

The details of the contract are included in privilegedactions.py, and
this contract is tested in privilegedactions_test.py.

actions/echo

@@ -0,0 +1 @@
+/bin/echo

actions/id

@@ -0,0 +1 @@
+/usr/bin/id

actions/privilegedactions.py

@@ -1,15 +1,116 @@
-import sys
+#! /usr/bin/env python
+# -*- mode: python; mode: auto-fill; fill-column: 80 -*-
+
+"""Run specified privileged actions as root.
+
+Privileged actions run commands with this contract (version 1.0):
+
+1. (promise) Privileged actions run as root.
+
+2. (promise) The actions directory can't be changed at run time.
+
+   This guarantees that we can only select from the correct set of actions.
+
+3. (restriction) Only whitelisted privileged actions can run.
+
+   A. Scripts in a directory above the actions directory can't be run.
+
+      Arguments (and options) can't coerce the system to run actions in
+      directories above the actions directory.
+
+      Arguments that fail this validation will raise a ValueError.
+
+   B. Scripts in a directory beneath the actions directory can't be run.
+
+      Arguments (and options) can't coerce the system to run actions in
+      sub-directories of the actions directory.
+
+      (An important side-effect of this is that the system will not try to
+      follow symlinks to other action directories.)
+
+      Arguments that fail this validation will raise a ValueError.
+
+   C. Only one action can be called at a time.
+
+      This prevents us from appending multiple (unexpected) actions to the call.
+
+      $ action="echo '$options'; echo 'oops'"
+      $ options="hi"
+      $ $action # oops, the file system is gone!
+
+      Arguments that fail this validation will raise a ValueError.
+
+   D. Options can't be used to run other actions:
+
+      $ action="echo '$options'"
+      $ options="hi'; rm -rf /;'"
+      $ $action # oops, the file system is gone!
+
+      Arguments that fail this validation won't, but probably should, raise a
+      ValueError.  They don't because sanitizing this case is significantly
+      easier than detecting if it occurs.
+
+   E. Actions must exist in the actions directory.
+
+4. (promise) Options are appended to the action.
+
+5. (promise) Output and error strings are returned from the command.
+
+6. (limitation) Providing the process with input is not possible.
+
+   Don't expect to give the process additional input after it's started.  Any
+   interaction with the spawned process must be carried out through some other
+   method (maybe the process opens a socket, or something).
+
+"""
+
+import contract
+import os
+import pipes
+import shlex
 import subprocess
-import cfg
 
-def privilegedaction_run(action, options):
-    cmd = ['sudo', '-n', "/usr/share/plinth/actions/%s" % action]
+contract.checkmod(__name__)
+
+def privilegedaction_run(action, options = None):
+    """Safely run a specific action as root.
+
+    pre:
+        os.sep not in actions
+    inv:
+        True # Actions directory hasn't changed.  It's hardcoded :)
+
+    """
+    DIRECTORY = "actions"
+
+    # contract 3A and 3B: don't call anything outside of the actions directory.
+    if os.sep in action:
+        raise ValueError("Action can't contain:" + os.sep)
+    if not isinstance(options, str):
+        raise ValueError("Options must be a string and cannot be a list.")
+
+    # contract: 3C, 3D: don't allow users to insert escape characters.
+    action = pipes.quote(action)
+    options = pipes.quote(options)
+
+    cmd = DIRECTORY + os.sep + action
+
+    # contract 3C: interpret shell escape sequences as literal file names.
+    # contract 3E: fail if the action doesn't exist or exists elsewhere.
+    if not os.access(cmd, os.F_OK):
+        raise ValueError("Action must exist in action directory.")
+
+    cmd = ["sudo", "-n", cmd]
+
+    # contract 4.
     if options:
-        cmd.extend(options)
-    cfg.log.info('running: %s ' % ' '.join(cmd))
+        cmd.extend(shlex.split(options))
 
+    # contract 3C: don't interpret shell escape sequences.
+    # contract 5 (and 6-ish).
     output, error = \
         subprocess.Popen(cmd,
                          stdout = subprocess.PIPE,
-                         stderr= subprocess.PIPE).communicate()
+                         stderr= subprocess.PIPE,
+                         shell=False).communicate()
     return output, error

actions/privilegedactions_test.py

@@ -0,0 +1,101 @@
+#! /usr/bin/env python
+# -*- mode: python; mode: auto-fill; fill-column: 80 -*-
+
+import sys
+from actions.privilegedactions import privilegedaction_run
+import unittest
+
+class TestPrivileged(unittest.TestCase):
+    """Verify that privileged actions perform as expected:
+
+    1. Privileged actions run as root.
+
+    2. Only whitelisted privileged actions can run.
+
+       A. Actions can't be used to run other actions:
+
+          $ action="echo 'hi'; rm -rf /"
+          $ $action
+
+       B. Options can't be used to run other actions:
+
+          $ options="hi'; rm -rf /;'"
+          $ "echo " + "'$options'"
+
+       C. Scripts in a directory above the actions directory can't be run.
+
+       D. Scripts in a directory beneath the actions directory can't be run.
+
+    3. The actions directory can't be changed at run time.
+
+    """
+    def test_run_as_root(self):
+        """1. Privileged actions run as root.
+
+        """
+        self.assertEqual(
+            "0", # user 0 is root
+            privilegedaction_run("id", "-ur")[0].strip())
+
+    def test_breakout_actions_dir(self):
+        """2. The actions directory can't be changed at run time.
+
+        Can't currently be tested, as the actions directory is hardcoded.
+
+        """
+        pass
+
+    def test_breakout_up(self):
+        """3A. Users can't call actions above the actions directory.
+
+        Tests both a relative and a literal path.
+
+        """
+        options="hi"
+
+        for arg in ("../echo", "/bin/echo"):
+            with self.assertRaises(ValueError):
+                privilegedaction_run(arg, options)
+
+    def test_breakout_down(self):
+        """3B. Users can't call actions beneath the actions directory."""
+        action="directory/echo"
+
+        self.assertRaises(ValueError, privilegedaction_run, action)
+
+    def test_breakout_actions(self):
+        """3C. Actions can't be used to run other actions.
+
+        If multiple actions are specified, bail out.
+
+        """
+        # counting is safer than actual badness.
+        actions = ("echo ''; echo $((1+1))",
+                   "echo '' && echo $((1+1))",
+                   "echo '' || echo $((1+1))")
+        options = ("good", "")
+
+        for action in actions:
+            for option in options:
+                with self.assertRaises(ValueError):
+                    output = privilegedaction_run(action, option)
+
+                    print(output)
+
+                    # if it doesn't error, we'd better not evaluate the data.
+                    self.assertFalse("2" in output[0])
+
+    def test_breakout_options(self):
+        """3D. Options can't be used to run other actions."""
+
+        action = "echo"
+        # counting is safer than actual badness.
+        options = "good; echo $((1+1))"
+
+        output, error = privilegedaction_run(action, options)
+
+        self.assertFalse("2" in output)
+
+if __name__ == "__main__":
+    unittest.main()
+

tests/test_privelegedactions.py

@@ -0,0 +1,120 @@
+#! /usr/bin/env python
+# -*- mode: python; mode: auto-fill; fill-column: 80 -*-
+
+import sys
+from actions.privilegedactions import privilegedaction_run
+import unittest
+
+class TestPrivileged(unittest.TestCase):
+    """Verify that privileged actions perform as expected:
+
+    1. Privileged actions run as root.
+
+    2. Only whitelisted privileged actions can run.
+
+       A. Actions can't be used to run other actions:
+
+          $ action="echo 'hi'; rm -rf /"
+          $ $action
+
+       B. Options can't be used to run other actions:
+
+          $ options="hi'; rm -rf /;'"
+          $ "echo " + "'$options'"
+
+       C. Scripts in a directory above the actions directory can't be run.
+
+       D. Scripts in a directory beneath the actions directory can't be run.
+
+    3. The actions directory can't be changed at run time.
+
+    """
+    def test_run_as_root(self):
+        """1. Privileged actions run as root.
+
+        """
+        self.assertEqual(
+            "0", # user 0 is root
+            privilegedaction_run("id", "-ur")[0].strip())
+
+    def test_breakout_actions_dir(self):
+        """2. The actions directory can't be changed at run time.
+
+        Can't currently be tested, as the actions directory is hardcoded.
+
+        """
+        pass
+
+    def test_breakout_up(self):
+        """3A. Users can't call actions above the actions directory.
+
+        Tests both a relative and a literal path.
+
+        """
+        options="hi"
+
+        for arg in ("../echo", "/bin/echo"):
+            with self.assertRaises(ValueError):
+                privilegedaction_run(arg, options)
+
+    def test_breakout_down(self):
+        """3B. Users can't call actions beneath the actions directory."""
+        action="directory/echo"
+
+        self.assertRaises(ValueError, privilegedaction_run, action)
+
+    def test_breakout_actions(self):
+        """3C. Actions can't be used to run other actions.
+
+        If multiple actions are specified, bail out.
+
+        """
+        # counting is safer than actual badness.
+        actions = ("echo ''; echo $((1+1))",
+                   "echo '' && echo $((1+1))",
+                   "echo '' || echo $((1+1))")
+        options = ("good", "")
+
+        for action in actions:
+            for option in options:
+                with self.assertRaises(ValueError):
+                    output = privilegedaction_run(action, option)
+
+                    # if it somewhow doesn't error, we'd better not evaluate the
+                    # data.
+                    self.assertFalse("2" in output[0])
+
+    def test_breakout_option_string(self):
+        """3D. Options can't be used to run other actions.
+
+        Verify that shell control characters aren't interpreted.
+
+        """
+        action = "echo"
+        # counting is safer than actual badness.
+        options = "good; echo $((1+1))"
+
+        output, error = privilegedaction_run(action, options)
+
+        self.assertFalse("2" in output)
+
+    def test_breakout_option_list(self):
+        """3D. Options can't be used to run other actions.
+
+        Verify that only a string of options is accepted and that we can't just
+        tack additional shell control characters onto the list.
+
+        """
+        action = "echo"
+        # counting is safer than actual badness.
+        options = ["good", ";", "echo $((1+1))"]
+
+        with self.assertRaises(ValueError):
+            output, error = privilegedaction_run(action, options)
+
+            # if it somehow doesn't error, we'd better not evaluate the data.
+            self.assertFalse("2" in output)
+
+if __name__ == "__main__":
+    unittest.main()
+