From 6571e5ab646f55ba9411f5a668c5224203a08707 Mon Sep 17 00:00:00 2001
From: Anant Patil <anant.patil@hp.com>
Date: Wed, 23 Sep 2015 11:56:05 +0530
Subject: [PATCH] Don't run commands given as list on shell

Commands from AWS::CloudFormation::Init, when supplied as list, should
be run with shell=False. Only when commands are given as string, they
are meant to be run on shell.

In principle, we are trying to give least access to the shell to avoid
any inadvertent shell injections.

Change-Id: I3dc6fe0c29a14f75be044846f737e1ade23a6d6b
Closes-Bug: 1498300
---
 heat_cfntools/cfntools/cfn_helper.py   |  7 ++-----
 heat_cfntools/tests/test_cfn_helper.py | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/heat_cfntools/cfntools/cfn_helper.py b/heat_cfntools/cfntools/cfn_helper.py
index e241e57..c340bda 100644
--- a/heat_cfntools/cfntools/cfn_helper.py
+++ b/heat_cfntools/cfntools/cfn_helper.py
@@ -1100,11 +1100,8 @@ class CommandsHandler(object):
         if "command" in properties:
             try:
                 command = properties["command"]
-                #FIXME bug 1498300
-                if isinstance(command, list):
-                    escape = lambda x: '"%s"' % x.replace('"', '\\"')
-                    command = ' '.join(map(escape, command))
-                command = CommandRunner(command, shell=True)
+                shell = isinstance(command, six.string_types)
+                command = CommandRunner(command, shell=shell)
                 command.run('root', cwd, env)
                 command_status = command.status
             except OSError as e:
diff --git a/heat_cfntools/tests/test_cfn_helper.py b/heat_cfntools/tests/test_cfn_helper.py
index 308ca03..06e6caf 100644
--- a/heat_cfntools/tests/test_cfn_helper.py
+++ b/heat_cfntools/tests/test_cfn_helper.py
@@ -1291,6 +1291,30 @@ class TestCfnInit(testtools.TestCase):
             md.cfn_init()
             mock_popen.assert_has_calls(calls)
 
+    @mock.patch.object(cfn_helper, 'controlled_privileges')
+    def test_cfn_init_runs_list_commands_without_shell(self, mock_cp):
+        calls = []
+        returns = []
+        # command supplied as list shouldn't run on shell
+        calls.extend(popen_root_calls([['/bin/command1', 'arg']], shell=False))
+        returns.append(FakePOpen('Doing something'))
+        # command supplied as string should run on shell
+        calls.extend(popen_root_calls(['/bin/command2'], shell=True))
+        returns.append(FakePOpen('All good'))
+
+        md_data = {"AWS::CloudFormation::Init": {"config": {"commands": {
+            "00_foo": {"command": ["/bin/command1", "arg"]},
+            "01_bar": {"command": "/bin/command2"}
+        }}}}
+
+        with mock.patch('subprocess.Popen') as mock_popen:
+            mock_popen.side_effect = returns
+            md = cfn_helper.Metadata('teststack', None)
+            self.assertTrue(
+                md.retrieve(meta_str=md_data, last_path=self.last_file))
+            md.cfn_init()
+            mock_popen.assert_has_calls(calls)
+
 
 class TestSourcesHandler(testtools.TestCase):
     def test_apply_sources_empty(self):