Repository navigation
Keep a settings file's Unix permissions across a save [patch] - #383
Open
matt-edmondson wants to merge 3 commits into
Open
matt-edmondson wants to merge 3 commits into
matt-edmondson wants to merge 3 commits into
Conversation
A save writes a .tmp and renames it over the settings file, so the file took the .tmp's default mode: a 0600 file holding a token came back 0644. The .tmp now takes the existing file's mode before any content is written. Windows already keeps attributes and ACLs through File.Replace. Fixes #380 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW44rALLJaHHtXa8D2t
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - Whether a save is outstanding is the `HasQueuedSave` flag, set by `QueueSave()` and cleared by a successful `Save()`. It is not `SaveQueuedTime > LastSaveTime`: `DateTime.UtcNow` advances in whole microseconds on macOS, so a queue straight after a save can read the same instant and would be dropped | ||
| - Automatic backup files (`.bk` suffix) with timestamped collision handling | ||
| - Safe write pattern: write `.tmp` and flush it to disk, then `File.Replace` it over the final file (keeping `.bk` until done), or move it into place when there is no final file yet | ||
| - Safe write pattern: write `.tmp` and flush it to disk, then `File.Replace` it over the final file (keeping `.bk` until done), or move it into place when there is no final file yet. On Unix (.NET 8+ builds) the `.tmp` takes the existing file's permissions before any content is written, so a 0600 file stays 0600 (ktsu-dev/AppDataStorage#380) |
Contributor
Author
There was a problem hiding this comment.
Confirmed. The .tmp was created with the default mode and only chmod-ed afterwards, so a descriptor opened in that gap could read everything written later. A stale .tmp was also truncated in place rather than replaced, which kept the same inode.
I fixed the code rather than the docs, in 82957e2:
- A stale
.tmpis deleted first. - The new one is created with
FileMode.CreateNewandUnixCreateModeset to the settings file's mode. If someone else puts a file there in between, the save fails instead of writing to that file. - It is then set exactly with
SetUnixFileMode. That call can only undo what the umask narrowed, never widen the file past the settings file's own mode. - CLAUDE.md now describes this.
New test AReaderHoldingAStaleTempFileNeverSeesTheNewContent: it opens a world-readable stale .tmp, saves, then reads through the held descriptor. The test fails on the previous commit and passes now. The full suite passes 151/151 on Linux / .NET 10.
Since this is a security finding, I'm leaving the thread open for the maintainer to resolve.
Generated by Claude Code
…it after Unix checks permissions at open, so a .tmp created 0644 and tightened afterwards could be opened by another account in between and read through that descriptor later. A stale .tmp is now deleted and the new one created with CreateNew and UnixCreateMode set to the settings file's mode, then set exactly to cover the umask. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW44rALLJaHHtXa8D2t
|
The dispose-on-failure catch around SetUnixFileMode was unreachable in tests and kept new-code coverage under the SonarCloud gate. Setting the mode after the using declaration lets the declaration dispose the stream on failure instead. Behaviour is unchanged: the file is still created with UnixCreateMode and only ever widened up to the settings file's mode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW44rALLJaHHtXa8D2t
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
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 #380
What was wrong
WriteTextwrites the new content to a.tmpand then puts it in place withFile.ReplaceorFile.Move. The.tmpwas created with the process's default mode (0666 less the umask). On Linux and macOS the settings file therefore took that mode on every save, so a file the user had restricted to 0600 because it holds a token came back 0644.Change
WriteDurablynow opens the.tmp, gives it the existing settings file's mode withSetUnixFileMode, and only then writes the content. A secret is never in a file more readable than the one it replaces.CopyUnixFileModedoes nothing on Windows, whereFile.Replacealready keeps attributes and ACLs, and nothing when there is no settings file yet. A brand-new file keeps today's default mode. I did not add a 0600 default for new files, because the issue left that as optional and it would change behaviour.IFileUnix mode APIs in System.IO.Abstractions exist from .NET 8 on. The .NET 7 and netstandard builds keep the old behaviour, and the doc comment says so.Tests
New
UnixFileModeTestsrun against the real file system and only on Linux and macOS (OSConditionplusSupportedOSPlatform):SaveKeepsTheSettingsFilePermissions: 0600, 0640 and 0666 each surviveSave(), and the new content is still written.SavingTwiceKeepsOwnerOnlyANewSettingsFileKeepsTheDefaultMode: a guard that passes either way.With
AppData.csreverted, 4 of the 5 new cases fail. Full suite on Linux / .NET 10: 150/150 passed. A Release build of the library succeeds for all six target frameworks.#381 (a symlinked settings file is replaced on save) comes from the same save path, but this PR does not touch it.
Generated by Claude Code