🛡️ Sentinel: [CRITICAL] Fix TOCTOU vulnerability in SysctlJournal#369
🛡️ Sentinel: [CRITICAL] Fix TOCTOU vulnerability in SysctlJournal#369acebytes wants to merge 1 commit into
Conversation
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: 6538f05cbf
ℹ️ 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".
| if #available(macOS 10.15.4, *) { | ||
| try? handle.close() |
There was a problem hiding this comment.
On macOS 14 this branch is always used, and try? handle.close() discards delayed write/close errors such as ENOSPC, quota, or I/O failures while flushing the file. flushState() then proceeds to rename the temp file and return true, so record(_:) can allow the sysctl write even though the journal state was not durably written, defeating the rollback guarantee. Please treat a thrown close as a failed flush and remove the temp file instead of committing it.
Useful? React with 👍 / 👎.
| let handle = FileHandle(fileDescriptor: fd, closeOnDealloc: true) | ||
| handle.write(data) |
There was a problem hiding this comment.
When the temp journal write itself fails, for example because /var/run is out of space or returns an I/O error, this non-throwing FileHandle.write(_:) path does not report a Swift error to the surrounding catch the way the previous Data.write(to:) did. That can leave flushState() unable to return false cleanly for record(_:), either aborting the helper or missing the durable-write failure before the sysctl change. Please use the throwing write(contentsOf:) API and handle the error like the open/rename failures.
Useful? React with 👍 / 👎.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: TOCTOU window during journal file creation where sensitive system metrics are briefly exposed with default process umask before permissions are hardened.
🎯 Impact: Attackers could open the temporary file before permissions are restricted, allowing them to read sensitive sysctl information or modify the journal to tamper with system state recovery.
🔧 Fix: Used POSIX
open()withO_EXCL | O_CREATand mode0600to securely create the temp file with root-only permissions immediately, followed by wrapping it inFileHandlefor writing.✅ Verification: Ensure the
.tmpfile is correctly created with0600permissions without a TOCTOU gap, and journal flushes continue to persist data atomically.PR created automatically by Jules for task 2862080380349742536 started by @acebytes