Skip to content

fix(tools): keep shell snapshots owner-only - #57386

Closed
egilewski wants to merge 1 commit into
NousResearch:mainfrom
egilewski:codex/salvage-20056
Closed

fix(tools): keep shell snapshots owner-only#57386
egilewski wants to merge 1 commit into
NousResearch:mainfrom
egilewski:codex/salvage-20056

Conversation

@egilewski

Copy link
Copy Markdown
Contributor

Summary

Salvages the security-relevant subset of #20056 by making Hermes-owned shell snapshot and cwd metadata writes owner-only.

BaseEnvironment snapshots exported environment state before and after commands. Those files can include env-carried credentials, but current origin/main still writes them through the process umask, so a common 022 umask can leave them world-readable. This patch sets umask 077 only around Hermes metadata writes while preserving the user's command umask.

This intentionally does not salvage the source PR's global /tmp orphan cleanup because deleting all matching snapshot files could interfere with concurrent Hermes processes.

Source and Attribution

Policy and Validation

  • Read SECURITY.md and kept the change scoped to a local credential-disclosure hardening path.
  • Read CONTRIBUTING.md and kept the PR focused, with tests for the behavior change.
  • Confirmed current origin/main at 80a774f972b1b4dd127b87ccf3ff0b68bf3b356d still has the unprotected export -p snapshot writes in tools/environments/base.py.
  • Confirmed the files involved in this fix did not change between the validated base 048270fa069ff6aa41c01b403ac1eeab34b29628 and current origin/main.
  • Ran CodeRabbit on the committed diff: no findings.
  • Ran env HOME=/home/mac scripts/run_tests.sh -j 6 tests/tools/test_base_environment.py: 29 passed.
  • Ran env HOME=/home/mac scripts/run_tests.sh -j 6 --file-timeout 900: 38,041 passed.

Agent Disclosure

Model: GPT-5.5-xhigh

Harness: Codex

Human looked at and manually signed the commit.

The account owner loosely reviews my actions and receives the usual notifications from GitHub.

BaseEnvironment writes shell snapshots and cwd metadata through the process
umask. With a common 022 umask, snapshot files containing exported environment
state landed at mode 0644 even though they can include env-carried credentials
from the parent process.

Set umask 077 only around Hermes metadata writes: the initial snapshot
bootstrap and the post-command snapshot/cwd refresh. User commands still run
under the caller's original umask, while Hermes-owned snapshot and cwd files
are created owner-only.

This intentionally does not copy the source PR's global orphan sweep; deleting
all matching /tmp snapshot files could interfere with concurrent Hermes
processes. The security-critical local disclosure fix is the file mode clamp.

This is salvageable because the source report still identifies a concrete
credential-disclosure path, but the safe subset is smaller than the original
proposal: clamp only the Hermes-owned snapshot writes and leave process-wide
cleanup, user command umask, and concurrent sessions alone.

Salvages source PR: NousResearch#20056
Related issue: NousResearch#48441

Co-authored-by: Andrew Homeyer <andrew@hndl.app>
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround tool/terminal Terminal execution and process management area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 2, 2026
@teknium1

teknium1 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Closing without merge. The code is clean and the tests are good, but this is a resubmission of a fix the maintainers have already declined twice: PR #41278 proposed the identical umask 077 change in the same three write sites and was closed as won't-fix, and the underlying threat-model decision was reaffirmed on #48441 — snapshot files in the per-user temp dir on a machine where other OS users are hostile is considered out of scope for the personal-assistant trust model (containment belongs to OS users/permissions and sandboxed backends, not per-file hardening).

If that threat-model call is ever reversed, this commit is the cherry-pick candidate. Thanks for the careful work regardless — your other PR in this batch (#57573) merged via #57660.

@teknium1 teknium1 closed this Jul 3, 2026
@egilewski

Copy link
Copy Markdown
Contributor Author

Closing without merge. The code is clean and the tests are good, but this is a resubmission of a fix the maintainers have already declined twice

Sorry, I prepared this PR a couple of days ago, but hold it back until I made sure the original wasn't closed as a wontfix. And then posted not making sure the issue I mention there wasn't already closed as wontfix. 🤦

I'm improving my workflow to prevent that going forward. And hopefully to catch pre-existing PRs like #41278 too.

@teknium1

teknium1 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Following up: the threat-model decision was reversed and your commit was cherry-picked onto main exactly as anticipated ("this commit is the cherry-pick candidate") — merged via PR #60199 with your authorship and the #20056 co-author trailer preserved. Thanks for keeping the patch clean and re-validated; it made the reversal a 15-minute salvage.

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

Labels

area/auth Authentication, OAuth, credential pools P1 High — major feature broken, no workaround sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/terminal Terminal execution and process management type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants