Tamper-resistant block timing: SCBlockClock + trusted-time verification#930
Open
ahmedkh19 wants to merge 11 commits into
Open
Tamper-resistant block timing: SCBlockClock + trusted-time verification#930ahmedkh19 wants to merge 11 commits into
ahmedkh19 wants to merge 11 commits into
Conversation
Closes the sudo-date bypass via a three-layer time gate: monotonic counter + persisted checkpoint + pinned HTTPS time check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce SCBlockClock as the entry point for tamper-resistant block timekeeping. recordBlockStartWithDuration: persists a BlockTimekeeping dictionary into SCSettings capturing the wall-clock start, the mach_continuous_time() snapshot, the boot-session identifier, and a zero elapsed counter. tickCheckpoint / elapsedSecondsForCurrentBlock / blockDurationHasElapsed are stubs that get implemented in Task 2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds fetchTimeFromHost:expectedSPKI:timeout:completion: which performs a HEAD request over an ephemeral URLSession with a custom server-trust challenge handler that pins the leaf cert's public key (SHA-256 of the SecKeyCopyExternalRepresentation output) and parses the Date: header. Adds verifyTimeIsAfter:completion: which fans out to all pinned hosts in parallel, requires a quorum of at least 2 responses agreeing within 5 minutes, and compares the median to the supplied threshold. Updates SCPinnedHosts.m: the previously baked-in pins were full ASN.1 SubjectPublicKeyInfo hashes (matching `openssl pkey -pubin -outform der`), but SecKeyCopyExternalRepresentation returns the raw public-key bytes without the algorithm-identifier wrapper, so all three pins were recomputed against the platform API output. All three (apple, google, cloudflare) needed updating because the format changed, not because of key rotation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…et message Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tack Discovered while end-to-end testing the tamper-resistant timing work against the threat model described in the design doc. Reproduction: start a block, then run `sudo date 010100002030`. The daemon correctly refuses to unlock — SCBlockClock clamps elapsed by mach_continuous_time, and the trusted-time verify gate rejects the unlock anyway. But the *timer window* would show "Finishing" within seconds, the strike counter would fire, and after ~7 strikes the "Stuck? Stop block manually" button became visible. Clicking it (with admin auth) ended the block. A working bypass. Root cause: TimerWindowController.updateTimerDisplay drove every check off `numSeconds = blockEndingDate - now` (wall-clock based). Jumping the system clock past BlockEndDate trips numSeconds < 0, which in turn triggers the gate query, the "Finishing" label, and the strike counter. The strike counter doesn't care what the daemon thinks. Fix: for modern blocks, source remaining time and the block-elapsed-but-still-running flag from SCBlockClock (the same tamper-resistant elapsed counter the daemon uses for actual unlock decisions). Legacy blocks keep the wall-clock path since they predate SCBlockClock. Adds two trivial accessors on SCBlockClock: + blockDurationSeconds — the recorded duration + remainingSecondsForCurrentBlock — max(0, duration - elapsed) TimerWindowController computes a new blockElapsedButStillRunning bool and uses that instead of `numSeconds < 0` for the three security- relevant branches (gate query / waiting-for-internet label / strike counter). The cosmetic dock-badge code keeps its wall-clock path; it shows garbage under attack but is not on the security path. After fix: clock-forward attack has zero UI effect. Countdown ticks down at real elapsed-time rate. No strikes, no kill button. The legitimate "daemon crashed and block elapsed for real" path still arms the kill button because SCBlockClock.elapsedSecondsForCurrentBlock keeps advancing (computed against mach_continuous_time + last on-disk checkpoint, independent of the daemon process being alive).
📝 WalkthroughWalkthroughThis PR implements a tamper-resistant block expiry system by combining monotonic elapsed-time tracking, persisted checkpoint recovery across reboots, and pinned-HTTPS quorum verification. The daemon now retries network verification with exponential backoff when a block duration has elapsed, deferring removal until the internet time check succeeds, and the timer UI displays "waiting for internet" during that state. ChangesTamper-resistant timing gate with monotonic clock and pinned HTTPS verification
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
TimerWindowController.m (1)
260-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBadge visibility check still uses wall-clock end date.
The numeric badge string is now computed from
SCBlockClock, but the gate[blockEndingDate_ timeIntervalSinceNow] > 0still consults the wall clock. A forwardsudo datejump pastBlockEndDatewill therefore drop the dock badge even whileSCBlockClockcorrectly reports the block is still running — defeating part of the UI-bypass hardening described in the PR objectives.- if([[NSUserDefaults standardUserDefaults] boolForKey: @"BadgeApplicationIcon"] && [blockEndingDate_ timeIntervalSinceNow] > 0) { + BOOL stillTicking = useBlockClock ? ![SCBlockClock blockDurationHasElapsed] + : ([blockEndingDate_ timeIntervalSinceNow] > 0); + if([[NSUserDefaults standardUserDefaults] boolForKey: @"BadgeApplicationIcon"] && stillTicking) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TimerWindowController.m` at line 260, The badge visibility check uses the wall-clock via [blockEndingDate_ timeIntervalSinceNow] but should use the secure monotonic clock used to compute the numeric badge (SCBlockClock); change the condition that gates setting the dock badge (the if checking BadgeApplicationIcon and blockEndingDate_) to consult SCBlockClock's remaining/block-running API (e.g., call the SCBlockClock method used elsewhere to compute remaining seconds or isRunning) instead of timeIntervalSinceNow so a forward system clock jump does not hide the badge while SCBlockClock reports the block active.Daemon/SCDaemonBlockMethods.m (1)
304-321:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheckpoint timer is not stopped when checkup discovers the block was already removed.
This branch already calls
[[SCDaemon sharedDaemon] stopCheckupTimer]because the block is gone (likely user tampering). With the new checkpoint timer wired in onstartBlock(line 140), it should be stopped here as well — otherwise the 30sSCBlockClockcheckpoint ticks keep firing against settings that no longer represent an active block.[[SCDaemon sharedDaemon] stopCheckupTimer]; + [[SCDaemon sharedDaemon] stopCheckpointTimer];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Daemon/SCDaemonBlockMethods.m` around lines 304 - 321, The checkpoint timer started by startBlock isn't being stopped when the checkup finds the block already removed; add a call to stop that timer in this branch (in addition to the existing [[SCDaemon sharedDaemon] stopCheckupTimer]) so the SCBlockClock checkpoint ticks stop firing against a non-active block—e.g. invoke the SCDaemon method that mirrors the startBlock wiring (such as [[SCDaemon sharedDaemon] stopCheckpointTimer] or the appropriate stop method you added to SCDaemon) immediately after removing the block and sending the configuration-changed notification.
🧹 Nitpick comments (9)
Common/Utility/SCBlockClock.m (1)
67-79: 💤 Low valueDead branch and slightly misleading comment in
inFlightDeltaFromTK.
deltaContis computed as((double)(cont - lastCont)) / 1e9where the subtraction is inuint64_t, so the result is always ≥ 0 as adouble.MAX(0.0, deltaWall)is also ≥ 0, sotrustedDelta = MIN(...)is never negative — thetrustedDelta < 0guard on Line 77 is unreachable. The comment "defense vs corrupt persisted lastCont" is also a bit misleading: iflastCont > cont(e.g. persisted from a different boot but UUID check missed it, or test fixture), the unsigned subtraction wraps to a huge positive value rather than going negative, and the real defense is theMINagainst the wall delta.Either drop the dead guard, or rewrite using signed arithmetic so the corrupted-
lastContcase actually produces a negativedeltaContthat the guard catches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Common/Utility/SCBlockClock.m` around lines 67 - 79, The guard "if (trustedDelta < 0) trustedDelta = 0;" in inFlightDeltaFromTK is dead because cont and lastCont are unsigned; fix by computing the counter difference using signed arithmetic so negative deltas from a corrupted/persisted lastCont are detectable: cast cont and lastCont to a signed 64-bit type (e.g., int64_t) and compute a signed diff, convert that diff to deltaCont as a double (divide by 1e9), then proceed to compute trustedDelta = MIN(deltaCont, MAX(0.0, deltaWall)) and keep the existing negative guard; update references to lastCont and deltaCont in inFlightDeltaFromTK accordingly.SelfControl.xcodeproj/project.pbxproj (1)
4656-4656: ⚡ Quick winRemove duplicate path and restore
$(inherited)inFRAMEWORK_SEARCH_PATHS.This setting overrides the framework search paths defined in
Pods-selfcontrol-cli.debug.xcconfigandPods-selfcontrol-cli.release.xcconfigwithout using$(inherited), causing any CocoaPods-provided paths to be lost. The path is also duplicated with escaped newlines, which should use the standard array form instead.Proposed fix
- FRAMEWORK_SEARCH_PATHS = "$(PLATFORM_DIR)/Developer/Library/Frameworks\n\n$(PLATFORM_DIR)/Developer/Library/Frameworks\n\n"; + FRAMEWORK_SEARCH_PATHS = ( + "$(inherited)", + "$(PLATFORM_DIR)/Developer/Library/Frameworks", + );Also applies to: 4689-4689
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SelfControl.xcodeproj/project.pbxproj` at line 4656, The FRAMEWORK_SEARCH_PATHS setting currently contains duplicated "$(PLATFORM_DIR)/Developer/Library/Frameworks" entries with escaped newlines and omits $(inherited), which overrides and drops CocoaPods paths from Pods-selfcontrol-cli.debug.xcconfig and Pods-selfcontrol-cli.release.xcconfig; update the FRAMEWORK_SEARCH_PATHS entry for the affected build configuration(s) to include $(inherited) and remove the duplicate path, using the standard array form (one entry per item) rather than escaped newlines so it preserves inherited values and does not duplicate "$(PLATFORM_DIR)/Developer/Library/Frameworks".Daemon/SCDaemon.m (1)
148-165: 💤 Low valueSingleton
deallocwill essentially never run.
+sharedDaemonretains the instance for process lifetime viadispatch_once, so the newcheckpointTimerinvalidation indeallocis dead code in practice (same caveat already applied to the pre-existingcheckupTimer/inactivityTimercleanup). Not introduced by this PR, just flagging since you're now extending the pattern — if you ever want guaranteed teardown semantics (e.g. for tests), introduce an explicitstop/tearDownmethod rather than relying ondealloc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Daemon/SCDaemon.m` around lines 148 - 165, The dealloc cleanup is effectively dead for the singleton created by +sharedDaemon; add an explicit public instance method (e.g. -stop or -tearDown) on SCDaemon that performs the same cleanup now in -dealloc (invalidate and nil out checkupTimer, checkpointTimer, inactivityTimer and call stopWatching/nil hostsFileWatcher) and update callers/tests that need deterministic teardown to call -stop/-tearDown instead of relying on dealloc; leave -dealloc as a fallback (or call [self stop] from -dealloc) to preserve safety for non-singleton usage.SelfControlTests/SCTrustedTimeTests.m (2)
30-30: ⚡ Quick winFragile hardcoded host index – break-the-build-on-reorder.
[SCPinnedHosts hosts][0]couples the test to the array ordering inSCPinnedHosts.m. A pin rotation that prepends a host (e.g. moving Apple behind a backup) will silently make this test exercise the wrong host. Look up by name instead.♻️ Suggested fix
- NSDictionary* apple = [SCPinnedHosts hosts][0]; // assumes apple is index 0 + NSDictionary* apple = nil; + for (NSDictionary* hp in [SCPinnedHosts hosts]) { + if ([hp[@"host"] isEqualToString: @"www.apple.com"]) { apple = hp; break; } + } + XCTAssertNotNil(apple);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SelfControlTests/SCTrustedTimeTests.m` at line 30, The test in SCTrustedTimeTests.m is using a hardcoded index into [SCPinnedHosts hosts] (NSDictionary* apple = [SCPinnedHosts hosts][0]) which breaks if host ordering changes; replace the index lookup by searching the hosts array for the host with the expected identifier (e.g., name or hostname key) — for example iterate or use NSPredicate on the array returned by +[SCPinnedHosts hosts] to find the dictionary whose @"name" or @"host" equals the Apple entry and assign that to the apple variable so the test no longer depends on array order.
28-54: ⚡ Quick winNetwork-dependent tests will be flaky and undermine CI signal.
testFetchFromAppleWithCorrectPinReturnsDate,testFetchWithWrongPinFails,testVerifyAfterPastThresholdSucceeds, andtestVerifyAfterFutureThresholdFailsall require real HTTPS connectivity to apple/google/cloudflare and valid system TLS. They will fail on:
- Sandboxed/offline CI runners
- Corporate proxies / MITM appliances (the wrong-pin test would pass for the wrong reason)
- Any future leaf rotation on these hosts (until pins are updated)
Recommend wrapping them in a "live network" test plan or skipping them via
XCTSkipUnless(NetworkAvailable())so default CI runs stay green and deterministic. The parser tests (testParseValidIMFFixdate,testParseGarbageReturnsNil) are correctly hermetic and should stay as-is.Additionally,
testVerifyAfterFutureThresholdFailspasses both when "quorum reached but median < threshold" and when "quorum failed entirely" — please tighten it to assertmed != nilso it actually exercises the threshold branch.Also applies to: 56-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SelfControlTests/SCTrustedTimeTests.m` around lines 28 - 54, Wrap the network-dependent tests (testFetchFromAppleWithCorrectPinReturnsDate, testFetchWithWrongPinFails, testVerifyAfterPastThresholdSucceeds, testVerifyAfterFutureThresholdFails) so they are skipped when live network/TLS is not available (e.g., use XCTSkipUnless(NetworkAvailable()) or a test plan flag) to keep CI deterministic; leave hermetic parser tests (testParseValidIMFFixdate, testParseGarbageReturnsNil) unchanged. Also tighten testVerifyAfterFutureThresholdFails by adding an assertion that the median returned (med) is non-nil before asserting failure conditions so the test exercises the "median < threshold" branch rather than a quorum-failure path. Ensure you modify the test methods named above and keep the existing completion/assertion structure intact.Common/Utility/SCTrustedTime.m (2)
99-136: 💤 Low valueQuorum aggregation logic – minor observations.
The dispatch-group fan-out and median selection look correct for 2-of-3 quorum with a 5-minute disagreement window. Two small notes worth considering:
dispatch_group_notifyfires only after every host's task completes (each capped at 8s). A single slow host will hold up the whole verification; consider giving the group itself a wait deadline so a stalled host doesn't push end-to-end latency to ~8 s even when two hosts have already agreed.sorted[sorted.count / 2]is the upper-mid element. For the n=2 case (one host failed) this picks the later of two times, which is friendlier to the gate semantics (median ≥ threshold), but it's worth a brief comment so future readers don't "fix" it to a true mean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Common/Utility/SCTrustedTime.m` around lines 99 - 136, verifyTimeIsAfter:completion: currently waits for every pinned host via dispatch_group_notify which lets a single slow host delay completion; add a bounded wait on the dispatch group (use dispatch_group_wait with a short deadline) before collecting results so the method proceeds when the quorum timeout elapses, and ensure you still handle remaining callbacks safely; also add an explicit comment next to the median computation (sorted[sorted.count / 2]) explaining that for n==2 this intentionally selects the later of the two samples (upper-mid) to bias toward allowing the gate, so future readers do not change it.
23-26: 💤 Low valueReplace deprecated SecTrust certificate-access APIs.
SecTrustGetCertificateCountandSecTrustGetCertificateAtIndexare deprecated in recent macOS SDKs in favor ofSecTrustCopyCertificateChain, which returns the certificate chain as a CFArray. Switching avoids deprecation warnings and is more future-proof.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Common/Utility/SCTrustedTime.m` around lines 23 - 26, Replace the deprecated SecTrust APIs by calling SecTrustCopyCertificateChain(trust) to obtain a CFArrayRef of certificates, use CFArrayGetCount and CFArrayGetValueAtIndex (or CFArray iteration) instead of SecTrustGetCertificateCount/SecTrustGetCertificateAtIndex, iterate that array to perform the same matching logic (preserving the 'matched' flag and 'trust' variable use), and remember to CFRelease the copied array after use; update any variable names as needed to reflect the array-based iteration and remove the deprecated API calls.Daemon/SCDaemonBlockMethods.h (1)
34-34: 💤 Low valueConsider adding a brief doc comment for parity with surrounding methods.
The neighbouring methods (
startBlockWithControllingUID:,checkupBlock,updateBlocklist:, etc.) each carry a one-line comment describing intent.attemptVerifiedUnlockis the new entry point for the trusted-internet quorum flow and would benefit from a short comment so callers understand its retry/backoff semantics and unlock-gate side effects without diving into the implementation.📝 Suggested comment
+// Performs an async pinned-HTTPS quorum check against trusted hosts and, on success, +// clears the unlock gate so checkupBlock can remove an elapsed block. Retries with +// exponential backoff on failure; safe to call repeatedly. + (void)attemptVerifiedUnlock;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Daemon/SCDaemonBlockMethods.h` at line 34, Add a one-line doc comment above the declaration of attemptVerifiedUnlock describing its purpose and side effects — e.g., note that attemptVerifiedUnlock is the entry point for the trusted-internet quorum flow, performs retry/backoff semantics, and may toggle the unlock gate on success/failure — mirroring the style of neighboring comments like startBlockWithControllingUID:, checkupBlock, and updateBlocklist: so callers can quickly understand intent without reading the implementation.Common/Utility/SCTrustedTime.h (1)
12-12: 💤 Low valueMove block typedefs outside the
@interfacebody.Placing
typedefdeclarations between method declarations inside an@interfaceis unidiomatic. Hoist both typedefs above the@interfaceso the type names are available at the top of the header.♻️ Suggested layout
NS_ASSUME_NONNULL_BEGIN +typedef void (^SCTrustedTimeCompletion)(NSDate* _Nullable verifiedTime, NSError* _Nullable error); +typedef void (^SCTrustedTimeQuorumCompletion)(BOOL verified, NSDate* _Nullable medianTime, NSError* _Nullable error); + /// Trusted-internet time verification for the block unlock gate. /// All callers must be prepared for any method to return nil/NO on failure. `@interface` SCTrustedTime : NSObject /// Parses an HTTP IMF-fixdate (RFC 7231 §7.1.1.1). Returns nil on any error. + (nullable NSDate*)parseHTTPDateHeader:(nullable NSString*)header; -typedef void (^SCTrustedTimeCompletion)(NSDate* _Nullable verifiedTime, NSError* _Nullable error); - /// Fetches https://<host>/ HEAD and returns the time from the Date: header IFF /// the server's leaf SubjectPublicKeyInfo SHA-256 (base64) matches expectedSPKI. + (void)fetchTimeFromHost:(NSString*)host expectedSPKI:(NSString*)spki timeout:(NSTimeInterval)timeoutSeconds completion:(SCTrustedTimeCompletion)completion; -typedef void (^SCTrustedTimeQuorumCompletion)(BOOL verified, NSDate* _Nullable medianTime, NSError* _Nullable error); - /// Returns verified=YES iff at least 2 pinned hosts respond, all returned times /// are within 5 minutes of each other, and the median response is >= threshold. + (void)verifyTimeIsAfter:(NSDate*)threshold completion:(SCTrustedTimeQuorumCompletion)completion;Also applies to: 21-21
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Common/Utility/SCTrustedTime.h` at line 12, The block typedef(s) (including SCTrustedTimeCompletion and the other block typedef declared in this header) are currently inside the `@interface`; move both typedef declarations above the `@interface` declaration so they are at top-level of the header file. Update the header so the typedefs are hoisted before the `@interface` SCTrustedTime : NSObject declaration, ensuring the type names are available to callers and other declarations within the header.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Common/Utility/SCBlockClock.m`:
- Around line 19-27: currentBootSessionUUID should not return the string
@"unknown" because that value can later be treated as a real bootSessionUUID and
allow attacker-controlled cross-boot credit; change currentBootSessionUUID to
return nil on sysctlbyname failure (and still honor sBootUUIDOverride), then
update recordBlockStartWithDuration: to detect a nil bootSessionUUID and refuse
to persist/start a block (log/error/early-return) rather than recording
"unknown"; also add a defensive check in tickCheckpoint to treat a nil or
overridden sBootUUIDOverride consistently (avoid taking the cross-boot branch
when the stored bootSessionUUID or currentBootSessionUUID is nil) so mismatches
caused by transient read failures cannot be credited.
In `@Common/Utility/SCBlockUtilities.m`:
- Around line 65-68: The current implementation of currentBlockIsTrulyExpired
returns NO when SCBlockClock's blockDurationHasElapsed is NO (which happens if
BlockTimekeeping is nil), causing pre-upgrade blocks to never be torn down;
modify currentBlockIsTrulyExpired to treat a missing BlockTimekeeping as truly
expired when the wall-clock expiry has passed: keep the initial
currentBlockIsExpired check, then if [SCBlockClock blockDurationHasElapsed] is
YES return YES, otherwise detect that the BlockTimekeeping entry for the current
block is absent (or nil) and if the block’s legacy wall-clock fields
(BlockEndDate or BlockStartDate compared to now) indicate expiry, return YES;
reference currentBlockIsTrulyExpired, currentBlockIsExpired, SCBlockClock
blockDurationHasElapsed, BlockTimekeeping, BlockEndDate and BlockStartDate when
implementing this branch.
In `@Common/Utility/SCPinnedHosts.m`:
- Around line 5-20: The hosts method in SCPinnedHosts.m currently returns a
single hardcoded leaf SPKI pin per host which will break on routine rotations;
update SCPinnedHosts + the hosts method to accept multiple pins per host (array
of current + staged leaf pins and/or intermediate/root SPKI pins), and
implement/consume a rotation-friendly strategy such as: include long-lived
intermediate/root pins alongside current leaf pins, allow multiple acceptable
pins when checking the trust chain, and add support for signed out-of-band pin
updates (fallback mechanism) so older builds can receive new pins; also verify
and replace the three provided pin strings by recomputing
SecKeyCopyExternalRepresentation() on the live leaf certificates before merging.
In `@Common/Utility/SCTrustedTime.h`:
- Around line 14-19: The public API and doc comment for
fetchTimeFromHost:expectedSPKI:timeout:completion are misleading: the
implementation hashes SecKeyCopyExternalRepresentation() (raw public key bytes),
not an RFC5280 SubjectPublicKeyInfo digest. Change the doc comment to state that
the method compares the SHA-256 (base64) of the public key's raw external
representation, rename the parameter expectedSPKI to expectedPublicKeyHash (or
similar) to reflect this, and update all callers/tests to use the new name and
the correct hash computation method so pins computed via openssl x509 -pubkey
... | openssl pkey -pubin -outform DER | sha256sum no longer mismatch.
In `@Common/Utility/SCTrustedTime.m`:
- Around line 29-39: The loop that pins SPKIs leaks the CFErrorRef from
SecKeyCopyExternalRepresentation when it returns NULL and also checks every cert
in the chain instead of only the leaf; fix by releasing the error on failure
(CFRelease(err)) before continuing and by limiting the SPKI comparison to the
leaf certificate only (i.e., only perform
SecKeyCopyExternalRepresentation/CC_SHA256/base64 compare for the cert at index
0), using the existing symbols SecKeyCopyExternalRepresentation, CFErrorRef err,
der, expectedSPKI and matched to locate the code to change.
In `@Daemon/SCDaemon.m`:
- Around line 110-123: startCheckpointTimer and stopCheckpointTimer are being
called from background XPC queues so their NSTimer is scheduled/invalidated off
the main thread; wrap both methods with the same main-thread guard used by
startCheckupTimer (check if (![NSThread isMainThread]) then
dispatch_async(dispatch_get_main_queue(), ^{ ... }) and return) so that creation
of self.checkpointTimer via [NSTimer
scheduledTimerWithTimeInterval:repeats:block:] and its invalidation happen on
the main thread; preserve the early-return checks (if self.checkpointTimer !=
nil / == nil) and operate on the same checkpointTimer property so the timer will
fire and be safely invalidated on the main run loop.
In `@Daemon/SCDaemonBlockMethods.m`:
- Around line 322-323: The checkupBlock path currently calls
SCDaemonBlockMethods::attemptVerifiedUnlock every tick causing overlapping
verification chains; add a single in-flight guard (e.g., a BOOL
verifiedUnlockInFlight on SCDaemon or a class-level static) and modify
checkupBlock to only call SCDaemonBlockMethods::attemptVerifiedUnlock when that
flag is false, then have attemptVerifiedUnlock set the flag before starting
SCTrustedTime::verifyTimeIsAfter and clear it only when the verification+retry
chain completes/fails (including after the dispatch_after retry finishes) so the
retry chain owns subsequent attempts and prevents re-entrant concurrent
verification and multiple BlockUnlockAttempt increments.
- Around line 388-423: The completion block in attemptVerifiedUnlock mutates
shared state and calls helpers without holding daemonMethodLock, so wrap all
reads/updates to SCSettings keys (BlockUnlockGate, BlockUnlockAttempt), calls to
[SCHelperToolUtilities removeBlock] and sendConfigurationChangedNotification,
and calls to [[SCDaemon sharedDaemon] stopCheckupTimer] / stopCheckpointTimer
inside a critical section that acquires daemonMethodLock from [SCDaemon
sharedDaemon] before performing these actions and releases it afterward; keep
the dispatch_after scheduling of the next attemptVerifiedUnlock outside the lock
to avoid holding the lock during the backoff wait.
In `@docs/superpowers/plans/2026-05-14-tamper-resistant-timing.md`:
- Around line 1328-1331: The fenced code block containing the two translation
lines ("Connect to the internet to finish your block." = ...; and "SelfControl
checks the time with a trusted server..." = ...;) is missing a fence language;
add a language tag after the opening backticks (for example ```text or ```ini)
so the block becomes ```text (or another appropriate language) to satisfy
Markdown linting.
- Line 613: The docs claim pins are "SPKI SHA-256" but the sample verifier
computes SHA-256 over SecKeyCopyExternalRepresentation (raw PKCS#1/X9.63 bytes),
so update the pin generation instructions (Task 6) to hash the same raw
public-key bytes produced by SecKeyCopyExternalRepresentation and rename all
mentions of "SPKI" to "raw public key bytes (PKCS#1 for RSA / X9.63 for EC)" to
avoid mismatch; also add a short round-trip test in the guide that generates a
pin using the revised generation step and verifies it with the sample verifier
(Task 7) to ensure matching hashes.
In `@docs/superpowers/specs/2026-05-14-tamper-resistant-timing-design.md`:
- Line 4: The status line in the spec header is stale: change the text that
currently reads “Implementation plan not yet written” to reflect that the
implementation plan is included (e.g., “Implementation plan written — see
docs/superpowers/plans/2026-05-14-tamper-resistant-timing.md”) so the spec
header (the Status line in the document titled tamper-resistant-timing-design)
accurately reflects the current state.
- Around line 60-62: The markdown code fences containing the snippet with
continuousElapsed and calls like mach_continuous_time() lack language
identifiers and trigger lint failures; update those fenced code blocks (e.g.,
the block that computes continuousElapsed = (mach_continuous_time() -
blockStartContinuousTime) / 1e9 and the other blocks referenced at lines 80-89,
95-105, and 196-202) by adding the appropriate language tag (such as `swift`,
`c`, or `pseudo` depending on the snippet) immediately after the opening ``` so
the markdown linter recognizes the code language.
- Around line 19-20: The spec's threat-model claim "Editing the on-disk
settings/checkpoint files" and "Deleting the on-disk settings/checkpoint files"
overstates protection because SCSettings wiping can remove blocks; update those
phrases (at the occurrences currently at line 19–20 and again at 121–122) to
downgrade the guarantee or mark it as a known limitation, e.g., change to
"Editing/deleting on-disk settings/checkpoint files: may be detectable but can
still enable SCSettings wiping that can remove blocks — see known limitation"
and add a one-line pointer to the PR/context that documents SCSettings wiping so
readers know the exact failure mode.
In `@TimerWindowController.m`:
- Around line 374-388: queryUnlockGateState currently sets gateQueryInFlight =
YES and only clears it inside the XPC reply block, which can hang indefinitely;
update queryUnlockGateState to capture self weakly (use __weak typeof(self)
weakSelf) before calling [self.gateXPCClient getBlockUnlockGateStateWithReply:]
and inside the reply first obtain a strongSelf and check a queryToken or
timestamp to ignore late replies, and also schedule a dispatch_after watchdog
(e.g. 5s) that clears gateQueryInFlight and updates UI on the main queue if the
reply hasn't arrived; alternatively register an invalidation handler on
gateXPCClient/NSXPCConnection to clear gateQueryInFlight — reference the symbols
queryUnlockGateState, gateQueryInFlight, gateXPCClient,
getBlockUnlockGateStateWithReply:, and waitingForNetworkUnlock when making these
changes.
- Around line 192-204: The timerLabel_ in TimerWindow.xib (id="33") is
configured to clip lines so waitingForInternetString (the string containing
"\n") gets truncated; open TimerWindow.xib, select the text field with id 33
(timerLabel_), change its cell's lineBreakMode from "clipping" to
"byWordWrapping" (or equivalent wrapping mode), disable single-line-only mode /
enable multi-line rendering on the NSTextFieldCell (ensure usesSingleLineMode =
NO / wraps = YES), and save the XIB so timerLabel_ can render both lines when
TimerWindowController sets the waitingForInternetString.
---
Outside diff comments:
In `@Daemon/SCDaemonBlockMethods.m`:
- Around line 304-321: The checkpoint timer started by startBlock isn't being
stopped when the checkup finds the block already removed; add a call to stop
that timer in this branch (in addition to the existing [[SCDaemon sharedDaemon]
stopCheckupTimer]) so the SCBlockClock checkpoint ticks stop firing against a
non-active block—e.g. invoke the SCDaemon method that mirrors the startBlock
wiring (such as [[SCDaemon sharedDaemon] stopCheckpointTimer] or the appropriate
stop method you added to SCDaemon) immediately after removing the block and
sending the configuration-changed notification.
In `@TimerWindowController.m`:
- Line 260: The badge visibility check uses the wall-clock via [blockEndingDate_
timeIntervalSinceNow] but should use the secure monotonic clock used to compute
the numeric badge (SCBlockClock); change the condition that gates setting the
dock badge (the if checking BadgeApplicationIcon and blockEndingDate_) to
consult SCBlockClock's remaining/block-running API (e.g., call the SCBlockClock
method used elsewhere to compute remaining seconds or isRunning) instead of
timeIntervalSinceNow so a forward system clock jump does not hide the badge
while SCBlockClock reports the block active.
---
Nitpick comments:
In `@Common/Utility/SCBlockClock.m`:
- Around line 67-79: The guard "if (trustedDelta < 0) trustedDelta = 0;" in
inFlightDeltaFromTK is dead because cont and lastCont are unsigned; fix by
computing the counter difference using signed arithmetic so negative deltas from
a corrupted/persisted lastCont are detectable: cast cont and lastCont to a
signed 64-bit type (e.g., int64_t) and compute a signed diff, convert that diff
to deltaCont as a double (divide by 1e9), then proceed to compute trustedDelta =
MIN(deltaCont, MAX(0.0, deltaWall)) and keep the existing negative guard; update
references to lastCont and deltaCont in inFlightDeltaFromTK accordingly.
In `@Common/Utility/SCTrustedTime.h`:
- Line 12: The block typedef(s) (including SCTrustedTimeCompletion and the other
block typedef declared in this header) are currently inside the `@interface`; move
both typedef declarations above the `@interface` declaration so they are at
top-level of the header file. Update the header so the typedefs are hoisted
before the `@interface` SCTrustedTime : NSObject declaration, ensuring the type
names are available to callers and other declarations within the header.
In `@Common/Utility/SCTrustedTime.m`:
- Around line 99-136: verifyTimeIsAfter:completion: currently waits for every
pinned host via dispatch_group_notify which lets a single slow host delay
completion; add a bounded wait on the dispatch group (use dispatch_group_wait
with a short deadline) before collecting results so the method proceeds when the
quorum timeout elapses, and ensure you still handle remaining callbacks safely;
also add an explicit comment next to the median computation (sorted[sorted.count
/ 2]) explaining that for n==2 this intentionally selects the later of the two
samples (upper-mid) to bias toward allowing the gate, so future readers do not
change it.
- Around line 23-26: Replace the deprecated SecTrust APIs by calling
SecTrustCopyCertificateChain(trust) to obtain a CFArrayRef of certificates, use
CFArrayGetCount and CFArrayGetValueAtIndex (or CFArray iteration) instead of
SecTrustGetCertificateCount/SecTrustGetCertificateAtIndex, iterate that array to
perform the same matching logic (preserving the 'matched' flag and 'trust'
variable use), and remember to CFRelease the copied array after use; update any
variable names as needed to reflect the array-based iteration and remove the
deprecated API calls.
In `@Daemon/SCDaemon.m`:
- Around line 148-165: The dealloc cleanup is effectively dead for the singleton
created by +sharedDaemon; add an explicit public instance method (e.g. -stop or
-tearDown) on SCDaemon that performs the same cleanup now in -dealloc
(invalidate and nil out checkupTimer, checkpointTimer, inactivityTimer and call
stopWatching/nil hostsFileWatcher) and update callers/tests that need
deterministic teardown to call -stop/-tearDown instead of relying on dealloc;
leave -dealloc as a fallback (or call [self stop] from -dealloc) to preserve
safety for non-singleton usage.
In `@Daemon/SCDaemonBlockMethods.h`:
- Line 34: Add a one-line doc comment above the declaration of
attemptVerifiedUnlock describing its purpose and side effects — e.g., note that
attemptVerifiedUnlock is the entry point for the trusted-internet quorum flow,
performs retry/backoff semantics, and may toggle the unlock gate on
success/failure — mirroring the style of neighboring comments like
startBlockWithControllingUID:, checkupBlock, and updateBlocklist: so callers can
quickly understand intent without reading the implementation.
In `@SelfControl.xcodeproj/project.pbxproj`:
- Line 4656: The FRAMEWORK_SEARCH_PATHS setting currently contains duplicated
"$(PLATFORM_DIR)/Developer/Library/Frameworks" entries with escaped newlines and
omits $(inherited), which overrides and drops CocoaPods paths from
Pods-selfcontrol-cli.debug.xcconfig and Pods-selfcontrol-cli.release.xcconfig;
update the FRAMEWORK_SEARCH_PATHS entry for the affected build configuration(s)
to include $(inherited) and remove the duplicate path, using the standard array
form (one entry per item) rather than escaped newlines so it preserves inherited
values and does not duplicate "$(PLATFORM_DIR)/Developer/Library/Frameworks".
In `@SelfControlTests/SCTrustedTimeTests.m`:
- Line 30: The test in SCTrustedTimeTests.m is using a hardcoded index into
[SCPinnedHosts hosts] (NSDictionary* apple = [SCPinnedHosts hosts][0]) which
breaks if host ordering changes; replace the index lookup by searching the hosts
array for the host with the expected identifier (e.g., name or hostname key) —
for example iterate or use NSPredicate on the array returned by +[SCPinnedHosts
hosts] to find the dictionary whose @"name" or @"host" equals the Apple entry
and assign that to the apple variable so the test no longer depends on array
order.
- Around line 28-54: Wrap the network-dependent tests
(testFetchFromAppleWithCorrectPinReturnsDate, testFetchWithWrongPinFails,
testVerifyAfterPastThresholdSucceeds, testVerifyAfterFutureThresholdFails) so
they are skipped when live network/TLS is not available (e.g., use
XCTSkipUnless(NetworkAvailable()) or a test plan flag) to keep CI deterministic;
leave hermetic parser tests (testParseValidIMFFixdate,
testParseGarbageReturnsNil) unchanged. Also tighten
testVerifyAfterFutureThresholdFails by adding an assertion that the median
returned (med) is non-nil before asserting failure conditions so the test
exercises the "median < threshold" branch rather than a quorum-failure path.
Ensure you modify the test methods named above and keep the existing
completion/assertion structure intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bd8fcf0-c561-4a84-923c-497ddece466a
📒 Files selected for processing (24)
Common/SCXPCClient.hCommon/SCXPCClient.mCommon/Utility/SCBlockClock.hCommon/Utility/SCBlockClock.mCommon/Utility/SCBlockUtilities.hCommon/Utility/SCBlockUtilities.mCommon/Utility/SCPinnedHosts.hCommon/Utility/SCPinnedHosts.mCommon/Utility/SCTrustedTime.hCommon/Utility/SCTrustedTime.mDaemon/SCDaemon.hDaemon/SCDaemon.mDaemon/SCDaemonBlockMethods.hDaemon/SCDaemonBlockMethods.mDaemon/SCDaemonProtocol.hDaemon/SCDaemonXPC.mSelfControl.xcodeproj/project.pbxprojSelfControlTests/SCBlockClockTests.mSelfControlTests/SCTrustedTimeTests.mSelfControlTests/SCUtilityTests.mTimerWindowController.mdocs/superpowers/plans/2026-05-14-tamper-resistant-timing.mddocs/superpowers/specs/2026-05-14-tamper-resistant-timing-design.mden.lproj/Localizable.strings
Comment on lines
+19
to
+27
| + (NSString*)currentBootSessionUUID { | ||
| if (sBootUUIDOverride != nil) return sBootUUIDOverride; | ||
| struct timeval boottime; | ||
| size_t size = sizeof(boottime); | ||
| if (sysctlbyname("kern.boottime", &boottime, &size, NULL, 0) != 0) { | ||
| return @"unknown"; | ||
| } | ||
| return [NSString stringWithFormat: @"%ld.%d", (long)boottime.tv_sec, boottime.tv_usec]; | ||
| } |
There was a problem hiding this comment.
Returning "unknown" on sysctlbyname failure can cause a spurious cross-boot credit.
If sysctlbyname fails at recordBlockStartWithDuration: (persisting bootSessionUUID = "unknown") but later succeeds at tickCheckpoint time (returning a real UUID), the mismatch on Line 89 will execute the cross-boot branch and credit MAX(0, wall-clock gap) toward elapsed time. That credit is fully attacker-controlled via sudo date between the failure and the next checkpoint. Reading kern.boottime is extremely unlikely to fail in practice, but the failure mode bypasses exactly the threat model this clock is built to resist.
Consider one of: (a) refusing to record a block start when the boot UUID is unavailable, (b) re-reading the UUID at recording time and asserting/logging on failure, or (c) treating "unknown" specially in tickCheckpoint so it doesn't trigger the cross-boot path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/Utility/SCBlockClock.m` around lines 19 - 27, currentBootSessionUUID
should not return the string @"unknown" because that value can later be treated
as a real bootSessionUUID and allow attacker-controlled cross-boot credit;
change currentBootSessionUUID to return nil on sysctlbyname failure (and still
honor sBootUUIDOverride), then update recordBlockStartWithDuration: to detect a
nil bootSessionUUID and refuse to persist/start a block (log/error/early-return)
rather than recording "unknown"; also add a defensive check in tickCheckpoint to
treat a nil or overridden sBootUUIDOverride consistently (avoid taking the
cross-boot branch when the stored bootSessionUUID or currentBootSessionUUID is
nil) so mismatches caused by transient read failures cannot be credited.
Comment on lines
+65
to
+68
| + (BOOL)currentBlockIsTrulyExpired { | ||
| if (![self currentBlockIsExpired]) return NO; | ||
| return [SCBlockClock blockDurationHasElapsed]; | ||
| } |
There was a problem hiding this comment.
Pre-upgrade / missing-checkpoint blocks may never be "truly expired".
SCBlockClock blockDurationHasElapsed returns NO when BlockTimekeeping is nil (Line 126 of SCBlockClock.m). Any block that exists in SCSettings without a corresponding BlockTimekeeping dictionary — e.g. an in-progress block that was started before this version was installed, or after a settings-store anomaly — will therefore have currentBlockIsTrulyExpired == NO indefinitely even after the wall-clock end date has long passed. The daemon will then refuse to tear down the firewall, stranding the user.
Consider either:
- Lazily seeding
BlockTimekeepingfromBlockEndDate/ start fields the first time the daemon observes a block without one (treat the block as having already accumulatednow - BlockStartDate). - Treating "no
BlockTimekeepingbut wall-clock expired" as truly expired (assuming the legacy expiry check is sufficient when monotonic state is simply absent rather than tampered).
The right answer depends on threat-model intent for the upgrade path, but the current behavior is failure-closed in a way that hurts honest users.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/Utility/SCBlockUtilities.m` around lines 65 - 68, The current
implementation of currentBlockIsTrulyExpired returns NO when SCBlockClock's
blockDurationHasElapsed is NO (which happens if BlockTimekeeping is nil),
causing pre-upgrade blocks to never be torn down; modify
currentBlockIsTrulyExpired to treat a missing BlockTimekeeping as truly expired
when the wall-clock expiry has passed: keep the initial currentBlockIsExpired
check, then if [SCBlockClock blockDurationHasElapsed] is YES return YES,
otherwise detect that the BlockTimekeeping entry for the current block is absent
(or nil) and if the block’s legacy wall-clock fields (BlockEndDate or
BlockStartDate compared to now) indicate expiry, return YES; reference
currentBlockIsTrulyExpired, currentBlockIsExpired, SCBlockClock
blockDurationHasElapsed, BlockTimekeeping, BlockEndDate and BlockStartDate when
implementing this branch.
Comment on lines
+5
to
+20
| + (NSArray<NSDictionary<NSString*, NSString*>*>*)hosts { | ||
| static NSArray* hosts = nil; | ||
| static dispatch_once_t once; | ||
| dispatch_once(&once, ^{ | ||
| // Pins are SHA-256 (base64) of SecKeyCopyExternalRepresentation() output | ||
| // for the leaf certificate's public key — i.e., the raw public-key bytes | ||
| // (RSA modulus+exponent, or EC point), NOT the full ASN.1 SPKI. | ||
| // To recompute, see the SCTrustedTime pinning delegate in SCTrustedTime.m. | ||
| hosts = @[ | ||
| @{ @"host": @"www.apple.com", @"spki": @"vGYr5lNKlPJ0wSZMROTsiP+dd8iCbBOe7YZMVoy4cK0=" }, | ||
| @{ @"host": @"www.google.com", @"spki": @"u6g6X+7KXDBCQ91pQEWODwB8+hbZW43/7pbjYIAzpNU=" }, | ||
| @{ @"host": @"www.cloudflare.com", @"spki": @"b1g23yk9nBZr6VXWRfLBYuSS/kzvrmf565y9Cs+3sNw=" }, | ||
| ]; | ||
| }); | ||
| return hosts; | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
#!/bin/bash
# Recompute SHA-256(base64) of SecKeyCopyExternalRepresentation()-equivalent
# raw public-key bytes for each pinned host and compare to the baked values.
set -euo pipefail
compute_pin() {
local host="$1"
# Get leaf cert
local pem
pem=$(echo | openssl s_client -servername "$host" -connect "${host}:443" -showcerts 2>/dev/null \
| awk '/-BEGIN CERTIFICATE-/,/-END CERTIFICATE-/{print; if(/-END/) exit}')
# Extract leaf public key in DER (this gives ASN.1 SPKI, NOT what the code hashes).
# The code hashes SecKeyCopyExternalRepresentation output:
# RSA -> PKCS#1 RSAPublicKey (modulus + exponent)
# EC -> X9.63 uncompressed point (0x04 || X || Y)
local key_alg
key_alg=$(echo "$pem" | openssl x509 -noout -pubkey | openssl pkey -pubin -text -noout 2>/dev/null | head -1)
echo "host=$host"
echo "key_alg=$key_alg"
if echo "$key_alg" | grep -qi 'RSA'; then
# PKCS#1 RSAPublicKey DER:
echo "$pem" | openssl x509 -noout -pubkey \
| openssl rsa -pubin -RSAPublicKey_out -outform DER 2>/dev/null \
| openssl dgst -sha256 -binary | base64
else
# EC X9.63 uncompressed point: extract the BIT STRING from SPKI minus header.
# Approximation: use openssl ec -pubin -outform DER then strip the SPKI prefix.
echo "(EC pin recomputation requires X9.63 raw point — recompute on a Mac with SecKeyCopyExternalRepresentation)"
fi
echo "---"
}
compute_pin www.apple.com
compute_pin www.google.com
compute_pin www.cloudflare.comRepository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 127
Script executed:
# Find references to SCPinnedHosts and understand the unlock/gate logic
rg -l "SCPinnedHosts" --type objc --type swiftRepository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 0
stdout:
SelfControlTests/SCTrustedTimeTests.m
Common/Utility/SCTrustedTime.m
Common/Utility/SCPinnedHosts.m
Common/Utility/SCPinnedHosts.h
Script executed:
# Search for usage of the hosts method and understand the verification flow
rg "hosts\]" --type objc -A 3 -B 3 | head -100Repository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 0
stdout:
SelfControlTests/SCTrustedTimeTests.m-
SelfControlTests/SCTrustedTimeTests.m-- (void)testFetchFromAppleWithCorrectPinReturnsDate {
SelfControlTests/SCTrustedTimeTests.m- XCTestExpectation* exp = [self expectationWithDescription: @"apple"];
SelfControlTests/SCTrustedTimeTests.m: NSDictionary* apple = [SCPinnedHosts hosts][0]; // assumes apple is index 0
SelfControlTests/SCTrustedTimeTests.m- [SCTrustedTime fetchTimeFromHost: apple[@"host"]
SelfControlTests/SCTrustedTimeTests.m- expectedSPKI: apple[@"spki"]
SelfControlTests/SCTrustedTimeTests.m- timeout: 10.0
--
Common/Utility/SCTrustedTime.m-}
Common/Utility/SCTrustedTime.m-
Common/Utility/SCTrustedTime.m-+ (void)verifyTimeIsAfter:(NSDate*)threshold completion:(SCTrustedTimeQuorumCompletion)completion {
Common/Utility/SCTrustedTime.m: NSArray* hosts = [SCPinnedHosts hosts];
Common/Utility/SCTrustedTime.m- dispatch_group_t group = dispatch_group_create();
Common/Utility/SCTrustedTime.m- NSMutableArray<NSDate*>* results = [NSMutableArray array];
Common/Utility/SCTrustedTime.m- NSLock* lock = [NSLock new];
Script executed:
# Look for quorum logic or multiple host verification
rg -i "quorum|verify|pin" --type objc --type swift | head -50Repository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 0
stdout:
Sparkle.framework/Versions/A/Headers/SUUpdater.h: This will find updates that the user has opted into skipping.
Sparkle.framework/Versions/A/Headers/SUUpdater.h: This will not find updates that the user has opted into skipping.
SelfControlTests/SCUtilityTests.m: NSMutableDictionary* tk = [[s valueForKey: @"BlockTimekeeping"] mutableCopy];
SelfControlTests/SCUtilityTests.m: [s setValue: tk forKey: @"BlockTimekeeping"];
SelfControlTests/SCUtilityTests.m: [s setValue: nil forKey: @"BlockTimekeeping"];
SelfControlTests/SCTrustedTimeTests.m:`#import` "SCPinnedHosts.h"
SelfControlTests/SCTrustedTimeTests.m:- (void)testFetchFromAppleWithCorrectPinReturnsDate {
SelfControlTests/SCTrustedTimeTests.m: NSDictionary* apple = [SCPinnedHosts hosts][0]; // assumes apple is index 0
SelfControlTests/SCTrustedTimeTests.m:- (void)testFetchWithWrongPinFails {
SelfControlTests/SCTrustedTimeTests.m: XCTestExpectation* exp = [self expectationWithDescription: @"badpin"];
SelfControlTests/SCTrustedTimeTests.m:- (void)testVerifyAfterPastThresholdSucceeds {
SelfControlTests/SCTrustedTimeTests.m: XCTestExpectation* exp = [self expectationWithDescription: @"quorum"];
SelfControlTests/SCTrustedTimeTests.m: [SCTrustedTime verifyTimeIsAfter: past
SelfControlTests/SCTrustedTimeTests.m:- (void)testVerifyAfterFutureThresholdFails {
SelfControlTests/SCTrustedTimeTests.m: [SCTrustedTime verifyTimeIsAfter: future
SelfControlTests/SCBlockClockTests.m: [[SCSettings sharedSettings] setValue: nil forKey: @"BlockTimekeeping"];
SelfControlTests/SCBlockClockTests.m:- (void)testRecordBlockStartPopulatesTimekeepingDict {
SelfControlTests/SCBlockClockTests.m: NSDictionary* tk = [[SCSettings sharedSettings] valueForKey: @"BlockTimekeeping"];
SelfControlTests/SCBlockClockTests.m: NSDictionary* first = [[[SCSettings sharedSettings] valueForKey: @"BlockTimekeeping"] copy];
SelfControlTests/SCBlockClockTests.m: NSDictionary* second = [[SCSettings sharedSettings] valueForKey: @"BlockTimekeeping"];
SelfControlTests/SCBlockClockTests.m: NSMutableDictionary* tk = [[[SCSettings sharedSettings] valueForKey: @"BlockTimekeeping"] mutableCopy];
SelfControlTests/SCBlockClockTests.m: [[SCSettings sharedSettings] setValue: tk forKey: @"BlockTimekeeping"];
SelfControlTests/SCBlockClockTests.m: NSMutableDictionary* tk = [[[SCSettings sharedSettings] valueForKey: @"BlockTimekeeping"] mutableCopy];
SelfControlTests/SCBlockClockTests.m: [[SCSettings sharedSettings] setValue: tk forKey: @"BlockTimekeeping"];
SelfControlTests/SCBlockClockTests.m: NSMutableDictionary* tk = [[[SCSettings sharedSettings] valueForKey: @"BlockTimekeeping"] mutableCopy];
SelfControlTests/SCBlockClockTests.m: [[SCSettings sharedSettings] setValue: tk forKey: @"BlockTimekeeping"];
SelfControlTests/SCBlockClockTests.m: NSMutableDictionary* tk = [[[SCSettings sharedSettings] valueForKey: @"BlockTimekeeping"] mutableCopy];
SelfControlTests/SCBlockClockTests.m: [[SCSettings sharedSettings] setValue: tk forKey: @"BlockTimekeeping"];
SCConstants.m: @"Ping",
SCConstants.m: @"VerifyInternetConnection": `@YES`,
Daemon/SCDaemonBlockMethods.m:static NSTimeInterval const kVerifyBackoffs[] = { 10.0, 30.0, 60.0, 120.0, 120.0 };
Daemon/SCDaemonBlockMethods.m:static const NSUInteger kVerifyBackoffsCount = sizeof(kVerifyBackoffs) / sizeof(kVerifyBackoffs[0]);
Daemon/SCDaemonBlockMethods.m: [SCTrustedTime verifyTimeIsAfter: endDate completion:^(BOOL ok, NSDate* med, NSError* err) {
Daemon/SCDaemonBlockMethods.m: NSTimeInterval backoff = kVerifyBackoffs[ MIN(attempt, kVerifyBackoffsCount - 1) ];
Common/Utility/SCTrustedTime.m:`#import` "SCPinnedHosts.h"
Common/Utility/SCTrustedTime.m:`@interface` SCTrustedTimePinningDelegate : NSObject <NSURLSessionDelegate>
Common/Utility/SCTrustedTime.m:`@implementation` SCTrustedTimePinningDelegate
Common/Utility/SCTrustedTime.m: SCTrustedTimePinningDelegate* d = [SCTrustedTimePinningDelegate new];
Common/Utility/SCTrustedTime.m:+ (void)verifyTimeIsAfter:(NSDate*)threshold completion:(SCTrustedTimeQuorumCompletion)completion {
Common/Utility/SCTrustedTime.m: NSArray* hosts = [SCPinnedHosts hosts];
Common/Utility/SCTrustedTime.m: userInfo: @{NSLocalizedDescriptionKey: @"Quorum not reached"}]);
Common/Utility/SCTrustedTime.m: userInfo: @{NSLocalizedDescriptionKey: @"Pinned hosts disagree"}]);
Common/Utility/SCPinnedHosts.m:`#import` "SCPinnedHosts.h"
Common/Utility/SCPinnedHosts.m:`@implementation` SCPinnedHosts
Common/Utility/SCPinnedHosts.m: // Pins are SHA-256 (base64) of SecKeyCopyExternalRepresentation() output
Common/Utility/SCPinnedHosts.m: // To recompute, see the SCTrustedTime pinning delegate in SCTrustedTime.m.
Common/Utility/SCPinnedHosts.h:/// Baked-in pin list for the trusted-time HTTPS gate. Each entry is
Common/Utility/SCPinnedHosts.h:/// The quorum check requires 2 of 3 to succeed, so a single rotation
Common/Utility/SCPinnedHosts.h:`@interface` SCPinnedHosts : NSObject
Common/Utility/SCMigrationUtilities.m: NSLog(@"Skipping copy to defaults because migration to V4 was already completed.");
Script executed:
# Find where the unlock gate logic is implemented
fd -name "*.m" -o -name "*.swift" | xargs rg -l "unlock|gate" | head -20Repository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 0
stdout:
fr.lproj/PreferencesAdvancedViewController.strings
docs/superpowers/specs/2026-05-14-tamper-resistant-timing-design.md
docs/superpowers/plans/2026-05-14-tamper-resistant-timing.md
TimerWindowController.m
TimerWindowController.h
Sparkle.framework/Versions/A/_CodeSignature/CodeResources
Sparkle.framework/Versions/A/Headers/Sparkle.h
Sparkle.framework/Versions/A/Headers/SUUpdaterDelegate.h
Sparkle.framework/Versions/A/Headers/SUUpdater.h
Sparkle.framework/Versions/A/Headers/SPUDownloaderDelegate.h
Sparkle.framework/Versions/A/Headers/SPUDownloader.h
SelfControl.xcodeproj/project.pbxproj
SelfControl Killer/fr.lproj/MainMenu.xib
SelfControl Killer/Base.lproj/MainMenu.xib
SelfControl Killer/AppDelegate.m
SelfControl Killer/AppDelegate.h
DomainListWindowController.m
Daemon/SCDaemonXPC.m
Daemon/SCDaemonProtocol.h
Daemon/SCDaemonBlockMethods.m
stderr:
error: Found argument '-n' which wasn't expected, or isn't valid in this context
If you tried to supply '-n' as a value rather than a flag, use '-- -n'
Usage: fd [OPTIONS] [pattern] [path]...
For more information try '--help'
Script executed:
cat -n Common/Utility/SCTrustedTime.m | head -150Repository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 0
stdout:
1 `#import` "SCTrustedTime.h"
2 `#import` "SCPinnedHosts.h"
3 `#import` <CommonCrypto/CommonDigest.h>
4 `#import` <Security/Security.h>
5
6 `@interface` SCTrustedTimePinningDelegate : NSObject <NSURLSessionDelegate>
7 `@property` (copy) NSString* expectedSPKI;
8 `@end`
9
10 `@implementation` SCTrustedTimePinningDelegate
11
12 - (void)URLSession:(NSURLSession*)session
13 didReceiveChallenge:(NSURLAuthenticationChallenge*)challenge
14 completionHandler:(void(^)(NSURLSessionAuthChallengeDisposition, NSURLCredential* _Nullable))ch
15 {
16 if (![challenge.protectionSpace.authenticationMethod isEqualToString: NSURLAuthenticationMethodServerTrust]) {
17 ch(NSURLSessionAuthChallengeCancelAuthenticationChallenge, nil);
18 return;
19 }
20 SecTrustRef trust = challenge.protectionSpace.serverTrust;
21 if (trust == NULL) { ch(NSURLSessionAuthChallengeCancelAuthenticationChallenge, nil); return; }
22
23 CFIndex count = SecTrustGetCertificateCount(trust);
24 BOOL matched = NO;
25 for (CFIndex i = 0; i < count && !matched; i++) {
26 SecCertificateRef cert = SecTrustGetCertificateAtIndex(trust, i);
27 SecKeyRef pubkey = SecCertificateCopyKey(cert);
28 if (pubkey == NULL) continue;
29 CFErrorRef err = NULL;
30 CFDataRef der = SecKeyCopyExternalRepresentation(pubkey, &err);
31 CFRelease(pubkey);
32 if (der == NULL) continue;
33 unsigned char digest[CC_SHA256_DIGEST_LENGTH];
34 CC_SHA256(CFDataGetBytePtr(der), (CC_LONG)CFDataGetLength(der), digest);
35 NSData* digestData = [NSData dataWithBytes: digest length: CC_SHA256_DIGEST_LENGTH];
36 NSString* b64 = [digestData base64EncodedStringWithOptions: 0];
37 CFRelease(der);
38 if ([b64 isEqualToString: self.expectedSPKI]) matched = YES;
39 }
40
41 if (matched) {
42 ch(NSURLSessionAuthChallengeUseCredential, [NSURLCredential credentialForTrust: trust]);
43 } else {
44 ch(NSURLSessionAuthChallengeCancelAuthenticationChallenge, nil);
45 }
46 }
47
48 `@end`
49
50 `@implementation` SCTrustedTime
51
52 + (NSDateFormatter*)imfFormatter {
53 static NSDateFormatter* f = nil;
54 static dispatch_once_t once;
55 dispatch_once(&once, ^{
56 f = [[NSDateFormatter alloc] init];
57 f.locale = [NSLocale localeWithLocaleIdentifier: @"en_US_POSIX"];
58 f.timeZone = [NSTimeZone timeZoneWithAbbreviation: @"GMT"];
59 f.dateFormat = @"EEE, dd MMM yyyy HH:mm:ss zzz";
60 });
61 return f;
62 }
63
64 + (NSDate*)parseHTTPDateHeader:(NSString*)header {
65 if (header.length == 0) return nil;
66 return [[self imfFormatter] dateFromString: header];
67 }
68
69 + (void)fetchTimeFromHost:(NSString*)host
70 expectedSPKI:(NSString*)spki
71 timeout:(NSTimeInterval)timeoutSeconds
72 completion:(SCTrustedTimeCompletion)completion
73 {
74 SCTrustedTimePinningDelegate* d = [SCTrustedTimePinningDelegate new];
75 d.expectedSPKI = spki;
76 NSURLSessionConfiguration* cfg = [NSURLSessionConfiguration ephemeralSessionConfiguration];
77 cfg.timeoutIntervalForRequest = timeoutSeconds;
78 NSURLSession* session = [NSURLSession sessionWithConfiguration: cfg delegate: d delegateQueue: nil];
79
80 NSURL* url = [NSURL URLWithString: [NSString stringWithFormat: @"https://%@/", host]];
81 NSMutableURLRequest* req = [NSMutableURLRequest requestWithURL: url];
82 req.HTTPMethod = @"HEAD";
83
84 [[session dataTaskWithRequest: req completionHandler:^(NSData* data, NSURLResponse* resp, NSError* error) {
85 [session finishTasksAndInvalidate];
86 if (error) { completion(nil, error); return; }
87 NSHTTPURLResponse* http = (NSHTTPURLResponse*)resp;
88 NSString* dateStr = http.allHeaderFields[@"Date"];
89 NSDate* parsed = [SCTrustedTime parseHTTPDateHeader: dateStr];
90 if (parsed == nil) {
91 completion(nil, [NSError errorWithDomain: @"SCTrustedTime" code: 1
92 userInfo: @{NSLocalizedDescriptionKey: @"No/invalid Date header"}]);
93 return;
94 }
95 completion(parsed, nil);
96 }] resume];
97 }
98
99 + (void)verifyTimeIsAfter:(NSDate*)threshold completion:(SCTrustedTimeQuorumCompletion)completion {
100 NSArray* hosts = [SCPinnedHosts hosts];
101 dispatch_group_t group = dispatch_group_create();
102 NSMutableArray<NSDate*>* results = [NSMutableArray array];
103 NSLock* lock = [NSLock new];
104
105 for (NSDictionary* hp in hosts) {
106 dispatch_group_enter(group);
107 [self fetchTimeFromHost: hp[@"host"]
108 expectedSPKI: hp[@"spki"]
109 timeout: 8.0
110 completion:^(NSDate* d, NSError* e) {
111 if (d != nil) {
112 [lock lock]; [results addObject: d]; [lock unlock];
113 }
114 dispatch_group_leave(group);
115 }];
116 }
117
118 dispatch_group_notify(group, dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{
119 if (results.count < 2) {
120 completion(NO, nil, [NSError errorWithDomain: @"SCTrustedTime" code: 2
121 userInfo: @{NSLocalizedDescriptionKey: @"Quorum not reached"}]);
122 return;
123 }
124 NSArray* sorted = [results sortedArrayUsingSelector: `@selector`(compare:)];
125 NSDate* min = sorted.firstObject;
126 NSDate* max = sorted.lastObject;
127 if ([max timeIntervalSinceDate: min] > 300.0) {
128 completion(NO, nil, [NSError errorWithDomain: @"SCTrustedTime" code: 3
129 userInfo: @{NSLocalizedDescriptionKey: @"Pinned hosts disagree"}]);
130 return;
131 }
132 NSDate* median = sorted[sorted.count / 2];
133 BOOL ok = [median compare: threshold] != NSOrderedAscending;
134 completion(ok, median, nil);
135 });
136 }
137
138 `@end`
Script executed:
cat -n Common/Utility/SCPinnedHosts.hRepository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 0
stdout:
1 `#import` <Foundation/Foundation.h>
2
3 NS_ASSUME_NONNULL_BEGIN
4
5 /// Baked-in pin list for the trusted-time HTTPS gate. Each entry is
6 /// @{ @"host": @"<hostname>", @"spki": @"<base64 SHA-256 of leaf SPKI>" }.
7 /// The quorum check requires 2 of 3 to succeed, so a single rotation
8 /// failure does not break the gate.
9 `@interface` SCPinnedHosts : NSObject
10
11 + (NSArray<NSDictionary<NSString*, NSString*>*>*)hosts;
12
13 `@end`
14
15 NS_ASSUME_NONNULL_END
Script executed:
# Look at where unlock/gate logic calls verifyTimeIsAfter
rg -A 10 "verifyTimeIsAfter" Daemon/SCDaemonBlockMethods.mRepository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 0
stdout:
[SCTrustedTime verifyTimeIsAfter: endDate completion:^(BOOL ok, NSDate* med, NSError* err) {
if (ok) {
NSLog(@"INFO: Verified unlock — removing block.");
[settings setValue: nil forKey: @"BlockUnlockGate"];
[settings setValue: @(0) forKey: @"BlockUnlockAttempt"];
[SCHelperToolUtilities removeBlock];
[SCHelperToolUtilities sendConfigurationChangedNotification];
[SCSentry addBreadcrumb: @"Daemon performed verified unlock" category: @"daemon"];
[[SCDaemon sharedDaemon] stopCheckupTimer];
[[SCDaemon sharedDaemon] stopCheckpointTimer];
return;
Script executed:
rg -i "rotate|fallback|backup|next.*pin|alternate" --type objc --type swift | grep -v "node_modules" | head -30Repository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 0
stdout:
Daemon/SCDaemonBlockMethods.m: NSLog(@"WARNING: Error removing host file block. Attempting to restore backup.");
Daemon/SCDaemonBlockMethods.m: if([hostFileBlockerSet restoreBackupHostsFile])
Daemon/SCDaemonBlockMethods.m: NSLog(@"INFO: Host file backup restored.");
Daemon/SCDaemonBlockMethods.m: NSLog(@"ERROR: Host file backup could not be restored. This may result in a permanent block.");
Daemon/SCDaemonBlockMethods.m: // Get rid of the backup file since we're about to make a new one.
Daemon/SCDaemonBlockMethods.m: [hostFileBlockerSet deleteBackupHostsFile];
Block Management/HostFileBlockerSet.m: NSArray<NSString*>* commonBackupHostFilePaths = @[
Block Management/HostFileBlockerSet.m: NSMutableArray* hostFileBlockers = [NSMutableArray arrayWithCapacity: commonBackupHostFilePaths.count + 1];
Block Management/HostFileBlockerSet.m: for (NSString* path in commonBackupHostFilePaths) {
Block Management/HostFileBlockerSet.m: NSLog(@"INFO: found backup VPN host file at %@", path);
Block Management/HostFileBlockerSet.m:- (BOOL)deleteBackupHostsFile {
Block Management/HostFileBlockerSet.m: ret = ret && [blocker deleteBackupHostsFile];
Block Management/HostFileBlockerSet.m:- (BOOL)createBackupHostsFile {
Block Management/HostFileBlockerSet.m: ret = ret && [blocker createBackupHostsFile];
Block Management/HostFileBlockerSet.m:- (BOOL)restoreBackupHostsFile {
Block Management/HostFileBlockerSet.m: ret = ret && [blocker restoreBackupHostsFile];
Block Management/HostFileBlocker.h:- (BOOL)deleteBackupHostsFile;
Block Management/HostFileBlocker.h:- (BOOL)createBackupHostsFile;
Block Management/HostFileBlocker.h:- (BOOL)restoreBackupHostsFile;
Block Management/BlockManager.m: [hostBlockerSet createBackupHostsFile];
Block Management/BlockManager.m: NSLog(@"WARNING: Error removing hostfile block. Attempting to restore host file backup.");
Block Management/BlockManager.m: [hostBlockerSet restoreBackupHostsFile];
Block Management/BlockManager.m: NSLog(@"ERROR: Host file backup could not be restored. This may result in a permanent block.");
Block Management/BlockManager.m: [hostBlockerSet deleteBackupHostsFile];
Block Management/BlockManager.m: NSLog(@"WARNING: Error removing hostfile block. Attempting to restore host file backup.");
Block Management/BlockManager.m: [hostBlockerSet restoreBackupHostsFile];
Block Management/BlockManager.m: NSLog(@"ERROR: Host file backup could not be restored. This may result in a permanent block.");
Block Management/HostFileBlocker.m:- (NSString*)backupHostFilePath {
Block Management/HostFileBlocker.m:- (BOOL)createBackupHostsFile {
Block Management/HostFileBlocker.m: [self deleteBackupHostsFile];
Script executed:
# Check the test to see if it actually validates the pins work
cat -n SelfControlTests/SCTrustedTimeTests.mRepository: SelfControlApp/selfcontrol
Repository: SelfControlApp/selfcontrol
Exit code: 0
stdout:
1 `#import` <XCTest/XCTest.h>
2 `#import` "SCTrustedTime.h"
3 `#import` "SCPinnedHosts.h"
4
5 `@interface` SCTrustedTimeTests : XCTestCase
6 `@end`
7
8 `@implementation` SCTrustedTimeTests
9
10 - (void)testParseValidIMFFixdate {
11 NSDate* d = [SCTrustedTime parseHTTPDateHeader: @"Tue, 14 May 2026 12:00:00 GMT"];
12 XCTAssertNotNil(d);
13 NSDateComponents* c = [[NSCalendar calendarWithIdentifier: NSCalendarIdentifierGregorian]
14 componentsInTimeZone: [NSTimeZone timeZoneWithAbbreviation: @"GMT"]
15 fromDate: d];
16 XCTAssertEqual(c.year, 2026);
17 XCTAssertEqual(c.month, 5);
18 XCTAssertEqual(c.day, 14);
19 XCTAssertEqual(c.hour, 12);
20 }
21
22 - (void)testParseGarbageReturnsNil {
23 XCTAssertNil([SCTrustedTime parseHTTPDateHeader: @"not a date"]);
24 XCTAssertNil([SCTrustedTime parseHTTPDateHeader: @""]);
25 XCTAssertNil([SCTrustedTime parseHTTPDateHeader: nil]);
26 }
27
28 - (void)testFetchFromAppleWithCorrectPinReturnsDate {
29 XCTestExpectation* exp = [self expectationWithDescription: @"apple"];
30 NSDictionary* apple = [SCPinnedHosts hosts][0]; // assumes apple is index 0
31 [SCTrustedTime fetchTimeFromHost: apple[@"host"]
32 expectedSPKI: apple[@"spki"]
33 timeout: 10.0
34 completion:^(NSDate* d, NSError* e) {
35 XCTAssertNil(e);
36 XCTAssertNotNil(d);
37 XCTAssertLessThan(ABS([d timeIntervalSinceNow]), 120.0);
38 [exp fulfill];
39 }];
40 [self waitForExpectations: @[exp] timeout: 15.0];
41 }
42
43 - (void)testFetchWithWrongPinFails {
44 XCTestExpectation* exp = [self expectationWithDescription: @"badpin"];
45 [SCTrustedTime fetchTimeFromHost: @"www.apple.com"
46 expectedSPKI: @"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="
47 timeout: 10.0
48 completion:^(NSDate* d, NSError* e) {
49 XCTAssertNotNil(e);
50 XCTAssertNil(d);
51 [exp fulfill];
52 }];
53 [self waitForExpectations: @[exp] timeout: 15.0];
54 }
55
56 - (void)testVerifyAfterPastThresholdSucceeds {
57 XCTestExpectation* exp = [self expectationWithDescription: @"quorum"];
58 NSDate* past = [NSDate dateWithTimeIntervalSinceNow: -3600];
59 [SCTrustedTime verifyTimeIsAfter: past
60 completion:^(BOOL ok, NSDate* med, NSError* err) {
61 XCTAssertTrue(ok);
62 XCTAssertNotNil(med);
63 [exp fulfill];
64 }];
65 [self waitForExpectations: @[exp] timeout: 25.0];
66 }
67
68 - (void)testVerifyAfterFutureThresholdFails {
69 XCTestExpectation* exp = [self expectationWithDescription: @"future"];
70 NSDate* future = [NSDate dateWithTimeIntervalSinceNow: 86400 * 365 * 10];
71 [SCTrustedTime verifyTimeIsAfter: future
72 completion:^(BOOL ok, NSDate* med, NSError* err) {
73 XCTAssertFalse(ok);
74 [exp fulfill];
75 }];
76 [self waitForExpectations: @[exp] timeout: 25.0];
77 }
78
79 `@end`
Hardcoded leaf-cert pins without rotation strategy will lock users on routine certificate rotation.
These three hosts rotate leaf certificates regularly. The quorum check (2 of 3) tolerates a single host failure, but simultaneous rotation on two hosts — or one rotation coinciding with any other outage — will hard-lock legitimate users with no recovery path:
- Multiple acceptable pins per host (current + staged next) enable rotation without a release cycle.
- Intermediate-level pinning (GTS, DigiCert, Let's Encrypt roots) lasts years, not months, and still prevents MITM.
- Signed, out-of-band pin updates unblock users on older builds without requiring a recompile.
Verify each pin is current against the live leaf certificates before merge. The code hashes SecKeyCopyExternalRepresentation() output (raw public-key bytes, not the full SPKI) and checks all certificates in the trust chain, so pins must match the actual leaf keys deployed today.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/Utility/SCPinnedHosts.m` around lines 5 - 20, The hosts method in
SCPinnedHosts.m currently returns a single hardcoded leaf SPKI pin per host
which will break on routine rotations; update SCPinnedHosts + the hosts method
to accept multiple pins per host (array of current + staged leaf pins and/or
intermediate/root SPKI pins), and implement/consume a rotation-friendly strategy
such as: include long-lived intermediate/root pins alongside current leaf pins,
allow multiple acceptable pins when checking the trust chain, and add support
for signed out-of-band pin updates (fallback mechanism) so older builds can
receive new pins; also verify and replace the three provided pin strings by
recomputing SecKeyCopyExternalRepresentation() on the live leaf certificates
before merging.
Comment on lines
+14
to
+19
| /// Fetches https://<host>/ HEAD and returns the time from the Date: header IFF | ||
| /// the server's leaf SubjectPublicKeyInfo SHA-256 (base64) matches expectedSPKI. | ||
| + (void)fetchTimeFromHost:(NSString*)host | ||
| expectedSPKI:(NSString*)spki | ||
| timeout:(NSTimeInterval)timeoutSeconds | ||
| completion:(SCTrustedTimeCompletion)completion; |
There was a problem hiding this comment.
Misleading "SPKI" wording in the public API – this is NOT a SubjectPublicKeyInfo hash.
The implementation hashes the output of SecKeyCopyExternalRepresentation() (raw key bytes: PKCS#1 RSA modulus+exponent or X9.63 EC point), as confirmed by the comment in SCPinnedHosts.m. That is not an RFC 5280 SubjectPublicKeyInfo digest, and anyone recomputing pins from openssl x509 -pubkey ... | openssl pkey -pubin -outform DER | sha256sum will get a different value and bake in pins that never match. Please correct the doc comment, and consider renaming the parameter (e.g. expectedPublicKeyHash) so the public surface matches what the code actually compares.
📝 Suggested wording fix
-/// Fetches https://<host>/ HEAD and returns the time from the Date: header IFF
-/// the server's leaf SubjectPublicKeyInfo SHA-256 (base64) matches expectedSPKI.
+/// Fetches https://<host>/ HEAD and returns the time from the Date: header IFF
+/// SHA-256 (base64) of `SecKeyCopyExternalRepresentation()` for some certificate
+/// in the server's chain matches `expectedPublicKeyHash`. NOTE: this is the raw
+/// public-key bytes (RSA modulus+exponent or EC point), NOT the ASN.1 SPKI hash
+/// produced by typical `openssl ... -pubkey` recipes.
+ (void)fetchTimeFromHost:(NSString*)host
- expectedSPKI:(NSString*)spki
+ expectedPublicKeyHash:(NSString*)keyHash
timeout:(NSTimeInterval)timeoutSeconds
completion:(SCTrustedTimeCompletion)completion;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/Utility/SCTrustedTime.h` around lines 14 - 19, The public API and doc
comment for fetchTimeFromHost:expectedSPKI:timeout:completion are misleading:
the implementation hashes SecKeyCopyExternalRepresentation() (raw public key
bytes), not an RFC5280 SubjectPublicKeyInfo digest. Change the doc comment to
state that the method compares the SHA-256 (base64) of the public key's raw
external representation, rename the parameter expectedSPKI to
expectedPublicKeyHash (or similar) to reflect this, and update all callers/tests
to use the new name and the correct hash computation method so pins computed via
openssl x509 -pubkey ... | openssl pkey -pubin -outform DER | sha256sum no
longer mismatch.
Comment on lines
+29
to
+39
| CFErrorRef err = NULL; | ||
| CFDataRef der = SecKeyCopyExternalRepresentation(pubkey, &err); | ||
| CFRelease(pubkey); | ||
| if (der == NULL) continue; | ||
| unsigned char digest[CC_SHA256_DIGEST_LENGTH]; | ||
| CC_SHA256(CFDataGetBytePtr(der), (CC_LONG)CFDataGetLength(der), digest); | ||
| NSData* digestData = [NSData dataWithBytes: digest length: CC_SHA256_DIGEST_LENGTH]; | ||
| NSString* b64 = [digestData base64EncodedStringWithOptions: 0]; | ||
| CFRelease(der); | ||
| if ([b64 isEqualToString: self.expectedSPKI]) matched = YES; | ||
| } |
There was a problem hiding this comment.
Leak of CFErrorRef on SecKeyCopyExternalRepresentation failure, plus chain-wide match diverges from "leaf" pinning docs.
Two issues in the cert-iteration block:
- CFErrorRef leak (Line 29-32). When
SecKeyCopyExternalRepresentationreturnsNULL, it populateserrwith a +1 retainedCFErrorRef. The code doescontinuewithout everCFRelease-ing it. - Chain-wide match vs leaf-only contract (Line 25-39). The header advertises leaf SPKI matching, but this loop accepts a match against any certificate in the chain (intermediates, root). For these pinned hosts this is unlikely to misfire in practice, but it broadens the pin's trust scope beyond what the docs promise and makes leaf pin rotation indistinguishable from CA rotation. If leaf pinning is the intent, restrict to index 0.
🛡️ Proposed fix
- CFIndex count = SecTrustGetCertificateCount(trust);
- BOOL matched = NO;
- for (CFIndex i = 0; i < count && !matched; i++) {
- SecCertificateRef cert = SecTrustGetCertificateAtIndex(trust, i);
- SecKeyRef pubkey = SecCertificateCopyKey(cert);
- if (pubkey == NULL) continue;
- CFErrorRef err = NULL;
- CFDataRef der = SecKeyCopyExternalRepresentation(pubkey, &err);
- CFRelease(pubkey);
- if (der == NULL) continue;
- unsigned char digest[CC_SHA256_DIGEST_LENGTH];
- CC_SHA256(CFDataGetBytePtr(der), (CC_LONG)CFDataGetLength(der), digest);
- NSData* digestData = [NSData dataWithBytes: digest length: CC_SHA256_DIGEST_LENGTH];
- NSString* b64 = [digestData base64EncodedStringWithOptions: 0];
- CFRelease(der);
- if ([b64 isEqualToString: self.expectedSPKI]) matched = YES;
- }
+ BOOL matched = NO;
+ // Leaf cert is always at index 0; pin only against the leaf per the contract.
+ SecCertificateRef cert = SecTrustGetCertificateAtIndex(trust, 0);
+ SecKeyRef pubkey = cert ? SecCertificateCopyKey(cert) : NULL;
+ if (pubkey != NULL) {
+ CFErrorRef err = NULL;
+ CFDataRef der = SecKeyCopyExternalRepresentation(pubkey, &err);
+ CFRelease(pubkey);
+ if (der != NULL) {
+ unsigned char digest[CC_SHA256_DIGEST_LENGTH];
+ CC_SHA256(CFDataGetBytePtr(der), (CC_LONG)CFDataGetLength(der), digest);
+ NSData* digestData = [NSData dataWithBytes: digest length: CC_SHA256_DIGEST_LENGTH];
+ NSString* b64 = [digestData base64EncodedStringWithOptions: 0];
+ CFRelease(der);
+ if ([b64 isEqualToString: self.expectedSPKI]) matched = YES;
+ } else if (err != NULL) {
+ CFRelease(err);
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Common/Utility/SCTrustedTime.m` around lines 29 - 39, The loop that pins
SPKIs leaks the CFErrorRef from SecKeyCopyExternalRepresentation when it returns
NULL and also checks every cert in the chain instead of only the leaf; fix by
releasing the error on failure (CFRelease(err)) before continuing and by
limiting the SPKI comparison to the leaf certificate only (i.e., only perform
SecKeyCopyExternalRepresentation/CC_SHA256/base64 compare for the cert at index
0), using the existing symbols SecKeyCopyExternalRepresentation, CFErrorRef err,
der, expectedSPKI and matched to locate the code to change.
| # Tamper-Resistant Block Timing — Design | ||
|
|
||
| **Date:** 2026-05-14 | ||
| **Status:** Approved (design phase). Implementation plan not yet written. |
There was a problem hiding this comment.
Status line is stale.
Line 4 says “Implementation plan not yet written,” but this PR includes docs/superpowers/plans/2026-05-14-tamper-resistant-timing.md. Update status text to match current state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-14-tamper-resistant-timing-design.md` at line
4, The status line in the spec header is stale: change the text that currently
reads “Implementation plan not yet written” to reflect that the implementation
plan is included (e.g., “Implementation plan written — see
docs/superpowers/plans/2026-05-14-tamper-resistant-timing.md”) so the spec
header (the Status line in the document titled tamper-resistant-timing-design)
accurately reflects the current state.
Comment on lines
+19
to
+20
| - Editing the on-disk settings/checkpoint files | ||
| - Deleting the on-disk settings/checkpoint files |
There was a problem hiding this comment.
Threat-model claim overstates protection against settings-file deletion.
Line 19–Line 20 says deletion of settings/checkpoint files is defended against, but the PR context documents that SCSettings wiping can remove blocks. The spec should explicitly downgrade this guarantee (or mark as known limitation) to avoid a false security claim.
Also applies to: 121-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-14-tamper-resistant-timing-design.md` around
lines 19 - 20, The spec's threat-model claim "Editing the on-disk
settings/checkpoint files" and "Deleting the on-disk settings/checkpoint files"
overstates protection because SCSettings wiping can remove blocks; update those
phrases (at the occurrences currently at line 19–20 and again at 121–122) to
downgrade the guarantee or mark it as a known limitation, e.g., change to
"Editing/deleting on-disk settings/checkpoint files: may be detectable but can
still enable SCSettings wiping that can remove blocks — see known limitation"
and add a one-line pointer to the PR/context that documents SCSettings wiping so
readers know the exact failure mode.
Comment on lines
+60
to
+62
| ``` | ||
| continuousElapsed = (mach_continuous_time() - blockStartContinuousTime) / 1e9 | ||
| ``` |
There was a problem hiding this comment.
Add fence languages for markdown code blocks.
These fenced blocks are missing language identifiers and will fail markdown lint checks.
Also applies to: 80-89, 95-105, 196-202
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-14-tamper-resistant-timing-design.md` around
lines 60 - 62, The markdown code fences containing the snippet with
continuousElapsed and calls like mach_continuous_time() lack language
identifiers and trigger lint failures; update those fenced code blocks (e.g.,
the block that computes continuousElapsed = (mach_continuous_time() -
blockStartContinuousTime) / 1e9 and the other blocks referenced at lines 80-89,
95-105, and 196-202) by adding the appropriate language tag (such as `swift`,
`c`, or `pseudo` depending on the snippet) immediately after the opening ``` so
the markdown linter recognizes the code language.
Comment on lines
+192
to
+204
| if (blockElapsedButStillRunning && self.waitingForNetworkUnlock) { | ||
| if (![timerLabel_.stringValue isEqualToString: waitingForInternetString]) { | ||
| [[NSApp dockTile] setBadgeLabel: nil]; | ||
| [timerLabel_ setStringValue: waitingForInternetString]; | ||
| [timerLabel_ setFont: [[NSFontManager sharedFontManager] | ||
| convertFont: [timerLabel_ font] | ||
| toSize: 14]]; | ||
| [timerLabel_ sizeToFit]; | ||
| [timerLabel_ setFrame: NSRectFromCGRect(CGRectMake(0, timerLabel_.frame.origin.y, self.window.frame.size.width, timerLabel_.frame.size.height))]; | ||
| } | ||
| [self resetStrikes]; | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "*.m" -type f 2>/dev/null | head -5Repository: SelfControlApp/selfcontrol
Length of output: 1966
🏁 Script executed:
git ls-files --others --exclude-standard | head -20Repository: SelfControlApp/selfcontrol
Length of output: 82
🏁 Script executed:
ls -laRepository: SelfControlApp/selfcontrol
Length of output: 5909
🏁 Script executed:
find . -name "*.xib" -o -name "*.storyboard" 2>/dev/nullRepository: SelfControlApp/selfcontrol
Length of output: 394
🏁 Script executed:
find . -name "TimerWindowController.m" -type f 2>/dev/nullRepository: SelfControlApp/selfcontrol
Length of output: 96
🏁 Script executed:
git ls-files | grep -E "\.(xib|storyboard|m)$" | head -20Repository: SelfControlApp/selfcontrol
Length of output: 680
🏁 Script executed:
wc -l TimerWindowController.mRepository: SelfControlApp/selfcontrol
Length of output: 98
🏁 Script executed:
sed -n '155,170p' TimerWindowController.mRepository: SelfControlApp/selfcontrol
Length of output: 1206
🏁 Script executed:
sed -n '192,210p' TimerWindowController.mRepository: SelfControlApp/selfcontrol
Length of output: 1079
🏁 Script executed:
head -100 Base.lproj/TimerWindow.xib | tail -50Repository: SelfControlApp/selfcontrol
Length of output: 4585
🏁 Script executed:
cat Base.lproj/TimerWindow.xib | grep -i "timerlabel" -A 5 -B 5Repository: SelfControlApp/selfcontrol
Length of output: 1082
🏁 Script executed:
grep -n "id=\"33\"" Base.lproj/TimerWindow.xib -A 10Repository: SelfControlApp/selfcontrol
Length of output: 1359
🏁 Script executed:
wc -l Base.lproj/TimerWindow.xibRepository: SelfControlApp/selfcontrol
Length of output: 101
🏁 Script executed:
grep "outlets" Base.lproj/TimerWindow.xib -A 20 | head -40Repository: SelfControlApp/selfcontrol
Length of output: 52
🏁 Script executed:
cat Base.lproj/TimerWindow.xib | sed -n '61,68p'Repository: SelfControlApp/selfcontrol
Length of output: 903
🏁 Script executed:
sed -n '61,75p' Base.lproj/TimerWindow.xibRepository: SelfControlApp/selfcontrol
Length of output: 1585
🏁 Script executed:
sed -n '159,165p' TimerWindowController.mRepository: SelfControlApp/selfcontrol
Length of output: 637
🏁 Script executed:
sed -n '159,165p' TimerWindowController.mRepository: SelfControlApp/selfcontrol
Length of output: 637
Fix timerLabel_ configuration in TimerWindow.xib to support multi-line text.
The waitingForInternetString (lines 159-165) contains a literal \n combining two sentences, but timerLabel_ (id="33" in Base.lproj/TimerWindow.xib) is configured with lineBreakMode="clipping" instead of a wrapping mode like byWordWrapping. This causes the second line to be clipped rather than displayed. Change the lineBreakMode in the XIB and ensure the textFieldCell allows multi-line rendering, or the waiting-for-internet message will only show the first sentence to users.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TimerWindowController.m` around lines 192 - 204, The timerLabel_ in
TimerWindow.xib (id="33") is configured to clip lines so
waitingForInternetString (the string containing "\n") gets truncated; open
TimerWindow.xib, select the text field with id 33 (timerLabel_), change its
cell's lineBreakMode from "clipping" to "byWordWrapping" (or equivalent wrapping
mode), disable single-line-only mode / enable multi-line rendering on the
NSTextFieldCell (ensure usesSingleLineMode = NO / wraps = YES), and save the XIB
so timerLabel_ can render both lines when TimerWindowController sets the
waitingForInternetString.
Comment on lines
+374
to
+388
| - (void)queryUnlockGateState { | ||
| if (self.gateQueryInFlight) return; | ||
| self.gateQueryInFlight = YES; | ||
|
|
||
| if (self.gateXPCClient == nil) { | ||
| self.gateXPCClient = [SCXPCClient new]; | ||
| } | ||
|
|
||
| [self.gateXPCClient getBlockUnlockGateStateWithReply:^(BOOL waiting, NSDate* lastAttemptAt, NSString* errorReason) { | ||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| self.waitingForNetworkUnlock = waiting; | ||
| self.gateQueryInFlight = NO; | ||
| }); | ||
| }]; | ||
| } |
There was a problem hiding this comment.
gateQueryInFlight relies on the XPC reply always firing.
The coalescing flag is only cleared inside the reply block (line 385). SCXPCClient.getBlockUnlockGateStateWithReply: does route both connectError and proxyError back through the same reply, so in practice the flag should clear — but there's no timeout, so if the daemon hangs (or the XPC connection sits in a stalled state without invalidating), gateQueryInFlight stays YES forever and the UI never re-polls.
Consider one of:
- a watchdog
dispatch_afterthat flips the flag back toNOafter a few seconds if it hasn't already, - or capturing
selfweakly and ignoring late replies if a newer query has fired, - or relying on
NSXPCConnection's invalidation handler to reset state.
Also worth using __weak typeof(self) weakSelf = self; so the block doesn't keep the window controller alive if the daemon is slow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TimerWindowController.m` around lines 374 - 388, queryUnlockGateState
currently sets gateQueryInFlight = YES and only clears it inside the XPC reply
block, which can hang indefinitely; update queryUnlockGateState to capture self
weakly (use __weak typeof(self) weakSelf) before calling [self.gateXPCClient
getBlockUnlockGateStateWithReply:] and inside the reply first obtain a
strongSelf and check a queryToken or timestamp to ignore late replies, and also
schedule a dispatch_after watchdog (e.g. 5s) that clears gateQueryInFlight and
updates UI on the main queue if the reply hasn't arrived; alternatively register
an invalidation handler on gateXPCClient/NSXPCConnection to clear
gateQueryInFlight — reference the symbols queryUnlockGateState,
gateQueryInFlight, gateXPCClient, getBlockUnlockGateStateWithReply:, and
waitingForNetworkUnlock when making these changes.
The stock SelfControl Killer (shipped at selfcontrolapp.com as a separate
.app) is intended as a recovery tool but, as shipped, is a full bypass:
running it during an in-progress block clears pf+hosts rules and resets
SCSettings, after which checkupBlock sees `!anyBlockIsRunning` and calls
removeBlock. The block is over.
The defense uses an existing accident in the codebase: SCSettings's
`resetAllSettingsToDefaults` only iterates keys present in
`defaultSettingsDict`. `BlockTimekeeping` is not in that dict, so it
survives the killer's reset. We extend that into a real recovery
mechanism.
Changes:
* SCBlockClock.recordBlockStartWithDuration:blocklist:isAllowlist:endDate:
persists the active blocklist + allowlist flag + end date alongside
the timekeeping data. The original duration-only API is preserved
(delegates to the new one with nil/0 metadata) so existing unit
tests are unaffected.
* `+ savedActiveBlocklist`, `+ savedActiveBlockAsWhitelist`,
`+ savedBlockEndDate`, `+ clearAllBlockState` are the read/clear
surface used by the daemon's tampering-recovery and legitimate-end
paths.
* SCDaemonBlockMethods.startBlock now calls the metadata-bearing
initializer so the recovery info is captured the moment the block
arms.
* SCDaemonBlockMethods.checkupBlock, in the previously-fatal
`!anyBlockIsRunning` branch, now checks BlockTimekeeping first.
If we have a saved blocklist AND SCBlockClock says the block has
not yet elapsed, we treat the missing SCSettings entries as
tampering: re-populate BlockIsRunning/ActiveBlocklist/BlockEndDate,
re-install pf+hosts rules via installBlockRulesFromSettings, and
keep the checkup timer running. If timekeeping says the block
elapsed or we have no saved config, fall through to the existing
removeBlock + stopCheckupTimer path (with a clearAllBlockState
added).
* SCDaemonBlockMethods.attemptVerifiedUnlock's success path also calls
clearAllBlockState so the next checkup cannot mistake stale
saved-config for evidence of tampering.
Separately, SCKillerHelper itself is hardened: after auth + key
validation but before any state-clearing work, it now refuses with
EX_TEMPFAIL if SCBlockClock says a block is still in progress
(blockDurationSeconds > 0 && !blockDurationHasElapsed). This makes
the killer purely a recovery tool — if the block is legitimately over
(or never started), it still works; otherwise the user is told to
wait. The defense above protects against an attacker using an
unmodified upstream Killer build that doesn't have this check.
Threat-model gap closed:
Original killer's behaviour, before:
settings wiped → daemon's next checkupBlock (~1s)
→ !anyBlockIsRunning → removeBlock → block over.
Original killer's behaviour, after:
settings wiped → BlockTimekeeping survives →
daemon's next checkupBlock detects saved blocklist + unelapsed
timekeeping → restores BlockIsRunning + re-arms pf+hosts → block
effectively un-killable until it legitimately elapses.
Own killer, after:
refuses to do anything during an active block; works only as a
recovery tool for already-finished or never-started blocks.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Daemon/SCDaemonBlockMethods.m`:
- Around line 305-352: The recovery path restores only
ActiveBlocklist/ActiveBlockAsWhitelist/BlockEndDate from BlockTimekeeping,
losing auxiliary keys (ClearCaches, AllowLocalNetworks,
EvaluateCommonSubdomains, IncludeLinkedDomains, BlockSoundShouldPlay,
BlockSound, EnableErrorReporting) so installBlockRulesFromSettings rebuilds
rules with defaults; extend SCBlockClock and its APIs (e.g.,
recordBlockStartWithDuration:blocklist:isAllowlist:endDate: and
startBlockWithControllingUID:) to persist an auxiliary settings dictionary
(savedAuxSettings) into BlockTimekeeping, add accessors (e.g., savedAuxSettings)
and in the recovery branch restore those keys into SCSettings before calling
installBlockRulesFromSettings and syncSettingsAndWait so restored blocks use the
original auxiliary options rather than defaults.
- Line 146: The timer in SCDaemon that currently calls [SCBlockClock
tickCheckpoint] directly must be serialized with other SCBlockClock mutators;
modify the NSTimer block in startCheckpointTimer so it acquires the same lock
used by SCDaemonBlockMethods (call [SCDaemonBlockMethods daemonMethodLock]
around the tickCheckpoint invocation) or dispatch the tick to the same serial
queue used by recordBlockStartWithDuration and clearAllBlockState; ensure you
reference SCBlockClock.tickCheckpoint, SCDaemon.startCheckpointTimer, and
SCDaemonBlockMethods.daemonMethodLock so the checkpoint fire is executed under
the same synchronization as recordBlockStartWithDuration and clearAllBlockState.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e3eeda3-c715-4ef9-a969-1c429e0be9ec
📒 Files selected for processing (4)
Common/Utility/SCBlockClock.hCommon/Utility/SCBlockClock.mDaemon/SCDaemonBlockMethods.mSCKillerHelper/main.m
|
|
||
| [[SCDaemon sharedDaemon] resetInactivityTimer]; | ||
| [[SCDaemon sharedDaemon] startCheckupTimer]; | ||
| [[SCDaemon sharedDaemon] startCheckpointTimer]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether tickCheckpoint runs under daemonMethodLock / a single serial queue.
ast-grep --pattern $'- (void)startCheckpointTimer { $$$ }'
ast-grep --pattern $'+ (void)startCheckpointTimer { $$$ }'
rg -nP -C5 '\b(startCheckpointTimer|stopCheckpointTimer|checkpointTimer)\b'
rg -nP -C5 '\btickCheckpoint\b'
# Are SCBlockClock mutators (recordBlockStart*, tickCheckpoint, clearAllBlockState) ever called outside daemonMethodLock?
rg -nP -C3 '\b(recordBlockStartWithDuration|tickCheckpoint|clearAllBlockState)\b' --type=m --type=hRepository: SelfControlApp/selfcontrol
Length of output: 43598
🏁 Script executed:
# Check the method containing startCheckpointTimer call to see lock scope
sed -n '130,150p' Daemon/SCDaemonBlockMethods.m
# Search for daemonMethodLock usage to understand synchronization strategy
rg -nP 'daemonMethodLock' --type=m --type=h -B2 -A2
# Look for any synchronization around tickCheckpoint calls
rg -nP '(synchronized|@synchronized|dispatch_sync|dispatch_async|performSelector)' --type=m -B2 -A2 | grep -A10 -B10 'tickCheckpoint'Repository: SelfControlApp/selfcontrol
Length of output: 907
🏁 Script executed:
# Find where daemonMethodLock is acquired in the startBlock method
rg -nP 'daemonMethodLock' Daemon/SCDaemonBlockMethods.m -B5 -A5
# Also check the full startBlock method signature
rg -nP '\+ \(void\)startBlock' Daemon/SCDaemonBlockMethods.m -A60 | head -80
# Confirm NSTimer default scheduling behavior
rg -nP 'NSTimer|scheduledTimer' Daemon/SCDaemon.m -B2 -A2Repository: SelfControlApp/selfcontrol
Length of output: 12777
🏁 Script executed:
# Check if tickCheckpoint or the timer block take any locks
rg -nP 'tickCheckpoint' Daemon/SCDaemon.m -B3 -A3
# Look for any `@synchronized` or lock usage around SCBlockClock methods
rg -nP '@synchronized|NSLock|daemonMethodLock' Common/Utility/SCBlockClock.m
# Check if there's a serial queue pattern for SCBlockClock
rg -nP 'dispatch_queue|serial|GCD' Common/Utility/SCBlockClock.mRepository: SelfControlApp/selfcontrol
Length of output: 388
🏁 Script executed:
# Check recordBlockStartWithDuration to confirm it's the read-modify-write method
rg -nP 'recordBlockStartWithDuration' Common/Utility/SCBlockClock.m -A20 | head -40
# Check clearAllBlockState usage in attemptVerifiedUnlock context
rg -nP 'clearAllBlockState' Daemon/SCDaemonBlockMethods.m -B5 -A5Repository: SelfControlApp/selfcontrol
Length of output: 2922
tickCheckpoint fires from the timer without serialization against other SCBlockClock mutators.
The checkpoint timer at Daemon/SCDaemon.m lines 112–116 calls [SCBlockClock tickCheckpoint] directly in an NSTimer block, which runs on the main runloop without daemonMethodLock. Meanwhile, recordBlockStartWithDuration (called at line 97 of SCDaemonBlockMethods.m under daemonMethodLock) and clearAllBlockState (lines 342, 438 under lock context) both perform read-modify-write operations on the same BlockTimekeeping dictionary. Although SCBlockClock.h lines 9–11 require these mutators to be invoked serialized, the timer block has no synchronization with daemonMethodLock or any serial queue, creating a race window where tickCheckpoint can interleave with recordBlockStartWithDuration or clearAllBlockState.
Wrap the timer's call to tickCheckpoint with [SCDaemonBlockMethods daemonMethodLock], or dispatch it to a serial queue shared by all SCBlockClock callsites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Daemon/SCDaemonBlockMethods.m` at line 146, The timer in SCDaemon that
currently calls [SCBlockClock tickCheckpoint] directly must be serialized with
other SCBlockClock mutators; modify the NSTimer block in startCheckpointTimer so
it acquires the same lock used by SCDaemonBlockMethods (call
[SCDaemonBlockMethods daemonMethodLock] around the tickCheckpoint invocation) or
dispatch the tick to the same serial queue used by recordBlockStartWithDuration
and clearAllBlockState; ensure you reference SCBlockClock.tickCheckpoint,
SCDaemon.startCheckpointTimer, and SCDaemonBlockMethods.daemonMethodLock so the
checkpoint fire is executed under the same synchronization as
recordBlockStartWithDuration and clearAllBlockState.
Comment on lines
+305
to
+352
| // SCSettings says no block, but BlockTimekeeping may still say otherwise. | ||
| // The stock SelfControl Killer wipes the user-facing settings keys (BlockIsRunning, | ||
| // ActiveBlocklist, BlockEndDate, …) via resetAllSettingsToDefaults but does NOT | ||
| // touch BlockTimekeeping (it is not in defaultSettingsDict). So if we still have | ||
| // an unelapsed timekeeping entry with a saved blocklist, treat this as tampering | ||
| // and rebuild the block instead of removing it. | ||
| NSArray<NSString*>* savedBlocklist = [SCBlockClock savedActiveBlocklist]; | ||
| BOOL savedIsAllowlist = [SCBlockClock savedActiveBlockAsWhitelist]; | ||
| NSDate* savedEndDate = [SCBlockClock savedBlockEndDate]; | ||
| BOOL haveSavedConfig = (savedBlocklist.count > 0 || savedIsAllowlist); | ||
|
|
||
| if (haveSavedConfig && ![SCBlockClock blockDurationHasElapsed]) { | ||
| NSLog(@"INFO: Checkup ran, settings were wiped but BlockTimekeeping still shows an active block. Restoring from saved config."); | ||
| [SCSentry captureMessage: @"Tampering detected (settings wiped). Restoring block from BlockTimekeeping."]; | ||
|
|
||
| SCSettings* settings = [SCSettings sharedSettings]; | ||
| [settings setValue: savedBlocklist forKey: @"ActiveBlocklist"]; | ||
| [settings setValue: @(savedIsAllowlist) forKey: @"ActiveBlockAsWhitelist"]; | ||
| if (savedEndDate != nil) { | ||
| [settings setValue: savedEndDate forKey: @"BlockEndDate"]; | ||
| } | ||
|
|
||
| [SCHelperToolUtilities installBlockRulesFromSettings]; | ||
| [settings setValue: @YES forKey: @"BlockIsRunning"]; | ||
|
|
||
| NSError* syncErr = [settings syncSettingsAndWait: 5]; | ||
| if (syncErr != nil) { | ||
| NSLog(@"WARNING: Sync failed after restoring block: %@", syncErr); | ||
| [SCSentry captureError: syncErr]; | ||
| } | ||
|
|
||
| [SCHelperToolUtilities sendConfigurationChangedNotification]; | ||
| } else { | ||
| // No saved config, or the block has legitimately elapsed: existing behavior. | ||
| NSLog(@"INFO: Checkup ran, no active block found."); | ||
| [SCSentry captureMessage: @"Checkup ran and no active block found! Removing block, tampering suspected..."]; | ||
| [SCHelperToolUtilities removeBlock]; | ||
| [SCBlockClock clearAllBlockState]; | ||
| [SCHelperToolUtilities sendConfigurationChangedNotification]; | ||
|
|
||
| // Temporarily disabled the TamperingDetection flag because it was sometimes causing false positives | ||
| // (i.e. people having the background set repeatedly despite no attempts to cheat) | ||
| // GitHub issue: https://github.com/SelfControlApp/selfcontrol/issues/621 | ||
| // [settings setValue: @YES forKey: @"TamperingDetected"]; | ||
|
|
||
| // once the checkups stop, the daemon will clear itself in a while due to inactivity | ||
| [[SCDaemon sharedDaemon] stopCheckupTimer]; | ||
| } |
There was a problem hiding this comment.
Recovery restores only blocklist/allowlist/endDate — auxiliary block settings revert to defaults.
recordBlockStartWithDuration:blocklist:isAllowlist:endDate: only persists those three fields into BlockTimekeeping. After resetAllSettingsToDefaults, the auxiliary settings written at startBlock (lines 104-110: ClearCaches, AllowLocalNetworks, EvaluateCommonSubdomains, IncludeLinkedDomains, BlockSoundShouldPlay, BlockSound, EnableErrorReporting) are gone. When this branch then calls installBlockRulesFromSettings at line 327, the block is rebuilt with whatever the defaults are for those keys — not what the user originally chose.
The most security-relevant ones are EvaluateCommonSubdomains and IncludeLinkedDomains: if the user enabled them and the defaults are off, the restored block silently stops blocking subdomain/linked-domain variants, which is exactly the kind of bypass this PR is trying to close. AllowLocalNetworks similarly flips a real semantic.
Consider persisting these alongside the blocklist in BlockTimekeeping (e.g., a savedAuxSettings dictionary) and restoring them here before installBlockRulesFromSettings.
🛡️ Sketch of the fix
In SCBlockClock, extend the start API and recovery accessors to carry the auxiliary keys (or a single opaque dictionary):
+ (void)recordBlockStartWithDuration:(NSTimeInterval)durationSeconds
blocklist:(nullable NSArray<NSString*>*)blocklist
isAllowlist:(BOOL)isAllowlist
- endDate:(nullable NSDate*)endDate;
+ endDate:(nullable NSDate*)endDate
+ auxSettings:(nullable NSDictionary*)auxSettings;
+
+ + (nullable NSDictionary*)savedAuxBlockSettings;Then in startBlockWithControllingUID: pass blockSettings (or the subset that affects rule installation) through, and in the recovery branch:
[settings setValue: savedBlocklist forKey: @"ActiveBlocklist"];
[settings setValue: @(savedIsAllowlist) forKey: @"ActiveBlockAsWhitelist"];
if (savedEndDate != nil) {
[settings setValue: savedEndDate forKey: @"BlockEndDate"];
}
+NSDictionary* aux = [SCBlockClock savedAuxBlockSettings];
+for (NSString* k in @[@"ClearCaches", @"AllowLocalNetworks",
+ @"EvaluateCommonSubdomains", @"IncludeLinkedDomains",
+ @"BlockSoundShouldPlay", @"BlockSound",
+ @"EnableErrorReporting"]) {
+ if (aux[k] != nil) [settings setValue: aux[k] forKey: k];
+}
[SCHelperToolUtilities installBlockRulesFromSettings];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Daemon/SCDaemonBlockMethods.m` around lines 305 - 352, The recovery path
restores only ActiveBlocklist/ActiveBlockAsWhitelist/BlockEndDate from
BlockTimekeeping, losing auxiliary keys (ClearCaches, AllowLocalNetworks,
EvaluateCommonSubdomains, IncludeLinkedDomains, BlockSoundShouldPlay,
BlockSound, EnableErrorReporting) so installBlockRulesFromSettings rebuilds
rules with defaults; extend SCBlockClock and its APIs (e.g.,
recordBlockStartWithDuration:blocklist:isAllowlist:endDate: and
startBlockWithControllingUID:) to persist an auxiliary settings dictionary
(savedAuxSettings) into BlockTimekeeping, add accessors (e.g., savedAuxSettings)
and in the recovery branch restore those keys into SCSettings before calling
installBlockRulesFromSettings and syncSettingsAndWait so restored blocks use the
original auxiliary options rather than defaults.
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.
Summary
Adds tamper-resistant block timing so a user with admin access cannot end a block early by manipulating the system clock or the network. This addresses GitHub issue #621-style concerns and the long-standing
sudo datebypass.Threat model:
sudo datejump during a block, intended to make the block "expire" early.Dateheaders).Architecture
SCBlockClock(Common/Utility/SCBlockClock.{h,m})Tamper-resistant elapsed-time accounting. On block start, persists
{wallClockStart, mach_continuous_time, kern.boottime UUID, duration}intoSCSettings. Every ~30s the daemon ticks a checkpoint that advanceselapsedSecondsAccumulatedbyMIN(deltaContinuous, MAX(0, deltaWall)):mach_continuous_time(which doesn't move withdate).MAX(0, ·)floor).mach_continuous_timeresets, so the cross-boot branch creditsMAX(0, wall-clock gap)instead.SCTrustedTime+SCPinnedHosts(Common/Utility/)When
SCBlockClocksays the block has elapsed, the daemon does not immediately unlock. It first verifies real-world time via pinned HTTPS HEAD requests to multiple hosts (Apple, Google, Cloudflare), parsing theDate:response header. Pinning uses SHA-256 of the leaf cert's public key (SecKeyCopyExternalRepresentation). Quorum requires ≥2 successful responses agreeing within 5 minutes; the median must be ≥BlockEndDate. If quorum fails (offline, MITM, hosts disagree), the unlock attempt is retried with exponential backoff (10s/30s/60s/120s/120s) and the UI shows a "waiting for internet" state surfaced from the daemon over XPC.UI integration (TimerWindowController.m)
The timer display now sources remaining time from
SCBlockClock(for modern blocks), not fromBlockEndDate - now. This closes a UI-layer bypass discovered in end-to-end testing: asudo datejump pastBlockEndDatewould trip the "Stuck? Stop block manually" escape hatch within ~7 seconds, exposing manual block removal. With this change, the countdown ticks down at real elapsed-time rate regardless of system clock; the manual-kill escape hatch only arms for genuine stuck-daemon scenarios.Files
Common/Utility/SCBlockClock.{h,m}(new) + testsCommon/Utility/SCTrustedTime.{h,m}(new) + testsCommon/Utility/SCPinnedHosts.{h,m}(new)Common/SCXPCClient.{h,m},Daemon/SCDaemonProtocol.h,Daemon/SCDaemonXPC.m— XPC surface forgetBlockUnlockGateStateDaemon/SCDaemon.{h,m}— 30s checkpoint timerDaemon/SCDaemonBlockMethods.{h,m}— verified-unlock state machineCommon/Utility/SCBlockUtilities.{h,m}—currentBlockIsTrulyExpiredpredicateTimerWindowController.m,en.lproj/Localizable.strings— UI surfacethoughts/shared/{plans,research}/2026-05-14-tamper-resistant-timing-*.md— design doc + implementation planTest plan
Unit tests
SCBlockClockTests— 9 tests covering forward-clock clamp, backward-clock floor, cross-boot wall-gap credit, in-flight delta accuracySCTrustedTimeTests— IMF date parsing, real-network fetch with correct pin, fetch with wrong pin fails, quorum-vs-threshold logicManual / end-to-end
sudo date 010100002030. Daemon refuses to unlock. UI countdown keeps ticking down based on real elapsed time. No "Stuck?" button.pfrule) — quorum not reached, block stays.Known limitations / follow-ups
BlockEndDate), so the block stays in "waiting for internet" — recoverable but ugly. Worth cappinggapWallat e.g.2 * blockDurationSeconds. Not addressed here.checkupBlocktreats "no active block in settings" asremoveBlockto avoid permablocks. A user withsudocanrm /Library/Preferences/org.eyebeam.SelfControl.plist. Worth tightening to "ifBlockTimekeepingexists and block hasn't elapsed, keep block on." Not addressed here.SelfControl Killeris unchanged and still provides admin-authorized manual removal; this PR does not attempt to remove that escape hatch.Notes for review
thoughts/is verbose; happy to drop it or move it to a wiki. Kept it inline because it documents the security reasoning behind specific decisions (e.g. why the 5-min disagreement window, why MIN(deltaCont, MAX(0, deltaWall)) instead of e.g. NTP).SCDaemon.mandproject.pbxprojin different regions. Merging either first should be straightforward; the other will rebase cleanly.Summary by CodeRabbit
New Features
Improvements
Tests & Docs