diff --git a/actions/__init__.py b/actions/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/actions/echo b/actions/echo new file mode 120000 index 000000000..7bbf7ccd5 --- /dev/null +++ b/actions/echo @@ -0,0 +1 @@ +/bin/echo \ No newline at end of file diff --git a/actions/id b/actions/id new file mode 120000 index 000000000..5f8fa7d02 --- /dev/null +++ b/actions/id @@ -0,0 +1 @@ +/usr/bin/id \ No newline at end of file diff --git a/actions/privilegedactions.py b/actions/privilegedactions.py index 37ac23891..e26dfe44a 100644 --- a/actions/privilegedactions.py +++ b/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 diff --git a/actions/privilegedactions_test.py b/actions/privilegedactions_test.py new file mode 100644 index 000000000..2b90b821a --- /dev/null +++ b/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() + diff --git a/tests/test_privelegedactions.py b/tests/test_privelegedactions.py new file mode 100644 index 000000000..dc8cb0e72 --- /dev/null +++ b/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() +