🛡️ Sentinel: [HIGH] Fix insecure storage of Universal Control shared secret#47
🛡️ Sentinel: [HIGH] Fix insecure storage of Universal Control shared secret#47NSEvent wants to merge 1 commit into
Conversation
…secret The `universalControlRelaySharedSecret` was being stored in plaintext in `UserDefaults` as a redundant fallback. This commit removes writes to the unencrypted `UserDefaults` storage and explicitly deletes the key when accessing data, ensuring the secret only persists within the secure Keychain. Co-authored-by: NSEvent <44446865+NSEvent@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe relay shared secret is migrated from insecure plaintext storage in ChangesSecret Storage Security Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
XboxControllerMapper/XboxControllerMapper/Services/Input/UniversalControlMouseRelay.swift (1)
2805-2811:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSurface and handle Keychain write failures explicitly.
Line 2805 and Line 2835 ignore
KeychainService.storePasswordresults. With plaintext defaults now removed, a failed Keychain write silently drops durable secret persistence and can break pairing continuity after restart whileconfigureRelayPairingSecretstill reports success.Proposed fix
func configureRelayPairingSecret(_ secret: String) -> Bool { guard let data = Self.decodeRelaySecret(secret) else { return false } - storeRelaySharedSecret(data) - return true + return storeRelaySharedSecret(data) } private func relaySharedSecretData() -> Data { if let configured = UserDefaults.standard.string(forKey: relaySharedSecretDefaultsKey), let data = Self.decodeRelaySecret(configured) { - _ = KeychainService.storePassword( + let stored = KeychainService.storePassword( data.base64EncodedString(), key: relaySecretKey, service: relaySecretKeychainService - ) + ) != nil UserDefaults.standard.removeObject(forKey: relaySharedSecretDefaultsKey) - return data + if stored { return data } + NSLog("[UCMouseRelay] Failed to migrate relay secret to Keychain") } if let stored = KeychainService.retrievePassword( key: relaySecretKey, service: relaySecretKeychainService @@ - private func storeRelaySharedSecret(_ data: Data) { + `@discardableResult` + private func storeRelaySharedSecret(_ data: Data) -> Bool { let normalized = data.base64EncodedString() UserDefaults.standard.removeObject(forKey: relaySharedSecretDefaultsKey) - _ = KeychainService.storePassword( + guard KeychainService.storePassword( normalized, key: relaySecretKey, service: relaySecretKeychainService - ) + ) != nil else { + NSLog("[UCMouseRelay] Failed to persist relay secret to Keychain") + return false + } resetAuthenticator(secretData: data) + return true }Also applies to: 2832-2840
🤖 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 `@XboxControllerMapper/XboxControllerMapper/Services/Input/UniversalControlMouseRelay.swift` around lines 2805 - 2811, The Keychain write result from KeychainService.storePassword is currently ignored which can silently drop secret persistence; update the code paths that call KeychainService.storePassword (the block that stores data.base64EncodedString() with key relaySecretKey and service relaySecretKeychainService and the similar block around lines 2832–2840) to check the returned success/failure, handle failures by returning an error or throwing so configureRelayPairingSecret does not report success on failure, and only remove UserDefaults.standard.removeObject(forKey: relaySharedSecretDefaultsKey) after a successful store; ensure failures surface up to the caller (or log and return nil/error) so pairing persistence problems are detectable.
🤖 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.
Outside diff comments:
In
`@XboxControllerMapper/XboxControllerMapper/Services/Input/UniversalControlMouseRelay.swift`:
- Around line 2805-2811: The Keychain write result from
KeychainService.storePassword is currently ignored which can silently drop
secret persistence; update the code paths that call
KeychainService.storePassword (the block that stores data.base64EncodedString()
with key relaySecretKey and service relaySecretKeychainService and the similar
block around lines 2832–2840) to check the returned success/failure, handle
failures by returning an error or throwing so configureRelayPairingSecret does
not report success on failure, and only remove
UserDefaults.standard.removeObject(forKey: relaySharedSecretDefaultsKey) after a
successful store; ensure failures surface up to the caller (or log and return
nil/error) so pairing persistence problems are detectable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72bcd6c8-dbbb-4a6a-a970-423a7252a421
📒 Files selected for processing (2)
.Jules/sentinel.mdXboxControllerMapper/XboxControllerMapper/Services/Input/UniversalControlMouseRelay.swift
1 participant
🚨 Severity: HIGH
💡 Vulnerability: The application was storing a sensitive shared secret (
universalControlRelaySharedSecret) in plaintext withinUserDefaults. Even though it also securely saved it to the Keychain, the redundant unencrypted fallback allowed unauthorized access to the secret through disk inspection.🎯 Impact: Local attackers or malicious processes could extract the unencrypted shared secret from
UserDefaultsand impersonate or hijack the Universal Control mouse relay.🔧 Fix: Removed
UserDefaults.standard.setfor the secret. Replaced it with.removeObjectcalls in both the store and retrieve flows to aggressively wipe any existing plaintext secrets left over from legacy installations, relying exclusively on the secureKeychainService.✅ Verification: Tested statically via regex syntax checks in Python. Evaluated positively by internal code review.
PR created automatically by Jules for task 18288302286118988901 started by @NSEvent
Summary by CodeRabbit
Bug Fixes
Documentation