Skip to content

internal/env residual hardening: temp file on SIGKILL, lone CR in encodeValue, unpinned child binary #513

Description

@cameronsjo

Context

Residual findings from the security review of internal/env's file-containment
control, run ahead of extending it to SOPS files (#498). The review's Critical
(the --any-file confirmation not binding to the written path) and the
lock-file symlink escape are fixed separately. These three are lower severity
and were deliberately left out of that fix so its diff stayed reviewable.

1. writeAtomic leaves a full-secret temp file behind on SIGKILL

internal/env/write.go. Every ordinary error path removes the temp file, but an
uncatchable signal arriving between os.CreateTemp and os.Rename leaves
.env-*.tmp on disk holding the complete new file contents — every key and
value, not just the one being set.

Mitigating facts, both real: the temp file is 0600 from creation, and
.env-abc.tmp does not match IsEnvFileName, so a later --file pointed at it
is refused. A .gitignore pattern of the form .env* does cover it, which is
common but not universal.

Suggested fix: sweep stale .env-*.tmp siblings at the start of the next
set against the same directory, under the lock. A signal handler is the wrong
tool — SIGKILL cannot be caught, which is the case that matters.

2. encodeValue does not escape a lone \r

internal/env/document.go. A value containing \r but no ' and no \n takes
the single-quote branch and is written verbatim. For .env this is harmless:
the line is still one line, and a consumer reading it back gets the \r.

It is worth settling rather than leaving implicit because a raw control
character inside a YAML scalar is a different question, and #498 puts a
second, YAML-shaped writer beside this one. (That writer refuses C0 control
bytes outright rather than escaping them, so it does not share the behaviour —
the point is that the two encoders should differ deliberately, with the reason
written down, not by accident.)

Suggested fix: decide and document. Either escape it, or state in
encodeValue's comment that a bare \r passes through and why that is correct
for .env.

3. Child binaries are resolved through the inherited PATH

internal/exec/exec.go:71 and siblings pass a bare command name to
exec.CommandContext, which resolves it via exec.LookPath against the live
process PATH. Runner's methods also inherit the environment wholesale.

Full environment inheritance is required for some children — a credential tool
has to find its own key material — so the inheritance itself is not the finding.
The resolution is: the binary should be resolved once and pinned, so a PATH
change between resolution and execution cannot substitute a different
executable. exec.SensitiveRunner already requires an absolute path for exactly
this reason (sensitive.go's validate), which is the pattern to follow.

Related: any future self-re-invocation must use os.Executable() and never
argv[0], which the caller controls.

Not filed

One further review observation needed no issue: the --any-file TTY gate is a
partial control that was described in its doc comment as a complete one (an
agent inside a terminal multiplexer pane has a real pty and can answer the
prompt). The comment is corrected in the containment fix rather than tracked
here, since the code was already correct and only the claim about it was wrong.

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