fix(security): close TOCTOU window when saving MCP OAuth credentials#21176
Merged
Conversation
_write_json (the persistence helper used by HermesTokenStorage for both tokens and client_info) created the temp file via Path.write_text and only chmod'd it to 0o600 afterward. Between create and chmod the file existed on disk at the process umask (commonly 0o644 = world-readable), briefly exposing MCP OAuth access/refresh tokens to other local users. Use os.open with O_WRONLY|O_CREAT|O_EXCL and an explicit S_IRUSR|S_IWUSR mode so the file is created atomically at 0o600, plus tighten the parent dir to 0o700 so siblings can't traverse to the creds file. The temp name also gains a per-process random suffix to avoid collisions between concurrent writers and stale leftovers from a crashed prior write. Mirrors the fix shipped for agent/google_oauth.py in #19673. Adds a regression test asserting the resulting file mode is 0o600 and the parent directory is 0o700 (skipped on Windows where POSIX mode bits aren't enforced).
Contributor
🔎 Lint report:
|
alt-glitch
added
type/security
1 task
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Salvage of #21148 by @Gutslabs onto current main. Cherry-picked clean.
Summary
MCP OAuth token writer (
tools/mcp_oauth.py:_write_json) created the temp file viaPath.write_textandchmodd to 0o600 afterward. Between create and chmod the file existed at the process umask (commonly 0o644), briefly exposing MCP OAuth access/refresh tokens to other local users. Usesos.open(O_EXCL, mode=0o600)+os.fdopen+fsyncnow so the file is atomic at 0o600 on creation. Tightensmcp-tokens/to 0o700 and switches the temp name to a per-process random suffix. Mirrors #19673 foragent/google_oauth.py.Changes
tools/mcp_oauth.py:_write_jsonrewritten aroundos.open/os.fdopen/fsync; parent dir chmod 0o700; random temp suffix.tests/tools/test_mcp_oauth.py: regression asserts final mode is 0o600 and parent dir 0o700 (POSIX-gated).scripts/release.py: AUTHOR_MAP entry for @Gutslabs.Validation
scripts/run_tests.sh tests/tools/test_mcp_oauth*.pyos.fdopento stat the fd targetCloses #21148. Original authorship preserved via rebase-merge.