Skip to content

Stop leaking env/stdin in cmd.run OSError messages (#69075)#69076

Open
co-cy wants to merge 1 commit into
saltstack:3006.xfrom
co-cy:security-cmdmod-redact-env-stdin-on-oserror
Open

Stop leaking env/stdin in cmd.run OSError messages (#69075)#69076
co-cy wants to merge 1 commit into
saltstack:3006.xfrom
co-cy:security-cmdmod-redact-env-stdin-on-oserror

Conversation

@co-cy
Copy link
Copy Markdown

@co-cy co-cy commented May 6, 2026

What does this PR do?

When salt.utils.timed_subprocess.TimedProc raises OSError (typically ENOENT because the binary does not exist, but any spawn-time errno triggers it) salt.modules.cmdmod._run builds a CommandExecutionError whose message is meant to help the operator debug. Previously the entire new_kwargs dict was interpolated into that message, leaking two routinely-secret-bearing fields:

  • env — the run environment, set by callers via cmd.run env={'DB_PASSWORD': '...'} or by states that pass credentials through env vars.
  • stdin — the bytes piped to the command, commonly used to feed a password to a CLI like mysql -p.

The resulting CommandExecutionError then leaks through three channels: minion log (where the calling state logs it at error level), master log (in invocation modes that forward errors back), and event-bus return data (so any user with cmd.run access via eauth can read another user's env-passed secrets just by mistyping the binary name).

This PR filters env and stdin out of the dict before formatting. The remaining keys (cwd, shell, executable, timeout, bg, with_communicate, close_fds) are kept — they are debugging context, not user-supplied secrets.

What issues does this PR fix or reference?

Fixes #69075

Previous Behavior

cmd.run /no/such/binary env='{\"DB_PASSWORD\": \"...\"}'CommandExecutionError containing the password value, which then ends up in logs and in API job-return events.

New Behavior

Same cmd.run, same OSError, but the resulting message contains only cwd, shell, executable etc. — no env, no stdin, no credential leak.

Merge requirements satisfied?

Tests written?

Two behavioural tests in tests/pytests/unit/modules/test_cmdmod.py:

  • test_run_oserror_message_does_not_leak_env_secretsenv={\"DB_PASSWORD\": \"...\"} value must not appear in the raised CommandExecutionError.
  • test_run_oserror_message_does_not_leak_stdinstdin=\"...\" payload must not appear in the message.

Both tests fail on the previous implementation and are the regression guard for this change. The pre-existing test_run_no_vt_os_error / test_run_no_vt_io_error tests continue to pass — the error message still ends with the underlying exception text.

Anything else reviewers should know?

  • Public API of cmd.run/cmd.run_all/cmd.retcode is unchanged. Same call signatures, same return shape, same exception type.
  • The audit that prompted this PR also flagged two debug-level secret dumps elsewhere — salt/minion.py:1903 log.debug("Command details %s", data) and salt/proxy/cimc.py:111 log.debug("=== opts %s ===", opts). Those are debug-level (operator has to enable it) and are intentionally left out of this PR to keep it small and focused; they are best handled in their own follow-up PRs.
  • The accompanying issue ([Bug]: cmd.run OSError messages leak env vars and stdin into logs and API return events #69075) writes up the leak channels in detail (CWE-532 / OWASP A09:2021).

When ``salt.utils.timed_subprocess.TimedProc`` raises ``OSError``
(typically ``ENOENT`` when the binary does not exist, but any
spawn-time errno triggers it) ``salt.modules.cmdmod._run`` builds a
``CommandExecutionError`` whose message is meant to help the operator
debug. Previously the entire ``new_kwargs`` dict was interpolated
into that message, leaking two routinely-secret-bearing fields:

* ``env`` -- the run environment, set by callers via
  ``cmd.run env={'DB_PASSWORD': '...'}`` or by states that pass
  credentials through env vars.
* ``stdin`` -- the bytes piped to the command, commonly used to
  feed a password to a CLI like ``mysql -p``.

Both leak channels matter: the resulting ``CommandExecutionError``
ends up in master/minion logs *and* in event-bus return data
visible to the API caller. ENOENT is not a rare condition; it
fires on any typo in a binary path. A typo should not exfiltrate
credentials.

Filter ``env`` and ``stdin`` out of the dict before formatting.
The remaining ``cwd``, ``shell``, ``executable``, ``timeout`` etc.
are still useful debugging context and do not contain user-supplied
secrets.

Two behavioural tests guard each leak channel; both fail on the
previous implementation.

Refs: saltstack#69075
@co-cy co-cy requested a review from a team as a code owner May 6, 2026 19:18
@dwoz dwoz added the test:full Run the full test suite label May 12, 2026
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.

2 participants