Improve robustness around dmg passwords (#2627)

* Handle failing to extract password protected disk images even when a decryption password isn't provided. * Propagate error with more information when hdiutil attach fails. * Wait for detaching disk images for unit tests (fixes not being able to run tests repeatably).
sparkle-project · Sep 15, 2024 · 661540d · 661540d
1 parent a5c26b5
commit 661540d
Show file tree

Hide file tree

Showing 8 changed files with 74 additions and 28 deletions.
diff --git a/Autoupdate/AppInstaller.m b/Autoupdate/AppInstaller.m
@@ -236,7 +236,7 @@ - (void)extractAndInstallUpdate SPU_OBJC_DIRECT
 
                  [self->_communicator handleMessageWithIdentifier:SPUExtractedArchiveWithProgress data:data];
              }
-         }];
+         } waitForCleanup:NO];
     }
 }
 
@@ -499,6 +499,7 @@ - (void)handleMessageWithIdentifier:(int32_t)identifier data:(NSData *)data
             self->_signatures = installationData.signatures;
             self->_updateDirectoryPath = cacheInstallationPath;
             self->_extractionDirectory = extractionDirectory;
+            self->_decryptionPassword = installationData.decryptionPassword;
             self->_host = [[SUHost alloc] initWithBundle:hostBundle];
             self->_verifierInformation = [[SPUVerifierInformation alloc] initWithExpectedVersion:installationData.expectedVersion expectedContentLength:installationData.expectedContentLength];
 

diff --git a/Autoupdate/SUBinaryDeltaUnarchiver.m b/Autoupdate/SUBinaryDeltaUnarchiver.m
@@ -83,7 +83,7 @@ - (instancetype)initWithArchivePath:(NSString *)archivePath extractionDirectory:
     return self;
 }
 
-- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock
+- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock waitForCleanup:(BOOL)__unused waitForCleanup
 {
     dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
         @autoreleasepool {

diff --git a/Autoupdate/SUDiskImageUnarchiver.m b/Autoupdate/SUDiskImageUnarchiver.m
@@ -11,6 +11,7 @@
 #import "SUDiskImageUnarchiver.h"
 #import "SUUnarchiverNotifier.h"
 #import "SULog.h"
+#import "SUErrors.h"
 
 
 #include "AppKitPrevention.h"
@@ -50,11 +51,11 @@ - (instancetype)initWithArchivePath:(NSString *)archivePath extractionDirectory:
     return self;
 }
 
-- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock
+- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock waitForCleanup:(BOOL)waitForCleanup
 {
     dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
         SUUnarchiverNotifier *notifier = [[SUUnarchiverNotifier alloc] initWithCompletionBlock:completionBlock progressBlock:progressBlock];
-        [self extractDMGWithNotifier:notifier];
+        [self extractDMGWithNotifier:notifier waitForCleanup:waitForCleanup];
     });
 }
 
@@ -78,7 +79,7 @@ - (BOOL)fileManager:(NSFileManager *)fileManager shouldCopyItemAtURL:(NSURL *)sr
 }
 
 // Called on a non-main thread.
-- (void)extractDMGWithNotifier:(SUUnarchiverNotifier *)notifier SPU_OBJC_DIRECT
+- (void)extractDMGWithNotifier:(SUUnarchiverNotifier *)notifier waitForCleanup:(BOOL)waitForCleanup SPU_OBJC_DIRECT
 {
 	@autoreleasepool {
         BOOL mountedSuccessfully = NO;
@@ -95,25 +96,32 @@ - (void)extractDMGWithNotifier:(SUUnarchiverNotifier *)notifier SPU_OBJC_DIRECT
         // Note: this check does not follow symbolic links, which is what we want
         while ([[NSURL fileURLWithPath:mountPoint] checkResourceIsReachableAndReturnError:NULL]);
 
-        NSData *promptData = [NSData dataWithBytes:"yes\n" length:4];
+        NSMutableData *inputData = [NSMutableData data];
 
-        // Finder doesn't verify disk images anymore beyond the code signing signature (if available)
-        // Opt out of the old CRC checksum checks
-        NSMutableArray *arguments = [@[@"attach", _archivePath, @"-mountpoint", mountPoint, @"-noverify", @"-nobrowse", @"-noautoopen"] mutableCopy];
-
-        if (_decryptionPassword) {
-            NSMutableData *passwordData = [[_decryptionPassword dataUsingEncoding:NSUTF8StringEncoding] mutableCopy];
+        // Prepare stdin data for passwords and license agreements
+        {
+            // If no password is supplied, we will still be asked a password.
+            // In that case we respond with an empty password.
+            NSData *decryptionPasswordData = [_decryptionPassword dataUsingEncoding:NSUTF8StringEncoding];
+            if (decryptionPasswordData != nil) {
+                [inputData appendData:decryptionPasswordData];
+            }
+
             // From the hdiutil docs:
             // read a null-terminated passphrase from standard input
             //
-            // Add the null terminator, then the newline
-            [passwordData appendData:[NSData dataWithBytes:"\0" length:1]];
-            [passwordData appendData:promptData];
-            promptData = passwordData;
+            // Add the null terminator
+            [inputData appendBytes:"\0" length:1];
 
-            [arguments addObject:@"-stdinpass"];
+            // Append prompt data for license agreements
+            [inputData appendBytes:"yes\n" length:4];
         }
 
+        // Finder doesn't verify disk images anymore beyond the code signing signature (if available)
+        // Opt out of the old CRC checksum checks
+        // Also always pass -stdinpass so we gracefully handle password protected disk images even if we aren't expecting them
+        NSArray *arguments = @[@"attach", _archivePath, @"-mountpoint", mountPoint, @"-noverify", @"-nobrowse", @"-noautoopen", @"-stdinpass"];
+
         NSData *output = nil;
         NSInteger taskResult = -1;
 
@@ -153,15 +161,15 @@ - (void)extractDMGWithNotifier:(SUUnarchiverNotifier *)notifier SPU_OBJC_DIRECT
             }
 
             if (@available(macOS 10.15, *)) {
-                if (![inputPipe.fileHandleForWriting writeData:promptData error:&error]) {
+                if (![inputPipe.fileHandleForWriting writeData:inputData error:&error]) {
                     goto reportError;
                 }
             }
 #if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_15
             else
             {
                 @try {
-                    [inputPipe.fileHandleForWriting writeData:promptData];
+                    [inputPipe.fileHandleForWriting writeData:inputData];
                 } @catch (NSException *) {
                     goto reportError;
                 }
@@ -175,12 +183,16 @@ - (void)extractDMGWithNotifier:(SUUnarchiverNotifier *)notifier SPU_OBJC_DIRECT
 
             taskResult = task.terminationStatus;
         }
-
+        
         if (taskResult != 0) {
             NSString *resultStr = output ? [[NSString alloc] initWithData:output encoding:NSUTF8StringEncoding] : nil;
             SULog(SULogLevelError, @"hdiutil failed with code: %ld data: <<%@>>", (long)taskResult, resultStr);
+
+            error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUUnarchivingError userInfo:@{NSLocalizedDescriptionKey:[NSString stringWithFormat:@"Extraction failed due to hdiutil returning %ld status: %@", (long)taskResult, resultStr]}];
+
             goto reportError;
         }
+
         mountedSuccessfully = YES;
 
         // Mounting can take some time, so increment progress
@@ -271,11 +283,11 @@ - (void)extractDMGWithNotifier:(SUUnarchiverNotifier *)notifier SPU_OBJC_DIRECT
 
         [notifier notifyProgress:1.0];
 
-        [notifier notifySuccess];
+        BOOL success = YES;
         goto finally;
 
     reportError:
-        [notifier notifyFailureWithError:error];
+        success = NO;
 
     finally:
         if (mountedSuccessfully) {
@@ -289,10 +301,18 @@ - (void)extractDMGWithNotifier:(SUUnarchiverNotifier *)notifier SPU_OBJC_DIRECT
             if (![task launchAndReturnError:&launchCleanupError]) {
                 SULog(SULogLevelError, @"Failed to unmount %@", mountPoint);
                 SULog(SULogLevelError, @"Error: %@", launchCleanupError);
+            } else if (waitForCleanup) {
+                [task waitUntilExit];
             }
         } else {
             SULog(SULogLevelError, @"Can't mount DMG %@", _archivePath);
         }
+
+        if (success) {
+            [notifier notifySuccess];
+        } else {
+            [notifier notifyFailureWithError:error];
+        }
     }
 }
 

diff --git a/Autoupdate/SUFlatPackageUnarchiver.m b/Autoupdate/SUFlatPackageUnarchiver.m
@@ -44,7 +44,7 @@ - (instancetype)initWithFlatPackagePath:(NSString *)flatPackagePath extractionDi
     return self;
 }
 
-- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock
+- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock waitForCleanup:(BOOL)__unused waitForCleanup
 {
     SUUnarchiverNotifier *notifier = [[SUUnarchiverNotifier alloc] initWithCompletionBlock:completionBlock progressBlock:progressBlock];
 

diff --git a/Autoupdate/SUPipedUnarchiver.m b/Autoupdate/SUPipedUnarchiver.m
@@ -96,7 +96,7 @@ - (instancetype)initWithArchivePath:(NSString *)archivePath extractionDirectory:
     return self;
 }
 
-- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock
+- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock waitForCleanup:(BOOL)__unused waitForCleanup
 {
     NSString *command = nil;
     NSArray<NSString *> *arguments = _argumentsConformingToTypeOfPath(_archivePath, YES, &command);

diff --git a/Autoupdate/SUUnarchiverProtocol.h b/Autoupdate/SUUnarchiverProtocol.h
@@ -14,7 +14,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 + (BOOL)mustValidateBeforeExtractionWithArchivePath:(NSString *)archivePath;
 
-- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock;
+- (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock waitForCleanup:(BOOL)waitForCleanup;
 
 - (NSString *)description;
 

diff --git a/Tests/SUUnarchiverTest.swift b/Tests/SUUnarchiverTest.swift
@@ -50,7 +50,7 @@ class SUUnarchiverTest: XCTestCase
         unarchiver.unarchive(completionBlock: {(error: Error?) -> Void in
             XCTAssertNotNil(error)
             testExpectation.fulfill()
-        }, progressBlock: nil)
+        }, progressBlock: nil, waitForCleanup: true)
     }
 
     // swiftlint:disable function_parameter_count
@@ -65,7 +65,7 @@ class SUUnarchiverTest: XCTestCase
                 XCTAssertNotNil(error)
             }
             testExpectation.fulfill()
-        }, progressBlock: nil)
+        }, progressBlock: nil, waitForCleanup: true)
     }
 
     func testUnarchivingZip()
@@ -132,11 +132,36 @@ class SUUnarchiverTest: XCTestCase
         self.unarchiveTestAppWithExtension("enc.nolicense.dmg", password: "testpass")
     }
 
+    func testUnarchivingEncryptedDmgWithLicenseAndWithIncorrectPassword()
+    {
+        self.unarchiveTestAppWithExtension("enc.dmg", password: "moo", expectingSuccess: false)
+    }
+
+    func testUnarchivingEncryptedDmgWithLicenseAndWithoutPassword()
+    {
+        self.unarchiveTestAppWithExtension("enc.dmg", expectingSuccess: false)
+    }
+
+    func testUnarchivingEncryptedDmgWithoutLicenseAndWithIncorrectPassword()
+    {
+        self.unarchiveTestAppWithExtension("enc.nolicense.dmg", password: "moo", expectingSuccess: false)
+    }
+
+    func testUnarchivingEncryptedDmgWithoutLicenseAndWithoutPassword()
+    {
+        self.unarchiveTestAppWithExtension("enc.nolicense.dmg", expectingSuccess: false)
+    }
+
     func testUnarchivingAPFSDMG()
     {
         self.unarchiveTestAppWithExtension("dmg", resourceName: "SparkleTestCodeSign_apfs")
     }
 
+    func testUnarchivingAPFSDMGWithBogusPassword()
+    {
+        self.unarchiveTestAppWithExtension("dmg", password: "moo", resourceName: "SparkleTestCodeSign_apfs")
+    }
+
     func testUnarchivingAPFSAdhocSignedDMGWithAuxFiles()
     {
         self.unarchiveTestAppWithExtension("dmg", resourceName: "SparkleTestCodeSign_apfs_lzma_aux_files")

diff --git a/generate_appcast/Unarchive.swift b/generate_appcast/Unarchive.swift
@@ -25,7 +25,7 @@ func unarchive(itemPath: URL, archiveDestDir: URL, callback: @escaping (Error?)
             } catch {
                 callback(error)
             }
-        }, progressBlock: nil)
+        }, progressBlock: nil, waitForCleanup: true)
     } else {
         callback(makeError(code: .unarchivingError, "Not a supported archive format: \(itemPath.path)"))
     }