Security hardening: Phase 1 socket + install robustness (#28) + review fixes - #38
Merged
Conversation
handleClient's blocking read() on the serial accept loop had no timeout, so a connect-and-hang client wedged the daemon for every other client indefinitely. Set a 5s SO_RCVTIMEO/SO_SNDTIMEO on the client fd (mirrors DaemonClient.sendRaw, v0.1.7); on timeout read()/write() return -1/EAGAIN and the existing guard n>0 closes the fd. No path/plist/protocol change (Phase 0).
…ase 1) Move the control socket from world-writable /tmp to root-owned /var/run (squatting structurally impossible); bind under a 0077 umask so it's 0700 from birth (no bind->chmod race); chown to the installing user + chmod 0600 — only that user and root connect. Daemon takes --owner-uid from the plist (Install.run reads SUDO_UID; absent/0 -> ValidationError, never a root-only socket that bricks the app) and refuses to start on uid 0. Migration: install unlinks legacy /tmp; uninstall removes both paths. Install also restarts the controlling user's app after an upgrade (captured non-destructively at install start) so it reloads the new /var/run path instead of the misleading daemon-down banner — non-fatal, never fails the install. Rollback (install-touching): git checkout main && git branch -D security-hardening-phase1, then ./setup.sh from main (rebuilds b92bf07, the /tmp-socket daemon) to restore. sudo thermalforge auto works via direct SMC regardless if fans stick. Daemon self-binds either way — no launchd/binary listener disagreement.
…rew keg Install.run() took the first EXISTING candidate bundle regardless of version, so a leftover Homebrew 0.1.x keg (opt symlink still points at it) got copied over /Applications on a direct 'sudo thermalforge install'. Now it accepts a candidate only if its CFBundleShortVersionString matches ThermalForgeVersion.current, else leaves /Applications untouched (from-source build-app assembles the correct bundle right after; a correct existing bundle is preserved). Else branch now prints each candidate's version so a miss is diagnosable.
Install.run() removed installPath before copying the daemon binary. When the binary being run IS the one at installPath (the natural sudo invocation once on PATH), that deleted both the running binary and the copy's own source, aborting mid-install: nothing on PATH, emergency reset gone, launchd pointing at a deleted inode. Split the fix into two concerns: Safety (unconditional): stage into installPath.new (same dir -> same fs -> atomic rename, no EXDEV) and rename() over installPath. installPath is never removed, so a same-path or failed install can't strand the machine, the emergency-reset binary always exists, and a dangling symlink is replaced. Source selection (re-sync, path-independent): install argv[0] when it's a distinct file from installPath (from-source builds and keg-via-/opt/homebrew, any version). When argv[0] IS installPath -- sudo's secure_path resolving to a stale /usr/local/bin copy after brew upgrade -- copying onto itself is a permanent no-op, so fall back to the Homebrew keg and install it iff strictly newer. The keg version is read from its Cellar path, never by executing the binary; a present-but-unparseable keg logs loudly instead of going silent.
1. Install already-current branch: drop try? on the ownership/perms re-assert and throw ValidationError. launchd execs installPath as root at every boot; a silent failure on the most common path could leave a user-owned/writable root binary. Matches installBinary()'s fail-loud treatment. 2. DaemonServer.init: guard chown/chmod return values (NSLog+errno, close(fd), throw writeFailed). Diagnosability, not a security boundary — umask(0o077) is the unconditional boundary; both failures fail closed or are socket-equivalent. A loud crash-loop beats a silent lockout presenting as daemon-down. 3. Install relaunch: track whether a version-matching bundle was actually installed this run; relaunch only when the app was running AND a fresh bundle landed. Otherwise print the socket-moved guidance instead of reopening the stale bundle (old /tmp socket compiled in). 4. /tmp socket unlink: document why the root unlink() in a world-writable dir is safe (symlink not followed; hardlink leaves victim intact; dir → EISDIR no-op). 5. Relaunch verification: confirm a genuinely new ThermalForgeApp pid (different from the pre-install pid) rather than trusting open's exit code, which can be 0 without launching into the GUI session.
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.
Milestone A — security hardening (Phases 0–1) + install robustness
Branch
security-hardening-phase1. Do not merge — under review (Fable).Base is
origin/mainat340cf2e("Bump version to 0.1.10"), which does not contain Phase 0.b92bf07(Phase 0 — server read timeout) was committed to local main but never pushed, so it is included here as a 5th commit and appears in this diff — contrary to the original review write-up, which assumed Phase 0 was already on main. We deliberately did not push Phase 0 to trunk just to make the PR match the write-up. Phase 0 is therefore in scope for this review and has not been reviewed by anyone before now.Commits (base
340cf2e)b92bf07Phase 0 — bound server-side client read withSO_RCVTIMEO/SNDTIMEO(5s). Included via the base gap above.30c6cbfPhase 1 — root-owned/var/runsocket,0600owned by installing user,--owner-uidplumbed through the launchd plist.2d1addainstall: copy only a version-matching app bundle, never a stale Homebrew keg.5b475b2Fix Bug:thermalforge installself-deletes and fails; custom profiles hidden in menu bar #28: install self-delete + path-independent upgrade re-sync (temp-file + atomicrename; self-referential keg fallback; keg version read from Cellar path, never executed).83d59d4Fable review fixes (items 1–5) — see below.Review fixes (83d59d4)
try?on the ownership/perms re-assert; now throwsValidationError. launchd execs the binary as root at every boot; a silent failure on the most common path could leave a user-owned/writable root binary.DaemonServer.init— guardchown/chmod(NSLog+errno,close(fd), throw). Diagnosability, not a boundary:umask(0o077)is the unconditional boundary; both failures fail closed or are socket-equivalent./tmpsocket unlink — documented why the rootunlink()in a world-writable dir is safe (symlink not followed; hardlink leaves victim intact; dir →EISDIRno-op).ThermalForgeApppid vs the pre-install pid, rather than trustingopen's exit code.What Phase 1 bought (don't overread)
Bar moved from "any local process can drive the fans" → "any process running as uid 501 can drive the fans." Closed: cross-user access + path squatting. Not closed: same-user access — peer identity is Phase 5, deferred.
Verified
-c releaseclean;swift test29/29.nobodyrawconnect()→EACCES [Errno 13](socket present, daemon reachable, owner connects).rm+launchctl kickstart -k→ recreatedsrw------- fatty wheel, job clean, owner connects./var/runon the same APFS Data volume (/dev/disk3s5), so/var/runexists whenever launchd can exec the daemon (settled by inspection, not reboot).Not verified without a privileged install (owner's to run)
sudo thermalforge install— compiled + reasoned, not runtime-exercised here.nobody).