Script 'mail_helper' called by obssrc Hello community, here is the log from the commit of package salt for openSUSE:Factory checked in at 2022-03-01 17:03:12 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Comparing /work/SRC/openSUSE:Factory/salt (Old) and /work/SRC/openSUSE:Factory/.salt.new.1958 (New) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Package is "salt" Tue Mar 1 17:03:12 2022 rev:126 rq:958078 version:3004 Changes: -------- --- /work/SRC/openSUSE:Factory/salt/salt.changes 2022-02-11 23:07:25.930609617 +0100 +++ /work/SRC/openSUSE:Factory/.salt.new.1958/salt.changes 2022-03-01 17:03:28.612331991 +0100 @@ -1,0 +2,25 @@ +Mon Feb 28 15:05:32 UTC 2022 - Pablo Su??rez Hern??ndez <pablo.suarezhernan...@suse.com> + +- Fix issues found around pre_flight_script_args + +- Added: + * prevent-shell-injection-via-pre_flight_script_args-4.patch + +------------------------------------------------------------------- +Thu Feb 24 14:06:44 UTC 2022 - Victor Zhestkov <victor.zhest...@suse.com> + +- Add salt-ssh with Salt Bundle support (venv-salt-minion) + (bsc#1182851, bsc#1196432) + +- Added: + * add-salt-ssh-support-with-venv-salt-minion-3004-493.patch + +------------------------------------------------------------------- +Thu Feb 17 15:27:00 UTC 2022 - Pablo Su??rez Hern??ndez <pablo.suarezhernan...@suse.com> + +- Restrict "state.orchestrate_single" to pass a pillar value if it exists (bsc#1194632) + +- Added: + * state.orchestrate_single-does-not-pass-pillar-none-4.patch + +------------------------------------------------------------------- New: ---- add-salt-ssh-support-with-venv-salt-minion-3004-493.patch prevent-shell-injection-via-pre_flight_script_args-4.patch state.orchestrate_single-does-not-pass-pillar-none-4.patch ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Other differences: ------------------ ++++++ salt.spec ++++++ --- /var/tmp/diff_new_pack.yPgP67/_old 2022-03-01 17:03:30.024332366 +0100 +++ /var/tmp/diff_new_pack.yPgP67/_new 2022-03-01 17:03:30.028332367 +0100 @@ -279,6 +279,16 @@ Patch70: add-missing-ansible-module-functions-to-whitelist-in.patch # PATCH-FIX_UPSTREAM https://github.com/saltstack/salt/pull/61256 Patch71: fix-salt-call-event.send-call-with-grains-and-pillar.patch +# PATCH-FIX_UPSTREAM https://github.com/saltstack/salt/pull/61093 +Patch72: state.orchestrate_single-does-not-pass-pillar-none-4.patch + +### SALT-SSH WITH SALT BUNDLE ### +# PATCH-FIX_UPSTREAM: https://github.com/saltstack/salt/pull/61715 (ssh_pre_flight_args) +# PATCH-FIX_OPENSUSE: https://github.com/openSUSE/salt/pull/493 +# PATCH-FIX_OPENSUSE: https://github.com/openSUSE/salt/pull/497 +Patch73: add-salt-ssh-support-with-venv-salt-minion-3004-493.patch +Patch74: prevent-shell-injection-via-pre_flight_script_args-4.patch +############### BuildRoot: %{_tmppath}/%{name}-%{version}-build ++++++ _lastrevision ++++++ --- /var/tmp/diff_new_pack.yPgP67/_old 2022-03-01 17:03:30.092332384 +0100 +++ /var/tmp/diff_new_pack.yPgP67/_new 2022-03-01 17:03:30.096332385 +0100 @@ -1,3 +1,3 @@ -e2c4840e38a6c4ab52bdba40139d9fb461d9b754 +8fe3232b41facbf938d591053c0f457ba6b5e3dc (No newline at EOF) ++++++ add-salt-ssh-support-with-venv-salt-minion-3004-493.patch ++++++ ++++ 841 lines (skipped) ++++++ prevent-shell-injection-via-pre_flight_script_args-4.patch ++++++ >From 24093156ace91a8766eb1f5acbc47eee8e634d8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= <psuarezhernan...@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 ++++++ state.orchestrate_single-does-not-pass-pillar-none-4.patch ++++++ >From d44207dc209894b36f2a2c8af4c81afcd86b4625 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= <psuarezhernan...@suse.com> Date: Wed, 9 Feb 2022 09:01:08 +0000 Subject: [PATCH] state.orchestrate_single does not pass pillar=None (#488) Passing a pillar to state.single can results in state functions not working when they don't accept a pillar keyword argument. One example where this is the case is salt.wait_for_event. When no pillar is provided, it does not need to be passed. Co-authored-by: Alexander Graul <agr...@suse.com> --- changelog/61092.fixed | 3 ++ salt/runners/state.py | 10 ++++-- tests/pytests/unit/runners/test_state.py | 41 ++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 3 deletions(-) create mode 100644 changelog/61092.fixed create mode 100644 tests/pytests/unit/runners/test_state.py diff --git a/changelog/61092.fixed b/changelog/61092.fixed new file mode 100644 index 0000000000..6ca66839c9 --- /dev/null +++ b/changelog/61092.fixed @@ -0,0 +1,3 @@ +state.orchestrate_single only passes a pillar if it is set to the state +function. This allows it to be used with state functions that don't accept a +pillar keyword argument. diff --git a/salt/runners/state.py b/salt/runners/state.py index f8fc1b0944..5642204ce9 100644 --- a/salt/runners/state.py +++ b/salt/runners/state.py @@ -150,12 +150,16 @@ def orchestrate_single(fun, name, test=None, queue=False, pillar=None, **kwargs) salt-run state.orchestrate_single fun=salt.wheel name=key.list_all """ - if pillar is not None and not isinstance(pillar, dict): - raise SaltInvocationError("Pillar data must be formatted as a dictionary") + if pillar is not None: + if isinstance(pillar, dict): + kwargs["pillar"] = pillar + else: + raise SaltInvocationError("Pillar data must be formatted as a dictionary") + __opts__["file_client"] = "local" minion = salt.minion.MasterMinion(__opts__) running = minion.functions["state.single"]( - fun, name, test=None, queue=False, pillar=pillar, **kwargs + fun, name, test=None, queue=False, **kwargs ) ret = {minion.opts["id"]: running} __jid_event__.fire_event({"data": ret, "outputter": "highstate"}, "progress") diff --git a/tests/pytests/unit/runners/test_state.py b/tests/pytests/unit/runners/test_state.py new file mode 100644 index 0000000000..df0a718a41 --- /dev/null +++ b/tests/pytests/unit/runners/test_state.py @@ -0,0 +1,41 @@ +#!/usr/bin/python3 + +import pytest +from salt.runners import state as state_runner +from tests.support.mock import Mock, patch + + +@pytest.fixture +def configure_loader_modules(): + return {state_runner: {"__opts__": {}, "__jid_event__": Mock()}} + + +def test_orchestrate_single_passes_pillar(): + """ + test state.orchestrate_single passes given pillar to state.single + """ + mock_master_minion = Mock() + mock_state_single = Mock() + mock_master_minion.functions = {"state.single": mock_state_single} + mock_master_minion.opts = {"id": "dummy"} + test_pillar = {"test_entry": "exists"} + with patch("salt.minion.MasterMinion", Mock(return_value=mock_master_minion)): + state_runner.orchestrate_single( + fun="pillar.get", name="test_entry", pillar=test_pillar + ) + assert mock_state_single.call_args.kwargs["pillar"] == test_pillar + + +def test_orchestrate_single_does_not_pass_none_pillar(): + """ + test state.orchestrate_single does not pass pillar=None to state.single + """ + mock_master_minion = Mock() + mock_state_single = Mock() + mock_master_minion.functions = {"state.single": mock_state_single} + mock_master_minion.opts = {"id": "dummy"} + with patch("salt.minion.MasterMinion", Mock(return_value=mock_master_minion)): + state_runner.orchestrate_single( + fun="pillar.get", name="test_entry", pillar=None + ) + assert "pillar" not in mock_state_single.call_args.kwargs -- 2.35.1
