Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip safe atomic swap if update has custom update security policy #2593

Merged
merged 2 commits into from
Jun 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions Autoupdate/SUPlainInstaller.m
Original file line number Diff line number Diff line change
Expand Up @@ -344,26 +344,42 @@ - (BOOL)performInitialInstallation:(NSError * __autoreleasing *)error

SUFileManager *fileManager = [[SUFileManager alloc] init];

BOOL updateHasCustomUpdateSecurityPolicy = NO;
if (@available(macOS 13.0, *)) {
// If the new update is notarized / developer ID code signed and Autoupdate is not signed with the same Team ID as the new update,
// then we may run into Privacy & Security prompt issues from the OS
// which think we are modifying the update (but we're not)
// To avoid these, we skip the gatekeeper scan and skip performing an atomic swap during install
// If the update has a custom update security policy, the same team ID policy may not apply,
// so in that case we will also skip performing an atomic swap

NSURL *mainExecutableURL = NSBundle.mainBundle.executableURL;
if (mainExecutableURL == nil) {
// This shouldn't happen
_canPerformSafeAtomicSwap = NO;
} else {
NSString *installerTeamIdentifier = [SUCodeSigningVerifier teamIdentifierAtURL:mainExecutableURL];
NSString *bundleTeamIdentifier = [SUCodeSigningVerifier teamIdentifierAtURL:bundle.bundleURL];

// If the new update is code signed and Autoupdate is not signed with the same Team ID as the new update,
// then we may run into Privacy & Security prompt issues from the OS
// To avoid these, we skip the gatekeeper scan and skip performing an atomic swap during install
_canPerformSafeAtomicSwap = (bundleTeamIdentifier == nil || (installerTeamIdentifier != nil && [installerTeamIdentifier isEqualToString:bundleTeamIdentifier]));
updateHasCustomUpdateSecurityPolicy = updateHost.hasUpdateSecurityPolicy;
if (updateHasCustomUpdateSecurityPolicy) {
// We don't handle working around a custom update security policy
_canPerformSafeAtomicSwap = NO;
} else {
NSString *installerTeamIdentifier = [SUCodeSigningVerifier teamIdentifierAtURL:mainExecutableURL];
NSString *bundleTeamIdentifier = [SUCodeSigningVerifier teamIdentifierAtURL:bundle.bundleURL];

// If bundleTeamIdentifier is nil, then the update isn't code signed so atomic swap is okay
_canPerformSafeAtomicSwap = (bundleTeamIdentifier == nil || (installerTeamIdentifier != nil && [installerTeamIdentifier isEqualToString:bundleTeamIdentifier]));
}
}
} else {
_canPerformSafeAtomicSwap = YES;
}

if (!_canPerformSafeAtomicSwap) {
SULog(SULogLevelDefault, @"Skipping atomic rename/swap and gatekeeper scan because Autoupdate is not signed with same identity as the new update %@", bundle.bundleURL.lastPathComponent);
if (updateHasCustomUpdateSecurityPolicy) {
SULog(SULogLevelDefault, @"Skipping atomic rename/swap and gatekeeper scan because new update %@ has a custom NSUpdateSecurityPolicy", bundle.bundleURL.lastPathComponent);
} else {
SULog(SULogLevelDefault, @"Skipping atomic rename/swap and gatekeeper scan because Autoupdate is not signed with same identity as the new update %@", bundle.bundleURL.lastPathComponent);
}
}

_newAndOldBundlesOnSameVolume = [fileManager itemAtURL:bundle.bundleURL isOnSameVolumeItemAsURL:_host.bundle.bundleURL];
Expand Down
9 changes: 9 additions & 0 deletions Sparkle/SPUUpdater.m
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ @implementation SPUUpdater
BOOL _showingPermissionRequest;
BOOL _loggedATSWarning;
BOOL _loggedNoSecureKeyWarning;
BOOL _loggedUpdateSecurityPolicyWarning;
BOOL _updatingMainBundle;
}

Expand Down Expand Up @@ -361,6 +362,14 @@ - (BOOL)checkIfConfiguredProperlyAndRequireFeedURL:(BOOL)requireFeedURL validate
}
}

if (_updatingMainBundle) {
if (!_loggedUpdateSecurityPolicyWarning && mainBundleHost.hasUpdateSecurityPolicy) {
SULog(SULogLevelDefault, @"Warning: %@ has a custom NSUpdateSecurityPolicy in its Info.plist. This may cause issues when installing updates. Please consider removing this key for your builds using Sparkle if you do not really require a custom update security policy.", hostName);

_loggedUpdateSecurityPolicyWarning = YES;
}
}

return YES;
}

Expand Down
2 changes: 2 additions & 0 deletions Sparkle/SUHost.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ SPU_OBJC_DIRECT_MEMBERS
@property (getter=isRunningTranslocated, nonatomic, readonly) BOOL runningTranslocated;
@property (readonly, nonatomic, copy, nullable) NSString *publicDSAKeyFileKey;

@property (nonatomic, readonly) BOOL hasUpdateSecurityPolicy;

- (nullable id)objectForInfoDictionaryKey:(NSString *)key;
- (BOOL)boolForInfoDictionaryKey:(NSString *)key;
- (nullable id)objectForUserDefaultsKey:(NSString *)defaultName;
Expand Down
7 changes: 7 additions & 0 deletions Sparkle/SUHost.m
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,13 @@ - (NSString *_Nullable)publicDSAKey SPU_OBJC_DIRECT
return key;
}

- (BOOL)hasUpdateSecurityPolicy
{
NSDictionary<NSString *, id> *updateSecurityPolicy = [self objectForInfoDictionaryKey:@"NSUpdateSecurityPolicy"];

return (updateSecurityPolicy != nil);
}

- (SUPublicKeys *)publicKeys
{
return [[SUPublicKeys alloc] initWithEd:[self publicEDKey]
Expand Down
Loading