Skip to content

Audit log-injection risk on user-controlled data (SonarCloud pythonsecurity:S5145, 10 sites) #312

Description

@karoltheguy

SonarCloud reports 10 open pythonsecurity:S5145 findings ("Change this code to not log user-controlled data"), spread across two files:

  • api/sockets.py lines 53, 73, 97, 102, 195, 198, 233, 240, 245
  • services/container_events.py line 40

These log values that arrive from WebSocket query parameters (unit names, container names) and from remote podman output.

Current state: mitigated, but by accident rather than by policy

Most of the flagged sites interpolate with !r, and that conversion is doing real work. repr() escapes control characters, so a crafted name containing a newline cannot forge a second log entry:

plain interpolation -> output spans 2 log lines (forged entry injected)
with !r             -> output spans 1 log line

So the concrete log-forging attack is already blocked wherever !r is used. The problem is that this is load-bearing behavior that nothing documents, nothing tests, and nothing enforces. Anyone dropping the !r while tidying an f-string would silently reintroduce log injection, and no test would notice.

Worth doing

  1. Audit all 10 sites and confirm which actually interpolate untrusted data, and which of those use !r. Not all 10 are necessarily the same shape; the container_events.py one takes remote command output rather than a query parameter.
  2. Decide on one deliberate approach rather than relying on an incidental repr(). Options: keep !r but document why it is required and add a test asserting a newline-bearing name produces a single log line; or sanitize explicitly at the log call; or attach the value as a structured logging field instead of interpolating it.
  3. Whichever is chosen, add a regression test, since the current protection is invisible to a reader.

Context

Noticed while reviewing #311, which consolidated two of these sites (sockets.py:53 and :198) into a single shared _reject_invalid_name() helper, taking the count from 10 to 9. That PR deliberately did not attempt a fix, since it was a pure refactor and this needs its own decision.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions