security: create secret files owner-only from the first byte (#143)#186
Open
Hemanth-Karnati-HK wants to merge 1 commit into
Open
Conversation
…ng#143) SecretStore._write and write_private_text wrote the plaintext temp file with Path.write_text (umask-default 0644) and only then chmod'd it to 0600, leaving a window where other local users could read credentials — and a failed chmod left them exposed indefinitely. Open the temp file with os.open(..., 0o600) instead so it is never readable by anyone else; the icacls-based _restrict_to_user still runs after for Windows ACLs. Regression test neuters the after-the-fact chmod and asserts the file is still 0600.
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.
Fixes #143
SecretStore._write() and write_private_text() wrote the plaintext temp file with Path.write_text(), which creates it with umask-default permissions (typically 0644), and only afterwards chmod'd it to 0600. That leaves a TOCTOU window where other local users can read credentials. If the chmod fails, the unprotected file also stays on disk indefinitely.
Fix: a new _write_user_only() helper opens the temp file with os.open(..., O_WRONLY | O_CREAT | O_TRUNC, 0o600) so it is owner-only from its very first byte. Both call sites now use it. The icacls-based _restrict_to_user() still runs afterwards for the Windows ACL path, since Windows ignores POSIX mode bits.
Testing: added test_secrets_never_world_readable_during_write, which monkeypatches the after-the-fact chmod to a no-op and asserts the file is still 0600. It fails on main (the file lands at 0644) and passes with this change. Full suite: 890 passed, 1 skipped.