security: secret files are born 0600 instead of chmod-after-write - #380
Open
Saidheerajgollu wants to merge 1 commit into
Open
security: secret files are born 0600 instead of chmod-after-write#380Saidheerajgollu wants to merge 1 commit into
Saidheerajgollu wants to merge 1 commit into
Conversation
secrets.json (all connector/MCP credentials) and the sidecar API token were written with Path.write_text — created umask-wide, typically 0644 — and only restricted to the user afterwards. That leaves a window where the content is world-readable, and no protection at all on the best-effort paths. Open the tmp file with mode 0600 instead so the content is private for its whole lifetime. O_EXCL also stops write_text's symlink-following: a pre-planted symlink at the predictable .tmp name used to receive the secret into its target. A stale tmp left by a crash is removed before the exclusive create.
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
1 participant
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.
SecretStore._write(secrets.json — every connector/MCP credential) andwrite_private_text(the sidecar API token) both write the tmp file withPath.write_textand restrict it to the user afterwards:Two problems with privatizing after the fact:
secrets.json— butwrite_private_texttakes arbitrary paths and its parent-dir restriction is explicitly best-effort (except OSError: pass), so the token file can land readable in a directory the user doesn't own.write_textfollows symlinks. The.tmpname is predictable, so a symlink parked there receives the secret into its target: the content is poured into the victim file, the chmod is applied to the victim, andos.replacethen renames the symlink over the real target. Reproduced on main — the third test below fails there with the victim file containing the secret.The fix opens the tmp file with
os.open(..., O_WRONLY | O_CREAT | O_EXCL, 0o600): private from its first byte, andO_EXCLrefuses to write through anything pre-planted at that name (a stale tmp from a crash is unlinked first; if an attacker re-plants between the unlink and the open, the create fails closed instead of following). The follow-up_restrict_to_usercall stays — on Windows privacy is the ACL, not mode bits, and that path is unchanged.Three regression tests, each red on current main: the two "private even if the restrict step does nothing" tests (mode is 0644 there today under a zeroed umask), and the pre-planted-symlink test. Full suite: 1018 passed.