File prevent-shell-injection-via-pre_flight_script_args-4.patch of Package salt
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
