show logging: remove shell command injection risk

[pkawatkar14]

Problem

show logging built a shell pipeline string and executed it with shell=True:

cmd += " | grep '{}'".format(process)
run_command(cmd, display_cmd=verbose, shell=True)

The process argument (and, for --lines, a tail stage) came from user input and was interpolated directly into a shell command string. A value containing shell metacharacters (;, |, backticks, $(), quotes, newlines) could alter the command structure that actually executes, i.e. a command injection vulnerability. Tracked as Microsoft ADO work item 39463623.

What I did

Removed shell execution from logging() in show/main.py. All commands are now built as argv lists and run with shell=False, so user input is always passed as data, never as shell syntax.

How I did it

• No filter, no --lines: run_command(['sudo', 'cat', <log_files>], ...) — unchanged privilege posture (root reads the log files, nothing else runs).
• With a process filter and/or --lines: sudo cat <log_files> is always the first pipeline stage; grep -h -- process and/or tail -n N are chained after it via subprocess.Popen, running unprivileged (as the invoking user), the same as the old sudo cat ... | grep ... | tail ... shell form. Only cat needs root, to read the log files.
  • -h keeps output filename-free across multiple files, preserving the previous cat ... | grep ... output shape.
  • The -- separator stops a process value starting with - from being parsed as a grep option.
  • Each stage's stdout feeds the next stage's stdin directly via a pipe; the final stage inherits the real stdout, so output streams straight through the kernel pipe instead of being buffered in Python. Closing the upstream stdout handle in the parent after starting the next stage lets SIGPIPE propagate normally if a downstream stage exits early.
• --follow (-f): unchanged — ['sudo', 'tail', '-F', <current_log>], already shell=False.
• /var/log vs /var/log.tmpfs selection and rotated syslog.1 detection logic are unchanged.

Justification for the exit-code change: previously, show logging PROCESS -l N ran as a single bash pipeline (cat | grep | tail) via shell=True. Bash pipelines report only the last command's exit status by default, so a grep failure/no-match (exit 1) was silently masked by tail's near-always-0 exit code. Since removing the shell also removes this implicit $? propagation, the new implementation explicitly checks every pipeline stage's exit code and returns the last non-zero one it finds, so failures are surfaced instead of silently reported as success. This is a deliberate, intentional behavior improvement, not a regression.

No shlex.quote() escaping was used — quoting a shell string is fragile and easy to get wrong; removing the shell entirely eliminates the injection vector rather than mitigating it.

Review feedback addressed

• qiluo-msft: noted that the original --lines fix used getstatusoutput_noshell_pipe(), which internally calls .communicate() and therefore buffers the entire pipeline's output before printing it — a streaming regression versus the old shell=True path (imperceptible for a small -l N, but delays the first line for a large N). Addressed by chaining subprocess.Popen directly: each stage's stdin reads from the previous stage's stdout pipe and the last stage inherits the real stdout, restoring line-by-line streaming while keeping the argv-list (no-shell) posture. getstatusoutput_noshell_pipe() remains imported/used elsewhere in this file, unaffected.
• qiluo-msft: noted a privilege-boundary shift — with a process filter, sudo had been scoped to the whole grep invocation, so the user-controlled regex ran as root instead of as the invoking user (as it did under the old sudo cat ... | grep ... shell form). A pathological/expensive regex would therefore burn root cycles. Fixed by always running sudo cat as the first, and only privileged, pipeline stage, with grep/tail chained after it unprivileged — restoring the original privilege boundary while keeping the argv-list (no-shell) posture.
• qiluo-msft: noted that this direct Popen pipeline bypasses run_command() → run_command_in_alias_mode(), so under interface_naming_mode = alias, show logging -l N (and, with this fix, any invocation that filters by process and/or lines) no longer substitutes port names with configured aliases in the log text, while plain show logging (no filter, no -l) still does. This is a real, intentional behavior difference from the pre-existing alias mode, called out below rather than restored, since doing substring alias-rewrites on raw, unstructured log lines is fragile and error-prone in general.

How to verify it

cd src/sonic-utilities
python3 -m pytest tests/show_test.py -v
python3 -m compileall -q show tests

Test cases added/updated (tests/show_test.py)

• All log path variants: /var/log and /var/log.tmpfs, each with and without rotated syslog.1, for no-filter, filter, --lines, and --follow
• --follow ignores other supplied options (process, -l) as before
• Process filter alone, --lines alone, and process filter + --lines together, each asserting the exact argv and stdin/stdout wiring passed to every subprocess.Popen stage (sudo cat always first and unprivileged grep/tail chained after it)
• Verbose command display for the piped path(s)
• Nonzero exit-code propagation when any pipeline stage fails (cat, grep no-match, or tail itself failing)
• Existing Click rejection of a non-numeric --lines value
• A process value starting with - (e.g. -rf /), verifying it is placed after grep's -- separator, not read as an option, and that cat (not grep) is the privileged stage
• Injection-style process values — quotes, ;, |, `, $(), newlines, spaces — for both the process-only and --lines Popen pipelines, asserting each stays a single argv element and is never shell-interpreted

Previous command output (if the output of a command-line utility has changed)

show logging, show logging PROCESS, show logging -l N, show logging -f — output unchanged under the default interface_naming_mode = native.

New command output (if the output of a command-line utility has changed)

Known behavior change under interface_naming_mode = alias: show logging PROCESS and/or show logging -l N no longer substitute port names with their configured aliases in the emitted log text (they now go through a direct, unprivileged Popen pipeline instead of run_command()/run_command_in_alias_mode()). Plain show logging (no process filter, no -l) and show logging -f are unaffected and still substitute aliases as before.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

🔵 Needs a closer look

Address negative --lines semantics, duplicate newline output, and missing alias conversion in the limited-output path.

Pull request overview

This PR removes shell command injection risk from show logging by using shell-free argv execution.

Changes:

• Replaces shell pipelines with safe argv-based commands.
• Adds non-shell piping and exit-status handling for --lines.
• Expands logging and injection-regression tests.

File summaries

File	Summary
show/main.py	Implements shell-free logging commands and pipeline handling.
tests/show_test.py	Adds comprehensive logging and security regression coverage.

Review details

Suppressed comments (3)

show/main.py:1735

• --lines still accepts negative integers because the Click option uses type=int. Before this change, tail -5 meant the last five lines, whereas tail -n -5 means all but the last five, so this silently changes behavior for an input the CLI currently accepts. Please either preserve the old semantics (for example, normalize the value) or reject negative values explicitly.

    tail_cmd = ["tail", "-n", str(lines)]

show/main.py:1741

• getstatusoutput_noshell_pipe() returns the captured output including the newline normally emitted by tail, but click.echo(output) appends another newline. As a result, normal show logging -l N invocations gain an extra blank line compared with the existing run_command() path, which strips each line ending before echoing. Emit the captured output without adding a newline.

        click.echo(output)

show/main.py:1741

• The --lines path calls the pipe helper directly, bypassing the alias-mode handling that run_command() applies at show/main.py:133-135. When interface naming mode is alias, show logging and the no---lines filter path rewrite interface names in log output, but show logging -l N does not, so the same command produces inconsistent output depending on whether a line limit is supplied. Route this pipeline output through the same alias conversion logic before printing it.

    exitcodes, output = getstatusoutput_noshell_pipe(cmd, tail_cmd)
    if output:
        click.echo(output)

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mssonicbld]

Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks!

_{---Powered by SONiC BuildBot}

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[pkawatkar14]

Pretest Static Analysis build failure fixed:
Root cause: the "Pretest Static Analysis" check runs a diff-scoped  flake8  (via pre-commit) on lines touched by the PR. My previous commit left an extra blank line in  tests/show_test.py  (3 blank lines instead of ≤2) right before  test_show_logging_process_argv_integrity , triggering  E303: too many blank lines (3)  at line 257.

Fix: removed the extra blank line and verified with the exact pre-commit invocation ( flake8==4.0.1 , diff-scoped). Pushed as a follow-up commit —  Pretest Static Analysis  now passes, along with DCO and EasyCLA. The remaining checks ( Analyze (python) ,  Azure.sonic-utilities Build Python3 ) are still running.