Skip to content

exec.Runner: child stderr persists to the log file, and both streams are uncapped #512

Description

@cameronsjo

Context

Found during a security review of internal/env's file-containment control, run
ahead of extending it to a second file format (#498). Neither finding is
reachable as a vulnerability today — they are both about what happens when a new
caller arrives — but the extension in #498 is exactly that new caller, so they
are worth closing before something depends on them.

1. runAndWrap persists child stderr to the log file at Error level

internal/exec/exec.go:147 and :218:

slog.Error(failureMsg, "cmd", name, "stderr", msg, "error", err)

Error level survives any configured log level, and config.SetupLogger
(internal/config/config.go:929) can point the handler at a file on disk. The
same stderr also rides CommandError.Error() into fang's rendered output.

Today's Runner callers are tmux, sesh, and brew, so nothing sensitive reaches
it. The hazard is structural: any future caller whose child prints sensitive
material on stderr turns a debug aid into a durable on-disk record of it, with
nothing at the call site to suggest that happened.

exec.SensitiveRunner already exists for precisely this and logs only metadata.
The gap is that Runner is the obvious, ergonomic choice and carries no warning
at the point of use.

Suggested fix: document the sink on the Runner interface itself — one line
naming SensitiveRunner as the seam for a command whose output may carry a
secret. A doc comment is the right altitude here; narrowing Runner would churn
every existing caller for no present gain.

2. Unbounded child output capture

internal/exec/exec.go:141 captures stderr into a strings.Builder with no
ceiling, and stdout via cmd.Output(), likewise uncapped. A child that emits
unbounded output grows the parent's heap until it is killed.

This is measurable rather than hypothetical: sops re-invokes its editor
forever when the editor hands it invalid YAML, and a bounded probe on sops
3.13.3 produced 8.4 MB of stderr across 36,851 editor invocations in about three
minutes before it was killed — still going. A Runner caller driving that would
accumulate all of it.

SensitiveRunner already enforces MaxOutputBytes (64 KiB) and kills on
overflow, so the pattern to copy is in the tree.

Suggested fix: wrap both streams in a capped writer in runAndWrap,
reporting truncation rather than silently discarding. SensitiveRunner's
BoundedOutput carries a completeness flag for the same reason — a caller that
parses output must be able to tell a whole stream from a prefix.

Out of scope here

Two sibling findings from the same review are filed separately because they sit
in internal/env rather than internal/exec.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:forgectlComponent: forgectl binaryarea:securitySubject: security. Severity lives on impact:*impact:lowCosmetic or local inconvenience; obvious workaround; nobody blockedkind:choreUpkeep with no intended behavior change: deps, CI, cleanuplikelihood:lowNeeds unusual preconditions; not expected within the horizon (30 days)

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions