🛡️ Sentinel: [CRITICAL] Fix TOCTOU vulnerability in file permissions#286
🛡️ Sentinel: [CRITICAL] Fix TOCTOU vulnerability in file permissions#286acebytes wants to merge 1 commit into
Conversation
Replaced `FileManager.default.setAttributes` with secure file creation using `open(2)` with `O_CREAT` and strict permissions, or used `open(2)` with `O_NOFOLLOW` followed by `fchmod(2)` to mitigate Time-of-Check to Time-of-Use (TOCTOU) symlink attacks. Co-authored-by: acebytes <2820910+acebytes@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bfc7c2ed7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let fd = open(ptr, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, S_IRUSR | S_IWUSR) | ||
| guard fd >= 0 else { return false } |
There was a problem hiding this comment.
Opening the temp file with O_CREAT | O_TRUNC does not reset permissions on an existing file, so a pre-existing .tmp with broader mode (for example from a prior run or custom writable directory) will keep those broader bits and then be atomically renamed into the live journal. This is a regression from the previous setAttributes(...0600...) step and can expose journal contents to unintended users unless you fchmod after open or use safer exclusive creation logic.
Useful? React with 👍 / 👎.
| let bytesWritten = data.withUnsafeBytes { buffer in | ||
| write(fd, buffer.baseAddress, buffer.count) | ||
| } |
There was a problem hiding this comment.
The new persistence path performs a single write and treats anything short of data.count as failure; on POSIX, write can return short counts or EINTR even for regular files. In those cases this drops a flush and returns false, which can silently lose rollback state under signal-heavy or resource-constrained conditions. The old Data.write path handled full-buffer writes; this should similarly retry until all bytes are written or a non-retryable error occurs.
Useful? React with 👍 / 👎.
1 participant
FileManager.default.setAttributes(which useschmod) to secure sensitive files and directories. This is vulnerable to Time-of-Check to Time-of-Use (TOCTOU) symlink attacks where an attacker can swap the file/directory for a symlink between creation and permission modification.FileManager.default.setAttributeswith secure file creation usingopen(2)withO_CREATand strict permissions, or usedopen(2)withO_NOFOLLOWfollowed byfchmod(2). For socket files, we ensure the parent directory is0700and rely onumask.O_NOFOLLOWandfchmodare used, or that atomic file creation flags are present. Checked that the project builds.PR created automatically by Jules for task 15386413449013675153 started by @acebytes