From 3deed9865140ab194c0b67747cd4fb5ea780af87 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Fri, 10 Jul 2026 13:32:31 -0400 Subject: [PATCH 1/2] Fix winexe logging command leaking plaintext password run_winexe_command reassigned cmd to the full command line before building logging_cmd, so the redacted logging_command interpolated the already-built command. This doubled the winexe prefix and embedded the real password in the INFO and DEBUG log lines despite the XXX-REDACTED-XXX intent. Build the logging command from the original cmd before reassigning cmd; the executed command is unchanged. Fixes #54910 --- changelog/54910.fixed.md | 1 + salt/utils/cloud.py | 2 +- tests/pytests/unit/utils/test_cloud.py | 73 ++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 changelog/54910.fixed.md diff --git a/changelog/54910.fixed.md b/changelog/54910.fixed.md new file mode 100644 index 000000000000..dbec1be61c30 --- /dev/null +++ b/changelog/54910.fixed.md @@ -0,0 +1 @@ +Stop run_winexe_command leaking the plaintext password and doubling the winexe prefix in its logged command diff --git a/salt/utils/cloud.py b/salt/utils/cloud.py index c32bbdbb5346..79d7dddfcfa5 100644 --- a/salt/utils/cloud.py +++ b/salt/utils/cloud.py @@ -979,8 +979,8 @@ def run_winexe_command(cmd, args, host, username, password, port=445): """ creds = f"-U '{username}%{password}' //{host}" logging_creds = f"-U '{username}%XXX-REDACTED-XXX' //{host}" - cmd = f"winexe {creds} {cmd} {args}" logging_cmd = f"winexe {logging_creds} {cmd} {args}" + cmd = f"winexe {creds} {cmd} {args}" return win_cmd(cmd, logging_command=logging_cmd) diff --git a/tests/pytests/unit/utils/test_cloud.py b/tests/pytests/unit/utils/test_cloud.py index f3288dbfef80..a49ecc1c768e 100644 --- a/tests/pytests/unit/utils/test_cloud.py +++ b/tests/pytests/unit/utils/test_cloud.py @@ -810,3 +810,76 @@ def test_userdata_template(): "renderer": "jinja", } assert cloud.userdata_template(opts=opts, vm_={}, userdata="test") == "True" + + +def test_run_winexe_command_redacts_password_54910(): + """ + The logging_command handed to win_cmd must redact the password and must + not double the winexe prefix (issue #54910). Before the fix, cmd was + reassigned to the full command line before logging_command was built, so + the "redacted" string interpolated the already-built command and leaked + the plaintext password behind a second, unredacted winexe -U fragment. + """ + # Production-exact argument shape: wait_for_winexe() calls + # run_winexe_command("sc", "query winexesvc", host, username, password, port) + win_cmd_mock = MagicMock(return_value=0) + with patch.object(cloud, "win_cmd", win_cmd_mock): + cloud.run_winexe_command( + "sc", + "query winexesvc", + "1.1.1.1", + "Administrator", + "s3cr3t-pw", + ) + logging_command = win_cmd_mock.call_args.kwargs["logging_command"] + assert ( + logging_command + == "winexe -U 'Administrator%XXX-REDACTED-XXX' //1.1.1.1 sc query winexesvc" + ) + # The plaintext password must never appear in the logged command. + assert "s3cr3t-pw" not in logging_command + # A single "winexe -U" fragment; the doubling bug embedded a second one + # carrying the real password. (Counting bare "winexe" would false-match + # the "winexesvc" service name in the queried command.) + assert logging_command.count("winexe -U ") == 1 + + +def test_run_winexe_command_executes_correct_command_54910(): + """ + Must-not-regress sibling: the actually executed command was already + correct before the fix and must stay correct. This assertion passes both + with and without the swap, guarding against a change that repairs the + logging string but corrupts the real command line. + """ + win_cmd_mock = MagicMock(return_value=0) + with patch.object(cloud, "win_cmd", win_cmd_mock): + cloud.run_winexe_command( + "sc", + "query winexesvc", + "1.1.1.1", + "Administrator", + "s3cr3t-pw", + ) + executed_command = win_cmd_mock.call_args.args[0] + assert ( + executed_command + == "winexe -U 'Administrator%s3cr3t-pw' //1.1.1.1 sc query winexesvc" + ) + + +def test_run_winexe_command_returns_win_cmd_result(): + """ + run_winexe_command should propagate win_cmd's return value to its caller + unchanged (wait_for_winexe relies on the return code to detect success). + """ + win_cmd_mock = MagicMock(return_value=0) + with patch.object(cloud, "win_cmd", win_cmd_mock): + result = cloud.run_winexe_command( + "sc", + "query winexesvc", + "1.1.1.1", + "Administrator", + "s3cr3t-pw", + ) + assert result == 0 + win_cmd_mock.assert_called_once() From 04d7ea0c13dbbc641e952018b0513182154064f6 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sat, 11 Jul 2026 05:45:28 -0400 Subject: [PATCH 2/2] Log only the program name in run_winexe_command Per review, follow the salt.modules.cmdmod convention of logging only the program name and never the arguments, rather than redacting the password in a fully-rendered command line. This keeps credentials (and any other argument) out of the logs entirely. --- salt/utils/cloud.py | 6 +++--- tests/pytests/unit/utils/test_cloud.py | 27 ++++++++++++-------------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/salt/utils/cloud.py b/salt/utils/cloud.py index 79d7dddfcfa5..e31a9e940c15 100644 --- a/salt/utils/cloud.py +++ b/salt/utils/cloud.py @@ -978,10 +978,10 @@ def run_winexe_command(cmd, args, host, username, password, port=445): Run a command remotely via the winexe executable """ creds = f"-U '{username}%{password}' //{host}" - logging_creds = f"-U '{username}%XXX-REDACTED-XXX' //{host}" - logging_cmd = f"winexe {logging_creds} {cmd} {args}" cmd = f"winexe {creds} {cmd} {args}" - return win_cmd(cmd, logging_command=logging_cmd) + # Only log the program name, never the arguments -- they carry the + # credentials. This mirrors how salt.modules.cmdmod logs commands. + return win_cmd(cmd, logging_command="winexe") def run_psexec_command(cmd, args, host, username, password, port=445): diff --git a/tests/pytests/unit/utils/test_cloud.py b/tests/pytests/unit/utils/test_cloud.py index a49ecc1c768e..df8fc119c0df 100644 --- a/tests/pytests/unit/utils/test_cloud.py +++ b/tests/pytests/unit/utils/test_cloud.py @@ -812,13 +812,13 @@ def test_userdata_template(): assert cloud.userdata_template(opts=opts, vm_={}, userdata="test") == "True" -def test_run_winexe_command_redacts_password_54910(): +def test_run_winexe_command_logs_program_name_only_54910(): """ - The logging_command handed to win_cmd must redact the password and must - not double the winexe prefix (issue #54910). Before the fix, cmd was - reassigned to the full command line before logging_command was built, so - the "redacted" string interpolated the already-built command and leaked - the plaintext password behind a second, unredacted winexe -U fragment. + The logging_command handed to win_cmd must be only the program name, so the + credentials (and every other argument) are never logged (issue #54910). + This mirrors how salt.modules.cmdmod logs commands. Before, cmd was + reassigned to the full credential-bearing command line before the logged + string was built, leaking the plaintext password into the logs. """ # Production-exact argument shape: wait_for_winexe() calls # run_winexe_command("sc", "query winexesvc", host, username, password, port) @@ -832,16 +832,13 @@ def test_run_winexe_command_redacts_password_54910(): "s3cr3t-pw", ) logging_command = win_cmd_mock.call_args.kwargs["logging_command"] - assert ( - logging_command - == "winexe -U 'Administrator%XXX-REDACTED-XXX' //1.1.1.1 sc query winexesvc" - ) - # The plaintext password must never appear in the logged command. + # Only the program name is logged -- no arguments at all. + assert logging_command == "winexe" + # The plaintext password, the username, the host and the remote command + # (all carried as arguments) must be absent from the logged string. assert "s3cr3t-pw" not in logging_command - # A single "winexe -U" fragment; the doubling bug embedded a second one - # carrying the real password. (Counting bare "winexe" would false-match - # the "winexesvc" service name in the queried command.) - assert logging_command.count("winexe -U ") == 1 + assert "Administrator" not in logging_command + assert "query winexesvc" not in logging_command def test_run_winexe_command_executes_correct_command_54910():