Merge pull request #881 from zorgiepoo/separate-validation

Separate validation

Sparkle.xcodeproj/project.pbxproj

@@ -205,6 +205,8 @@
 		7275F9C11B5F1F2900B1D19E /* SUFileManager.h in Headers */ = {isa = PBXBuildFile; fileRef = 7275F9BF1B5F1F2900B1D19E /* SUFileManager.h */; };
 		7275F9C21B5F1F2900B1D19E /* SUFileManager.m in Sources */ = {isa = PBXBuildFile; fileRef = 7275F9C01B5F1F2900B1D19E /* SUFileManager.m */; };
 		7275F9C31B5F1F2900B1D19E /* SUFileManager.m in Sources */ = {isa = PBXBuildFile; fileRef = 7275F9C01B5F1F2900B1D19E /* SUFileManager.m */; };
+		729924731DF3478A00DBCDF5 /* SUUpdateValidator.h in Headers */ = {isa = PBXBuildFile; fileRef = 729924711DF3478A00DBCDF5 /* SUUpdateValidator.h */; };
+		729924741DF3478A00DBCDF5 /* SUUpdateValidator.m in Sources */ = {isa = PBXBuildFile; fileRef = 729924721DF3478A00DBCDF5 /* SUUpdateValidator.m */; };
 		72A4A2401BB6567D00E7820D /* SUFileManagerTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 72A4A23F1BB6567D00E7820D /* SUFileManagerTest.swift */; };
 		72AC6B261B9AAC8800F62325 /* SparkleTestCodeSignApp.tar.gz in Resources */ = {isa = PBXBuildFile; fileRef = 72AC6B251B9AAC8800F62325 /* SparkleTestCodeSignApp.tar.gz */; };
 		72AC6B281B9AAD6700F62325 /* SparkleTestCodeSignApp.tar in Resources */ = {isa = PBXBuildFile; fileRef = 72AC6B271B9AAD6700F62325 /* SparkleTestCodeSignApp.tar */; };
@@ -691,6 +693,8 @@
 		726F2CE41BC9C33D001971A4 /* SUOperatingSystem.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SUOperatingSystem.m; sourceTree = "<group>"; };
 		7275F9BF1B5F1F2900B1D19E /* SUFileManager.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SUFileManager.h; sourceTree = "<group>"; };
 		7275F9C01B5F1F2900B1D19E /* SUFileManager.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SUFileManager.m; sourceTree = "<group>"; };
+		729924711DF3478A00DBCDF5 /* SUUpdateValidator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SUUpdateValidator.h; sourceTree = "<group>"; };
+		729924721DF3478A00DBCDF5 /* SUUpdateValidator.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = SUUpdateValidator.m; sourceTree = "<group>"; };
 		729F10FD1C65A9B500DFCCC5 /* ConfigUITest.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = ConfigUITest.xcconfig; sourceTree = "<group>"; };
 		729F10FE1C65A9B500DFCCC5 /* ConfigUITestCoverage.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = ConfigUITestCoverage.xcconfig; sourceTree = "<group>"; };
 		729F10FF1C65A9B500DFCCC5 /* ConfigUITestDebug.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = ConfigUITestDebug.xcconfig; sourceTree = "<group>"; };
@@ -1135,6 +1139,8 @@
 		61CFB2C10E384958007A1735 /* Support */ = {
 			isa = PBXGroup;
 			children = (
+				729924711DF3478A00DBCDF5 /* SUUpdateValidator.h */,
+				729924721DF3478A00DBCDF5 /* SUUpdateValidator.m */,
 				61B078CC15A5FB6100600039 /* SUCodeSigningVerifier.h */,
 				61B078CD15A5FB6100600039 /* SUCodeSigningVerifier.m */,
 				61299A2D09CA2DAB00B7442F /* SUDSAVerifier.h */,
@@ -1260,6 +1266,7 @@
 				7275F9C11B5F1F2900B1D19E /* SUFileManager.h in Headers */,
 				767B61AC1972D488004E0C3C /* SUGuidedPackageInstaller.h in Headers */,
 				61EF67590E25C5B400F754E0 /* SUHost.h in Headers */,
+				729924731DF3478A00DBCDF5 /* SUUpdateValidator.h in Headers */,
 				723B25301CEAB3A600909873 /* bscommon.h in Headers */,
 				618FA5010DAE88B40026945C /* SUInstaller.h in Headers */,
 				55C14F06136EF6DB00649790 /* SULog.h in Headers */,
@@ -1815,6 +1822,7 @@
 				61EF67560E25B58D00F754E0 /* SUHost.m in Sources */,
 				618FA5020DAE88B40026945C /* SUInstaller.m in Sources */,
 				55C14F07136EF6DB00649790 /* SULog.m in Sources */,
+				729924741DF3478A00DBCDF5 /* SUUpdateValidator.m in Sources */,
 				723B252D1CEAB3A600909873 /* bscommon.c in Sources */,
 				726F2CE61BC9C33D001971A4 /* SUOperatingSystem.m in Sources */,
 				618FA5230DAE8E8A0026945C /* SUPackageInstaller.m in Sources */,

Sparkle/SUBasicUpdateDriver.m

@@ -10,16 +10,14 @@
 
 #import "SUHost.h"
 #import "SUOperatingSystem.h"
-#import "SUDSAVerifier.h"
-#import "SUInstaller.h"
 #import "SUStandardVersionComparator.h"
 #import "SUUnarchiver.h"
 #import "SUConstants.h"
 #import "SULog.h"
 #import "SUBinaryDeltaCommon.h"
-#import "SUCodeSigningVerifier.h"
 #import "SUUpdater_Private.h"
 #import "SUFileManager.h"
+#import "SUUpdateValidator.h"
 
 @interface SUBasicUpdateDriver ()
 
@@ -31,6 +29,8 @@ @interface SUBasicUpdateDriver ()
 @property (copy) NSString *tempDir;
 @property (copy) NSString *relaunchPath;
 
+@property (nonatomic) SUUpdateValidator *updateValidator;
+
 @end
 
 @implementation SUBasicUpdateDriver
@@ -43,6 +43,8 @@ @implementation SUBasicUpdateDriver
 @synthesize tempDir;
 @synthesize relaunchPath;
 
+@synthesize updateValidator = _updateValidator;
+
 - (void)checkForUpdatesAtURL:(NSURL *)URL host:(SUHost *)aHost
 {
     [super checkForUpdatesAtURL:URL host:aHost];
@@ -274,98 +276,6 @@ - (void)download:(NSURLDownload *)__unused d decideDestinationWithSuggestedFilen
     [self.download setDestination:self.downloadPath allowOverwrite:YES];
 }
 
-/**
- * If the update is a package, then it must be signed using DSA. No other verification is done.
- *
- * If the update is a bundle, then it must meet any one of:
- *
- *  * old and new DSA public keys are the same and valid (it allows change of Code Signing identity), or
- *
- *  * old and new Code Signing identity are the same and valid
- *
- */
-- (BOOL)validateUpdateForHost:(SUHost *)host downloadedToPath:(NSString *)downloadedPath extractedToPath:(NSString *)extractedPath DSASignature:(NSString *)DSASignature
-{
-    BOOL isPackage = NO;
-    NSString *installSourcePath = [SUInstaller installSourcePathInUpdateFolder:extractedPath forHost:self.host isPackage:&isPackage isGuided:NULL];
-    if (installSourcePath == nil) {
-        SULog(@"No suitable install is found in the update. The update will be rejected.");
-        return NO;
-    }
-
-    NSString *publicDSAKey = host.publicDSAKey;
-
-    // Modern packages are not distributed as bundles and are code signed differently than regular applications
-    if (isPackage) {
-        if (nil == publicDSAKey) {
-            SULog(@"The existing app bundle does not have a DSA key, so it can't verify installer packages.");
-        }
-
-        BOOL packageValidated = [SUDSAVerifier validatePath:downloadedPath withEncodedDSASignature:DSASignature withPublicDSAKey:publicDSAKey];
-
-        if (!packageValidated) {
-            SULog(@"DSA signature validation of the package failed. The update contains an installer package, and valid DSA signatures are mandatory for all installer packages. The update will be rejected. Sign the installer with a valid DSA key or use an .app bundle update instead.");
-        }
-
-        return packageValidated;
-    }
-
-    NSBundle *newBundle = [NSBundle bundleWithPath:installSourcePath];
-    if (newBundle == nil) {
-        SULog(@"No suitable bundle is found in the update. The update will be rejected.");
-        return NO;
-    }
-
-    SUHost *newHost = [[SUHost alloc] initWithBundle:newBundle];
-    NSString *newPublicDSAKey = newHost.publicDSAKey;
-
-    // Downgrade in DSA security should not be possible
-    if (publicDSAKey != nil && newPublicDSAKey == nil) {
-        SULog(@"A public DSA key is found in the old bundle but no public DSA key is found in the new update. For security reasons, the update will be rejected.");
-        return NO;
-    }
-
-    BOOL dsaKeysMatch = (publicDSAKey == nil || newPublicDSAKey == nil) ? NO : [publicDSAKey isEqualToString:newPublicDSAKey];
-
-    // If the new DSA key differs from the old, then this check is not a security measure, because the new key is not trusted.
-    // In that case, the check ensures that the app author has correctly used DSA keys, so that the app will be updateable in the next version.
-    // However if the new and old DSA keys are the same, then this is a security measure.
-    if (newPublicDSAKey != nil) {
-        if (![SUDSAVerifier validatePath:downloadedPath withEncodedDSASignature:DSASignature withPublicDSAKey:newPublicDSAKey]) {
-            SULog(@"DSA signature validation failed. The update has a public DSA key and is signed with a DSA key, but the %@ doesn't match the signature. The update will be rejected.",
-                  dsaKeysMatch ? @"public key" : @"new public key shipped with the update");
-            return NO;
-        }
-    }
-
-    BOOL updateIsCodeSigned = [SUCodeSigningVerifier bundleAtURLIsCodeSigned:newHost.bundle.bundleURL];
-
-    if (dsaKeysMatch) {
-        NSError *error = nil;
-        if (updateIsCodeSigned && ![SUCodeSigningVerifier codeSignatureIsValidAtBundleURL:newHost.bundle.bundleURL error:&error]) {
-            SULog(@"The update archive has a valid DSA signature, but the app is also signed with Code Signing, which is corrupted: %@. The update will be rejected.", error);
-            return NO;
-        }
-    } else {
-        BOOL hostIsCodeSigned = [SUCodeSigningVerifier bundleAtURLIsCodeSigned:host.bundle.bundleURL];
-
-        NSString *dsaStatus = newPublicDSAKey ? @"has a new DSA key that doesn't match the previous one" : (publicDSAKey ? @"removes the DSA key" : @"isn't signed with a DSA key");
-        if (!hostIsCodeSigned || !updateIsCodeSigned) {
-            NSString *acsStatus = !hostIsCodeSigned ? @"old app hasn't been signed with app Code Signing" : @"new app isn't signed with app Code Signing";
-            SULog(@"The update archive %@, and the %@. At least one method of signature verification must be valid. The update will be rejected.", dsaStatus, acsStatus);
-            return NO;
-        }
-
-        NSError *error = nil;
-        if (![SUCodeSigningVerifier codeSignatureAtBundleURL:host.bundle.bundleURL matchesSignatureAtBundleURL:newHost.bundle.bundleURL error:&error]) {
-            SULog(@"The update archive %@, and the app is signed with a new Code Signing identity that doesn't match code signing of the original app: %@. At least one method of signature verification must be valid. The update will be rejected.", dsaStatus, error);
-            return NO;
-        }
-    }
-
-    return YES;
-}
-
 - (void)downloadDidFinish:(NSURLDownload *)__unused d
 {
     assert(self.updateItem);
@@ -407,13 +317,27 @@ - (BOOL)download:(NSURLDownload *)__unused download shouldDecodeSourceDataOfMIME
 - (void)extractUpdate
 {
     SUUnarchiver *unarchiver = [SUUnarchiver unarchiverForPath:self.downloadPath updatingHostBundlePath:self.host.bundlePath withPassword:self.updater.decryptionPassword];
+
+    BOOL success;
     if (!unarchiver) {
         SULog(@"Error: No valid unarchiver for %@!", self.downloadPath);
+
+        success = NO;
+    } else {
+        // Currently unsafe archives are the only case where we can prevalidate before extraction, but that could change in the future
+        BOOL needsPrevalidation = [[unarchiver class] unsafeIfArchiveIsNotValidated];
+
+        self.updateValidator = [[SUUpdateValidator alloc] initWithDownloadPath:self.downloadPath dsaSignature:self.updateItem.DSASignature host:self.host performingPrevalidation:needsPrevalidation];
+
+        success = self.updateValidator.canValidate;
+    }
+
+    if (!success) {
         [self unarchiverDidFail:nil];
-        return;
+    } else {
+        unarchiver.delegate = self;
+        [unarchiver start];
     }
-    unarchiver.delegate = self;
-    [unarchiver start];
 }
 
 - (void)failedToApplyDeltaUpdate
@@ -434,6 +358,9 @@ - (void)unarchiverDidFinish:(SUUnarchiver *)__unused ua
 
 - (void)unarchiverDidFail:(SUUnarchiver *)__unused ua
 {
+    // No longer needed
+    self.updateValidator = nil;
+
     if ([self.updateItem isDeltaUpdate]) {
         [self failedToApplyDeltaUpdate];
         return;
@@ -476,13 +403,14 @@ - (BOOL)preparePathForRelaunchTool:(NSString *)targetPath error:(NSError * __aut
 - (void)installWithToolAndRelaunch:(BOOL)relaunch displayingUserInterface:(BOOL)showUI
 {
     assert(self.updateItem);
-
-    if (![self validateUpdateForHost:self.host downloadedToPath:self.downloadPath extractedToPath:self.tempDir DSASignature:self.updateItem.DSASignature])
-    {
+    assert(self.updateValidator);
+
+    BOOL validationCheckSuccess = [self.updateValidator validateWithUpdateDirectory:self.tempDir];
+    if (!validationCheckSuccess) {
         NSDictionary *userInfo = @{
-            NSLocalizedDescriptionKey: SULocalizedString(@"An error occurred while extracting the archive. Please try again later.", nil),
-            NSLocalizedFailureReasonErrorKey: SULocalizedString(@"The update is improperly signed.", nil),
-        };
+                                   NSLocalizedDescriptionKey: SULocalizedString(@"An error occurred while extracting the archive. Please try again later.", nil),
+                                   NSLocalizedFailureReasonErrorKey: SULocalizedString(@"The update is improperly signed.", nil),
+                                   };
         [self abortUpdateWithError:[NSError errorWithDomain:SUSparkleErrorDomain code:SUSignatureError userInfo:userInfo]];
         return;
     }

Sparkle/SUBinaryDeltaUnarchiver.m

@@ -21,6 +21,11 @@ + (BOOL)canUnarchivePath:(NSString *)path
     return [[path pathExtension] isEqualToString:@"delta"];
 }
 
++ (BOOL)unsafeIfArchiveIsNotValidated
+{
+    return YES;
+}
+
 // According to https://developer.apple.com/library/mac/documentation/Carbon/Conceptual/MDImporters/Concepts/Troubleshooting.html
 // We should make sure mdimporter bundles have an up to date time in the event they were delta updated.
 // We used to invoke mdimport on the bundle but this is not a very good approach.

Sparkle/SUDiskImageUnarchiver.m

@@ -18,6 +18,11 @@ + (BOOL)canUnarchivePath:(NSString *)path
     return [[path pathExtension] isEqualToString:@"dmg"];
 }
 
++ (BOOL)unsafeIfArchiveIsNotValidated
+{
+    return NO;
+}
+
 // Called on a non-main thread.
 - (void)extractDMG
 {

Sparkle/SUPipedUnarchiver.m

@@ -41,6 +41,11 @@ + (SEL)selectorConformingToTypeOfPath:(NSString *)path
     return NULL;
 }
 
++ (BOOL)unsafeIfArchiveIsNotValidated
+{
+    return NO;
+}
+
 - (void)start
 {
     [NSThread detachNewThreadSelector:[[self class] selectorConformingToTypeOfPath:self.archivePath] toTarget:self withObject:nil];

Sparkle/SUUIBasedUpdateDriver.m

@@ -115,10 +115,6 @@ - (void)updateAlertFinishedWithChoice:(SUUpdateAlertChoice)choice
     [self.host setObject:nil forUserDefaultsKey:SUSkippedVersionKey];
     switch (choice) {
         case SUInstallUpdateChoice:
-            self.statusController = [[SUStatusController alloc] initWithHost:self.host];
-            [self.statusController beginActionWithTitle:SULocalizedString(@"Downloading update...", @"Take care not to overflow the status window.") maxProgressValue:0.0 statusText:nil];
-            [self.statusController setButtonTitle:SULocalizedString(@"Cancel", nil) target:self action:@selector(cancelDownload:) isDefault:NO];
-            [self.statusController showWindow:self];
             [self downloadUpdate];
             break;
 
@@ -138,6 +134,25 @@ - (void)updateAlertFinishedWithChoice:(SUUpdateAlertChoice)choice
     }
 }
 
+- (void)downloadUpdate
+{
+    BOOL createdStatusController = NO;
+    if (self.statusController == nil) {
+        self.statusController = [[SUStatusController alloc] initWithHost:self.host];
+        createdStatusController = YES;
+    }
+
+    [self.statusController beginActionWithTitle:SULocalizedString(@"Downloading update...", @"Take care not to overflow the status window.") maxProgressValue:0.0 statusText:nil];
+    [self.statusController setButtonTitle:SULocalizedString(@"Cancel", nil) target:self action:@selector(cancelDownload:) isDefault:NO];
+    [self.statusController setButtonEnabled:YES];
+
+    if (createdStatusController) {
+        [self.statusController showWindow:self];
+    }
+
+    [super downloadUpdate];
+}
+
 - (void)download:(NSURLDownload *)__unused download didReceiveResponse:(NSURLResponse *)response
 {
     [self.statusController setMaxProgressValue:[response expectedContentLength]];

Sparkle/SUUnarchiver.h

@@ -23,7 +23,10 @@
 
 + (SUUnarchiver *)unarchiverForPath:(NSString *)path updatingHostBundlePath:(NSString *)host withPassword:(NSString *)decryptionPassword;
 
++ (BOOL)unsafeIfArchiveIsNotValidated;
+
 - (void)start;
+
 @end
 
 @protocol SUUnarchiverDelegate <NSObject>

Sparkle/SUUnarchiver.m

@@ -55,6 +55,11 @@ + (BOOL)canUnarchivePath:(NSString *)__unused path
     return NO;
 }
 
++ (BOOL)unsafeIfArchiveIsNotValidated
+{
+    return NO;
+}
+
 - (void)notifyDelegateOfProgress:(double)progress
 {
     if ([self.delegate respondsToSelector:@selector(unarchiver:extractedProgress:)]) {

Sparkle/SUUpdateValidator.h

@@ -0,0 +1,25 @@
+//
+//  SUUpdateValidator.h
+//  Sparkle
+//
+//  Created by Mayur Pawashe on 12/3/16.
+//  Copyright © 2016 Sparkle Project. All rights reserved.
+//
+
+#import <Foundation/Foundation.h>
+
+@class SUHost;
+
+@interface SUUpdateValidator : NSObject
+
+// Pass YES to performingPrevalidation if archive validation must be done immediately, before extraction
+- (instancetype)initWithDownloadPath:(NSString *)downloadPath dsaSignature:(NSString *)dsaSignature host:(SUHost *)host performingPrevalidation:(BOOL)performingPrevalidation;
+
+// Indicates whether we can perform (post) validation later
+@property (nonatomic, readonly) BOOL canValidate;
+
+// precondition: validation must be possible (see -canValidate)
+// This is "post" validation, after an archive has been extracted
+- (BOOL)validateWithUpdateDirectory:(NSString *)updateDirectory;
+
+@end