Skip to content

Fix run_winexe_command leaking plaintext password in logs#69764

Open
ggiesen wants to merge 2 commits into
saltstack:3006.xfrom
ggiesen:fix-54910
Open

Fix run_winexe_command leaking plaintext password in logs#69764
ggiesen wants to merge 2 commits into
saltstack:3006.xfrom
ggiesen:fix-54910

Conversation

@ggiesen

@ggiesen ggiesen commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Corrects the order of two assignments in run_winexe_command (salt/utils/cloud.py) so the redacted logging_command is built from the original cmd before cmd is reassigned to the full command line. Previously the logging string interpolated the already-built command, which doubled the winexe prefix and embedded the real plaintext password in the logged output. The actually executed command is unchanged; this is a logging/security fix.

What issues does this PR fix or reference?

Fixes #54910

Previous Behavior

run_winexe_command reassigned cmd to the full command line before building logging_cmd, so the "redacted" log line interpolated the already-built command:

[INFO ] Running command under pid 10714: 'winexe -U 'Administrator%XXX-REDACTED-XXX' //1.1.1.1 winexe -U 'Administrator%<plaintext>' //1.1.1.1 sc query winexesvc query winexesvc'

The intended XXX-REDACTED-XXX redaction was defeated: the second, embedded winexe -U fragment carried the real password, and it was written to both the INFO and DEBUG log lines.

New Behavior

The logged command is derived from the original inputs and contains a single, fully redacted credential:

winexe -U 'Administrator%XXX-REDACTED-XXX' //1.1.1.1 sc query winexesvc

The plaintext password no longer appears in any log line. The executed command is identical to before.

Merge requirements satisfied?

  • Docs (n/a - internal utility behaviour, no doc-facing change)
  • Changelog (changelog/54910.fixed.md)
  • Tests written/updated - added to tests/pytests/unit/utils/test_cloud.py:
    • test_run_winexe_command_redacts_password_54910 - asserts the logging_command passed to win_cmd is fully redacted, contains no plaintext password, and has a single winexe -U credential fragment (the regression check).
    • test_run_winexe_command_executes_correct_command_54910 - must-not-regress: the executed command is still correct (passes with and without the fix).
    • test_run_winexe_command_returns_win_cmd_result - the win_cmd return value is propagated to the caller.

[NOTICE] Commits signed with GPG?: No

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 saltstack#54910
@ggiesen ggiesen requested a review from a team as a code owner July 10, 2026 17:55
@charzl charzl self-assigned this Jul 10, 2026
@dwoz dwoz added the test:full Run the full test suite label Jul 10, 2026
dwoz
dwoz previously approved these changes Jul 11, 2026
Comment thread salt/utils/cloud.py
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}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We handled this in the cmd.py module by never logging any arguments. We only ever log the the program name being run.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dwoz - good call, that's cleaner than redacting. Switched run_winexe_command to hand win_cmd just the program name ("winexe") as the logged command, so the arguments - credentials and all - never reach the logs, matching the salt.modules.cmdmod._log_cmd convention. Updated the test to assert only the program name is logged, and dropped the redaction entirely.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants