-
Notifications
You must be signed in to change notification settings - Fork 0
π‘οΈ Sentinel: [CRITICAL] Fix TOCTOU vulnerability in SysctlJournal #369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -307,14 +307,24 @@ public final class SysctlJournal { | |
| do { | ||
| let data = try PropertyListEncoder().encode(state) | ||
|
|
||
| // Write to temp file (non-atomic β we control the rename ourselves). | ||
| try data.write(to: tmpURL) | ||
| try? FileManager.default.removeItem(at: tmpURL) | ||
|
|
||
| // Set permissions to 0600 (root-only) on temp file before rename. | ||
| try FileManager.default.setAttributes( | ||
| [.posixPermissions: 0o600], | ||
| ofItemAtPath: tmpURL.path | ||
| ) | ||
| let fd = tmpURL.withUnsafeFileSystemRepresentation { pathPtr -> Int32 in | ||
| guard let ptr = pathPtr else { return -1 } | ||
| return open(ptr, O_CREAT | O_WRONLY | O_EXCL | O_CLOEXEC, S_IRUSR | S_IWUSR) | ||
| } | ||
| guard fd >= 0 else { | ||
| logger.error("Failed to securely open temp journal file: errno \(errno, privacy: .public)") | ||
| return false | ||
| } | ||
|
|
||
| let handle = FileHandle(fileDescriptor: fd, closeOnDealloc: true) | ||
| handle.write(data) | ||
| if #available(macOS 10.15.4, *) { | ||
| try? handle.close() | ||
|
Comment on lines
+323
to
+324
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On macOS 14 this branch is always used, and Useful? React with πΒ / π. |
||
| } else { | ||
| handle.closeFile() | ||
| } | ||
|
|
||
| // Atomic rename(2) β atomicity on APFS/HFS+. | ||
| if rename(tmpURL.path, url.path) != 0 { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the temp journal write itself fails, for example because
/var/runis out of space or returns an I/O error, this non-throwingFileHandle.write(_:)path does not report a Swift error to the surroundingcatchthe way the previousData.write(to:)did. That can leaveflushState()unable to returnfalsecleanly forrecord(_:), either aborting the helper or missing the durable-write failure before the sysctl change. Please use the throwingwrite(contentsOf:)API and handle the error like the open/rename failures.Useful? React with πΒ / π.