File prevent-shell-injection-via-pre_flight_script_a... of Package venv-salt-minion

Overview Repositories Revisions Requests Users Attributes Meta

File prevent-shell-injection-via-pre_flight_script_args-4.patch of Package venv-salt-minion (Revision bb58e062a4c1ab4912931eee49f3c71e)

Currently displaying revision bb58e062a4c1ab4912931eee49f3c71e , Show latest

From 24093156ace91a8766eb1f5acbc47eee8e634d8e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?=
 <psuarezhernandez@suse.com>
Date: Mon, 28 Feb 2022 14:25:43 +0000
Subject: [PATCH] Prevent shell injection via pre_flight_script_args
 (#497)

Add tests around preflight script args

Readjust logic to validate script args

Use RLock to prevent issues in single threads
---
 salt/_logging/impl.py                    |  2 +-
 salt/client/ssh/__init__.py              |  9 ++--
 tests/integration/ssh/test_pre_flight.py | 56 ++++++++++++++++++++++--
 tests/unit/client/test_ssh.py            | 35 +++++++++++++++
 4 files changed, 92 insertions(+), 10 deletions(-)

diff --git a/salt/_logging/impl.py b/salt/_logging/impl.py
index 953490b284..4f48672032 100644
--- a/salt/_logging/impl.py
+++ b/salt/_logging/impl.py
@@ -92,7 +92,7 @@ MODNAME_PATTERN = re.compile(r"(?P<name>%%\(name\)(?:\-(?P<digits>[\d]+))?s)")
 
 # LOG_LOCK is used to prevent deadlocks on using logging
 # in combination with multiprocessing with salt-api
-LOG_LOCK = threading.Lock()
+LOG_LOCK = threading.RLock()
 
 
 # ----- REMOVE ME ON REFACTOR COMPLETE ------------------------------------------------------------------------------>
diff --git a/salt/client/ssh/__init__.py b/salt/client/ssh/__init__.py
index 0066f4597b..3e032c7197 100644
--- a/salt/client/ssh/__init__.py
+++ b/salt/client/ssh/__init__.py
@@ -15,6 +15,7 @@ import os
 import psutil
 import queue
 import re
+import shlex
 import subprocess
 import sys
 import tarfile
@@ -1458,11 +1459,9 @@ ARGS = {arguments}\n'''.format(
         """
         args = ""
         if script_args:
-            args = " {}".format(
-                " ".join([str(el) for el in script_args])
-                if isinstance(script_args, (list, tuple))
-                else script_args
-            )
+            if not isinstance(script_args, (list, tuple)):
+                script_args = shlex.split(str(script_args))
+            args = " {}".format(" ".join([shlex.quote(str(el)) for el in script_args]))
         if extension == "ps1":
             ret = self.shell.exec_cmd('"powershell {}"'.format(script))
         else:
diff --git a/tests/integration/ssh/test_pre_flight.py b/tests/integration/ssh/test_pre_flight.py
index 6233ec0fe7..9c39219e9d 100644
--- a/tests/integration/ssh/test_pre_flight.py
+++ b/tests/integration/ssh/test_pre_flight.py
@@ -25,10 +25,14 @@ class SSHPreFlightTest(SSHCase):
             RUNTIME_VARS.TMP, "test-pre-flight-script-worked.txt"
         )
 
-    def _create_roster(self):
-        self.custom_roster(self.roster, self.data)
+    def _create_roster(self, pre_flight_script_args=None):
+        data = dict(self.data)
+        if pre_flight_script_args:
+            data["ssh_pre_flight_args"] = pre_flight_script_args
 
-        with salt.utils.files.fopen(self.data["ssh_pre_flight"], "w") as fp_:
+        self.custom_roster(self.roster, data)
+
+        with salt.utils.files.fopen(data["ssh_pre_flight"], "w") as fp_:
             fp_.write("touch {}".format(self.test_script))
 
     @pytest.mark.slow_test
@@ -58,6 +62,45 @@ class SSHPreFlightTest(SSHCase):
         )
         assert os.path.exists(self.test_script)
 
+    @pytest.mark.slow_test
+    def test_ssh_run_pre_flight_args(self):
+        """
+        test ssh when --pre-flight is passed to salt-ssh
+        to ensure the script runs successfully passing some args
+        """
+        self._create_roster(pre_flight_script_args="foobar test")
+        # make sure we previously ran a command so the thin dir exists
+        self.run_function("test.ping", wipe=False)
+        assert not os.path.exists(self.test_script)
+
+        assert self.run_function(
+            "test.ping", ssh_opts="--pre-flight", roster_file=self.roster, wipe=False
+        )
+        assert os.path.exists(self.test_script)
+
+    @pytest.mark.slow_test
+    def test_ssh_run_pre_flight_args_prevent_injection(self):
+        """
+        test ssh when --pre-flight is passed to salt-ssh
+        and evil arguments are used in order to produce shell injection
+        """
+        injected_file = os.path.join(RUNTIME_VARS.TMP, "injection")
+        self._create_roster(
+            pre_flight_script_args="foobar; echo injected > {}".format(injected_file)
+        )
+        # make sure we previously ran a command so the thin dir exists
+        self.run_function("test.ping", wipe=False)
+        assert not os.path.exists(self.test_script)
+        assert not os.path.isfile(injected_file)
+
+        assert self.run_function(
+            "test.ping", ssh_opts="--pre-flight", roster_file=self.roster, wipe=False
+        )
+
+        assert not os.path.isfile(
+            injected_file
+        ), "File injection suceeded. This shouldn't happend"
+
     @pytest.mark.slow_test
     def test_ssh_run_pre_flight_failure(self):
         """
@@ -77,7 +120,12 @@ class SSHPreFlightTest(SSHCase):
         """
         make sure to clean up any old ssh directories
         """
-        files = [self.roster, self.data["ssh_pre_flight"], self.test_script]
+        files = [
+            self.roster,
+            self.data["ssh_pre_flight"],
+            self.test_script,
+            os.path.join(RUNTIME_VARS.TMP, "injection"),
+        ]
         for fp_ in files:
             if os.path.exists(fp_):
                 os.remove(fp_)
diff --git a/tests/unit/client/test_ssh.py b/tests/unit/client/test_ssh.py
index 6f3d87d493..23cb3d0700 100644
--- a/tests/unit/client/test_ssh.py
+++ b/tests/unit/client/test_ssh.py
@@ -234,6 +234,41 @@ class SSHSingleTests(TestCase):
             mock_flight.assert_called()
             assert ret == cmd_ret
 
+    def test_run_with_pre_flight_with_args(self):
+        """
+        test Single.run() when ssh_pre_flight is set
+        and script successfully runs
+        """
+        target = self.target.copy()
+        target["ssh_pre_flight"] = os.path.join(RUNTIME_VARS.TMP, "script.sh")
+        target["ssh_pre_flight_args"] = "foobar"
+        single = ssh.Single(
+            self.opts,
+            self.opts["argv"],
+            "localhost",
+            mods={},
+            fsclient=None,
+            thin=salt.utils.thin.thin_path(self.opts["cachedir"]),
+            mine=False,
+            **target
+        )
+
+        cmd_ret = ("Success", "foobar", 0)
+        mock_flight = MagicMock(return_value=cmd_ret)
+        mock_cmd = MagicMock(return_value=cmd_ret)
+        patch_flight = patch("salt.client.ssh.Single.run_ssh_pre_flight", mock_flight)
+        patch_cmd = patch("salt.client.ssh.Single.cmd_block", mock_cmd)
+        patch_exec_cmd = patch(
+            "salt.client.ssh.shell.Shell.exec_cmd", return_value=("", "", 1)
+        )
+        patch_os = patch("os.path.exists", side_effect=[True])
+
+        with patch_os, patch_flight, patch_cmd, patch_exec_cmd:
+            ret = single.run()
+            mock_cmd.assert_called()
+            mock_flight.assert_called()
+            assert ret == cmd_ret
+
     def test_run_with_pre_flight_stderr(self):
         """
         test Single.run() when ssh_pre_flight is set
-- 
2.35.1

Places

File prevent-shell-injection-via-pre_flight_script_args-4.patch of Package venv-salt-minion (Revision bb58e062a4c1ab4912931eee49f3c71e)

Places